Skip to content
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

ARO HCP MSI Enablement Enhancement #1659

Closed
wants to merge 3 commits into from

Conversation

bryan-cox
Copy link
Member

@bryan-cox bryan-cox commented Aug 2, 2024

This enhancement proposes introducing an environment variable in the image registry, ingress, cloud network config,
and storage operators. This variable would allow overriding the Azure authentication strategy used by these operators to
leverage Azure managed service identity (MSI), regardless of the underlying cloud configuration.

In Azure Red Hat OpenShift (ARO) Hosted Control Plane (HCP), operators running in the control plane need to
authenticate using Azure managed service identities to communicate with cloud services.

@bryan-cox bryan-cox force-pushed the OCPSTRAT-979 branch 3 times, most recently from 553fe13 to 8bf76be Compare August 2, 2024 18:02
@flavianmissi
Copy link
Member

LGTM from the image registry side.

This EP doesn't cover implementation. For the record, I left an overview of my investigation for MSI support in the registry in IR-460

@bryan-cox
Copy link
Member Author

@flavianmissi - I think there would be minor changes to image registry, please see this discussion

bryan-cox added a commit to bryan-cox/cluster-ingress-operator that referenced this pull request Aug 5, 2024
For ARO HCP, we need to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
bryan-cox added a commit to bryan-cox/cloud-network-config-controller that referenced this pull request Aug 5, 2024
For ARO HCP, we be able to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
@jsafrane
Copy link
Contributor

jsafrane commented Aug 6, 2024

The overall idea LGTM from storage side. Passing env. var from control-plane-operator to cluster-storage-operator and from cluster-storage-operator to azure-disk / azure-file-csi-driver-operator is straightforward.

It gets quite blurry in the azure-*-csi-driver-operator - it needs to generate the right secret + env vars + whatnot for the driver and we don't have much experience and test env. there. We (storage) expect hypershift to do that.

@kyrtapz
Copy link
Contributor

kyrtapz commented Aug 6, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2024
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2024
Copy link
Contributor

openshift-ci bot commented Aug 6, 2024

New changes are detected. LGTM label has been removed.

@Miciah
Copy link
Contributor

Miciah commented Aug 6, 2024

The proposal LGTM from the Network Edge (ingress) side.

bryan-cox added a commit to bryan-cox/cloud-network-config-controller that referenced this pull request Aug 6, 2024
For ARO HCP, we be able to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
bryan-cox added a commit to bryan-cox/cluster-network-operator that referenced this pull request Aug 7, 2024
For ARO HCP, we need to be able to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
bryan-cox added a commit to bryan-cox/cloud-network-config-controller that referenced this pull request Aug 7, 2024
For ARO HCP, we be able to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
bryan-cox added a commit to bryan-cox/cluster-network-operator that referenced this pull request Aug 7, 2024
For ARO HCP, we need to be able to override the authentication type to be MSI. For more information please see openshift/enhancements#1659.
Copy link

@bennerv bennerv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we missing MSI support for other HCP management components?

  • KMS pod, capi provider, etc?

Comment on lines +73 to +75
### API Extensions

N/A
Copy link
Contributor

@jsafrane jsafrane Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not correspond to https://github.com/openshift/hypershift/pull/4484/files#diff-4189d32544d44da5947597ddd941129f67cf3ca329f8fa8abf5da86490ece944R1616
How are operators supposed to read StorageMSIClientID? That looks like an API extension to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will pass in the client ID through an env var to the operator deployments.

@openshift-bot
Copy link

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 /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 6, 2024
@openshift-bot
Copy link

Stale enhancement proposals rot after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Rotten proposals close after an additional 7d of inactivity.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 13, 2024
@bryan-cox
Copy link
Member Author

bryan-cox commented Sep 13, 2024

We received the sidecar containers last week and have started integrating them.

I updated the enhancement to rename the override variable. Originally, we thought we did not need to set a client ID when creating a new managed identity credential due to the code documentation here - https://github.com/Azure/azure-sdk-for-go/blob/bd891cb0615f6148f9884be97bff7a3e2598bcc6/sdk/azidentity/managed_identity_credential.go#L128. This is not the case and a valid client ID will need to be passed to the operators.

@bryan-cox
Copy link
Member Author

/remove-lifecycle rotten.

@bryan-cox
Copy link
Member Author

Are we missing MSI support for other HCP management components?

  • KMS pod, capi provider, etc?

We (HyperShift) fully control the deployments for CPO, CAPZ, KMS, and Azure CCM so we don't need to pass an env var to those deployments like we do with the other operators mentioned in the enhancement.

@openshift-bot
Copy link

Rotten enhancement proposals close after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

Copy link
Contributor

openshift-ci bot commented Sep 21, 2024

@openshift-bot: Closed this PR.

In response to this:

Rotten enhancement proposals close after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

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.

@openshift-ci openshift-ci bot closed this Sep 21, 2024
@bryan-cox
Copy link
Member Author

/reopen

@openshift-ci openshift-ci bot reopened this Sep 24, 2024
Copy link
Contributor

openshift-ci bot commented Sep 24, 2024

@bryan-cox: Reopened this PR.

In response to this:

/reopen

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.

@openshift-bot
Copy link

Rotten enhancement proposals close after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

@openshift-ci openshift-ci bot closed this Oct 2, 2024
Copy link
Contributor

openshift-ci bot commented Oct 2, 2024

@openshift-bot: Closed this PR.

In response to this:

Rotten enhancement proposals close after 7d of inactivity.

See https://github.com/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

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.

@bryan-cox
Copy link
Member Author

/reopen

Copy link
Contributor

openshift-ci bot commented Oct 7, 2024

@bryan-cox: Reopened this PR.

In response to this:

/reopen

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.

@openshift-ci openshift-ci bot reopened this Oct 7, 2024
Originally, we thought we did not need to set a client ID when creating a new managed identity credential due to the code documentation here - https://github.com/Azure/azure-sdk-for-go/blob/bd891cb0615f6148f9884be97bff7a3e2598bcc6/sdk/azidentity/managed_identity_credential.go#L128. This is not the case and a valid client ID will need to be passed to the operators.

Signed-off-by: Bryan Cox <[email protected]>
@bryan-cox
Copy link
Member Author

/close

Superseded by #1694

Copy link
Contributor

openshift-ci bot commented Oct 7, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign runcom for approval. 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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot closed this Oct 7, 2024
Copy link
Contributor

openshift-ci bot commented Oct 7, 2024

@bryan-cox: Closed this PR.

In response to this:

/close

Superseded by #1694

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants