Skip to content

Conversation

@r4f4
Copy link
Contributor

@r4f4 r4f4 commented Jun 14, 2022

Microsoft is in the process of upgrading the Azure SDK for Go from V1 to V2. Our clients and authentication are on V1. The V1 authentication utilizes ADAL which will be deprecated June 30, 2022. All V2 clients, except the V2 auth client azidentity, are in beta (azidentity is scheduled to be stable in Q2 2022). [0]

These changes remove the dependency on the ADAL API, replace the authentication with azidentity, and use an adapter so the auth will work with V1 clients.

[0] https://azure.github.io/azure-sdk/releases/latest/index.html#go

https://issues.redhat.com/browse/CORS-1910

@openshift-ci openshift-ci bot requested review from fabianofranz and jstuever June 14, 2022 15:06
@r4f4
Copy link
Contributor Author

r4f4 commented Jun 14, 2022

/test e2e-azure

@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from 5313974 to 92362fc Compare June 14, 2022 22:28
@r4f4
Copy link
Contributor Author

r4f4 commented Jun 15, 2022

imports github.com/Azure/azure-sdk-for-go/sdk/azcore: build constraints exclude all Go files in /go/src/github.com/openshift/installer/vendor/github.com/Azure/azure-sdk-for-go/sdk/azcore
imports github.com/Azure/azure-sdk-for-go/sdk/azidentity: build constraints exclude all Go files in /go/src/github.com/openshift/installer/vendor/github.com/Azure/azure-sdk-for-go/sdk/azidentity

Starting with pre-release v0.23.0 of azcore [1] and v0.14.0 of azidentity, they now require golang-1.18 for the use of generics. So we need to either bump our golang version requirement or use earlier pre-releases.

[1] https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazcore%2Fv0.23.0
[2] https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazidentity%2Fv0.14.0

@rna-afk
Copy link
Contributor

rna-afk commented Jun 15, 2022

/retest

@r4f4
Copy link
Contributor Author

r4f4 commented Jun 15, 2022

/hold

@openshift-ci 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 15, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 24, 2022

@r4f4: PR needs rebase.

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.

@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from 92362fc to c1eb297 Compare September 13, 2022 21:19
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2022
@r4f4
Copy link
Contributor Author

r4f4 commented Sep 13, 2022

/test e2e-azurestack

@r4f4
Copy link
Contributor Author

r4f4 commented Sep 14, 2022

/test yaml-lint
/test okd-verify-codegen
/test aro-unit

@r4f4
Copy link
Contributor Author

r4f4 commented Sep 14, 2022

/test e2e-azure-ovn

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2022
@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from dd43eb3 to 7536938 Compare October 4, 2022 21:58
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2022
@jstuever
Copy link
Contributor

/cc @patrickdillon
/uncc

@openshift-ci openshift-ci bot requested review from patrickdillon and removed request for jstuever October 12, 2022 16:06
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2022
@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch 2 times, most recently from ffc3f4c to 08b028e Compare October 20, 2022 21:38
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2022
@r4f4
Copy link
Contributor Author

r4f4 commented Oct 21, 2022

/test e2e-azurestack

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 21, 2022
@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from 096be8d to 60e3f4b Compare November 21, 2022 20:58
@openshift-ci-robot
Copy link
Contributor

/hold

Revision 8984312 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2022
@r4f4
Copy link
Contributor Author

r4f4 commented Dec 2, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2022
@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD c45c58e and 2 for PR HEAD 8984312 in total

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 2, 2022

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD a2ed5c0 and 1 for PR HEAD 8984312 in total

@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from 8984312 to 984057a Compare December 3, 2022 10:01
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2022
@r4f4
Copy link
Contributor Author

r4f4 commented Dec 3, 2022

Fixed a typo 's/HasPrefix/HasSuffix'

r4f4 added 2 commits December 5, 2022 18:08
Microsoft is in the process of upgrading the Azure SDK for Go from V1 to
V2. Our clients and authentication are on V1. The V1 authentication
utilizes ADAL which will be deprecated June 30, 2022. All V2 clients,
except the V2 auth client azidentity, are in beta (azidentity is
scheduled to be stable in Q2 2022). [0]

These changes remove the dependency on the ADAL API, replace the
authentication with azidentity, and use an adapter so the auth will work
with V1 clients.

[0] https://azure.github.io/azure-sdk/releases/latest/index.html#go

