Changes to onboarding token api to include quota in body #2600

rchikatw · 2024-05-08T08:26:06Z

Passing nil quota to function means unlimited quota
Quota will be sent as HTTP request body
Sample request: {"quota": "123Mi"} or no body

services/types.go

nb-ohad · 2024-05-08T11:03:46Z

@rchikatw Why use 0 or unlimited/unbound? why not have it a pointer and use nil instead?

nb-ohad · 2024-05-08T11:05:41Z

controllers/util/provider.go

@@ -19,14 +19,15 @@ import (

 // GenerateOnboardingToken generates a token valid for a duration of "tokenLifetimeInHours".
 // The token content is predefined and signed by the private key which'll be read from supplied "privateKeyPath".
-func GenerateOnboardingToken(tokenLifetimeInHours int, privateKeyPath string) (string, error) {
+func GenerateOnboardingToken(tokenLifetimeInHours int, privateKeyPath, quota string) (string, error) {


Quota shouldn't be a string, it should be wither a number (assuming a specific unit) or a size object.
Also it should preferably be optional (a pointer) so we can generate a token that does not have a quota

I have considered this as a string because in ClusterResourceQuota we also need to mention the unit. It will be better if we get that from the UI itself.

Even it that case this should not be a string but a k8s quantity object

Okay will do the changes according

nb-ohad · 2024-05-08T11:05:56Z

services/types.go

@@ -3,4 +3,5 @@ package services
 type OnboardingTicket struct {
 	ID             string `json:"id"`
 	ExpirationDate int64  `json:"expirationDate,string"`
+	Quota          string `json:"quota"`


Quota shouldn't be a string, it should be wither a number (assuming a specific unit) or a size object.
Also it should preferably be optional (a pointer) so we can generate a token that does not have a quota

nb-ohad · 2024-05-08T11:06:58Z

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

@@ -16,14 +16,15 @@ const (
 func HandleMessage(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours int) {
 	switch r.Method {
 	case "POST":
-		handlePost(w, tokenLifetimeInHours)
+		quota := r.URL.Query().Get("quota")


If this is a Post request the common practice is to use the request body to convey payload not the URL query line

Okay. i will consider that.

openshift-ci · 2024-05-14T10:30:44Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rchikatw
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

controllers/util/provider.go

leelavg · 2024-05-21T12:48:25Z

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

+		type Message struct {
+			Quota string `json:"quota"`
+		}
+		var quota Message


I don't think we should be processing the body before sending it to handlePost cause in the switch you are only deciding the method

No need to create a type if we are not going to initialize it, can get away w/ var. Unrelated one, for encoding & decoding you only need fields to be exported not necessarily the struct.

Suggested change

type Message struct {

Quota string `json:"quota"`

}

var quota Message

var quota = struct { Quota string `json:"quota"`}{}

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

leelavg · 2024-05-21T12:50:52Z

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

+		}
+		var quota Message
+		err := json.NewDecoder(r.Body).Decode(&quota)
+		if err != nil {


nit, can perform err handling in the same statement/line.

leelavg · 2024-05-27T02:52:23Z

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

+	// When length is 0 that means either request body is empty or
+	// quota parameter is not passed in that case quota is considerred
+	// to be unlimited


no, we already established that empty body represents unlimited quota and better not to club that w/ empty value for quota.

btw, as mentioned in the comment, I don't think quota is a param.

Yes, correct, we are only checking with the request body only. I missed updating this comment.

leelavg · 2024-05-27T02:54:39Z

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

 	default:
 		handleUnsupportedMethod(w, r)
 	}
 }

-func handlePost(w http.ResponseWriter, tokenLifetimeInHours int) {
-	if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath); err != nil {
+func handlePost(w http.ResponseWriter, tokenLifetimeInHours int, r *http.Request) {


could you pls swap 2nd & 3rd param?

in body Signed-off-by: rchikatw <rchikatw@redhat.com>

openshift-ci · 2024-05-27T07:24:26Z

@rchikatw: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ocs-operator-bundle-e2e-aws	`7c796cb`	link	true	`/test ocs-operator-bundle-e2e-aws`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

nb-ohad · 2024-05-27T14:57:21Z

controllers/util/provider.go

 	tokenExpirationDate := time.Now().
 		Add(time.Duration(tokenLifetimeInHours) * time.Hour).
 		Unix()

 	payload, err := json.Marshal(services.OnboardingTicket{
 		ID:             uuid.New().String(),
 		ExpirationDate: tokenExpirationDate,
+		StorageQuota:   quantity,


@rchikatw How does the marshaled form of quantity look like? Is it just a string containing value and unit? is it unmarshalabe?.

Also, why do we need a complex data type in the token? why can we just normalize the quantity to an int representing the number of MiB? This will be beneficial for users who want to read the token content in other languages (e.g. Bash, Phyton, etc.)

After marshal quantity will look like <size><Unit> ex: 1Gi. Is it unmarshalabe? -> Yes it.

Why do we need a complex type for quantity? In your previous comment you suggested to using k8s quantity Refer: #2600 (comment)

Advantages of using quantity:

we can validate quota unit is valid (using parseQuantity function) no need to add manual checks for the unit

You have also suggested using a pointer so we can generate a token that does not have a quota. Refer Changes to onboarding token api to include quota in body #2600 (comment)

If we don't want to use complex data type then we should not allow the user to select the unit in the UI and make quantity int that should solve our problem not using complex data type.

Let me know which approach should I use k8s quantity or int

nb-ohad · 2024-05-27T14:58:50Z

services/types.go

-	ExpirationDate int64  `json:"expirationDate,string"`
+	ID             string             `json:"id"`
+	ExpirationDate int64              `json:"expirationDate,string"`
+	StorageQuota   *resource.Quantity `json:"storageQuota,omitempty"`


Can we please keep the token fields as non-complex types? in this case I would expect an int

Suggested change

StorageQuota *resource.Quantity `json:"storageQuota,omitempty"`

StorageQuotaInMiB *int `json:"storageQuotaInMiB,omitempty"`

nb-ohad · 2024-05-27T14:59:52Z

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

-func handlePost(w http.ResponseWriter, tokenLifetimeInHours int) {
-	if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath); err != nil {
+func handlePost(w http.ResponseWriter, r *http.Request, tokenLifetimeInHours int) {
+	quantity, err := getQuantity(r)


Why Quantity? are we expecting the user to select both size and unit? or are we expecting a standardized unit?

Yes from the UI design slides user can select both size and unit.

nb-ohad · 2024-05-27T15:02:23Z

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

+func getQuantity(r *http.Request) (*resource.Quantity, error) {
+	var quota = struct {
+		StorageQuota string `json:"storageQuota"`
+	}{}
+	err := json.NewDecoder(r.Body).Decode(&quota)
+	if err != nil && err.Error() != "EOF" {
+		return nil, err
+	}
+
+	// When length is 0 that means request body is empty and quota
+	// is unlimited
+	if len(quota.StorageQuota) == 0 {
+		return nil, nil
+	}
+	quantity, err := resource.ParseQuantity(quota.StorageQuota)
+	if err != nil {
+		return nil, fmt.Errorf("invalid StorageQuota value sent in request body: %v", err)
+	}
+	return &quantity, nil
+
+}


I am not sure about the logic here. If the UI is passing us a quantity I would expect to see 2 fields in the body. One for the unit and one for the size. But if there is an assumption about the unit then there is no reason to parse it as a quantity, it should be just an int.

Anything else just add unnecessary complexity to the system

UI will be passed as a string in the body with the format 1Gi/1Mi/1Ti to indicate quantity. I will decode and parse it to validate the correctness of the passed value. Invalid values will look like 1/1gi/1ti.

Madhu-1 reviewed May 8, 2024

View reviewed changes

services/types.go Outdated Show resolved Hide resolved

nb-ohad requested changes May 8, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad May 8, 2024

rchikatw force-pushed the quotachanges branch from 57fa741 to b013a5f Compare May 14, 2024 10:30

rchikatw force-pushed the quotachanges branch 4 times, most recently from aabb7fd to 45e90bc Compare May 15, 2024 16:26

leelavg reviewed May 21, 2024

View reviewed changes

rchikatw force-pushed the quotachanges branch 3 times, most recently from 094ccac to f7803db Compare May 22, 2024 06:52

leelavg reviewed May 27, 2024

View reviewed changes

rchikatw force-pushed the quotachanges branch from f7803db to 887e830 Compare May 27, 2024 06:03

rchikatw changed the title ~~Changes to onboarding token api to add quota parameter~~ Changes to onboarding token api to include quota in body May 27, 2024

Changes to onboarding token api to include quota

7c796cb

in body Signed-off-by: rchikatw <rchikatw@redhat.com>

rchikatw force-pushed the quotachanges branch from 887e830 to 7c796cb Compare May 27, 2024 06:25

nb-ohad requested changes May 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes to onboarding token api to include quota in body #2600

Changes to onboarding token api to include quota in body #2600

rchikatw commented May 8, 2024 •

edited

nb-ohad commented May 8, 2024

nb-ohad May 8, 2024

rchikatw May 9, 2024

nb-ohad May 11, 2024

rchikatw May 13, 2024

nb-ohad May 8, 2024

nb-ohad May 8, 2024

rchikatw May 9, 2024

openshift-ci bot commented May 14, 2024

leelavg May 21, 2024

leelavg May 21, 2024

leelavg May 27, 2024

rchikatw May 27, 2024

leelavg May 27, 2024

rchikatw May 27, 2024

openshift-ci bot commented May 27, 2024

nb-ohad May 27, 2024

rchikatw May 28, 2024

nb-ohad May 27, 2024

nb-ohad May 27, 2024

rchikatw May 28, 2024

nb-ohad May 27, 2024

rchikatw May 28, 2024

	StorageQuota *resource.Quantity `json:"storageQuota,omitempty"`
	StorageQuotaInMiB *int `json:"storageQuotaInMiB,omitempty"`

Changes to onboarding token api to include quota in body #2600

Are you sure you want to change the base?

Changes to onboarding token api to include quota in body #2600

Conversation

rchikatw commented May 8, 2024 • edited

nb-ohad commented May 8, 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

openshift-ci bot commented May 14, 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

openshift-ci bot commented May 27, 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

rchikatw commented May 8, 2024 •

edited