extend the onboarding struct to make it modular #2791
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rewantsoni The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rewantsoni
changed the title
extend the onboarding struct have clients field nested instead of on the root. This will provide us option to have onboarding ticket for different roles. Signed-off-by: Rewant Soni <[email protected]>
rewantsoni
force-pushed
the
onboarding-ticket-new
branch
from
9b02785
to
7ab0e86
Compare
|
/retest-required |
|
ci failure is due to noobaa stuck in connecting phase |
rewantsoni
requested review from
Madhu-1,
nb-ohad and
leelavg
and removed request for
Madhu-1
|
/retest-required |
|
/cherry-pick release-4.17 |
|
@rewantsoni: once the present PR merges, I will cherry-pick it on top of release-4.17 in a new PR and assign it to you. In response to this:
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. |
There was a problem hiding this comment.
to me the proposed API seems more opinionated than required, my assumptions are
- quota will always be >0 if set
- not all tickets will have quota
- tickets can be of two explicit types: no role, client and peer role
this PR is trying to club no role and client role, is that the expectation? this resulted in mandating a role for local client which could be no different than supplying an explicit nil.
I propose for each of roles to be distinct even though being verbose it doesn't force quota in normal case and accept pointer in func signature for backward compat? Think of some automation outside of odf generating tokens and doesn't want newer futures and current API forces to set client role (I understand you are setting the default but that has a different comment).
wdyt? pls do ping other reviewers before acting on the review if required. open to be corrected as always.
| // storage quota is unlimited | ||
|
|
||
| var roleSpec services.OnboardingSubjectRoleSpec | ||
| roleSpec.Role = services.ClientRole |
There was a problem hiding this comment.
setting fields at multiple places forces us to look for where all these are set, better to move this to line 59 only where you are setting the options?
There was a problem hiding this comment.
Since we have only one role up to this point, I am always setting the role as clientRole to avoid further changes to console. With 4.18, we will explicitly set the role while calling this endpoint
| } | ||
| if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, storageQuotaInGiB); err != nil { | ||
| if onboardingToken, err := util.GenerateOnboardingToken(tokenLifetimeInHours, onboardingPrivateKeyFilePath, roleSpec); err != nil { |
There was a problem hiding this comment.
if contentLength is 0 then you'll be setting empty roleSpec (after doing above change), is that still fine?
There was a problem hiding this comment.
If that is done, the ticket will be generated for an empty role if the body is empty, which will always lead to failure while onboarding since we cannot verify the role
| type OnboardingSubjectRole string | ||
|
|
||
| type OnboardingSubjectRoleSpec struct { | ||
| Role OnboardingSubjectRole `json:"role"` |
There was a problem hiding this comment.
you are trying to make role as mandatory but the original could be empty quota, thought of having OnboardingSubjectRoleSpec as a pointer in parent struct?
There was a problem hiding this comment.
original could be empty quota Could you elaborate on what you mean by original could be empty quota?
| if ticketData.SubjectRole.Role != expectedRole { | ||
| return nil, fmt.Errorf("unsupported role sent in onboarding ticket %s, expectedRole is %s", ticketData.SubjectRole.Role, expectedRole) | ||
| } |
There was a problem hiding this comment.
isn't this repeating the default case of below switch?
There was a problem hiding this comment.
Yes, I am expecting this function to be called while onboarding (onboarding clients and onboarding peers). This check is required so that we don't enter the peer role ticket while onboarding the client and vice-versa.
I could remove the default case in the switch as the code would never reach it.
In the Onboarding struct
I think If we have an external automation generating tokens, we should have them specify the role of the ticket they want to generate (for client or peer) just like they specify the quota they need. Or, for backward compatibility of such automation, we can always set the default role to ocs-client. |
|
|
closing it as not required |
Extend the onboarding struct have clients field nested instead of on the root. This will provide us the option to have onboarding ticket for different roles.