support deploy klusterlet outside of managed cluster by zhujian7 · Pull Request #172 · open-cluster-management-io/registration-operator

zhujian7 · 2021-11-17T04:28:11Z

Related PRs:

related issue: #158

zhujian7 · 2021-12-16T01:23:06Z

@qiujian16 @zhiweiyin318 @xuezhaojun @ianzhang366 This PR Is ready for review.

but still need to:
~~- [ ] change the namespace from klusterlet name to {klusterlet name}-open-cluster-management-agent~~

add UT, integration tests

qiujian16

I might think it is a bit complicated with a lot of detachMode check in it.

My suggestion is we should separate the manifests and operations to

those always put on managed cluster
those always put on management cluster (it could be still managedcluster)
those you have to check the mode

qiujian16 · 2021-12-16T02:09:51Z

@@ -0,0 +1,13 @@
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding


why do we need to separated these. I think the manifests has only nuance with non-detached mode that you can render. Maintaining two lists of manifests would be hard since it is easy to update one and miss another

qiujian16 · 2021-12-16T02:11:35Z

+// KlusterletNamespace returns the klusterletNamespace to deploy the agents.
+func KlusterletNamespace(mode operatorapiv1.InstallMode, klusterletName, specNamespace string) string {
+	if mode == operatorapiv1.InstallModeDetached {
+		// return fmt.Sprintf("%s-%s", klusterletName, KlusterletDefaultNamespace)


rm this line. Add more comment on this function

qiujian16 · 2021-12-16T02:13:27Z

+		return klusterletName
+	}
+
+	if specNamespace == "" {


if len(specNamespace) == 0

it is safer

qiujian16 · 2021-12-16T02:51:04Z

+// resource status will be saved in the relatedResourcesStatuses, and will save the
+// result to the klusterlet status if there is any error.
+func (n *klusterletController) applyStaticFiles(ctx context.Context, klusterletName string,
+	staticFiles []string, relatedResourcesStatuses *[]operatorapiv1.RelatedResourceMeta,


return relatedResourcesStatuses rathe than adding a pointer

qiujian16 · 2021-12-16T02:53:40Z


+	if detachedMode {
+		// In Deatched mode, apply role, rolebinding, service account for registration and work to the management cluster.
+		err = n.applyStaticFiles(ctx, klusterletName, detachedStaticResourceFiles, &relatedResources, &config, n.kubeClient,


why the apply static files is different from what you did on managed cluster

qiujian16 · 2021-12-16T02:55:24Z

+}
+
+// syncSecret forked from resourceapply.SyncSecret, add an addition snippet to support sync secret to another cluster.
+func syncSecret(client, targetClient coreclientv1.SecretsGetter, recorder events.Recorder,


put this in the helper. It is better to add ctx as the parameter

This func was put in the helper. and I'd prefer to add ctx after upgrading openshift/library-go to contain openshift/library-go#1121

qiujian16 · 2021-12-16T03:03:01Z

+		return err
+	}

+	if detachedMode {


this is confused..do we really need to apply any role/rolebinding in this ns?

need to create service account in this ns

qiujian16 · 2021-12-16T03:03:52Z

 			controllerContext.Recorder(),
 			func(name string) ([]byte, error) {
-				template, err := manifests.Klusterlet111ManifestFiles.ReadFile(name)
+				template, err := manifests.KlusterletManifestFiles.ReadFile(name)


why change this line?

I put all klusterlet files in KlusterletManifestFiles to avoid duplication of code

Signed-off-by: zhujian <jiazhu@redhat.com>

zhiweiyin318 · 2021-12-17T12:22:16Z

do we need to add e2e cases for this scenario? like create 2 kind clusters in action and deploy with detach mode?
or add some integration tests?

zhujian7 · 2021-12-18T03:39:23Z

do we need to add e2e cases for this scenario? like create 2 kind clusters in action and deploy with detach mode? or add some integration tests?

Of course, this is what I am doing these days.

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 · 2021-12-21T08:56:41Z

@qiujian16 all comments have been fixed, PTAL.
@zhiweiyin318 tests were added.

And the RBAC part needs a focused reviewing, please.

qiujian16

Thanks, I think it looks pretty neat. I still have some comments, but most of the can be done as a followup.

/approve

qiujian16 · 2021-12-22T02:45:44Z

 OLM_VERSION?=0.16.1

-KUSTOMIZE?=$(PERMANENT_TMP_GOPATH)/bin/kustomize
+PWD=$(shell pwd)


I think we should update makefile in a followup PR.

qiujian16 · 2021-12-22T02:55:09Z

 	}
 	klusterlet = klusterlet.DeepCopy()
+
+	detachedMode := klusterlet.Spec.DeployOption.Mode == operatorapiv1.InstallModeDetached


can we use eq in the template so we do not need to set this bool var?

qiujian16 · 2021-12-22T02:55:49Z

+		ExternalManagedKubeConfigSecret:             helpers.ExternalManagedKubeConfig,
+		ExternalManagedKubeConfigRegistrationSecret: helpers.ExternalManagedKubeConfigRegistration,
+		ExternalManagedKubeConfigWorkSecret:         helpers.ExternalManagedKubeConfigWork,
+		DetachedMode:                                detachedMode,


InstallMode

qiujian16 · 2021-12-22T02:57:49Z

+	if detachedMode {
+		managedClusterClients, err = n.buildManagedClusterClientsDetachedMode(n.kubeClient, config.KlusterletNamespace, config.ExternalManagedKubeConfigSecret)
+		if err != nil {
+			return err


Add a TODO here, you would need to set some status condition as the followup

qiujian16 · 2021-12-22T03:05:30Z

+	if detachedMode {
+		// In detached mode, we should ensure the namespace on the managed cluster since
+		// some resources(eg:service account) are still deployed on managed cluster.
+		err := n.ensureNamespace(ctx, managedClusterClients.kubeClient, klusterletName, config.KlusterletNamespace)


should we still need this? OR why we only need to ensure this on detached mode?

why we only need to ensure this on detached mode?

In detached mode, the managed cluster and management cluster are separated, the namespace in the management cluster has been ensured previously.

openshift-ci · 2021-12-22T03:11:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhujian7

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~~ [qiujian16]

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

Signed-off-by: zhujian <jiazhu@redhat.com>

… cluster Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 · 2021-12-23T07:25:38Z

@qiujian16 all comments have been fixed, and I add another commit(last commit of this PR) to distinguish the RBAC resource names of managed cluster and management cluster, the RBAC resources names are not in conflict now but may conflict in the future, I think it is necessary to separate. PTAL. Thanks.

zhujian7 · 2021-12-23T08:10:53Z

/hold

zhujian7 · 2021-12-23T09:32:37Z

/hold cancel

zhiweiyin318 · 2021-12-23T09:41:48Z

+          "kind": "Klusterlet",
+          "metadata": {
+            "name": "klusterlet"
+          },


do not need detached klusterlet in alm-examples. this example user can apply directly on operatorhub. so we cannot put 2 klusterlet here.

zhujian7 · 2021-12-24T03:56:11Z

The makefile and README.md will be updated at a follow-up PR.

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 · 2021-12-24T12:49:51Z

@zhiweiyin318 csv updated. PTAL.

zhiweiyin318 · 2022-01-04T07:22:30Z

/lgtm

openshift-ci Bot added the do-not-merge/work-in-progress label Nov 17, 2021

openshift-ci Bot requested review from qiujian16 and zhiweiyin318 November 17, 2021 04:28

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from 0a51d92 to 0003b05 Compare November 23, 2021 08:50

openshift-ci Bot added the needs-rebase label Dec 3, 2021

zhujian7 force-pushed the klusterlet-outside branch from 0003b05 to 245592b Compare December 8, 2021 06:35

openshift-ci Bot removed the needs-rebase label Dec 8, 2021

zhujian7 force-pushed the klusterlet-outside branch 3 times, most recently from 76c52ae to 3b47a08 Compare December 15, 2021 04:11

qiujian16 reviewed Dec 16, 2021

View reviewed changes

zhujian7 added 4 commits December 17, 2021 03:16

support deploy klusterlet outside of managed cluster

e22d8c7

Signed-off-by: zhujian <jiazhu@redhat.com>

create manged cluster kubeconfig by sa syncer

1d2c6e4

Signed-off-by: zhujian <jiazhu@redhat.com>

fix CI issues

2ad2030

Signed-off-by: zhujian <jiazhu@redhat.com>

add unit test

07b951b

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from 468d2ef to cec4ec8 Compare December 17, 2021 10:17

zhujian7 force-pushed the klusterlet-outside branch from cec4ec8 to bc5beb1 Compare December 18, 2021 03:37

zhujian7 force-pushed the klusterlet-outside branch from bc5beb1 to 0a4afbc Compare December 18, 2021 03:41

separate the manifests and operations for klusterlet operator

d439130

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from f103ad0 to 5922b9a Compare December 20, 2021 16:18

zhujian7 added 2 commits December 21, 2021 01:16

add unit test

90cd778

Signed-off-by: zhujian <jiazhu@redhat.com>

add integration test

9e0927c

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from 80c0c21 to d2f4afd Compare December 21, 2021 08:09

zhujian7 force-pushed the klusterlet-outside branch from d2f4afd to 6c318d9 Compare December 21, 2021 08:34

add e2e test

fe286ee

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch from 6c318d9 to fe286ee Compare December 21, 2021 08:52

qiujian16 reviewed Dec 22, 2021

View reviewed changes

openshift-ci Bot added the approved label Dec 22, 2021

address code review comments

4ca76bb

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch from 57fa7f2 to 4ca76bb Compare December 22, 2021 14:45

distinguish the RBAC resource names of managed cluster and management…

591c1fa

… cluster Signed-off-by: zhujian <jiazhu@redhat.com>

openshift-ci Bot added the do-not-merge/hold label Dec 23, 2021

zhujian7 force-pushed the klusterlet-outside branch from b639ad6 to c24a08f Compare December 23, 2021 09:31

openshift-ci Bot removed the do-not-merge/hold label Dec 23, 2021

zhiweiyin318 reviewed Dec 23, 2021

View reviewed changes

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from 66c0784 to c307e35 Compare December 24, 2021 03:52

zhujian7 force-pushed the klusterlet-outside branch 2 times, most recently from 88a626d to 73cf438 Compare December 24, 2021 10:11

zhujian7 added 2 commits December 24, 2021 10:53

ensure the klusterlet namespace early

ebd4e3e

Signed-off-by: zhujian <jiazhu@redhat.com>

update csv

3963de0

Signed-off-by: zhujian <jiazhu@redhat.com>

zhujian7 force-pushed the klusterlet-outside branch from c74c9f3 to 3963de0 Compare December 24, 2021 11:04

openshift-ci Bot assigned zhiweiyin318 Jan 4, 2022

openshift-ci Bot added the lgtm label Jan 4, 2022

openshift-merge-robot merged commit dcf93fa into open-cluster-management-io:main Jan 4, 2022

		@@ -0,0 +1,13 @@
		apiVersion: rbac.authorization.k8s.io/v1
		kind: RoleBinding

Conversation

zhujian7 commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhujian7 commented Dec 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiujian16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhiweiyin318 commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhujian7 commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhujian7 commented Dec 21, 2021

Uh oh!

qiujian16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhujian7 Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Dec 22, 2021

Uh oh!

zhujian7 commented Dec 23, 2021

Uh oh!

zhujian7 commented Dec 23, 2021

Uh oh!

zhujian7 commented Dec 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhujian7 commented Dec 24, 2021

Uh oh!

zhujian7 commented Dec 24, 2021

Uh oh!

zhiweiyin318 commented Jan 4, 2022

Uh oh!

Reviewers

Assignees

Labels

zhujian7 commented Nov 17, 2021 •

edited

Loading

zhujian7 commented Dec 16, 2021 •

edited

Loading

zhiweiyin318 commented Dec 17, 2021 •

edited

Loading

zhujian7 commented Dec 18, 2021 •

edited

Loading

zhujian7 Dec 22, 2021 •

edited

Loading