Added support for ocs provider server to fetch noobaa client resources #2680

ezio-auditore · 2024-07-02T05:12:39Z

Added an annotation is-local-client:true for local storage client
Create a Noobaa Account CR for auth_token generation of remote noobaa client
a.Added support to join external noobaa system from hosted clusters noobaa/noobaa-operator#1389
Assemble noobaa remote auth token and management address to be sent via GetStorageConfig
Updated test cases

openshift-ci · 2024-07-02T05:12:44Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2024-07-02T05:12:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ezio-auditore
Once this PR has been reviewed and has the lgtm label, please assign obnoxxx for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

vendor/github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1/noobaaaccount_types.go

services/provider/server/server.go

services/provider/server/consumer.go

services/provider/server/server.go

services/provider/server/consumer.go

services/provider/server/server.go

ezio-auditore · 2024-07-24T14:13:09Z

/test ocs-operator-bundle-e2e-aws

Signed-off-by: Kaustav Majumder <[email protected]>

nb-ohad · 2024-08-01T09:43:44Z

services/provider/server/server.go

+	if err := s.consumerManager.CreateNoobaaAccount(ctx, req.StorageConsumerUUID); err != nil {
+		return nil, status.Errorf(codes.Internal, "Failed to create noobaa account for storageconsumer. %v", err)


We should not run create operations inside the ack operation, and even more importantly, after we already enabled the consumer.

Creation of the noobaa account is a detail of storage consumer reconciliation, not of the onboard call.

Please remove this call from here.

nb-ohad · 2024-08-01T09:44:52Z

services/provider/server/server.go

+	err := s.consumerManager.DeleteNoobaaAccount(ctx, req.StorageConsumerUUID)
+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed to delete noobaaAccount resource with the provided UUID. %v", err)
+	}


The noobaa account should be owned (via an owner ref) by the consumer object.
If it is, there will be no need for manual deletion as k8s GC will take care of it.

nb-ohad · 2024-08-01T09:49:25Z

services/provider/server/consumer.go

+func (c *ocsConsumerManager) CreateNoobaaAccount(ctx context.Context, id string) error {
+
+	consumerObj, err := c.Get(ctx, id)
+	if err != nil {
+		return err
+	}
+	consumerClusterID := strings.TrimPrefix(consumerObj.Name, "storageconsumer-")
+	if consumerClusterID != "" && len(consumerClusterID) == 0 {
+		return fmt.Errorf("failed to get clusterID from consumerResource Name: %s %v", consumerObj.Name, err)
+	}
+
+	noobaaAccountName := fmt.Sprintf("noobaa-remote-%s", consumerClusterID)
+	nbAccountObj := &nbv1.NooBaaAccount{}
+	nbAccountObj.Name = noobaaAccountName
+	nbAccountObj.Namespace = consumerObj.Namespace
+	// the following annotation will enable noobaa-operator to create a auth_token secret based on this account
+	util.AddAnnotation(nbAccountObj, "remote-operator", "true")
+
+	err = c.client.Create(ctx, nbAccountObj)
+	if err != nil {
+		return fmt.Errorf("failed to create noobaa account for storageConsumer %v: %v", consumerObj.Name, err)
+	}
+	return nil
+}


Why are we creating the noobaa account in the server and not part of the reconciliation of the StorageConsumer CR?

The server does not implement reconciliation logic and every operation is considered a one-time operation.

nb-ohad · 2024-08-01T09:49:53Z

services/provider/server/consumer.go

+func (c *ocsConsumerManager) DeleteNoobaaAccount(ctx context.Context, id string) error {
+	consumerObj, err := c.Get(ctx, id)
+	if err != nil {
+		return err
+	}
+	clusterID := strings.TrimPrefix(consumerObj.Name, "storageconsumer-")
+	if clusterID != "" && len(clusterID) == 0 {
+		return fmt.Errorf("failed to get clusterID from consumerResource Name: %s %v", consumerObj.Name, err)
+	}
+	noobaaAccountName := fmt.Sprintf("noobaa-remote-%s", clusterID)
+	nbAccountObj := &nbv1.NooBaaAccount{}
+	nbAccountObj.Name = noobaaAccountName
+	nbAccountObj.Namespace = consumerObj.Namespace
+	if err := c.client.Delete(ctx, nbAccountObj); err != nil {
+		return fmt.Errorf("failed to delete Noobaa account %q. %v", nbAccountObj.Name, err)
+	}
+	return nil
+}


