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

feat: Support workload identity #1193

Merged
merged 1 commit into from Apr 25, 2023
Merged

feat: Support workload identity #1193

merged 1 commit into from Apr 25, 2023

Conversation

cvvz
Copy link
Member

@cvvz cvvz commented Feb 16, 2023 •
edited

What type of PR is this?
/kind feature

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1138

Requirements:

Special notes for your reviewer:

Release note:

none

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 16, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @cvvz. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 16, 2023
@cvvz cvvz changed the title Support workload identity feat: Support workload identity Feb 16, 2023
@cvvz
Copy link
Member Author

cvvz commented Feb 16, 2023

workload identity failed to inject to csi driver when csi driver was deployed in kube-system namespace. @karataliu is confirming.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 18, 2023
@cvvz cvvz mentioned this pull request Feb 23, 2023
Closed
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 23, 2023
@cvvz cvvz mentioned this pull request Feb 23, 2023
Closed
@andyzhangx
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 23, 2023
@andyzhangx
Copy link
Member

cc @RomanBednar for this PR, let me know if you have any concern.

@andyzhangx
Copy link
Member

also we need to check how to enable workload identity on capz, there is a draft PR to support workload identity on CAPZ: kubernetes-sigs/cluster-api-provider-azure#2814

@RomanBednar
Copy link
Contributor

cc @RomanBednar for this PR, let me know if you have any concern.

Thank you @andyzhangx and @cvvz for the PR. The proposed changes look great for the purposes of our storage operator I believe. We don't use helm to deploy the driver but we can implement something similar.

@RomanBednar
Copy link
Contributor

@cvvz I gave it a bit more thought and believe it would be much cleaner for deployments and enabling/disabling workload identity authentication feature if the auth lib could also decide about the method used based on cloud config. Keeping the env variables for the sake of webhook is still ok - we could have both.

It makes sense for us in OpenShift enhancement but I'd like you to weight in on that. Here's the enhancement discussion: openshift/enhancements#1301 (comment)

@cvvz
Copy link
Member Author

cvvz commented Mar 16, 2023 •
edited

@RomanBednar It seems good to me to support federated workload identity in cloud auth config, which is a global configuration can effect both cloud-provider-azure and azure csi driver. And we should still keep using the env variables and webhook in csi driver, since some users may want to configure workload identity separately.

@cvvz
Copy link
Member Author

cvvz commented Apr 4, 2023

Hi, @RomanBednar I just checked again, and I think we already support workload identity based on cloud config in this PR.

You can set aadClientId, tenantId, aadFederatedTokenFile, and useFederatedWorkloadIdentityExtension in auth config file, which will eventually be parsed as AzureAuthConfig, and don't set any env, so the configuration won't be overwrited.

@RomanBednar
Copy link
Contributor

You can set aadClientId, tenantId, aadFederatedTokenFile, and useFederatedWorkloadIdentityExtension in auth config file

Ah I see, initially I misunderstood the config parsing, thanks for pointing that out! @cvvz

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 24, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2023
@cvvz cvvz force-pushed the support-workload-identity branch from 0d3506b to 9c903ce Compare April 24, 2023 00:38
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2023
@cvvz
Copy link
Member Author

cvvz commented Apr 24, 2023

/retest

andyzhangx
andyzhangx approved these changes Apr 24, 2023
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

pls squash all commits, thanks.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2023
@cvvz
Squashed commit of the following:
22f111a 
commit f8a9cdf
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 04:11:22 2023 +0000

    fix helm

commit 9f94bad
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 04:01:46 2023 +0000

    fix yaml lint

commit 46c8cab
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 03:47:07 2023 +0000

    fix go mod

commit 4d55075
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 02:53:00 2023 +0000

    add readme

commit 126a8c2
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 02:46:40 2023 +0000

    use go 1.20

commit 3ad423d
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 02:31:37 2023 +0000

    fix e2e: add context

commit 9c903ce
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 00:35:41 2023 +0000

    fix vendor

commit bdb8d8a
Author: weizhichen <weizhichen@microsoft.com>
Date:   Mon Apr 24 00:26:43 2023 +0000

    update vendor

commit 97492b4
Author: weizhichen <weizhichen@microsoft.com>
Date:   Tue Apr 4 07:20:53 2023 +0000

    upgrade adal

commit ba33edf
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 1 23:18:43 2023 +0000

    fix docs

commit 2a478f5
Author: weizhichen <weizhichen@microsoft.com>
Date:   Wed Mar 1 01:24:18 2023 +0000

    fix

commit 6571797
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Feb 24 07:50:40 2023 +0000

    fix

commit 4e0f80c
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Feb 24 07:49:10 2023 +0000

    fix

commit 56f9d20
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Feb 24 06:53:18 2023 +0000

    fix docs

commit c1a111c
Author: weizhichen <weizhichen@microsoft.com>
Date:   Fri Feb 24 06:43:35 2023 +0000

    fix

commit 4c82dda
Author: weizhichen <weizhichen@microsoft.com>
Date:   Thu Feb 23 08:03:01 2023 +0000

    Squashed commit of the following:

    commit e26eae4
    Merge: 020a6c3 141ab79
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 23 07:59:46 2023 +0000

        Merge branch 'master' of https://github.com/kubernetes-sigs/azurefile-csi-driver into support-workload-identity

    commit 020a6c3
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 23 07:58:28 2023 +0000

        fix

    commit e23c004
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 23 07:49:26 2023 +0000

        fix

    commit 6336c4e
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 23 07:42:15 2023 +0000

        add docs

    commit 7e84f91
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Wed Feb 22 08:15:00 2023 +0000

        fix

    commit 6a866db
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Tue Feb 21 11:15:22 2023 +0000

        fix

    commit f7caea6
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Tue Feb 21 08:40:31 2023 +0000

        fix

    commit bccdb92
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Tue Feb 21 08:22:43 2023 +0000

        fix

    commit 3f99c86
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Tue Feb 21 05:03:17 2023 +0000

        fix

    commit d2663f3
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Tue Feb 21 04:42:51 2023 +0000

        fix

    commit ca11365
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 16 10:54:38 2023 +0000

        fix

    commit 0ef4233
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 16 09:59:50 2023 +0000

        support workload identity

    commit b06461d
    Author: weizhichen <weizhichen@microsoft.com>
    Date:   Thu Feb 16 03:25:40 2023 +0000

        chore: update cloud-provider
@cvvz cvvz force-pushed the support-workload-identity branch from f8a9cdf to 22f111a Compare April 24, 2023 11:16
@cvvz
Copy link
Member Author

cvvz commented Apr 24, 2023

/retest

@andyzhangx
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, cvvz

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

@k8s-ci-robot k8s-ci-robot merged commit 3972ae7 into kubernetes-sigs:master Apr 25, 2023
27 checks passed
@RomanBednar RomanBednar mentioned this pull request May 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abutcher abutcher abutcher left review comments

@primeroz primeroz primeroz left review comments

@andyzhangx andyzhangx andyzhangx approved these changes

@gnufied gnufied Awaiting requested review from gnufied

Assignees

@andyzhangx andyzhangx

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Workload Identity on OpenShift cluster
6 participants
@cvvz @k8s-ci-robot @andyzhangx @RomanBednar @abutcher @primeroz