CCO-324,CCO-325: add support for workload identity #245

RomanBednar · 2023-04-19T07:06:48Z

In scope of Azure workload identity feature we need to extend injector to parse new configuration values.

RomanBednar · 2023-04-20T11:39:25Z

@abutcher can you please take a look as well?

I've noticed one thing I can't wrap my head around, the enhancement says:

"create workload identity clients when the azure_client_secret is absent and when azure_federated_token_file fields are found"

Based on that the injector code change proposed here prioritizes client secret. That means that if for some reason there is both secret and token in the secret from CCO, we use the secret and ignore workload identity values.

However, the current driver patch proposal (not merged yet!), seems to do quite the opposite - it first checks if workload identity can be used.

So I'm wondering, do we still want to prioritize client secret like this? Can we even expect to get a secret from CCO where we have both? And if we do should we rather just "pass through" all the values and let driver/cloud provider pick the auth method?

abutcher · 2023-04-20T18:58:00Z

So I'm wondering, do we still want to prioritize client secret like this? Can we even expect to get a secret from CCO where we have both? And if we do should we rather just "pass through" all the values and let driver/cloud provider pick the auth method?

CCO should never provide a secret which has both client secret and workload identity fields set. If priority is a concern I think it would be okay to prioritize workload identity if both fields are present; or perhaps if the federatedTokenFile is set then it should be assumed that workload identity is requested.

RomanBednar · 2023-04-21T13:32:19Z

CCO should never provide a secret which has both client secret and workload identity fields set. If priority is a concern I think it would be okay to prioritize workload identity if both fields are present; or perhaps if the federatedTokenFile is set then it should be assumed that workload identity is requested.

@abutcher Thanks for the details, assuming workload identity method sounds reasonable to me. I understand both methods should never be set by CCO, but we can have a check here, just in case. I've updated the PR again - it's ready for review now.

openshift-ci-robot · 2023-04-21T13:46:50Z

@RomanBednar: This pull request references CCO-325 which is a valid jira issue.

Details

In response to this:

In scope of Azure workload identity feature we need to extend injector to parse new configuration values.

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.

RomanBednar · 2023-04-24T06:49:40Z

/retest

elmiko

i think this is mostly making sense to me, although i do not know the azure credentials system at a deep level. i do have a question about one of the log messages.

elmiko · 2023-04-24T10:19:17Z

cmd/azure-config-credentials-injector/main.go

+		// Workload identity is available, but client secret is provided too.
+		// If we don't fail here we would use client secret unnecessarily, also we can assume that user intended to use workload identity because all required values are provided.
+		return fmt.Errorf("%s env variable is set along with env variables %s and %s which indicate workload identity auth should be used, this should never happen\n"+
+			"Please consider reporting a bug to CCO team: https://github.com/openshift/cloud-credential-operator/issues", clientSecretEnvKey, tenantIDEnvKey, federatedTokenEnvKey)


should we direct them to issues.redhat.com or does the CCO team process bug reports from the github issues?

i just want to make sure we don't miss any bug reports.

You're right, all bug reports should probably land in issues.redhat.com, thank you.

i think this is mostly making sense to me, although i do not know the azure credentials system at a deep level. i do have a question about one of the log messages.

Here we're mostly just passing through the values we get from CCO, most of the interesting stuff is done there so that's why I wanted @abutcher to review this PR as well.

RomanBednar · 2023-04-27T07:55:36Z

/retest

RomanBednar · 2023-05-02T08:55:09Z

/retest

RomanBednar · 2023-05-04T10:28:10Z

/retest

elmiko

just a couple more questions

elmiko · 2023-05-04T13:28:20Z

cmd/azure-config-credentials-injector/main.go

+	}
+	if err != nil && !secretFound {
+		// Workload identity failed, and we don't have a secret - can't continue.
+		return fmt.Errorf("user+secret method failed: %s is missing; workload identity method failed: %v", clientSecretEnvKey, err)


would it be possible to have err != nil && secretFound ?

just wondering if we need to cover all cases

With the conditions here we rule out the failure cases which are:

we have both secret and workload identity

we have neither

So what remains is that we either have secret or we have workload identity. Later we resolve this in prepareCloudConfig() where we take one or the other, prioritizing workload identity. So all cases are covered.

So sure we can have the conditions explicitly here, but then I believe we'd have to refactor prepareCloudConfig() so it does not do any decisions (which it already does with useManagedIdentityExtensionConfigKey). That would make the code more readable perhaps by having the logic at one place - I was tempted to go that route, but we have to consider the time invested to refactoring and value added.

elmiko · 2023-05-04T13:28:43Z

cmd/azure-config-credentials-injector/main.go

+	} else {
+		// Workload identity failed, but we have a secret.
+		klog.V(4).Info("%s env variable is set, client secret authentication will be used", clientSecretEnvKey)
+		cloudConfig[clientSecretCloudConfigKey] = clientSecret


do we need to check len(clientSecret) > 0 ?

Good point, I'm not sure if we want to pass through "" - I don't think CCO should let that through. But it does make sense to check for this. But if we do that with secret we should do that with all other values to be consistent. See my latest commit.

RomanBednar · 2023-05-09T08:24:37Z

/retest

RomanBednar · 2023-05-09T12:49:24Z

/hold

RomanBednar · 2023-05-24T11:19:52Z

/retest

RomanBednar · 2023-06-01T09:46:57Z

/retest

RomanBednar · 2023-06-01T14:57:23Z

/retest

RomanBednar · 2023-06-05T10:17:32Z

@elmiko This PR needs one more, hopefully last review. I had to change this to drive the feature using a gate instead of assumptions from variables, the rest should be the same.

RomanBednar · 2023-06-06T12:51:26Z

/hold cancel

elmiko

/approve
/lgtm

openshift-ci · 2023-06-08T11:27:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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~~ [elmiko]

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

openshift-ci-robot · 2023-06-15T14:49:43Z

@RomanBednar: This pull request references CCO-324 which is a valid jira issue.

This pull request references CCO-325 which is a valid jira issue.

Details

In response to this:

In scope of Azure workload identity feature we need to extend injector to parse new configuration values.

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-robot · 2023-06-19T10:35:37Z

/retest-required

Remaining retests: 0 against base HEAD 87d0d22 and 2 for PR HEAD 892fe8a in total

openshift-ci · 2023-06-19T15:19:47Z

@RomanBednar: 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.

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 Apr 19, 2023

openshift-ci bot requested review from damdo and elmiko April 19, 2023 07:07

RomanBednar force-pushed the workload-identity branch from 78fa349 to c07769b Compare April 20, 2023 11:08

RomanBednar force-pushed the workload-identity branch 2 times, most recently from 296fff0 to 29921ff Compare April 21, 2023 13:31

RomanBednar changed the title ~~WIP: add support for workload identity~~ add support for workload identity Apr 21, 2023

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 Apr 21, 2023

This was referenced Apr 21, 2023

CCO-324: add support for workload identity openshift/azure-disk-csi-driver-operator#78

Merged

CCO-325: add support for workload identity openshift/azure-file-csi-driver-operator#54

Merged

RomanBednar changed the title ~~add support for workload identity~~ CCO-324 CCO-325: add support for workload identity Apr 21, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 21, 2023

elmiko reviewed Apr 24, 2023

View reviewed changes

RomanBednar force-pushed the workload-identity branch 3 times, most recently from 520ac86 to b55409e Compare April 26, 2023 15:02

elmiko reviewed May 4, 2023

View reviewed changes

RomanBednar force-pushed the workload-identity branch from b55409e to 6581b44 Compare May 5, 2023 13:15

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 9, 2023

prevent using empty strings from env

a3d17c4

RomanBednar force-pushed the workload-identity branch from 6581b44 to 9c3f5b9 Compare May 23, 2023 14:43

RomanBednar added 2 commits June 5, 2023 12:15

add support for workload identity

cb3a63f

add unit tests for workload identity

892fe8a

RomanBednar force-pushed the workload-identity branch from 7a71922 to 892fe8a Compare June 5, 2023 10:15

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2023

elmiko reviewed Jun 8, 2023

View reviewed changes

openshift-ci bot assigned elmiko Jun 8, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 8, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 8, 2023

RomanBednar changed the title ~~CCO-324 CCO-325: add support for workload identity~~ CCO-324,CCO-325: add support for workload identity Jun 15, 2023

openshift-merge-robot merged commit 785ce67 into openshift:master Jun 19, 2023

CCO-324,CCO-325: add support for workload identity #245

CCO-324,CCO-325: add support for workload identity #245

Uh oh!

Conversation

RomanBednar commented Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomanBednar commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abutcher commented Apr 20, 2023

Uh oh!

RomanBednar commented Apr 21, 2023

Uh oh!

openshift-ci-robot commented Apr 21, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RomanBednar commented Apr 24, 2023

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko Apr 24, 2023

Choose a reason for hiding this comment

Uh oh!

RomanBednar Apr 26, 2023

Choose a reason for hiding this comment

Uh oh!

RomanBednar commented Apr 27, 2023

Uh oh!

RomanBednar commented May 2, 2023

Uh oh!

RomanBednar commented May 4, 2023

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko May 4, 2023

Choose a reason for hiding this comment

Uh oh!

RomanBednar May 5, 2023

Choose a reason for hiding this comment

Uh oh!

elmiko May 4, 2023

Choose a reason for hiding this comment

Uh oh!

RomanBednar May 5, 2023

Choose a reason for hiding this comment

Uh oh!

RomanBednar commented May 9, 2023

Uh oh!

RomanBednar commented May 9, 2023

Uh oh!

RomanBednar commented May 24, 2023

Uh oh!

RomanBednar commented Jun 1, 2023

Uh oh!

RomanBednar commented Jun 1, 2023

Uh oh!

RomanBednar commented Jun 5, 2023

Uh oh!

RomanBednar commented Jun 6, 2023

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 8, 2023

Uh oh!

openshift-ci-robot commented Jun 15, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 19, 2023

Uh oh!

openshift-ci bot commented Jun 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RomanBednar commented Apr 19, 2023 •

edited

Loading

RomanBednar commented Apr 20, 2023 •

edited

Loading

openshift-ci-robot commented Apr 21, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Jun 15, 2023 •

edited by openshift-ci bot

Loading