Initial AWS CCM deployment using upstream image by Danil-Grigorev · Pull Request #15 · openshift/cluster-cloud-controller-manager-operator

Danil-Grigorev · 2021-02-11T16:43:43Z

Requirements to work:

Needs to additionally be merged:

[OCPCLOUD-1106] Add afterburn task to update AWS hostname to match instance metadata machine-config-operator#2401 - fix for AWS hostname for successful migration
[OCPCLOUD-1107] External cloud-provider support via FeatureGate in post-install machine-config-operator#2386 - Kubelet FG switch to external providers
[OCPCLOUD-1070] Add observe external switch to cloudpovider observer library-go#895 (+ revendor in KCMO and KAPIO) - KCM and KAPI FG switch to external providers
Revert "Revert "Add label to include in CI release images"" #26 - Include in CCCMO in release

Danil-Grigorev · 2021-02-11T17:03:38Z

/hold We need to merge prereq PRs first.

Danil-Grigorev · 2021-02-24T14:27:11Z

/retest

Danil-Grigorev · 2021-03-22T12:22:25Z

/hold cancel

This implementation works in my manual testing. Unit tests are already in place, the PR is ready for review. (#17)

Danil-Grigorev · 2021-03-22T12:25:18Z

---
-apiVersion: rbac.authorization.k8s.io/v1
-kind: Role
-metadata:
-  name: cloud-controller-manager
-  namespace: kube-system
-  annotations:
-    include.release.openshift.io/self-managed-high-availability: "true"
-    include.release.openshift.io/single-node-developer: "true"
-rules:
-  - apiGroups:
-      - ""
-    resources:
-      - serviceaccounts
-    resourceNames:
-      - node-controller
-      - service-controller
-    verbs:
-      - get
-
-  # This grants CCMs permission to list and read all secrets in kube-system.
-  # This is required by client builder to fetch the ServiceAccount token
-  # secret. Ideally we would restrict this to the token secrets of specific
-  # ServiceAccounts.
-  - apiGroups:
-      - ""
-    resources:
-      - secrets
-    verbs:
-      - list
-      - get
-      - watch
-
---
-apiVersion: rbac.authorization.k8s.io/v1
-kind: RoleBinding
-metadata:
-  name: cloud-controller-manager
-  namespace: kube-system
-  annotations:
-    include.release.openshift.io/self-managed-high-availability: "true"
-    include.release.openshift.io/single-node-developer: "true"
-roleRef:
-  kind: Role
-  name: cloud-controller-manager
-  apiGroup: rbac.authorization.k8s.io
-subjects:
-  - kind: ServiceAccount
-    namespace: openshift-cloud-controller-manager
-    name: cloud-controller-manager
-


AWS covers these RBAC changes with upaded cluster role here. I consider removing this for now, as for example Azure and vSphere both are using same clusterRole, which will work for OpenStack too. We will investigate and possible reduce the scope of it later.

Ok, sure. As I mentioned elsewhere I think we could potentially eliminate these entirely later.

Fedosin

What about adding unit tests similar to what we have with OpenStack: https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/pkg/cloud/openstack/openstack_test.go

Danil-Grigorev · 2021-03-23T16:18:29Z

@Fedosin It is already implemented and covered in https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/controllers/clusteroperator_controller_test.go#L271-L336

elmiko

the code mostly makes sense, i have a few questions inline. also, is the main goal here for the code to dynamically load these manifests at run time?

elmiko · 2021-03-29T19:24:42Z

+		obj   client.Object
+		asset string
+	}{
+		{&appsv1.Deployment{}, "aws/assets/deployment.yaml"},


is the idea here that more resources will be added later?

Correct, it is build with intention that it might need to deploy some SA or another Deployment potentially in the future.

elmiko

thanks Danil!
/approve

openshift-ci-robot · 2021-04-15T15:19:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [elmiko]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

elmiko · 2021-04-15T15:21:11Z

/hold
didn't realize it added both labels. just putting a hold so others can review

Danil-Grigorev · 2021-04-15T15:24:34Z

/lgtm cancel

We need to fix the CI to prevent double label addition @Fedosin
Let's wait for tests to pass again.

Danil-Grigorev · 2021-04-15T15:25:10Z

/hold cancel
/retest

JoelSpeed · 2021-04-15T15:32:10Z

Can we make this look more similar to the openstack one? Keeping it in its own package in case it expands later?

Danil-Grigorev · 2021-04-15T15:44:46Z

/retest

elmiko · 2021-04-15T15:48:59Z

Can we make this look more similar to the openstack one? Keeping it in its own package in case it expands later?

+1, i think that makes good sense

Danil-Grigorev · 2021-04-15T15:50:53Z

@JoelSpeed OpenStack manifests are located incorrectly. They should belong to higher level, so cloud provider switch case selection won't have to import N packages per provider just to select correct collection of manifests: see https://github.com/openshift/cluster-cloud-controller-manager-operator/blob/master/controllers/clusteroperator_controller.go#L25-L26

JoelSpeed · 2021-04-16T09:22:45Z

/lgtm

openshift-ci-robot requested review from Fedosin and JoelSpeed February 11, 2021 16:43

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2021

Danil-Grigorev mentioned this pull request Feb 22, 2021

[OCPCLOUD-1110] Add e2e tests for managed resources provisioning #17

Merged

Danil-Grigorev force-pushed the include-aws branch from 6fb4441 to e62c57a Compare March 22, 2021 12:02

Add controller RBAC

3f27dca

Danil-Grigorev force-pushed the include-aws branch from e62c57a to a6746e5 Compare March 22, 2021 12:15

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 22, 2021

Danil-Grigorev commented Mar 22, 2021

View reviewed changes

Fedosin suggested changes Mar 23, 2021

View reviewed changes

openshift-ci-robot assigned Fedosin Mar 23, 2021

Danil-Grigorev requested a review from Fedosin March 23, 2021 16:58

Danil-Grigorev mentioned this pull request Mar 25, 2021

Provider image substitution #29

Merged

1 task

elmiko reviewed Mar 29, 2021

View reviewed changes

elmiko approved these changes Apr 15, 2021

View reviewed changes

openshift-ci-robot assigned elmiko Apr 15, 2021

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 15, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

Sample AWS CCM Deployment

7d395f4

Danil-Grigorev force-pushed the include-aws branch from c56249f to 7d395f4 Compare April 15, 2021 15:52

openshift-ci-robot assigned JoelSpeed Apr 16, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 16, 2021

Danil-Grigorev mentioned this pull request Apr 16, 2021

Add anti-affinity settings to preserve HA for CCM pods #33

Merged

openshift-merge-robot merged commit 22a11cf into openshift:master Apr 19, 2021

Danil-Grigorev deleted the include-aws branch April 20, 2021 09:13

Conversation

Danil-Grigorev commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Requirements to work:

Uh oh!

Danil-Grigorev commented Feb 11, 2021

Uh oh!

Danil-Grigorev commented Feb 24, 2021

Uh oh!

Danil-Grigorev commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Danil-Grigorev Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

mdbooth Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

Fedosin left a comment

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Mar 23, 2021

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Apr 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Apr 15, 2021

Uh oh!

elmiko commented Apr 15, 2021

Uh oh!

Danil-Grigorev commented Apr 15, 2021

Uh oh!

Danil-Grigorev commented Apr 15, 2021

Uh oh!

JoelSpeed commented Apr 15, 2021

Uh oh!

Danil-Grigorev commented Apr 15, 2021

Uh oh!

elmiko commented Apr 15, 2021

Uh oh!

Danil-Grigorev commented Apr 15, 2021

Uh oh!

JoelSpeed commented Apr 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Danil-Grigorev commented Feb 11, 2021 •

edited

Loading

Danil-Grigorev commented Mar 22, 2021 •

edited

Loading