Noobaa account should be just GC collected via storage consumer ownership. This code is unnecessary

nb-ohad · 2024-08-01T09:54:47Z

services/provider/server/server.go

@@ -400,6 +416,59 @@ func (s *OCSProviderServer) getExternalResources(ctx context.Context, consumerRe

 	}

+	// Fetch noobaa remote secret and management address and append to extResources
+	noobaaOperatorSecret := &v1.Secret{}
+	clusterID := strings.TrimPrefix(consumerResource.Name, "storageconsumer-")


We should not fetch based on naming conventions. Always fetch by ownership or by labels.

Consumer's name is decided by the client, don't assume it will contain the clusterID

Cluster ID is available as part of the consumer's status section, if you need it (Which I don't understand why), fetch it from there

nb-ohad · 2024-08-01T09:57:32Z

services/provider/server/server.go

@@ -400,6 +416,59 @@ func (s *OCSProviderServer) getExternalResources(ctx context.Context, consumerRe

 	}

+	// Fetch noobaa remote secret and management address and append to extResources
+	noobaaOperatorSecret := &v1.Secret{}


Define the var just above the code that uses it

nb-ohad · 2024-08-01T10:02:25Z

services/provider/server/server.go

+	noobaaMgmtAddress := noobaMgmtRoute.Status.Ingress[0].Host
+	if noobaaMgmtAddress == "" {
+		return nil, fmt.Errorf("no Host found in noobaa-mgmt route Ingress")
+	}


Are we assuming a default port always? is it always 8080?

nb-ohad · 2024-08-01T10:02:58Z

services/provider/server/server.go

