Support passing custom headers to AKS Managed Cluster and Node Pool create/update requests by michalno1 · Pull Request #2020 · kubernetes-sigs/cluster-api-provider-azure

michalno1 · 2022-01-28T10:10:19Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
To use some AKS features one needs to pass custom headers when creating/updating a resource. One example is when you want to use a GPU node pool, you need to register GPUDedicatedVHDPreview feature and pass a custom header to node pool creation request:

az aks nodepool add \
   ...
   --aks-custom-headers UseGPUDedicatedVHD=true \
   ...

This PR adds a mechanism where you can pass custom headers to cluster and node pool create/update requests by using annotations prefixed with custom-headers.azure.dev/. E.g. to create mentioned GPU node pool one would set "custom-headers.azure.dev/UseGPUDedicatedVHD": true annotation on AzureManagedMachinePool.

Which issue(s) this PR fixes:
fixes #1981, fixes #1979

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Support passing custom headers to cluster and node pool create/update requests to enable additional features

linux-foundation-easycla · 2022-01-28T10:10:22Z

The committers are authorized under a signed CLA.

✅ Michal Duda (d3bf732)

k8s-ci-robot · 2022-01-28T10:10:22Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

k8s-ci-robot · 2022-01-28T10:10:27Z

Welcome @michalno1!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. 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/cluster-api-provider-azure 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 · 2022-01-28T10:10:28Z

Hi @michalno1. 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.

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.

linux-foundation-easycla · 2022-01-28T10:20:10Z

The committers are authorized under a signed CLA.

✅ Michal Duda (147cbd8)

zmalik · 2022-01-28T10:50:03Z

/area managedclusters

CecileRobertMichon · 2022-01-28T20:53:38Z

/ok-to-test

zmalik · 2022-01-31T09:23:10Z

looks like the tests hit the quota issue

Operation could not be completed as it results in exceeding approved standardDSv3Family Cores quota. Additional details - Deployment Model: Resource Manager, Location: eastus, Current Limit: 200, Current Usage: 200, Additional Required: 6, (Minimum) New Limit Required: 206

/retest

michalno1 · 2022-01-31T14:22:05Z

/test pull-cluster-api-provider-azure-e2e-exp

sonasingh46 · 2022-01-31T17:42:26Z

+	// Whatever follows the prefix will be passed as a header to cluster/node pool creation/update requests.
+	// E.g. add `"custom-headers.azure.dev/UseGPUDedicatedVHD": "true"` annotation to AzureManagedMachinePool CR
+	// to enable creating GPU nodes by the node pool.
+	CustomHeaderPrefix = "custom-headers.azure.dev/"


nit:
Do annotation/labels follow a specific pattern? In case of annotations I have seen cluster.x-k8s.io in CAPI. May be worth discussing as his will help us being consistent while we may introduce more annotations in near future. WDYT?

Definitely. Do you suggest opening a thread in Disussions or should we do it here? Do you have any suggestions? Maybe we can use cluster-azure.x-k8s.io/ for all project annotations and cluster-azure.x-k8s.io/custom-header-{HEADER_NAME} specifically for this feature?

cluster-azure.x-k8s.io -- This one sounds good to me. Copying @CecileRobertMichon @shysank to provide more input.

how about infrastructure.cluster.x-k8s.io? cluster-azure.x-k8s.io is the API Group for Cluster API. CAPZ resources use infrastructure.cluster.x-k8s.io as its Group.

cluster-api-provider-azure/api/v1beta1/groupversion_info.go

Line 29 in 2349883

GroupVersion = schema.GroupVersion{Group: "infrastructure.cluster.x-k8s.io", Version: "v1beta1"}

@CecileRobertMichon makes sense, thanks. I've changed it.

sonasingh46 · 2022-01-31T17:51:03Z

+func (s *ManagedControlPlaneScope) ManagedClusterAnnotations() map[string]string {
+	return s.ControlPlane.Annotations
+}
+


nit:
Do we need this function? Can't we simply use s.ControlPlane.Annotations at places wherever required?

I see why it is so. You basically implemented it as part of ManagedMachinePoolScope interface.

This particular method implements ManagedClusterScope rather than ManagedMachinePoolScope. This is done in the same spirit as ManagedControlPlaneScope.ManagedClusterSpec().

sonasingh46 · 2022-01-31T17:51:17Z

+func (s *ManagedControlPlaneScope) AgentPoolAnnotations() map[string]string {
+	return s.InfraMachinePool.Annotations
+}
+


Same as above

sonasingh46 · 2022-01-31T18:15:17Z

+	var result = map[string]string{}
+	for key, value := range s.scope.AgentPoolAnnotations() {
+		if strings.HasPrefix(key, azure.CustomHeaderPrefix) {
+			result[strings.TrimPrefix(key, azure.CustomHeaderPrefix)] = value


Just a thought:
What if the value is empty? We can have probably move this function to another that takes input as annotations and prefix and returns the filtered values. This could help reuse.

And here we can probably just sanitise the values if required.

Refactored for reuse. What issue do you see with an empty value? We'll just pass a header with empty value if that is what someone intends.

Sure. My only point was -- does the azure api tolerates the empty value. I do not know. Just thought to bring up, in case. If yes, I do not think we need to do anything here.

Just checked- API does accept empty values.

sonasingh46 · 2022-01-31T18:24:15Z

+		}
+	}
+	return result
+}


Refer: https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/2020/files#r795940312

jackfrancis

/lgtm

shysank

Thanks for the pr @michalno1! The changes in general looks good. Just a couple of clarifications regarding cluster/nodepool update. wdyt about adding some documentation for this feature? I'm fine with doing it in a followup pr if it's easier.

