-
Notifications
You must be signed in to change notification settings - Fork 170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create and populate OIDC blob store for the cluster #3564
Conversation
58d9129
to
1b8076a
Compare
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a full review, just a few questions as you're working through the draft and testing. looks great!
Please rebase pull request. |
/azp run e2e |
Pull request contains merge conflicts. |
1b8076a
to
52b785b
Compare
Please rebase pull request. |
52b785b
to
feb76f9
Compare
feb76f9
to
c145d49
Compare
d684aeb
to
a067322
Compare
ebe1600
to
85d7994
Compare
85d7994
to
66f608e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
ResourceGroupID string `json:"resourceGroupId,omitempty"` | ||
FipsValidatedModules FipsValidatedModules `json:"fipsValidatedModules,omitempty"` | ||
OIDCIssuer *OIDCIssuer `json:"oidcIssuer,omitempty"` | ||
BoundServiceAccountSigningKey *SecureString `json:"boundServiceAccountSigningKey,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we make the signing key a pointer to SecureString
, but we make pull secret a SecureString
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been discussed here: #3564 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm suggesting one change to ensure this code works in FairFax, and I left a few smaller comments/questions.
Edit: I think we're all good WRT the FairFax thing, so I just have some small questions and nits.
LGTM, but my recommendation is to wait until we have E2E available before merging because of the size and scope of the PR. |
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
As a note to other reviewers, we've decided to rely on a healthy signal from local e2e since we don't have PR e2e available yet. The CI e2e was passing when it was healthy, and we've confirmed it's still passing locally. |
/azp run ci,e2e |
Azure Pipelines successfully started running 2 pipeline(s). |
/azp run e2e |
Azure Pipelines successfully started running 1 pipeline(s). |
Which issue this PR addresses:
Jira issue :- ARO-4373
Related Docs:-
https://msazure.visualstudio.com/AzureRedHatOpenShift/_wiki/wikis/AzureRedHatOpenShift.wiki/603739/OIDC-Traffic-Flow
Similar Implementation:-
https://gitlab.cee.redhat.com/service/uhc-clusters-service/-/tree/master/pkg/aws/cloudcredentialbuilder
What this PR does / why we need it:
Test plan for issue:
Is there any documentation that needs to be updated for this PR?
How do you know this will function as expected in production?
Testing the implementation in all the environments.