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.

Sign up for GitHub

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

Jump to bottom

Azure Workload Identity #1301

Merged
merged 24 commits into from
May 1, 2023
Merged

Azure Workload Identity #1301

merged 24 commits into from
May 1, 2023

Conversation

abutcher
Copy link
Member

@abutcher abutcher commented Dec 8, 2022

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2022
@openshift-ci openshift-ci bot requested review from deads2k and markmc December 8, 2022 19:37
@abutcher
Copy link
Member Author

abutcher commented Dec 8, 2022

cc @2uasimojo

2uasimojo
2uasimojo reviewed Dec 8, 2022
View reviewed changes
Copy link
Member

@2uasimojo 2uasimojo left a comment

Choose a reason for hiding this comment

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

As usual, I'm mostly good for pointing out typos :P

abutcher and others added 4 commits December 9, 2022 10:11
@abutcher @2uasimojo
Correct typo.
5900dc4 
Co-authored-by: Eric Fried <2uasimojo@users.noreply.github.com>
@abutcher @2uasimojo
Correct typo.
91404c3 
Co-authored-by: Eric Fried <2uasimojo@users.noreply.github.com>
@abutcher @2uasimojo
Correct typo.
994c73f 
Co-authored-by: Eric Fried <2uasimojo@users.noreply.github.com>
@abutcher @2uasimojo
Correct typo.
556eeda 
Co-authored-by: Eric Fried <2uasimojo@users.noreply.github.com>
This was referenced Dec 9, 2022
Merged
Merged
@abutcher abutcher changed the title WIP: Azure Workload Identity Azure Workload Identity Dec 12, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 12, 2022
@abutcher
Copy link
Member Author

abutcher commented Dec 12, 2022 •
edited
Loading

  • Added who I think will be reviewers but still need to figure out who can be approvers.
  • Addressed some comments.

@abutcher @2uasimojo
Fix lint, quote reviewers.
6879ed8 
Co-authored-by: Eric Fried <2uasimojo@users.noreply.github.com>
@abutcher
Copy link
Member Author

After meeting with MSFT there are some aspects of managed cluster identities that we'll need to consider to make sure that this proposal fits for managed/standalone. In particular there could be a chicken and egg problem where we need to scope the permissions granted to the identity within boundaries that don't exist yet or within the network where we couldn't simply scope the identity to the subscription in a managed setting. Will hold off on pinging reviewers based on this feedback while we research this.

@bharath-b-rh
Copy link
Contributor

There is an enhancement to support tagging of azure resources created by Openshift and identities created as part of this enhancement may need to support tags. Can ccoctl be enhanced to add tags to the created azure resources?
Please refer comment

@abutcher
Copy link
Member Author

@bharath-b-rh These identity resources will be created by ccoctl prior to installation and we will not know the clusterID at creation time.