shysank · 2022-02-11T00:13:00Z

 		if diff != "" {
 			log.V(2).Info(fmt.Sprintf("Update required (+new -old):\n%s", diff))
-			err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile)
+			err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name,


can we enable/disable features after node pool creation?

it depends on the feature, the headers are usually used with experimental features in AKS so lets say in case of GPU nodepool, we cannot. We cannot convert an existing agentpool to support GPU, its just a creation operation.

I'm ok with providing a way for users to use these experimental features and at most add some warning documentation about untested behaviors.

I think the most sane way to move forward is to support "enable feature via custom header" on create only. There may be some aks nodepool update operations that are initiated via an "empty" PUT w/ HTTP header data, but that's kind of weird from a UX perspective and I would not expect to rely upon that.

As @zmalik suggests, we should consider feature configuration delivered via HTTP headers as an interface for AKS to rapidly deliver experimental features, and then eventually they may graduate to fully supported features (after which point capz would have to implement them in their final form).

Does that make sense? Is the PR as-is implemented to forbid updates of custom headers after cluster/nodepool creation?

The PR right now allows for updates, although not intentionally. If there is a non zero diff along with annotations changes, the new headers will be set. I'd rather we be more explicit by adding a validation webhook to make it immutable. wdyt?

@shysank added immutability check

shysank · 2022-02-11T00:13:46Z

 		if diff != "" {
 			klog.V(2).Infof("Update required (+new -old):\n%s", diff)
-			managedCluster, err = s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroupName, managedClusterSpec.Name, managedCluster)
+			managedCluster, err = s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroupName, managedClusterSpec.Name, managedCluster, customHeaders)


similar comment as above regarding enabling/disabling features after cluster creation?

@shysank added validation webhook with immutability check to AzureManagedCluster as well.

michalno1 · 2022-02-19T22:26:44Z

Thanks for the pr @michalno1! The changes in general looks good. Just a couple of clarifications regarding cluster/nodepool update. wdyt about adding some documentation for this feature? I'm fine with doing it in a followup pr if it's easier.

@shysank I've added some docs

shysank · 2022-02-23T16:37:13Z

/test pull-cluster-api-provider-azure-e2e

k8s-ci-robot · 2022-02-23T17:43:53Z

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

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	`c0416bc`	link	false	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

michalno1 · 2022-02-23T18:13:31Z

/test pull-cluster-api-provider-azure-e2e

shysank · 2022-02-23T19:44:56Z

/lgtm
/approve
Great job @michalno1! Thanks for being patient with the reviews.

k8s-ci-robot · 2022-02-23T19:45:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shysank

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

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 28, 2022

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 28, 2022

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 28, 2022

k8s-ci-robot requested review from alexeldeib and shysank January 28, 2022 10:10

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/provider/azure Issues or PRs related to azure provider sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jan 28, 2022

michalno1 force-pushed the main branch from bbda25a to 147cbd8 Compare January 28, 2022 10:20

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 28, 2022

k8s-ci-robot added the area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type label Jan 28, 2022

michalno1 changed the title ~~Support passing custom headers to cluster and node pool create/update requests~~ Support passing custom headers to AKS Managed Cluster and Node Pool create/update requests Jan 28, 2022

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 Jan 28, 2022

sonasingh46 reviewed Jan 31, 2022

View reviewed changes

jackfrancis approved these changes Feb 10, 2022

View reviewed changes

k8s-ci-robot assigned jackfrancis Feb 10, 2022

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

shysank reviewed Feb 11, 2022

View reviewed changes

michalno1 force-pushed the main branch from 4ea7afa to d3bf732 Compare February 19, 2022 22:23

michalno1 force-pushed the main branch from d3bf732 to 57bb472 Compare February 19, 2022 22:25

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

michalno1 requested review from jackfrancis and shysank February 19, 2022 22:29

shysank reviewed Feb 22, 2022

View reviewed changes

Comment thread exp/api/v1beta1/azuremanagedmachinepool_webhook.go Outdated

michalno1 force-pushed the main branch from 57bb472 to 98d6ad0 Compare February 23, 2022 11:44

passing custom headers to cluster and node pool create/update requests

c0416bc

michalno1 force-pushed the main branch from 98d6ad0 to c0416bc Compare February 23, 2022 11:46

michalno1 requested a review from shysank February 23, 2022 11:50

k8s-ci-robot assigned shysank Feb 23, 2022

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

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

k8s-ci-robot merged commit b37fc68 into kubernetes-sigs:main Feb 23, 2022

k8s-ci-robot added this to the v1.2 milestone Feb 23, 2022

Conversation

michalno1 commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linux-foundation-easycla Bot commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jan 28, 2022

Uh oh!

k8s-ci-robot commented Jan 28, 2022

Uh oh!

k8s-ci-robot commented Jan 28, 2022

Uh oh!

linux-foundation-easycla Bot commented Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zmalik commented Jan 28, 2022

Uh oh!

CecileRobertMichon commented Jan 28, 2022

Uh oh!

zmalik commented Jan 31, 2022

Uh oh!

michalno1 commented Jan 31, 2022

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!

michalno1 Feb 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonasingh46 Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jackfrancis left a comment

Choose a reason for hiding this comment

Uh oh!

shysank left a comment

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!

michalno1 commented Jan 28, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Jan 28, 2022 •

edited

Loading

linux-foundation-easycla Bot commented Jan 28, 2022 •

edited

Loading

michalno1 Feb 4, 2022 •

edited

Loading

sonasingh46 Jan 31, 2022 •

edited

Loading

k8s-ci-robot commented Feb 23, 2022 •

edited

Loading