custom Azure Active Directory Authority Host #4210

Jeremy-Boyle · 2024-01-25T22:04:35Z

Description
This allows custom Azure Active Directory Authority Host field to be set, this allows the configuration of the value and enables externaldns to work in private Azure environments that are not apart of the Azure SDK.

Looking for feedback on how i went about adding the fix, open to suggestions / improvements if the env route is undesired.

Inspiration for the env: https://learn.microsoft.com/en-us/azure/developer/go/azure-sdk-authorization#authentication-on-azure-stack

Issue in Azure Cloud ADK:
https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore@v1.9.1/cloud#pkg-variables

Fixes #4209

Checklist

Unit tests updated
End user documentation updated (TODO)

k8s-ci-robot · 2024-01-25T22:04:44Z

Welcome @Jeremy-Boyle!

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. 😃

k8s-ci-robot · 2024-01-25T22:04:45Z

Hi @Jeremy-Boyle. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

szuecs · 2024-02-08T14:47:44Z

/ok-to-test
/approve
/lgtm

k8s-ci-robot · 2024-02-08T14:47:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: szuecs

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

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

szuecs · 2024-02-08T18:04:00Z

#4240

pascalgn · 2024-02-08T18:28:12Z

provider/azure/config.go

@@ -149,6 +149,17 @@ func getCloudConfiguration(name string) (cloud.Configuration, error) {
 		return cloud.AzureGovernment, nil
 	case "AZURECHINACLOUD":
 		return cloud.AzureChina, nil
+	case "AZURECUSTOMCLOUD":
+		azureAdEndpoint := os.Getenv("AZURE_AD_ENDPOINT")


I'm not sure this is the best way to get this. This method (getCloudConfiguration) gets called with the cloud type (e.g. "AZURECLOUD"), which comes from the config file but then we take the endpoint config from some environment variable. I think it could be better to have the endpoint config as part of the config file

@Jeremy-Boyle we are waiting for this one to comment or fix

szuecs · 2024-02-09T11:12:36Z

please rebase the test was fixed

Signed-off-by: Jeremy-Boyle <9406398+Jeremy-Boyle@users.noreply.github.com>

Jeremy-Boyle · 2024-02-22T17:49:03Z

Rebased

mloiseleur · 2024-02-23T08:41:58Z

@Jeremy-Boyle Wdyt of @pascalgn comment ? BTW, Would you please update the documentation about this new settings ?

Jeremy-Boyle · 2024-04-16T16:08:20Z

I haven't had a response back, I'm going to do the suggestion.

…s-4209

…need to update docs next

Jeremy-Boyle · 2024-04-22T17:33:24Z

@szuecs @pascalgn

Does this look good now with your recommendations ?

pascalgn · 2024-04-25T06:24:59Z

Thanks for updating, I like the PR much better now. To me the PR looks good!

pascalgn · 2024-04-25T06:31:29Z

docs/tutorials/azure.md

@@ -58,6 +58,7 @@ The following fields are used:
 * `aadClientID` and `aadClientSecret` are associated with the Service Principal.  This is only used with Service Principal method documented in the next section.
 * `useManagedIdentityExtension` - this is set to `true` if you use either AKS Kubelet Identity or AAD Pod Identities methods documented in the next section.
 * `userAssignedIdentityID` - this contains the client id from the Managed identitty when using the AAD Pod Identities method documented in the next setion.
+* `activeDirectoryAuthorityHost` - this contains the uri to overwrite the default provided AAD Endpoint. This is useful for providing additional support where the endpoint is not available in the default cloud config from the [azure-sdk-for-go](https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud#pkg-variables).
 * `useWorkloadIdentityExtension` - this is set to `true` if you use Workload Identity method documented in the next section.


Not really required I guess, but it would be super great if we could add documentation for the cloud field

pascalgn · 2024-04-25T06:33:07Z

provider/azure/config.go

+	// If activeDirectoryAuthorityHost is provided explicitly, override existing one in config file
+	if activeDirectoryAuthorityHost != "" {
+		cfg.ActiveDirectoryAuthorityHost = activeDirectoryAuthorityHost
+	}


One thing I thought about: what happens if cloud is set to a non-empty value (e.g. "AZURECHINACLOUD") but activeDirectoryAuthorityHost is also set to some non-empty value. I think we should return an error in that case

szuecs · 2024-04-25T14:44:34Z

/lgtm

@Jeremy-Boyle @pascalgn I guess it makes sense to add the docs, but maybe we just do it in another PR to save some time.

Jeremy-Boyle · 2024-05-01T15:18:29Z

@szuecs

Is there a issue that you know of or a roadmap of when 0.14.2 will be released ? I'm assuming this would be the next release with these changes added.

k8s-ci-robot requested review from mloiseleur and szuecs January 25, 2024 22:04

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2024

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 25, 2024

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 25, 2024

szuecs changed the title ~~Adding feature for #4209~~ custom Azure Active Directory Authority Host Feb 8, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 8, 2024

k8s-ci-robot assigned szuecs Feb 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024

pascalgn reviewed Feb 8, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

Adding feature for #4209

e39959d

Signed-off-by: Jeremy-Boyle <9406398+Jeremy-Boyle@users.noreply.github.com>

Jeremy-Boyle force-pushed the feature/azure-add-endpoints-4209 branch from 3ca267a to e39959d Compare February 22, 2024 17:48

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 22, 2024

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

Jeremy-Boyle and others added 2 commits April 16, 2024 09:13

Merge branch 'kubernetes-sigs:master' into feature/azure-add-endpoint…

c7a0561

…s-4209

Removed the env, and moved to config file changes for ADD endpoints, …

fe0af65

…need to update docs next

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 16, 2024

Added documentation for overriding the activeDirectoryAuthorityHost

104adb0

pascalgn reviewed Apr 25, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2024

k8s-ci-robot merged commit 9f81bbe into kubernetes-sigs:master Apr 25, 2024
12 checks passed

Jeremy-Boyle deleted the feature/azure-add-endpoints-4209 branch May 1, 2024 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

custom Azure Active Directory Authority Host #4210

custom Azure Active Directory Authority Host #4210

Jeremy-Boyle commented Jan 25, 2024 •

edited

k8s-ci-robot commented Jan 25, 2024

k8s-ci-robot commented Jan 25, 2024

szuecs commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

szuecs commented Feb 8, 2024

pascalgn Feb 8, 2024

szuecs Feb 29, 2024

szuecs commented Feb 9, 2024

Jeremy-Boyle commented Feb 22, 2024

mloiseleur commented Feb 23, 2024

Jeremy-Boyle commented Apr 16, 2024

Jeremy-Boyle commented Apr 22, 2024

pascalgn commented Apr 25, 2024

pascalgn Apr 25, 2024

pascalgn Apr 25, 2024

szuecs commented Apr 25, 2024

Jeremy-Boyle commented May 1, 2024

custom Azure Active Directory Authority Host #4210

custom Azure Active Directory Authority Host #4210

Conversation

Jeremy-Boyle commented Jan 25, 2024 • edited

k8s-ci-robot commented Jan 25, 2024

k8s-ci-robot commented Jan 25, 2024

szuecs commented Feb 8, 2024

k8s-ci-robot commented Feb 8, 2024

szuecs commented Feb 8, 2024

pascalgn Feb 8, 2024

Choose a reason for hiding this comment

szuecs Feb 29, 2024

Choose a reason for hiding this comment

szuecs commented Feb 9, 2024

Jeremy-Boyle commented Feb 22, 2024

mloiseleur commented Feb 23, 2024

Jeremy-Boyle commented Apr 16, 2024

Jeremy-Boyle commented Apr 22, 2024

pascalgn commented Apr 25, 2024

pascalgn Apr 25, 2024

Choose a reason for hiding this comment

pascalgn Apr 25, 2024

Choose a reason for hiding this comment

szuecs commented Apr 25, 2024

Jeremy-Boyle commented May 1, 2024

Jeremy-Boyle commented Jan 25, 2024 •

edited