https://issues.redhat.com/browse/CORS-1910
Notice that azblob also had to be upgraded to 0.4.1, otherwise we hit
the following build issue:

vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_blob_lease_client.go:25:16: undefined: to.StringPtr
vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_block_blob_client.go:145:20: undefined: to.StringPtr
vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_container_lease_client.go:25:16: undefined: to.StringPtr
vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zc_shared_policy_shared_key_credential.go:190:17: undefined: log.EventResponse
vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_lease_request_options.go:63:16: undefined: to.StringPtr
vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_lease_request_options.go:142:16: undefined: to.StringPtr

and newer versions cause

pkg/gather/azure/azure.go:224:29: undefined: azblob.NewBlobClientWithSharedKey
pkg/gather/azure/azure.go:235:48: unknown field 'MaxRetryRequests' in struct literal of type blob.RetryReaderOptions
@r4f4 r4f4 force-pushed the azure-azid-adapter-2 branch from 984057a to 897b622 Compare December 5, 2022 17:09
@r4f4
Copy link
Contributor Author

r4f4 commented Dec 5, 2022

Rebased on top of master

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 5, 2022

/retest-required

@patrickdillon
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 5, 2022

@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-crc 92362fc430fb6bce74e2373148dc7617212e28bf link false /test e2e-crc
ci/prow/e2e-azure-resourcegroup 92362fc430fb6bce74e2373148dc7617212e28bf link false /test e2e-azure-resourcegroup
ci/prow/e2e-azure-shared-vpc 92362fc430fb6bce74e2373148dc7617212e28bf link false /test e2e-azure-shared-vpc
ci/prow/e2e-ibmcloud 92362fc430fb6bce74e2373148dc7617212e28bf link false /test e2e-ibmcloud
ci/prow/okd-e2e-aws 92362fc430fb6bce74e2373148dc7617212e28bf link false /test okd-e2e-aws
ci/prow/e2e-gcp-upi 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-gcp-upi
ci/prow/e2e-aws-upi 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-aws-upi
ci/prow/e2e-azure-upi 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-azure-upi
ci/prow/e2e-aws 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-aws
ci/prow/e2e-vsphere 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-vsphere
ci/prow/e2e-gcp 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-gcp
ci/prow/e2e-azure 92362fc430fb6bce74e2373148dc7617212e28bf link true /test e2e-azure
ci/prow/okd-e2e-gcp-ovn-upgrade dd43eb3ecf719b33f099faf0dc2bcfcef03e8d05 link false /test okd-e2e-gcp-ovn-upgrade
ci/prow/e2e-metal-ipi 46bd64f49af0066941bc3666044b5ad697eef329 link false /test e2e-metal-ipi
ci/prow/okd-e2e-aws-upgrade c71808abe9eecd64c34290e6fef1ac49be9f6106 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-agent-mce c71808abe9eecd64c34290e6fef1ac49be9f6106 link false /test e2e-agent-mce
ci/prow/e2e-agent-sno 8984312 link false /test e2e-agent-sno
ci/prow/e2e-aws-ovn-workers-rhel8 897b622 link false /test e2e-aws-ovn-workers-rhel8
ci/prow/e2e-azure-ovn-shared-vpc 897b622 link false /test e2e-azure-ovn-shared-vpc
ci/prow/okd-scos-e2e-aws-ovn 897b622 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-agent-sno-ipv6 897b622 link false /test e2e-agent-sno-ipv6
ci/prow/e2e-azurestack 897b622 link false /test e2e-azurestack
ci/prow/okd-scos-verify-codegen 897b622 link false /test okd-scos-verify-codegen
ci/prow/e2e-metal-assisted 897b622 link false /test e2e-metal-assisted
ci/prow/e2e-ovirt-sdn 897b622 link false /test e2e-ovirt-sdn
ci/prow/e2e-agent-ha-dualstack 897b622 link false /test e2e-agent-ha-dualstack
ci/prow/e2e-aws-ovn-disruptive 897b622 link false /test e2e-aws-ovn-disruptive
ci/prow/e2e-metal-ipi-sdn 897b622 link false /test e2e-metal-ipi-sdn
ci/prow/okd-e2e-aws-ovn-upgrade 897b622 link false /test okd-e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn-upgrade 897b622 link false /test e2e-aws-ovn-upgrade
ci/prow/e2e-openstack 897b622 link false /test e2e-openstack
ci/prow/e2e-ibmcloud-ovn 897b622 link false /test e2e-ibmcloud-ovn
ci/prow/tf-fmt 897b622 link false /test tf-fmt
ci/prow/e2e-azure-ovn-resourcegroup 897b622 link false /test e2e-azure-ovn-resourcegroup
ci/prow/okd-scos-e2e-aws-upgrade 897b622 link false /test okd-scos-e2e-aws-upgrade
ci/prow/e2e-libvirt 897b622 link false /test e2e-libvirt
ci/prow/okd-scos-unit 897b622 link false /test okd-scos-unit

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.

@r4f4
Copy link
Contributor Author

r4f4 commented Dec 5, 2022

/retest-required

@openshift-merge-robot openshift-merge-robot merged commit 082ccd2 into openshift:master Dec 6, 2022
@r4f4
Copy link
Contributor Author

r4f4 commented Dec 6, 2022

/cherry-pick release-4.12

@openshift-cherrypick-robot

@r4f4: #6003 failed to apply on top of branch "release-4.12":

Applying: azure: use azidentity with adapter
Using index info to reconstruct a base tree...
M	pkg/asset/installconfig/azure/session.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/asset/installconfig/azure/session.go
Applying: azure: azidentity: update go.mod and vendor
.git/rebase-apply/patch:7977: trailing whitespace.
* Fixed Issue #17152 , Issue #17131 , Issue #17061 : `UploadStreamToBlockBlob` / `UploadStreamToBlockBlob` methods ignoring the options bag. 
.git/rebase-apply/patch:7980: trailing whitespace.
* Fixed Issue #16679 : Response parsing issue in List blobs API. 
.git/rebase-apply/patch:8495: trailing whitespace.
        } 
.git/rebase-apply/patch:30479: trailing whitespace.
  
.git/rebase-apply/patch:30531: trailing whitespace.
If further differentiation is required, we can add custom errors that use Go error wrapping on top of CallErr to achieve our diagnostic goals (such as detecting when to retry a call due to transient errors).  
warning: squelched 10 whitespace errors
warning: 15 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	go.mod
M	go.sum
M	vendor/modules.txt
Falling back to patching base and 3-way merge...
Auto-merging vendor/modules.txt
Removing vendor/golang.org/x/sys/unix/zsyscall_darwin_arm64.1_13.s
Removing vendor/golang.org/x/sys/unix/zsyscall_darwin_arm64.1_13.go
Removing vendor/golang.org/x/sys/unix/zsyscall_darwin_amd64.1_13.s
Removing vendor/golang.org/x/sys/unix/zsyscall_darwin_amd64.1_13.go
Removing vendor/golang.org/x/sys/unix/syscall_darwin.1_13.go
Removing vendor/golang.org/x/sys/unix/syscall_darwin.1_12.go
Removing vendor/golang.org/x/sys/unix/str.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zz_generated_directory_client.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zz_generated_connection.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_service_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_page_blob_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_lease_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_container_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_client_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_block_blob_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_blob_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/zm_append_blob_request_options.go
Removing vendor/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/version.go
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging go.mod
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 azure: azidentity: update go.mod and vendor
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.12

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
Copy link
Contributor Author

r4f4 commented Dec 6, 2022

/retitle OCPBUGS-4541: Azure: use azidentity with adapter

@openshift-ci openshift-ci bot changed the title Azure: use azidentity with adapter OCPBUGS-4541: Azure: use azidentity with adapter Dec 6, 2022
@openshift-ci-robot
Copy link
Contributor

@r4f4: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-4541 has been moved to the MODIFIED state.

Details

In response to this:

Microsoft is in the process of upgrading the Azure SDK for Go from V1 to V2. Our clients and authentication are on V1. The V1 authentication utilizes ADAL which will be deprecated June 30, 2022. All V2 clients, except the V2 auth client azidentity, are in beta (azidentity is scheduled to be stable in Q2 2022). [0]

These changes remove the dependency on the ADAL API, replace the authentication with azidentity, and use an adapter so the auth will work with V1 clients.

[0] https://azure.github.io/azure-sdk/releases/latest/index.html#go

https://issues.redhat.com/browse/CORS-1910

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 r4f4 deleted the azure-azid-adapter-2 branch December 6, 2022 13:16
r4f4 added a commit to r4f4/installer that referenced this pull request Feb 17, 2023
Fallout from openshift#6003

If we pass `nil` to `azidentity.ParseCertificates`, it assumes the
private key isn't encrypted. Let's pipe the password through instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants