external-dns icon indicating copy to clipboard operation
external-dns copied to clipboard

WIP: Update Azure SDK and remove deprecated autorest dependency

Open phillebaba opened this issue 2 years ago • 4 comments

Description

This PR updates the Azure SDK to the latest stable version and replaces the use of the deprecated autorest with azidentity.

Fixes #ISSUE

Checklist

  • [ ] Unit tests updated
  • [ ] End user documentation updated

phillebaba avatar Sep 22 '22 15:09 phillebaba

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: phillebaba / name: Philip Laine (70eda8b84b57376ab7f1e09c67ce65f29a031634, d42df08c17845ba815db074b3a1c7ec7f1d685a3, 55c93e78237487e648aed3262d74ff8b281fcf2e, 985a53217cfa0fc6ef3876934e0f7f3260084398)

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: phillebaba Once this PR has been reviewed and has the lgtm label, please assign szuecs for approval by writing /assign @szuecs in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

k8s-ci-robot avatar Sep 22 '22 15:09 k8s-ci-robot

@phillebaba: PR needs rebase.

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.

k8s-ci-robot avatar Sep 22 '22 15:09 k8s-ci-robot

Welcome @phillebaba!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot avatar Sep 22 '22 15:09 k8s-ci-robot

I have been able to validate this in Azure Public with MSI authentication. Are there any more e2e tests that can be run against Azure?

phillebaba avatar Dec 02 '22 14:12 phillebaba

@phillebaba do you have any more context on why there is still a reference to the go-autorest library?

github.com/Azure/go-autorest/autorest v0.11.27 // indirect github.com/Azure/go-autorest/autorest/adal v0.9.20 // indirect

stevehipwell avatar Dec 05 '22 09:12 stevehipwell

Running go mod graph shows that kubernetes/client-go actually still uses autorest and adal in v0.25. This have however been removed in v0.26 so they should disappear. Note that the dependency is indirect so it is not a dependency that is imported by this project but indirectly from another dependency.

phillebaba avatar Dec 05 '22 09:12 phillebaba

Thanks for the confirmation @phillebaba, that was my assumption.

stevehipwell avatar Dec 05 '22 09:12 stevehipwell

@Raffo could you approve the GH workflows?

/ok-to-test

stevehipwell avatar Dec 05 '22 09:12 stevehipwell

@phillebaba Seems like 1.3.0 will be released only around April after the Workload Identity extension is promoted to GA (https://github.com/Azure/azure-sdk-for-go/issues/19765#issuecomment-1376512434), though I guess it'll land with the same code as 1.3.0-beta.2, so might be worth trying to support workload identity. Releases:

  • https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazidentity%2Fv1.3.0-beta.1
  • https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazidentity%2Fv1.3.0-beta.2

weisdd avatar Jan 21 '23 14:01 weisdd

@phillebaba can you rebase this? I'd love to get the azure changes in.

Raffo avatar Jan 25 '23 09:01 Raffo

@phillebaba once your changes are merged, I'll update my PR targeting Azure Workload Identity (#3111). I guess I can bump version of the azidentity to v1.3.0-beta.2 there then.

weisdd avatar Jan 25 '23 10:01 weisdd

FYI @Raffo

Created this https://github.com/kubernetes-sigs/external-dns/pull/3380, as @phillebaba appears to be busy. I've got some free time to see this through.

siiimooon avatar Feb 08 '23 19:02 siiimooon

@Raffo @siiimooon @weisdd sorry guys I have been busy with other stuff and kind of left this on ice until workload identities would be released. As stated this seems to be delayed once again so I have rebased so others can get on with other solutions.

phillebaba avatar Feb 08 '23 20:02 phillebaba

@Raffo I was wondering if there's any chance to have this PR reviewed in coming days/weeks. Thanks!

weisdd avatar Feb 14 '23 15:02 weisdd

Workload identity release candidate just came out - any chance to get this merged?

hbuckle avatar Mar 22 '23 05:03 hbuckle

Workload identity v1.0.0 was released two days ago. Today, the workload identity graduated to GA: https://azure.microsoft.com/en-ca/updates/ga-azure-active-directory-workload-identity-with-aks/ So, we will probably see stable release of azidentity in a few days.

weisdd avatar Mar 29 '23 17:03 weisdd

@weisdd the announcement link has gone dead and the AKS docs are still referring to it as in preview so I wonder if it's been pulled?

stevehipwell avatar Mar 30 '23 08:03 stevehipwell

@stevehipwell that's interesting, I still have the cached RSS feed item:

Screenshot

telegram-cloud-photo-size-2-5271839023607957033-y

With Azure docs, I've seen some examples before where "Preview" mark was left forgotten for a while after a GA announcement. So, that is not necessarily a concern, whereas the pulled announcement is. Anyway, let's wait then to see if they publish another GA announcement in coming days. :)

weisdd avatar Mar 30 '23 09:03 weisdd

image

https://github.com/Azure/AKS/issues/1480#issuecomment-1489840667

weisdd avatar Mar 30 '23 09:03 weisdd

My long-open PR which adds NS support for Azure, would need to completely rebuild after this is merged. How about integrating NS support into this PR directly? https://github.com/kubernetes-sigs/external-dns/pull/2835

sebader avatar Apr 13 '23 11:04 sebader

/ok-to-test /lgtm /assign Raffo

szuecs avatar Apr 22 '23 10:04 szuecs

@phillebaba are you in a position to rebase this?

@Raffo is this something which could be prioritised given the need to support Azure AD Workload Identity (https://github.com/kubernetes-sigs/external-dns/pull/3111) which is dependent on this PR.

stevehipwell avatar May 03 '23 10:05 stevehipwell

Looks like the official 1.3.0 release with workload identity support has just been released, would be great to be able to get this merged in so external-dns can support workload identity

https://github.com/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazidentity%2Fv1.3.0

nbjohnson avatar May 09 '23 18:05 nbjohnson

@phillebaba Are you able to update this PR? The official release of the identity sdk with workload identity support was published last week, so would be really great to get this guy merged in to move towards workload identity support in external-dns

nbjohnson avatar May 17 '23 14:05 nbjohnson

Sorry been busy fixing other stuff with Kubernetes upgrades. Will get this rebased and updated. I have other work with Thanos getting similar work done. My worry is that there is some configuration changes that have to be made that would create issues with AAD Pod ID and WI users. Especially because it's been a while since I created this PR.

phillebaba avatar May 18 '23 22:05 phillebaba

@phillebaba Do you think you can rebase this PR ? The CI is now fixed.

mloiseleur avatar Aug 10 '23 07:08 mloiseleur

@phillebaba I left one small suggestion on error handling, otherwise it LGTM.

mloiseleur avatar Aug 10 '23 14:08 mloiseleur

/lgtm

mloiseleur avatar Aug 16 '23 06:08 mloiseleur

Excited to see there is movement on this MR, can't wait for this feature to be available. Do we know when a release might occur with this feature included?

nbjohnson avatar Aug 23 '23 21:08 nbjohnson

@mloiseleur @johngmyers can any of you put the approved tag ? 😅

jbpaux avatar Aug 29 '23 12:08 jbpaux

Neither of us is an approver

johngmyers avatar Aug 29 '23 14:08 johngmyers