Added support to join external noobaa system from hosted clusters by bernerhat · Pull Request #1405 · noobaa/noobaa-operator

bernerhat · 2024-08-05T08:03:32Z

Explain the changes

continuation of Kaustav's PR

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

Doc added/updated
Tests added

dannyzaken · 2024-08-06T12:23:34Z

pkg/noobaaaccount/reconciler.go

+				nbremoteAuthToken := &corev1.Secret{}
+				nbremoteAuthToken.Name = r.NooBaaAccount.Name
+				nbremoteAuthToken.Namespace = r.NooBaaAccount.Namespace
+				err = r.Client.Delete(r.Ctx, r.Secret)


If I'm not mistaken, R.Secret is deleted automtically, since it's owned by the noobaaAccount CR. I don't think that anything is changed in this PR with respect to deletion. Can you please verify it?

dannyzaken · 2024-08-06T12:24:16Z

pkg/noobaaaccount/reconciler.go

+	// create join secret conatining auth token for remote noobaa account
+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-operator")
+	if exists {
+		if strings.ToLower(annotationValue) != strconv.FormatBool(false) {


Suggested change

if strings.ToLower(annotationValue) != strconv.FormatBool(false) {

if strings.ToLower(annotationValue) == "true" {

dannyzaken · 2024-08-06T12:27:38Z

pkg/noobaaaccount/reconciler.go

+			if err != nil {
+				return fmt.Errorf("cannot create an auth token for remote operator, error: %v", err)
+			}
+			if res.Token == "" {


Is an empty token somthing that can be returned by noobaa-core in case there is no error? did you encounter this?

dannyzaken · 2024-08-06T12:32:36Z

pkg/system/phase1_verifying.go

+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-client-noobaa")
+	annotationBoolVal := true


I suggest giving clearer names to the variables. annotationVal and annotationBoolVal are too generic

dannyzaken · 2024-08-06T12:33:16Z

pkg/system/phase1_verifying.go

+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-client-noobaa")
+	annotationBoolVal := true
+	if exists {
+		annotationBoolVal = strings.ToLower(annotationValue) == strconv.FormatBool(true)


Again, unless I'm missing somthing, I don't see a reason to use strconv.FormatBool(true) and not "true"

dannyzaken · 2024-08-06T12:38:18Z

pkg/system/phase4_configuring.go

+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-client-noobaa")
+	annotationBoolVal := true
+	if exists {
+		annotationBoolVal = strings.ToLower(annotationValue) == strconv.FormatBool(true)
+	}
+	if !exists || !annotationBoolVal {


since you repeat this code more than once, mayb extract this to a function isRemoteClientNoobaa

dannyzaken · 2024-08-06T12:52:13Z

pkg/noobaaaccount/reconciler.go

 		return err
 	}
+	// create join secret conatining auth token for remote noobaa account
+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-operator")


If the remote-operator annotation exist, do you need to also create access keys (r.Secret)?

Maybe just reuse the same r.Secret and populate it with the relevant values according to the annotatioin?

it's a valid point but since it was Kaustav's decision to use a new secret, i'm not sure what led to him creating a new secret.
Lets leave the new secret in place and when Kaustav returns from his leave we can change this. i'll open an issue for it as well (since changing it will most likely cause changes to code in other repos such as ocs-operator which uses this secret)

I prefer to make the code simpler and not to create 2 secrets for a single noobaa account. I suggest that for now add an "auth_token" key on r.Secret. If there is a reason to create a separate secret than we can make this change later

dannyzaken · 2024-08-06T12:52:48Z

pkg/noobaaaccount/reconciler.go

+			nbremoteAuthToken.StringData["auth_token"] = res.Token
+			r.Own(nbremoteAuthToken)
+
+			err = r.Client.Create(r.Ctx, r.Secret)


did you mean to create nbremoteAuthToken?

Suggested change

err = r.Client.Create(r.Ctx, r.Secret)

err = r.Client.Create(r.Ctx, nbremoteAuthToken)

dannyzaken · 2024-08-11T12:16:28Z

pkg/noobaaaccount/reconciler.go

 		return err
 	}
+	// create join secret conatining auth token for remote noobaa account
+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-operator")


I prefer to make the code simpler and not to create 2 secrets for a single noobaa account. I suggest that for now add an "auth_token" key on r.Secret. If there is a reason to create a separate secret than we can make this change later

dannyzaken · 2024-08-11T12:22:24Z

pkg/noobaaaccount/reconciler.go

 		return err
 	}
+	// create join secret conatining auth token for remote noobaa account
+	annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-operator")


I see now that the annotation is on the noobaa CR and not the noobaaAccount CR. In that case, what's the difference between remote-operator and MulticloudObjectGatewayProviderMode?

The remote-operator is an annotation which is being added to the account CR by the ocs-operator while we are on the provider side in order to indicate when we onboard a new consumer and therefore need a new auth token to be added to the account. The MulticloudObjectGatewayProviderMode annotation is being added to the Noobaa CR while we are running remotely and need to indicate that we only wish to deploy the operator and connect to a remote cluster.

But here you check the annotaion on the noobaa CR and not the account CR (r.NooBaa.GetAnnotations())

you are correct it's a mistake, will fix.

bernerhat · 2024-08-20T14:46:26Z

@dannyzaken everything is done and tested can you please re-review

dannyzaken · 2024-08-21T07:19:20Z

pkg/util/util.go

+	annotationValue, exists := GetAnnotationValue(annotations, "remote-client-noobaa")
+	annotationBoolVal := false
+	if exists {
+		annotationBoolVal = strings.ToLower(annotationValue) == strconv.FormatBool(true)


trueStr instead of strconv

Signed-off-by: Kaustav Majumder <kaustav.majumder@ibm.com>

bernerhat · 2024-08-21T14:12:07Z

i've rebased, can we merge? i dont have permissions to merge myself. @dannyzaken

pull-request-size bot added the size/L label Aug 5, 2024

bernerhat force-pushed the nb-remote branch from 3474354 to b2216c7 Compare August 5, 2024 08:27

dannyzaken requested changes Aug 6, 2024

View reviewed changes

dannyzaken reviewed Aug 6, 2024

View reviewed changes

bernerhat force-pushed the nb-remote branch from b2216c7 to b217ce6 Compare August 8, 2024 12:42

pull-request-size bot added size/M and removed size/L labels Aug 8, 2024

bernerhat force-pushed the nb-remote branch from b217ce6 to 600d868 Compare August 8, 2024 12:50

dannyzaken reviewed Aug 11, 2024

View reviewed changes

bernerhat force-pushed the nb-remote branch from 600d868 to c285607 Compare August 18, 2024 07:20

pull-request-size bot added size/L and removed size/M labels Aug 18, 2024

bernerhat force-pushed the nb-remote branch 2 times, most recently from c3e5779 to e02bf8c Compare August 20, 2024 06:48

bernerhat requested a review from dannyzaken August 20, 2024 14:46

dannyzaken reviewed Aug 21, 2024

View reviewed changes

bernerhat force-pushed the nb-remote branch from e02bf8c to 83cd4da Compare August 21, 2024 08:30

dannyzaken approved these changes Aug 21, 2024

View reviewed changes

bernerhat force-pushed the nb-remote branch from 83cd4da to e3f2822 Compare August 21, 2024 12:15

Added support to join external noobaa system from hosted clusters

05c054c

Signed-off-by: Kaustav Majumder <kaustav.majumder@ibm.com>

bernerhat force-pushed the nb-remote branch from e3f2822 to 05c054c Compare August 21, 2024 13:28

liranmauda merged commit 7c4efc4 into noobaa:master Aug 21, 2024

ezio-auditore mentioned this pull request Sep 3, 2024

[Backport to 5.17] Added support to join external noobaa system from hosted clusters #1425

Closed

	if strings.ToLower(annotationValue) != strconv.FormatBool(false) {
	if strings.ToLower(annotationValue) == "true" {

		annotationValue, exists := util.GetAnnotationValue(r.NooBaa.GetAnnotations(), "remote-client-noobaa")
		annotationBoolVal := true

	err = r.Client.Create(r.Ctx, r.Secret)
	err = r.Client.Create(r.Ctx, nbremoteAuthToken)

Conversation

bernerhat commented Aug 5, 2024

Explain the changes

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

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!

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!

bernerhat commented Aug 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernerhat commented Aug 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants