Skip to content

Conversation

@joelddiaz
Copy link
Contributor

@joelddiaz joelddiaz commented Dec 20, 2021

Update docs to reflect removed support for Azure Mint mode

xref: https://issues.redhat.com/browse/CCO-173

@openshift-ci openshift-ci bot requested review from 2uasimojo and suhanime December 20, 2021 21:16
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2021
@joelddiaz
Copy link
Contributor Author

wait for #433 to merge
/hold
/assign @akhil-rane
/assign @lwan-wanglin @jeana-redhat @sferich888

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2021
@jeana-redhat
Copy link

LGTM! /label docs-approved

@jeana-redhat
Copy link

oops I think my approval might need to be on a new line?
/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Dec 20, 2021
Copy link
Contributor

@akhil-rane akhil-rane left a comment

Choose a reason for hiding this comment

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

minor nits


For a cluster previously installed/running in Mint mode, CCO will update existing Secrets containing the credentials of previously minted App Registrations/Service Principals with the contents of the Secret kube-system/azure-credentials (normally containing the credentials used during installation). It is required that the permissions associated with the credentials in this Secret be sufficient to be used by all in-cluster components needing to interact with Azure APIs.

CCO will also try to clean up previously minted App Registrations/Service Principals while the Azure AD Graph API is still functional. If the cluster is upgraded to a version of OpenShift that no longer supports Mint mode after the Azure AD Graph API is sunset, CCO will set a condition on the associated CredentialsRequest and will not treat the error as fatal. Cleanup after the Azure AD Graph API is sunset will require manual intervention to remove the App Registrations/Service Principals that are no longer necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to mention the name of the condition that CCO will set?

Choose a reason for hiding this comment

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

That would be nice esp. if it could help troubleshoot/manual clean up if needed (not sure if that's the case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the name of the condition and an example of the kind of message that would be associated with that condition.


## Future

Rather than re-implement support for Mint mode using the new [Microsoft Graph API](https://docs.microsoft.com/en-us/graph/sdks/create-requests?tabs=Go), the intention is to support Azure federated OpenID identities along with pod/workload identity as the prefered in-cluster credentials/authentication mode if/when Azure releases support for this feature.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Rather than re-implement support for Mint mode using the new [Microsoft Graph API](https://docs.microsoft.com/en-us/graph/sdks/create-requests?tabs=Go), the intention is to support Azure federated OpenID identities along with pod/workload identity as the prefered in-cluster credentials/authentication mode if/when Azure releases support for this feature.
Rather than re-implement support for Mint mode using the new [Microsoft Graph API](https://docs.microsoft.com/en-us/graph/sdks/create-requests?tabs=Go), the intention is to support Azure federated OpenID identities along with pod/workload identity as the preferred in-cluster credentials/authentication mode if/when Azure releases support for this feature.

@joelddiaz joelddiaz force-pushed the azure-passthrough-docs branch from 0979707 to 081cc80 Compare December 20, 2021 22:23
@codecov
Copy link

codecov bot commented Dec 20, 2021

Codecov Report

Merging #435 (0979707) into master (32b6237) will not change coverage.
The diff coverage is n/a.

❗ Current head 0979707 differs from pull request most recent head 78b398f. Consider uploading reports for the commit 78b398f to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #435   +/-   ##
=======================================
  Coverage   46.17%   46.17%           
=======================================
  Files          91       91           
  Lines        9236     9236           
=======================================
  Hits         4265     4265           
  Misses       4455     4455           
  Partials      516      516           

@joelddiaz
Copy link
Contributor Author

#433 merged
/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 20, 2021
@joelddiaz joelddiaz force-pushed the azure-passthrough-docs branch from 081cc80 to 5eebd6d Compare December 20, 2021 23:05
@lwan-wanglin
Copy link

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Dec 21, 2021
@joelddiaz joelddiaz force-pushed the azure-passthrough-docs branch from 5eebd6d to 78b398f Compare December 21, 2021 14:43
@akhil-rane
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akhil-rane, joelddiaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [akhil-rane,joelddiaz]

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

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

19 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@joelddiaz
Copy link
Contributor Author

/override e2e-azure-upgrade

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

@joelddiaz: /override requires a failed status context or a job name to operate on.
The following unknown contexts were given:

  • e2e-azure-upgrade

Only the following contexts were expected:

  • ci/prow/e2e-azure-upgrade
  • pull-ci-openshift-cloud-credential-operator-master-e2e-azure-upgrade
  • tide
Details

In response to this:

/override e2e-azure-upgrade

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.

@joelddiaz
Copy link
Contributor Author

/override ci/prow/e2e-azure-upgrade

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

@joelddiaz: Overrode contexts on behalf of joelddiaz: ci/prow/e2e-azure-upgrade

Details

In response to this:

/override ci/prow/e2e-azure-upgrade

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 22, 2021

@joelddiaz: all tests passed!

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@joelddiaz
Copy link
Contributor Author

/label px-approved
literally only a doc update for the things we already discussed that would change for 4.10

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Jan 11, 2022
@openshift-merge-robot openshift-merge-robot merged commit 05c9019 into openshift:master Jan 11, 2022
ming1013 pushed a commit to ming1013/cloud-credential-operator that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants