changes to onboarding token api to include role in body #2669

rewantsoni · 2024-06-19T08:00:01Z

Update the onboarding Ticket to include an OnboardingSubjectRole - ocs-client, ocs-peer

ocs-client will be used to onboard a Client
ocs-peer role will be used to onboard a StorageCluster

Add a new endpoint to generate the peer onboarding token.

Part of RHSTOR-4886

rewantsoni · 2024-06-19T09:13:21Z

/cc @nb-ohad

services/ux-backend/handlers/onboardingtokens/handler.go

rchikatw · 2024-07-08T08:02:57Z

PR looks good to me.

rewantsoni · 2024-07-15T07:49:37Z

/retest-required

nb-ohad · 2024-08-06T14:01:24Z

controllers/util/provider.go

}
+

Suggested change

nb-ohad · 2024-08-06T14:03:12Z

services/types.go

+
+const (
+	ClientRole        Role = "client"
+	MirroringPeerRole Role = "mirroring-peer"


The rule is not related to mirroring.
As for the agreed architecture, there are 2 rules: ocs-client and ocs-peer,

Please align everything with the given terminology

nb-ohad · 2024-08-06T14:04:19Z

services/types.go

 type OnboardingTicket struct {
 	ID                string `json:"id"`
 	ExpirationDate    int64  `json:"expirationDate,string"`
 	StorageQuotaInGiB uint   `json:"storageQuotaInGiB,omitempty"`
+	// Role can be either client or mirroring-peer
+	Role Role `json:"role,string"`


The rule is not for the token, it is for the subject.

Suggested change

Role Role `json:"role,string"`

SubjectRole Role `json:"subjectRole,string"`

nb-ohad · 2024-08-06T14:06:21Z

services/types.go

@@ -1,7 +1,16 @@
 package services

+type Role string


This is a very generic name and might create collision or confusion.
Please rename to something more meaningful and more specific.

Suggested change

type Role string

type OnboardingSubjectRole string

nb-ohad · 2024-08-06T14:07:46Z

services/ux-backend/handlers/onboardingtokens/handler.go

+		if strings.ToLower(body.Role) == string(services.MirroringPeerRole) {
+			role = services.MirroringPeerRole
+		} else if strings.ToLower(body.Role) == string(services.ClientRole) || body.Role == "" {
+			// to ensure backward compatibility, if the role is empty, we assume it to be client
+			role = services.ClientRole
+			if body.Value == 0 || body.Value > math.MaxInt {
+				http.Error(w, fmt.Sprintf("invalid value sent in request body, value should be greater than 0 and less than %v: %v", math.MaxInt, body.Value), http.StatusBadRequest)
+				return
+			}
+			unitAsGiB, ok := unitToGib[body.Unit]
+			if !ok {
+				http.Error(w, fmt.Sprintf("invalid Unit type sent in request body, Valid types are [Gi,Ti,Pi]: %v", body.Unit), http.StatusBadRequest)
+				return
+			}
+			storageQuotaInGiB = ptr.To(unitAsGiB * body.Value)
+		} else {
+			http.Error(w, fmt.Sprintf("invalid Role sent in request body, Valid roles are [mirroring-peer,client]: %v", body.Role), http.StatusBadRequest)


We should not do that. This will allow anyone to generate a peer onboarding token.

openshift-ci · 2024-08-06T16:37:20Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rewantsoni
Once this PR has been reviewed and has the lgtm label, please ask for approval from nb-ohad. 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

nb-ohad · 2024-09-08T11:05:48Z

services/types.go

 type OnboardingTicket struct {
 	ID                string `json:"id"`
 	ExpirationDate    int64  `json:"expirationDate,string"`
 	StorageQuotaInGiB uint   `json:"storageQuotaInGiB,omitempty"`
+	// OnboardingSubjectRole can be either ocs-client or ocs-peer


If you are adding a comment please separate using a blank line

Suggested change

// OnboardingSubjectRole can be either ocs-client or ocs-peer

// OnboardingSubjectRole can be either ocs-client or ocs-peer

nb-ohad · 2024-09-08T11:09:37Z

services/ux-backend/handlers/onboardingtokens/handler.go

+			SubjectRole string `json:"subjectRole"`
+			Value       uint   `json:"value"`
+			Unit        string `json:"unit"`


These does not belong on the same level

nb-ohad · 2024-09-08T11:10:45Z

services/ux-backend/handlers/onboardingtokens/handler.go

+		var body = struct {
+			SubjectRole string `json:"subjectRole"`
+			Value       uint   `json:"value"`
+			Unit        string `json:"unit"`
 		}{}
-		if err = json.NewDecoder(r.Body).Decode(&quota); err != nil {
+		if err = json.NewDecoder(r.Body).Decode(&body); err != nil {
 			http.Error(w, err.Error(), http.StatusBadRequest)
 			return
 		}
-
-		if quota.Value == 0 || quota.Value > math.MaxInt {
-			http.Error(w, fmt.Sprintf("invalid value sent in request body, value should be greater than 0 and less than %v: %v", math.MaxInt, quota.Value), http.StatusBadRequest)
+		if strings.ToLower(body.SubjectRole) == string(services.PeerRole) {
+			role = services.PeerRole
+		} else if strings.ToLower(body.SubjectRole) == string(services.ClientRole) || body.SubjectRole == "" {
+			// to ensure backward compatibility, if the role is empty, we assume it to be client
+			role = services.ClientRole
+			if body.Value == 0 || body.Value > math.MaxInt {
+				http.Error(w, fmt.Sprintf("invalid value sent in request body, value should be greater than 0 and less than %v: %v", math.MaxInt, body.Value), http.StatusBadRequest)
+				return
+			}
+			unitAsGiB, ok := unitToGib[body.Unit]
+			if !ok {
+				http.Error(w, fmt.Sprintf("invalid Unit type sent in request body, Valid types are [Gi,Ti,Pi]: %v", body.Unit), http.StatusBadRequest)
+				return
+			}
+			storageQuotaInGiB = ptr.To(unitAsGiB * body.Value)
+		} else {
+			http.Error(w, fmt.Sprintf("invalid OnboardingSubjectRole sent in request body, Valid roles are [mirroring-peer,client]: %v", body.SubjectRole), http.StatusBadRequest)


The flow is structured incorrectly. The current structure allows sending peer role with quota fields and the quota fields are just ignored. This is the wrong behavior. I would expect a bad request response instead.

I would suggest using a switch case based on the role and delegate each of the 2 cases into separate functions. In addition, you could use the "default" case to return an error for an unsupported role

rewantsoni · 2024-09-10T10:41:34Z

/retest-required

The PR does the following: 1. add role to the onboarding ticket 2. add a new endpoint for peer-onboarding-tokens Signed-off-by: Rewant Soni <resoni@redhat.com>

openshift-ci bot requested a review from nb-ohad June 19, 2024 09:13

rewantsoni mentioned this pull request Jun 25, 2024

Implement the server side rpc calls for peering #2675

Open

rchikatw reviewed Jun 26, 2024

View reviewed changes

services/ux-backend/handlers/onboardingtokens/handler.go Outdated Show resolved Hide resolved

rewantsoni force-pushed the onboarding-ticket branch 3 times, most recently from 252d888 to e768114 Compare July 3, 2024 06:55

rewantsoni force-pushed the onboarding-ticket branch 3 times, most recently from d7a8e4e to f9750a0 Compare July 11, 2024 09:12

rewantsoni mentioned this pull request Jul 16, 2024

Add RPC calls for onboarding/offboarding secondary provider #2671

Open

rewantsoni force-pushed the onboarding-ticket branch from f9750a0 to a3b7ebd Compare July 19, 2024 06:32

nb-ohad requested changes Aug 6, 2024

View reviewed changes

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

rewantsoni force-pushed the onboarding-ticket branch from a3b7ebd to d9a8fbe Compare August 6, 2024 16:37

rewantsoni force-pushed the onboarding-ticket branch from d9a8fbe to 61456e7 Compare September 2, 2024 11:22

rewantsoni requested a review from nb-ohad September 2, 2024 11:24

nb-ohad requested changes Sep 8, 2024

View reviewed changes

rewantsoni force-pushed the onboarding-ticket branch 5 times, most recently from 0b57dbe to 1c07f07 Compare September 9, 2024 07:01

rewantsoni requested a review from nb-ohad September 9, 2024 07:04

rewantsoni force-pushed the onboarding-ticket branch 2 times, most recently from 3852dd1 to 8942fc3 Compare September 16, 2024 11:10

add new endpoint for onboarding peers

a73be38

The PR does the following: 1. add role to the onboarding ticket 2. add a new endpoint for peer-onboarding-tokens Signed-off-by: Rewant Soni <resoni@redhat.com>

rewantsoni force-pushed the onboarding-ticket branch from 8942fc3 to a73be38 Compare September 16, 2024 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

changes to onboarding token api to include role in body #2669

changes to onboarding token api to include role in body #2669

rewantsoni commented Jun 19, 2024 •

edited

Loading

rewantsoni commented Jun 19, 2024

rchikatw commented Jul 8, 2024

rewantsoni commented Jul 15, 2024

nb-ohad Aug 6, 2024

nb-ohad Aug 6, 2024

nb-ohad Aug 6, 2024

nb-ohad Aug 6, 2024

nb-ohad Aug 6, 2024

openshift-ci bot commented Aug 6, 2024

nb-ohad Sep 8, 2024

nb-ohad Sep 8, 2024

nb-ohad Sep 8, 2024

rewantsoni Sep 9, 2024

rewantsoni commented Sep 10, 2024

	Role Role `json:"role,string"`
	SubjectRole Role `json:"subjectRole,string"`

	// OnboardingSubjectRole can be either ocs-client or ocs-peer

	// OnboardingSubjectRole can be either ocs-client or ocs-peer

changes to onboarding token api to include role in body #2669

Are you sure you want to change the base?

changes to onboarding token api to include role in body #2669

Conversation

rewantsoni commented Jun 19, 2024 • edited Loading

rewantsoni commented Jun 19, 2024

rchikatw commented Jul 8, 2024

rewantsoni commented Jul 15, 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

openshift-ci bot commented Aug 6, 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

rewantsoni commented Sep 10, 2024

rewantsoni commented Jun 19, 2024 •

edited

Loading