-
Notifications
You must be signed in to change notification settings - Fork 431
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
Add support for Azure authentication in ASO #3698
Conversation
Hi @adriananeci. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
/ok-to-test |
/retest |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3698 +/- ##
==========================================
+ Coverage 54.74% 54.83% +0.09%
==========================================
Files 187 188 +1
Lines 19051 19331 +280
==========================================
+ Hits 10429 10600 +171
- Misses 8057 8150 +93
- Partials 565 581 +16
☔ View full report in Codecov by Sentry. |
Adding WIP while we're not ready to merge this yet: |
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.
Overall approach looks good to me.
For the other ASO PRs, I've been splitting up those into non-functional changes (w.r.t. the current behavior) that get merged and functional changes (to be included in a PR resolving #3527) that I've been managing in my fork. The new controller and creation of ASO secrets here don't seem like they would break current behavior though so we may not need to split anything out here.
@CecileRobertMichon Do we have existing e2e coverage for all these different ways of authenticating to Azure?
(reminder to squash once we get lgtms) /hold And @adriananeci feel free to remove the [WIP] from the title once you're finished and let me know so I can give this another look. |
/retest |
@nojnhuh I think this is ready for another round reviews now |
controllers/asosecret_controller.go
Outdated
APIVersion: gvk.GroupVersion().String(), | ||
Kind: gvk.Kind, | ||
Name: asoSecretOwner.GetName(), | ||
UID: asoSecretOwner.GetUID(), |
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 think it seems fair to say this is controlling the secret?
UID: asoSecretOwner.GetUID(), | |
UID: asoSecretOwner.GetUID(), | |
Controller: pointer.Bool(true), |
@CecileRobertMichon Is there a reason the AzureJSON controllers don't set this? It seems like that's necessary for Owns()
to work:
Owns(&corev1.Secret{}). |
// Owns defines types of Objects being generated by the ControllerManagedBy, and configures the ControllerManagedBy to respond to
// create / delete / update events by reconciling the owner object. This is the equivalent of calling
// Watches(&source.Kind{Type: }, &handler.EnqueueRequestForOwner{OwnerType: apiType, IsController: true}).
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.
Don't set which part?
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.
The controller: true
in the ownerRef on the generated Secret.
1767f39
to
da7d444
Compare
da7d444
to
27e7a83
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.
I could've sworn I posted these comments the other day, but I see that GitHub still shows (most of) them as "pending." I think I've seen some other weird things with review comments lately too so maybe I'm not as crazy as I think...
controllers/asosecret_controller.go
Outdated
APIVersion: gvk.GroupVersion().String(), | ||
Kind: gvk.Kind, | ||
Name: asoSecretOwner.GetName(), | ||
UID: asoSecretOwner.GetUID(), |
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.
The controller: true
in the ownerRef on the generated Secret.
4c64981
to
d2f23d8
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
/assign @CecileRobertMichon
e334e2d
to
9d8e6c8
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
LGTM label has been added. Git tree hash: 80f6232c5213b9662b6cd5f9a37373628b2f499e
|
/assign @CecileRobertMichon |
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
/approve
Great work @adriananeci 🎉
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind feature
What this PR does / why we need it:
More context is described in the ASO proposal: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/main/docs/proposals/20230123-azure-service-operator.md#security-model
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #3526
Special notes for your reviewer:
Because we can have multiple
AzureClusterIdentity
objects created inside the same namespace, we cannot make use of https://azure.github.io/azure-service-operator/guide/authentication/credential-scope/#namespace-scope so we'll have to reside only on global scope or resource scopeHaven't added any tests yet. I'll add those after the initial review since I'm looking for some early feedback to make sure I'm on the right path.
TODOs:
Release note: