CCO-318: Enable Azure Workload Identity authentication. #906

jstuever · 2023-04-05T22:05:17Z

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

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

openshift-ci-robot · 2023-04-19T21:31:16Z

@jstuever: This pull request references CCO-318 which is a valid jira issue.

In response to this:

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

WIP TODO:

Update azure identity sdk to v1.3.0 (as opposed to beta)

Update openshift api to include config/v1: Add AzureWorkloadIdentity to TechPreviewNoUpgrade api#1436

Rebase onto CCO-318: Read feature gates for future usage #908

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

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.

pkg/operator/operator.go

openshift-ci-robot · 2023-05-01T21:32:13Z

@jstuever: This pull request references CCO-318 which is a valid jira issue.

In response to this:

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

WIP TODO:

Update azure identity sdk to v1.3.0 (as opposed to beta)

Update openshift api to include config/v1: Add AzureWorkloadIdentity to TechPreviewNoUpgrade api#1436

Rebase onto CCO-318: Read feature gates for future usage #908

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

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-05-01T21:32:32Z

@jstuever: This pull request references CCO-318 which is a valid jira issue.

In response to this:

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

WIP TODO:

Update azure identity sdk to v1.3.0 (as opposed to beta)

Rebase onto CCO-318: Read feature gates for future usage #908

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

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-05-09T23:13:17Z

@jstuever: This pull request references CCO-318 which is a valid jira issue.

In response to this:

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

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

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.

Miciah

I've made some minor suggestions, but this looks fine to me overall.
/approve

Miciah · 2023-05-15T22:14:24Z

pkg/dns/azure/client/auth.go

+	var (
+		cred azcore.TokenCredential
+		err  error
+	)
+	if config.AzureWorkloadIdentityEnabled && strings.TrimSpace(config.ClientSecret) == "" {
+		options := azidentity.WorkloadIdentityCredentialOptions{
+			ClientOptions: azcore.ClientOptions{
+				Cloud: cloudConfig,
+			},
+			ClientID:      config.ClientID,
+			TenantID:      config.TenantID,
+			TokenFilePath: config.FederatedTokenFile,
+		}
+		cred, err = azidentity.NewWorkloadIdentityCredential(&options)
+		if err != nil {
+			return nil, err
+		}
+	} else {
+		options := azidentity.ClientSecretCredentialOptions{
+			ClientOptions: azcore.ClientOptions{
+				Cloud: cloudConfig,
+			},
+		}
+		cred, err = azidentity.NewClientSecretCredential(config.TenantID, config.ClientID, config.ClientSecret, &options)


Is there a reason to declare err in the outer scope?

Suggested change

var (

cred azcore.TokenCredential

err error

)

if config.AzureWorkloadIdentityEnabled && strings.TrimSpace(config.ClientSecret) == "" {

options := azidentity.WorkloadIdentityCredentialOptions{

ClientOptions: azcore.ClientOptions{

Cloud: cloudConfig,

},

ClientID: config.ClientID,

TenantID: config.TenantID,

TokenFilePath: config.FederatedTokenFile,

}

cred, err = azidentity.NewWorkloadIdentityCredential(&options)

if err != nil {

return nil, err

}

} else {

options := azidentity.ClientSecretCredentialOptions{

ClientOptions: azcore.ClientOptions{

Cloud: cloudConfig,

},

}

cred, err = azidentity.NewClientSecretCredential(config.TenantID, config.ClientID, config.ClientSecret, &options)

var cred azcore.TokenCredential

if config.AzureWorkloadIdentityEnabled && strings.TrimSpace(config.ClientSecret) == "" {

options := azidentity.WorkloadIdentityCredentialOptions{

ClientOptions: azcore.ClientOptions{

Cloud: cloudConfig,

},

ClientID: config.ClientID,

TenantID: config.TenantID,

TokenFilePath: config.FederatedTokenFile,

}

var err error

cred, err = azidentity.NewWorkloadIdentityCredential(&options)

if err != nil {

return nil, err

}

} else {

options := azidentity.ClientSecretCredentialOptions{

ClientOptions: azcore.ClientOptions{

Cloud: cloudConfig,

},

}

var err error

cred, err = azidentity.NewClientSecretCredential(config.TenantID, config.ClientID, config.ClientSecret, &options)

No reason, was just blindly following the enhancement.
Updated.

Did you mean to delete config.AzureWorkloadIdentityEnabled && in https://github.com/openshift/cluster-ingress-operator/compare/02a3c1434367c4ce82e84f3a42b393de92765541..d8c1cafad42eff9f781248a166e6e591a203ff24?

No, I did not. I must have fumbled it during a rebase. Fixed. Thanks!

Miciah · 2023-05-15T22:53:29Z

pkg/dns/azure/client/client.go

+	Environment                  azure.Environment
+	SubscriptionID               string
+	ClientID                     string
+	ClientSecret                 string
+	FederatedTokenFile           string
+	TenantID                     string
+	AzureWorkloadIdentityEnabled bool


Would you mind adding godoc for these fields? In particular, it's getting confusing which fields are used in what situations. Looks like SubscriptionID, ClientID, and TenantID are always required; FederatedTokenFile is required when using workload identity; and ClientSecret is required when not using workload identity. Is that correct? Following is my attempt at describing these fields—please check for errors and discard or modify as necessary:

Suggested change

Environment azure.Environment

SubscriptionID string

ClientID string

ClientSecret string

FederatedTokenFile string

TenantID string

AzureWorkloadIdentityEnabled bool

// Environment describes the Azure environment: ChinaCloud,

// USGovernmentCloud, PublicCloud, or AzureStackCloud. If empty,

// AzureStackCloud is assumed.

Environment azure.Environment

// SubscriptionID is the subscription id for the Azure identity.

SubscriptionID string

// ClientID is an Azure application client id.

ClientID string

// ClientSecret is an Azure application client secret. It is required

// if Azure workload identity is not used.

ClientSecret string

// FederatedTokenFile is the path to a file containing a workload

// identity token. If FederatedTokenFile is specified and

// AzureWorkloadIdentityEnabled is true, then Azure workload identity is

// used instead of using a client secret.

FederatedTokenFile string

// TenantID is the Azure tenant ID.

TenantID string

// AzureWorkloadIdentityEnabled indicates whether the

// "AzureWorkloadIdentity" feature gate is enabled.

AzureWorkloadIdentityEnabled bool

Done, and thanks!

Miciah · 2023-05-15T23:04:33Z

pkg/operator/operator.go

 	// example of future featuregate read and usage to set a variable to pass to a controller
+	AzureWorkloadIdentityEnabled := featureGates.Enabled(configv1.FeatureGateAzureWorkloadIdentity)


The comment can be removed (I missed that 3d26626 removed the _ = sets.New[configv1.FeatureGateName](enabled...).Has("AzureWorkloadIdentity") line that the comment described). Uppercase AzureWorkloadIdentityEnabled looks funny for a function-local variable.

Suggested change

// example of future featuregate read and usage to set a variable to pass to a controller

AzureWorkloadIdentityEnabled := featureGates.Enabled(configv1.FeatureGateAzureWorkloadIdentity)

azureWorkloadIdentityEnabled := featureGates.Enabled(configv1.FeatureGateAzureWorkloadIdentity)

openshift-ci · 2023-05-15T23:11:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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:

~~OWNERS~~ [Miciah]

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

candita · 2023-05-17T14:53:47Z

/retest-required

jstuever · 2023-05-23T18:51:59Z

/retest

jstuever · 2023-05-25T21:22:05Z

/retest

ahardin-rh · 2023-05-26T15:41:38Z

/label docs-approved

lihongan · 2023-05-29T01:56:12Z

/label qe-approved

davemulford · 2023-05-30T14:07:22Z

Adding px-approved per a conversation with cfields.

/label px-approved

…Credential.

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

jstuever · 2023-06-01T16:29:07Z

Rebased to resolve merge conflicts in go.mod and related module files.

Miciah · 2023-06-01T17:00:21Z

/lgtm

jstuever · 2023-06-01T18:41:41Z

/retest

Miciah · 2023-06-01T19:48:58Z

/hold
#939 is top priority; make sure it merges first.

Miciah · 2023-06-02T02:38:59Z

/hold cancel
and
/test all
now that #939 has merged.

Miciah · 2023-06-02T05:48:49Z

e2e-gcp-operator failed because TestInternalLoadBalancerGlobalAccessGCP and TestInternalLoadBalancer failed. This is a known issue (OCPBUGS-13106), unrelated to the changes in this PR.
/override ci/prow/e2e-gcp-operator

openshift-ci · 2023-06-02T05:49:07Z

@Miciah: Overrode contexts on behalf of Miciah: ci/prow/e2e-gcp-operator

In response to this:

e2e-gcp-operator failed because TestInternalLoadBalancerGlobalAccessGCP and TestInternalLoadBalancer failed. This is a known issue (OCPBUGS-13106), unrelated to the changes in this PR.
/override ci/prow/e2e-gcp-operator

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 · 2023-06-02T05:49:09Z

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

dgoodwin · 2023-06-06T11:03:17Z

/label jira/valid-bug

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 5, 2023

openshift-ci bot requested review from candita and lmzuccarelli April 5, 2023 22:05

This was referenced Apr 10, 2023

[poc] Feature gates #907

Closed

CCO-318: Read feature gates for future usage #908

Merged

jstuever force-pushed the CCO-318 branch 3 times, most recently from 8c1e78d to 97d0dbd Compare April 11, 2023 21:31

jstuever force-pushed the CCO-318 branch from 97d0dbd to 66b276a Compare April 18, 2023 22:34

jstuever changed the title ~~WIP: Enable Azure Workload Identity authentication.~~ WIP: CCO-318: Enable Azure Workload Identity authentication. Apr 19, 2023

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

JoelSpeed reviewed Apr 25, 2023

View reviewed changes

pkg/operator/operator.go Outdated Show resolved Hide resolved

jstuever force-pushed the CCO-318 branch from 66b276a to f800e86 Compare May 5, 2023 21:31

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 5, 2023

jstuever force-pushed the CCO-318 branch from f800e86 to c01edec Compare May 9, 2023 23:12

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2023

jstuever changed the title ~~WIP: CCO-318: Enable Azure Workload Identity authentication.~~ CCO-318: Enable Azure Workload Identity authentication. May 9, 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 May 9, 2023

jstuever force-pushed the CCO-318 branch from c01edec to 02a3c14 Compare May 10, 2023 18:42

Miciah reviewed May 15, 2023

View reviewed changes

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

jstuever force-pushed the CCO-318 branch from 02a3c14 to d8c1caf Compare May 16, 2023 22:34

jstuever requested a review from Miciah May 16, 2023 22:42

jstuever force-pushed the CCO-318 branch from d8c1caf to c66b372 Compare May 22, 2023 21:42

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label May 26, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 29, 2023

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label May 30, 2023

jstuever added 3 commits June 1, 2023 09:22

Bump azidentity to v1.3.0 which includes support for WorkloadIdentity…

8e64b2f

…Credential.

Enable Azure Workload Identity authentication.

b37da5a

Authenticate with an Azure Workload Identity (AZWI) serviceaccount token when client secret is absent from auth config.

Add AzureWorkloadIdentity featureGate

475c9b6

jstuever force-pushed the CCO-318 branch from c66b372 to 475c9b6 Compare June 1, 2023 16:27

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

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

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

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 2, 2023

openshift-ci bot added the jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. label Jun 6, 2023

openshift-merge-robot merged commit e068d04 into openshift:master Jun 6, 2023

This was referenced Jun 6, 2023

NE-1294: Add support for AWS shared VPC in another account #928

Merged

OCPBUGS-13190: Avoid spurious updates for internalTrafficPolicy #927

Merged

jstuever deleted the CCO-318 branch July 7, 2023 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCO-318: Enable Azure Workload Identity authentication. #906

CCO-318: Enable Azure Workload Identity authentication. #906

jstuever commented Apr 5, 2023 •

edited

Loading

openshift-ci-robot commented Apr 19, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 9, 2023 •

edited by openshift-ci bot

Loading

Miciah left a comment

Miciah May 15, 2023

jstuever May 16, 2023

Miciah May 19, 2023

jstuever May 22, 2023

Miciah May 15, 2023

jstuever May 16, 2023

Miciah May 15, 2023

jstuever May 16, 2023

openshift-ci bot commented May 15, 2023

candita commented May 17, 2023

jstuever commented May 23, 2023

jstuever commented May 25, 2023

ahardin-rh commented May 26, 2023

lihongan commented May 29, 2023

davemulford commented May 30, 2023

jstuever commented Jun 1, 2023 •

edited

Loading

Miciah commented Jun 1, 2023

jstuever commented Jun 1, 2023

Miciah commented Jun 1, 2023

Miciah commented Jun 2, 2023

Miciah commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

dgoodwin commented Jun 6, 2023

		// example of future featuregate read and usage to set a variable to pass to a controller
		AzureWorkloadIdentityEnabled := featureGates.Enabled(configv1.FeatureGateAzureWorkloadIdentity)

CCO-318: Enable Azure Workload Identity authentication. #906

CCO-318: Enable Azure Workload Identity authentication. #906

Conversation

jstuever commented Apr 5, 2023 • edited Loading

openshift-ci-robot commented Apr 19, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 1, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 1, 2023 • edited by openshift-ci bot Loading

openshift-ci-robot commented May 9, 2023 • edited by openshift-ci bot Loading

Miciah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented May 15, 2023

candita commented May 17, 2023

jstuever commented May 23, 2023

jstuever commented May 25, 2023

ahardin-rh commented May 26, 2023

lihongan commented May 29, 2023

davemulford commented May 30, 2023

jstuever commented Jun 1, 2023 • edited Loading

Miciah commented Jun 1, 2023

jstuever commented Jun 1, 2023

Miciah commented Jun 1, 2023

Miciah commented Jun 2, 2023

Miciah commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

openshift-ci bot commented Jun 2, 2023

dgoodwin commented Jun 6, 2023

jstuever commented Apr 5, 2023 •

edited

Loading

openshift-ci-robot commented Apr 19, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 1, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented May 9, 2023 •

edited by openshift-ci bot

Loading

jstuever commented Jun 1, 2023 •

edited

Loading