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

feat(oci): Add support for Azure Workload Identity #450

Merged
merged 2 commits into from
Mar 22, 2023

Conversation

weisdd
Copy link
Contributor

@weisdd weisdd commented Jan 17, 2023

As I pointed out here, we can add support for Azure Workload Identity to several controllers that rely on the oci package by just bumping the azidentity library to v1.3.0-beta.2. That is possible because the oci package itself relies on NewDefaultAzureCredential in case credentials are not passed explicitly while the updated function offers support for Workload Identity:

// Use default credentials if no token credential is provided.
// NOTE: NewDefaultAzureCredential() performs a lot of environment lookup
// for creating default token credential. Load it only when it's needed.
if c.credential == nil {
cred, err := azidentity.NewDefaultAzureCredential(nil)

The question is whether FluxCD team would accept the usage of v1.3.0-beta.2 (which is very likely to land to v1.3.0 as is) or would rather prefer to wait for several months till the Workload Identity AKS extension turns GA (as explained below, azidentity will have v1.3.0 tag only after the extension is considered GA).

This PR is meant to either have the change accepted or, if not, explicitly indicate the fact that the project is waiting for v1.3.0.

Details

Logs source-controller-57774ccfc5-64bvd manager {"level":"info","ts":"2023-01-14T23:33:34.841Z","msg":"logging in to Azure ACR for fluxcdtestacr123.azurecr.io/charts","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","HelmRepository":{"name":"charts","namespace":"flux-system"},"namespace":"flux-system","name":"charts","reconcileID":"8c61bb40-7a54-40a4-b7b0-e3a1fbe9b875"} source-controller-57774ccfc5-64bvd manager {"level":"info","ts":"2023-01-14T23:33:35.400Z","msg":"Helm repository is ready","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","HelmRepository":{"name":"charts","namespace":"flux-system"},"namespace":"flux-system","name":"charts","reconcileID":"8c61bb40-7a54-40a4-b7b0-e3a1fbe9b875"} source-controller-57774ccfc5-64bvd manager {"level":"info","ts":"2023-01-14T23:38:35.444Z","msg":"logging in to Azure ACR for fluxcdtestacr123.azurecr.io/charts","controller":"helmrepository","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmRepository","HelmRepository":{"name":"charts","namespace":"flux-system"},"namespace":"flux-system","name":"charts","reconcileID":"bb59958b-0c44-4b35-871f-f7679c37d097"} source-controller-57774ccfc5-64bvd manager {"level":"info","ts":"2023-01-14T23:39:01.314Z","msg":"logging in to Azure ACR for fluxcdtestacr123.azurecr.io/charts","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","HelmChart":{"name":"default-podinfo","namespace":"flux-system"},"namespace":"flux-system","name":"default-podinfo","reconcileID":"a0faa635-370a-4ad8-bb37-e60e16f3607c"} source-controller-57774ccfc5-64bvd manager {"level":"info","ts":"2023-01-14T23:39:02.021Z","msg":"pulled 'base' chart with version '0.3.2'","controller":"helmchart","controllerGroup":"source.toolkit.fluxcd.io","controllerKind":"HelmChart","HelmChart":{"name":"default-podinfo","namespace":"flux-system"},"namespace":"flux-system","name":"default-podinfo","reconcileID":"a0faa635-370a-4ad8-bb37-e60e16f3607c"}

@weisdd
Copy link
Contributor Author

weisdd commented Jan 17, 2023

@stefanprodan I see that the test job failed complaining about diff in oci/tests/integration/go.mod (https://github.com/fluxcd/pkg/actions/runs/3937973714/jobs/6740711323#step:5:1), I've just run make all and pushed the updated go.sum and go.mod for the integration test. I guess all checks will pass this time.

@weisdd
Copy link
Contributor Author

weisdd commented Jan 21, 2023

@phillebaba might be something of your interest since we had a brief discussion on workload identity for external-dns and cert-manager.

@stefanprodan stefanprodan added the area/oci OCI related issues and pull requests label Feb 7, 2023
@stefanprodan
Copy link
Member

@weisdd can you please rebase your PR with the main branch.

@weisdd weisdd force-pushed the feat/azure-oci-workload-identity branch from 3c45a34 to 051443a Compare February 8, 2023 22:00
@weisdd
Copy link
Contributor Author

weisdd commented Feb 8, 2023

@stefanprodan done

@weisdd weisdd force-pushed the feat/azure-oci-workload-identity branch from 051443a to b0760ad Compare February 14, 2023 15:07
@weisdd
Copy link
Contributor Author

weisdd commented Feb 14, 2023

@stefanprodan I've just rebased my branch again to resolve another round of merge conflicts (azidentity got recently updated in main).

@hiddeco
Copy link
Member

hiddeco commented Feb 15, 2023

Will ensure this gets in after #472, and before we tag the new package version.

@hiddeco hiddeco self-assigned this Feb 15, 2023
@hiddeco hiddeco force-pushed the feat/azure-oci-workload-identity branch from b0760ad to 016294a Compare February 16, 2023 09:21
@stefanprodan
Copy link
Member

@hiddeco I think we need to do the same in kustomize-controller for SOPS decryption to work with workload identity.

@hiddeco
Copy link
Member

hiddeco commented Feb 16, 2023

@stefanprodan please fill an issue for this in kustomize-controller as it would require changing some additional logic as well I think (https://github.com/fluxcd/kustomize-controller/blob/main/internal/sops/azkv/config.go#L67), and I won't be able to make time for it this week.

@hiddeco
Copy link
Member

hiddeco commented Feb 16, 2023

Need to test if the bump of azidentity causes issues in other places where we e.g. make use of another Azure API subset.

@stefanprodan
Copy link
Member

@hiddeco I think we should delay this further, I don't feel comfortable with shipping this in Flux v0.40.0 without any E2E tests.

@hiddeco
Copy link
Member

hiddeco commented Feb 16, 2023

Matches my thinking as I am concerned we would have issues with https://github.com/fluxcd/source-controller/blob/main/pkg/azure/blob.go#L87 as well.

@hiddeco hiddeco added hold Issues and pull requests put on hold enhancement New feature or request labels Feb 16, 2023
@stefanprodan stefanprodan changed the title feat(oci): bump azidentity to v1.3.0-beta.2 to support Azure Workload Identity feat(oci): Add support for Azure Workload Identity Feb 16, 2023
To add support for Workload Identities.

Signed-off-by: Igor Beliakov <demtis.register@gmail.com>
@hiddeco hiddeco force-pushed the feat/azure-oci-workload-identity branch from 1186e0d to 99152a0 Compare March 22, 2023 09:28
@hiddeco hiddeco requested a review from stefanprodan as a code owner March 22, 2023 09:28
- github.com/aws/aws-sdk-go-v2 to v1.17.7
- github.com/aws/aws-sdk-go-v2/config to v1.18.19
- github.com/aws/aws-sdk-go-v2/credentials to v1.13.18
- github.com/aws/aws-sdk-go-v2/service/ecr to v1.18.7
- github.com/google/go-containerregistry to v0.14.0
- github.com/onsi/gomega to v1.27.4

Signed-off-by: Hidde Beydals <hidde@hhh.computer>
@hiddeco hiddeco force-pushed the feat/azure-oci-workload-identity branch from 99152a0 to a8d276c Compare March 22, 2023 09:29
@hiddeco hiddeco merged commit b3e1ed2 into fluxcd:main Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oci OCI related issues and pull requests enhancement New feature or request hold Issues and pull requests put on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants