CORS-2467: dns: azure: use azidentity with an adapter #846

r4f4 · 2022-10-21T13:00:58Z

Similar work to what I've been doing for the openshift-installer openshift/installer#6003

Marking as WIP while I figure out why it's not working with AzureStack.

r4f4 · 2022-11-22T15:43:32Z

/retitle dns: azure: use azidentity with an adapter

r4f4 · 2022-12-03T12:11:26Z

Fixed a typo s/HasPrefix/HasSuffix/

r4f4 · 2023-01-11T16:11:22Z

Context for this change: https://issues.redhat.com/browse/CORS-2467
@frobware @rfredette What is missing to get this reviewed and merged?

candita · 2023-01-11T16:59:14Z

/retitle CORS-2467: dns: azure: use azidentity with an adapter

rfredette · 2023-01-11T17:54:10Z

Thanks @r4f4, I'll give this a review soon.

/assign
/assign @gcs278

frobware · 2023-01-17T13:00:43Z

pkg/dns/azure/client/auth.go

+	switch config.Environment {
+	case azure.ChinaCloud:
+		cloudConfig = cloud.AzureChina
+	// GermanCloud was closed on Oct 29, 2021


Do we have any CI jobs that still expect the GermanCloud region/zone to be present?

I didn't find anything in openshift/release.

But I can un-comment it if you think it's safer.

frobware · 2023-01-17T13:05:17Z

pkg/dns/azure/client/auth.go


 	scope := config.Environment.TokenAudience
-	if !strings.HasPrefix(scope, "/.default") {
+	if !strings.HasSuffix(scope, "/.default") {


Can we create a unit test case at all -- something that would have caught this change up front?

A unit test for getAuthorizerForResource or do you mean to extract this part out into a function and write some unit tests for that?

A bit of both; if we had a test (or had written a test) would we have discovered the need to use HasSuffix (over HasPrefix) because the test would have failed upfront.

gcs278 · 2023-01-17T19:56:02Z

pkg/dns/azure/client/auth.go

-		config.Environment.ActiveDirectoryEndpoint, config.TenantID)
+	var cloudConfig cloud.Configuration
+	switch config.Environment {
+	case azure.ChinaCloud:


Is there a reason this logic flow differs from https://github.com/r4f4/installer/blob/897b6226e87ec5c9e25c0c952b7f6a301204fe67/pkg/asset/installconfig/azure/session.go#L73-L89? The default here is AzureStackCloud, but the default in openshift/installer is AzurePublic.

No particular reason other than I was changing the Installer code a lot during my testing of AzureStack and figuring out how to get it to work. I can change this to make it like the Installer's if you prefer.

I have no preference, just wanted to make sure there it isn't supposed to be exactly the same. As long as it works as expected.

Hmm, sorry. I've done quite a few of these so I'm getting them mixed up. In this case there was a simple reason for the swap of Public <-> Stack: autorest doesn't define azure.StackCloud [1]. In the installer, we have our own definition for the cloud environments [2]. So to get around that, I've left StackCloud as the last branch.

[1] https://github.com/Azure/go-autorest/blob/main/autorest/azure/environments.go#L34-L41
[2] https://github.com/openshift/installer/blob/master/pkg/types/azure/platform.go#L95-L110

Okay seems reasonable, thanks.

r4f4 · 2023-01-24T10:48:58Z

/hold
while I get some feedback and then squash the commits.

gcs278 · 2023-01-25T16:13:19Z

/lgtm
We can wait for @frobware to review recent changes before approval.

gcs278 · 2023-01-31T15:39:56Z

my lgtm didn't get applied
/lgtm

gcs278 · 2023-01-31T15:42:00Z

@r4f4 I think we need a bug like https://issues.redhat.com/browse/OCPBUGS-4541 for this. Would you mind making it?

r4f4 · 2023-01-31T15:54:54Z

/retitle OCPBUGS-6863: dns: azure: use azidentity with an adapter

openshift-ci-robot · 2023-01-31T15:55:06Z

@r4f4: This pull request references Jira Issue OCPBUGS-6863, which is invalid:

expected the bug to target the "4.13.0" version, but it targets "4.13" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Similar work to what I've been doing for the openshift-installer openshift/installer#6003

Marking as WIP while I figure out why it's not working with AzureStack.

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.

r4f4 · 2023-01-31T15:55:34Z

/jira refresh

Miciah · 2023-02-20T19:31:43Z

@r4f4, can you rebase this PR again?

r4f4 · 2023-02-20T20:02:48Z

Rebased to fix merge conflicts.

CFields651 · 2023-02-21T14:41:38Z

/label px-approved

lihongan · 2023-02-22T10:00:23Z

/label qe-approved
launch cluster with the PR and do some regression test, no issue found

$ oc -n openshift-ingress get svc
NAME                      TYPE           CLUSTER-IP       EXTERNAL-IP     PORT(S)                      AGE
router-default            LoadBalancer   172.30.89.178    20.96.134.116   80:31311/TCP,443:32033/TCP   102m
router-extlb              LoadBalancer   172.30.254.31    20.75.60.76     80:32131/TCP,443:32581/TCP   4m19s

$ dig any.extlb.ci-ln-hif9n2k-1d09d.ci.azure.devcluster.openshift.com +short
20.75.60.76

$ oc get co/ingress
NAME      VERSION                                                   AVAILABLE   PROGRESSING   DEGRADED   SINCE   MESSAGE
ingress   4.13.0-0.ci.test-2023-02-22-074341-ci-ln-hif9n2k-latest   True        False         False      97m

openshift-ci · 2023-02-22T10:00:27Z

@lihongan: The label(s) /label qe-aaproved cannot be applied. These labels are supported: platform/aws, platform/azure, platform/baremetal, platform/google, platform/libvirt, platform/openstack, ga, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, px-approved, docs-approved, qe-approved, downstream-change-needed, approved, backport-risk-assessed, bugzilla/valid-bug, cherry-pick-approved, jira/valid-bug, staff-eng-approved. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

Details

In response to this:

/label qe-aaproved
launch cluster with the PR and do some regression test, no issue found

$ oc -n openshift-ingress get svc
NAME                      TYPE           CLUSTER-IP       EXTERNAL-IP     PORT(S)                      AGE
router-default            LoadBalancer   172.30.89.178    20.96.134.116   80:31311/TCP,443:32033/TCP   102m
router-extlb              LoadBalancer   172.30.254.31    20.75.60.76     80:32131/TCP,443:32581/TCP   4m19s

$ dig any.extlb.ci-ln-hif9n2k-1d09d.ci.azure.devcluster.openshift.com +short
20.75.60.76

$ oc get co/ingress
NAME      VERSION                                                   AVAILABLE   PROGRESSING   DEGRADED   SINCE   MESSAGE
ingress   4.13.0-0.ci.test-2023-02-22-074341-ci-ln-hif9n2k-latest   True        False         False      97m

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.

lihongan · 2023-02-22T10:01:24Z

/label qe-approved

Miciah · 2023-02-22T17:18:54Z

/approve

openshift-ci · 2023-02-22T17:19:08Z

[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

Details

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

gcs278 · 2023-02-22T19:49:16Z

Looks good to me, but we are going to wait on #890 to get merged, which will most likely cause a rebase, and we can proceed from there.

DCChadwick · 2023-02-23T20:24:55Z

/label docs-approved

ADAL will be deprecated in Dec 2022. Let's move to azidentity with an adapter so the new authentication can work the V1 clients.

r4f4 · 2023-02-23T23:47:28Z

/retest-required

openshift-ci · 2023-02-24T01:24:53Z

@r4f4: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn-serial	`843892e`	link	true	`/test e2e-gcp-ovn-serial`
ci/prow/e2e-azure-ovn	`82c6757`	link	false	`/test e2e-azure-ovn`

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.

Miciah · 2023-02-25T00:43:30Z

/lgtm

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 Oct 21, 2022

openshift-ci bot requested review from frobware and rfredette October 21, 2022 13:02

r4f4 mentioned this pull request Oct 21, 2022

WIP: Use DNS clients from Azure SDK v2 #848

Closed

r4f4 force-pushed the azure-adal-deprecation branch from 14e683d to 8e2fe2e Compare November 22, 2022 15:42

openshift-ci bot changed the title ~~WIP: Azure adal deprecation~~ dns: azure: use azidentity with an adapter Nov 22, 2022

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 Nov 22, 2022

openshift-ci bot changed the title ~~dns: azure: use azidentity with an adapter~~ CORS-2467: dns: azure: use azidentity with an adapter Jan 11, 2023

openshift-ci bot assigned gcs278 and rfredette Jan 11, 2023

frobware reviewed Jan 17, 2023

View reviewed changes

gcs278 reviewed Jan 17, 2023

View reviewed changes

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

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

openshift-ci bot changed the title ~~CORS-2467: dns: azure: use azidentity with an adapter~~ OCPBUGS-6863: dns: azure: use azidentity with an adapter Jan 31, 2023

openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels Jan 31, 2023

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

r4f4 force-pushed the azure-adal-deprecation branch from 70089bd to ad45e7d Compare February 20, 2023 20:02

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

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

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

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

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

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

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

r4f4 added 3 commits February 23, 2023 15:29

dns: azure: use azidentity with an adapter

10af42c

ADAL will be deprecated in Dec 2022. Let's move to azidentity with an adapter so the new authentication can work the V1 clients.

dns: azure: vendor in changes

a8ba16a

dns: azure: add unit test for scope

82c6757

Miciah force-pushed the azure-adal-deprecation branch from ad45e7d to 82c6757 Compare February 23, 2023 20:38

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

openshift-ci bot assigned Miciah Feb 25, 2023

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

openshift-merge-robot merged commit 8eb2c66 into openshift:master Feb 25, 2023

CORS-2467: dns: azure: use azidentity with an adapter #846

CORS-2467: dns: azure: use azidentity with an adapter #846

Uh oh!

Conversation

r4f4 commented Oct 21, 2022

Uh oh!

r4f4 commented Nov 22, 2022

Uh oh!

r4f4 commented Dec 3, 2022

Uh oh!

r4f4 commented Jan 11, 2023

Uh oh!

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

Uh oh!

Uh oh!

rfredette commented Jan 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

r4f4 commented Jan 24, 2023

Uh oh!

gcs278 commented Jan 25, 2023

Uh oh!

gcs278 commented Jan 31, 2023

Uh oh!

gcs278 commented Jan 31, 2023

Uh oh!

r4f4 commented Jan 31, 2023

Uh oh!

openshift-ci-robot commented Jan 31, 2023

Uh oh!

r4f4 commented Jan 31, 2023

Uh oh!

Miciah commented Feb 20, 2023

Uh oh!

r4f4 commented Feb 20, 2023

Uh oh!

CFields651 commented Feb 21, 2023

Uh oh!

lihongan commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Feb 22, 2023

Uh oh!

lihongan commented Feb 22, 2023

Uh oh!

Miciah commented Feb 22, 2023

Uh oh!

openshift-ci bot commented Feb 22, 2023

Uh oh!

gcs278 commented Feb 22, 2023

Uh oh!

DCChadwick commented Feb 23, 2023

Uh oh!

r4f4 commented Feb 23, 2023

Uh oh!

openshift-ci bot commented Feb 24, 2023

Uh oh!

candita commented Jan 11, 2023 •

edited by openshift-ci bot

Loading

lihongan commented Feb 22, 2023 •

edited

Loading