+	joinSecret := &corev1.Secret{
+		Data: map[string][]byte{
+			"auth_token": authToken,
+			"mgmt_addr":  []byte(noobaaMgmtAddress),


Why the Byte encoding?

nb-ohad · 2024-08-01T10:05:03Z

services/provider/server/server.go

+	extR = append(extR, &pb.ExternalResource{
+		Name: "noobaa-remote-join-secret",
+		Kind: "Secret",
+		Data: mustMarshal(joinSecret),


This is wrong, for secrets, we only send the data section, not the entire secret.

Suggested change

Data: mustMarshal(joinSecret),

Data: mustMarshal(map[string][]byte{

"auth_token": authToken,

"mgmt_addr": []byte(noobaaMgmtAddress),

}),

nb-ohad · 2024-08-01T10:06:18Z

services/provider/server/server.go

+	noobaaSpec := &nbv1.NooBaaSpec{
+		JoinSecret: &v1.SecretReference{
+			Name: "noobaa-remote-join-secret",
+		},
+	}
+	extR = append(extR, &pb.ExternalResource{
+		Name: "noobaa-remote",
+		Kind: "Noobaa",
+		Data: mustMarshal(noobaaSpec),
+	})


The spec is very small, please inline it.

Suggested change

noobaaSpec := &nbv1.NooBaaSpec{

JoinSecret: &v1.SecretReference{

Name: "noobaa-remote-join-secret",

},

}

extR = append(extR, &pb.ExternalResource{

Name: "noobaa-remote",

Kind: "Noobaa",

Data: mustMarshal(noobaaSpec),

})

noobaaSpec :=

extR = append(extR, &pb.ExternalResource{

Name: "noobaa-remote",

Kind: "Noobaa",

Data: mustMarshal(&nbv1.NooBaaSpec{

JoinSecret: &v1.SecretReference{

Name: "noobaa-remote-join-secret",

},

}),

})

leelavg · 2024-08-09T09:59:26Z

possible to mention the relation b/n this PR and #2733?

openshift-merge-robot · 2024-08-12T16:42:38Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 2, 2024

ezio-auditore force-pushed the noobaa_obc branch from 569cc45 to 6bc76c0 Compare July 2, 2024 11:46

ezio-auditore force-pushed the noobaa_obc branch 4 times, most recently from 9b30587 to 6113607 Compare July 18, 2024 07:02

ezio-auditore commented Jul 18, 2024

View reviewed changes

vendor/github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1/noobaaaccount_types.go Outdated Show resolved Hide resolved

ezio-auditore marked this pull request as ready for review July 18, 2024 09:02

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 18, 2024

rewantsoni suggested changes Jul 18, 2024

View reviewed changes

services/provider/server/server.go Outdated Show resolved Hide resolved

services/provider/server/consumer.go Show resolved Hide resolved

services/provider/server/server.go Outdated Show resolved Hide resolved

openshift-ci bot assigned rewantsoni Jul 18, 2024

rewantsoni reviewed Jul 18, 2024

View reviewed changes

services/provider/server/server.go Outdated Show resolved Hide resolved

rchikatw reviewed Jul 22, 2024

View reviewed changes

services/provider/server/consumer.go Show resolved Hide resolved

ezio-auditore force-pushed the noobaa_obc branch from 6113607 to aea4843 Compare July 22, 2024 10:56

rewantsoni suggested changes Jul 22, 2024

View reviewed changes

services/provider/server/server.go Outdated Show resolved Hide resolved

services/provider/server/server.go Outdated Show resolved Hide resolved

services/provider/server/server.go Outdated Show resolved Hide resolved

ezio-auditore force-pushed the noobaa_obc branch 3 times, most recently from ddc88b6 to 94ea6f2 Compare July 22, 2024 18:20

rchikatw reviewed Jul 23, 2024

View reviewed changes

services/provider/server/server.go Show resolved Hide resolved

ezio-auditore force-pushed the noobaa_obc branch 2 times, most recently from a6fe781 to cede821 Compare July 24, 2024 12:26

ezio-auditore force-pushed the noobaa_obc branch from cede821 to 86f0848 Compare July 24, 2024 17:54

Kaustav Majumder added 2 commits July 25, 2024 11:51

added support for ocs provider server to fetch noobaa resources

79eb451

Signed-off-by: Kaustav Majumder <[email protected]>

generated changes

b54dab3

Signed-off-by: Kaustav Majumder <[email protected]>

ezio-auditore force-pushed the noobaa_obc branch from 86f0848 to b54dab3 Compare July 25, 2024 06:21

nb-ohad requested changes Aug 1, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Aug 1, 2024

bernerhat mentioned this pull request Aug 7, 2024

Added support for ocs provider server to fetch noobaa client resources #2733

Merged

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2024

ezio-auditore closed this Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for ocs provider server to fetch noobaa client resources #2680

Added support for ocs provider server to fetch noobaa client resources #2680

ezio-auditore commented Jul 2, 2024 •

edited

Loading

openshift-ci bot commented Jul 2, 2024

openshift-ci bot commented Jul 2, 2024

ezio-auditore commented Jul 24, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

nb-ohad Aug 1, 2024

leelavg commented Aug 9, 2024

openshift-merge-robot commented Aug 12, 2024

		if err := s.consumerManager.CreateNoobaaAccount(ctx, req.StorageConsumerUUID); err != nil {
		return nil, status.Errorf(codes.Internal, "Failed to create noobaa account for storageconsumer. %v", err)

Added support for ocs provider server to fetch noobaa client resources #2680

Added support for ocs provider server to fetch noobaa client resources #2680

Conversation

ezio-auditore commented Jul 2, 2024 • edited Loading

openshift-ci bot commented Jul 2, 2024

openshift-ci bot commented Jul 2, 2024

ezio-auditore commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg commented Aug 9, 2024

openshift-merge-robot commented Aug 12, 2024

ezio-auditore commented Jul 2, 2024 •

edited

Loading