@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 Feb 10, 2023
@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 Feb 18, 2023
jharrington22
jharrington22 reviewed Feb 22, 2023
View reviewed changes
enhancements/cloud-integration/azure/azure-workload-identity.md
Comment on lines +127 to +128
The resource group in which the installer will create infrastructure will not be known when these secrets are generated by `ccoctl` ahead of installation and operators which rely on `azure_resourcegroup` and `azure_resource_prefix` such as the
[image-registry](https://github.com/openshift/cluster-image-registry-operator/blob/8556fd48027f89e19daad36e280b60eb93d012d4/pkg/storage/azure/azure.go#L95-L100) should obtain the resource group details from the cluster `Infrastructure` object instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

How are OpenShift operators with Azure SDKs consuming the credentials today? I assume the operator is configuring the SDK client based on the contents of the secret vs the SDK reading the secret as a configuration? I assume its the former?

enhancements/cloud-integration/azure/azure-workload-identity.md Outdated
Comment on lines 144 to 146
In order to create Azure clients which utilize a `ClientAssertionCredential`, operators must update to version `>= v1.2.0` of the azidentity package within [azure-sdk-for-go](https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azidentity@v1.2.0). Ahead of this work, due to the [end of life
announcement](https://techcommunity.microsoft.com/t5/microsoft-entra-azure-ad-blog/microsoft-entra-change-announcements-september-2022-train/ba-p/2967454) of the Azure Active Directory Authentication Library (ADAL), PRs (ex. [openshift/cluster-ingress-operator](https://github.com/openshift/cluster-ingress-operator/pull/846)) have been opened for operators to migrate to creating clients via
azidentity which are converted into an authorizer for use with v1 clients. Once these changes have been made, we propose that OpenShift operators continue to utilize a config secret to obtain authentication details as described in the previous section but create workload identity clients when the `azure_client_secret` is absent and when `azure_federated_token_file` fields are found in the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this answers my question above on how operators configure credentials. They are configuring the SDK by reading the secret.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, today we have service principal details in the secret and operators (rather than the SDK) read that secret and configure the SDK.

@abutcher abutcher mentioned this pull request Mar 28, 2023
Merged
This was referenced Mar 28, 2023
Merged
Merged
abutcher added 3 commits March 29, 2023 12:47
@abutcher
Remove reference to installer re: operator changes.
45e9257
@abutcher
Address comments.
c15f2a1 
* Add note about using the TechPreviewNoUpgrade feature gate.
* Fill out support scenarios.
@abutcher
Fix indentation lint.
b318c51
@abutcher abutcher mentioned this pull request Apr 6, 2023
Merged
@dlom dlom mentioned this pull request Apr 24, 2023
Merged
deads2k
deads2k reviewed Apr 27, 2023
View reviewed changes
enhancements/cloud-integration/azure/azure-workload-identity.md

##### How to detect that operator credentials are incorrect / insufficient?

Operators will be degraded when credentials are insufficient / incorrect because operators will be unable to authenticate using the provided credentials or the permissions granted to the associated identity were insufficient. CCO will not monitor the state of the credentials on-cluster because CCO will be disabled based on clusters operating in `manual` credentials mode.
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add commitment to test this. I don't think that all operators do this automatically.

Copy link
Member

@sdodson sdodson Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

https://issues.redhat.com/browse/CCO-370

As an openshift developer I need to know that my operator goes degraded if the Azure Managed Identity is insufficient or incorrect so that my operator is admin friendly in clusters that leverage Azure Managed Identity.

Acceptance criteria:

Serial test that picks a random operator of those identified and updates it to use an invalid role reference, verify the operator goes Degraded in a timely manner. Revert, confirm return to Degraded=False
Serial test that picks a random operator of those identified and updates it to use a role with insufficient permissions (null or otherwise). Revert, confirm return to Degraded=False

@deads2k sound good?

@sdodson sdodson force-pushed the azure-workload-identity branch from dfb719c to 1de46ee Compare April 28, 2023 17:30
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2023

@abutcher: all tests passed!

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

@sdodson
Copy link
Member

sdodson commented Apr 28, 2023

/lgtm
/approve
/hold
to give @jharrington22 a bit to ack

@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 Apr 28, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2023
@jharrington22
Copy link
Contributor

@sdodson LGTM

@sdodson
Copy link
Member

sdodson commented May 1, 2023

/hold cancel

@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 May 1, 2023
@openshift-merge-robot openshift-merge-robot merged commit 8cbeb7e into openshift:master May 1, 2023
@abutcher abutcher deleted the azure-workload-identity branch May 3, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdodson sdodson sdodson left review comments

@dmage dmage dmage left review comments

@jharrington22 jharrington22 jharrington22 left review comments

@patrickdillon patrickdillon patrickdillon left review comments

@deads2k deads2k deads2k left review comments

@2uasimojo 2uasimojo 2uasimojo left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@RomanBednar RomanBednar RomanBednar left review comments

@jstuever jstuever jstuever left review comments

@markmc markmc Awaiting requested review from markmc

Assignees

@sdodson sdodson

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.