-
Notifications
You must be signed in to change notification settings - Fork 474
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
Enable Authenticating with Azure with Certificates using Azure SDK for Go's Generic NewDefaultAzureCredential #1694
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
52974c9
to
3b9cdb4
Compare
Signed-off-by: Bryan Cox <[email protected]>
3b9cdb4
to
ed18676
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.
Also should capture AZURE_CLIENT_SEND_CERTIFICATE_CHAIN
but you said there's be followup.
Otherwise lgtm
Signed-off-by: Bryan Cox <[email protected]>
Co-authored-by: Ben Vesel <[email protected]>
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.
Left a question about the image registry (operand). Looks good otherwise!
HyperShift would pass the following environment variables - AZURE_CLIENT_ID, AZURE_TENANT_ID, and | ||
AZURE_CLIENT_CERTIFICATE_PATH - to its deployments of image registry, ingress, cloud network config, and storage | ||
operators (azure-file and azure-disk) on the hosted control plane. Each of these components would then pass these | ||
variables along to NewDefaultAzureCredential. |
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.
When configured with workload identity, the image registry operator will configure the image registry in a similar fashion to what's described in this EP (source).
Do we need to consider this when changing the operator code to use these env vars? In other words, do we also want the image registry (operand!) to authenticate using this proposed mechanism?
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.
Yeah probably so. On ARO HCP, on the HCP side image registry will authenticate with the cert method, while the operand would use workload identity.
Signed-off-by: Bryan Cox <[email protected]>
Signed-off-by: Bryan Cox <[email protected]>
3ba8d36
to
667e461
Compare
Signed-off-by: Bryan Cox <[email protected]>
@bryan-cox: The following test failed, say
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. |
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
This enhancement proposes enabling image registry, ingress, cloud network config, and storage operators(azure-file and azure-disk) to accept authenticating with Azure with certificates using Azure SDK for Go's generic function NewDefaultAzureCredential.