-
Notifications
You must be signed in to change notification settings - Fork 471
Support passing custom headers to AKS Managed Cluster and Node Pool create/update requests #2020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -391,6 +391,10 @@ func (s *ManagedControlPlaneScope) FailureDomains() []string { | |
| return []string{} | ||
| } | ||
|
|
||
| func (s *ManagedControlPlaneScope) ManagedClusterAnnotations() map[string]string { | ||
| return s.ControlPlane.Annotations | ||
| } | ||
|
|
||
| // ManagedClusterSpec returns the managed cluster spec. | ||
| func (s *ManagedControlPlaneScope) ManagedClusterSpec() (azure.ManagedClusterSpec, error) { | ||
| decodedSSHPublicKey, err := base64.StdEncoding.DecodeString(s.ControlPlane.Spec.SSHPublicKey) | ||
|
|
@@ -557,6 +561,10 @@ func (s *ManagedControlPlaneScope) GetAllAgentPoolSpecs(ctx context.Context) ([] | |
| return ammps, nil | ||
| } | ||
|
|
||
| func (s *ManagedControlPlaneScope) AgentPoolAnnotations() map[string]string { | ||
| return s.InfraMachinePool.Annotations | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above |
||
| // AgentPoolSpec returns an azure.AgentPoolSpec for currently reconciled AzureManagedMachinePool. | ||
| func (s *ManagedControlPlaneScope) AgentPoolSpec() azure.AgentPoolSpec { | ||
| return buildAgentPoolSpec(s.ControlPlane, s.MachinePool, s.InfraMachinePool) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,7 @@ import ( | |
| "github.com/pkg/errors" | ||
| infrav1alpha4 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" | ||
| "sigs.k8s.io/cluster-api-provider-azure/azure" | ||
| "sigs.k8s.io/cluster-api-provider-azure/util/maps" | ||
| "sigs.k8s.io/cluster-api-provider-azure/util/tele" | ||
| ) | ||
|
|
||
|
|
@@ -35,6 +36,7 @@ type ManagedMachinePoolScope interface { | |
| azure.ClusterDescriber | ||
|
|
||
| NodeResourceGroup() string | ||
| AgentPoolAnnotations() map[string]string | ||
| AgentPoolSpec() azure.AgentPoolSpec | ||
| SetAgentPoolProviderIDList([]string) | ||
| SetAgentPoolReplicas(int32) | ||
|
|
@@ -64,7 +66,6 @@ func (s *Service) Reconcile(ctx context.Context) error { | |
| defer done() | ||
|
|
||
| agentPoolSpec := s.scope.AgentPoolSpec() | ||
|
|
||
| profile := containerservice.AgentPool{ | ||
| ManagedClusterAgentPoolProfileProperties: &containerservice.ManagedClusterAgentPoolProfileProperties{ | ||
| VMSize: &agentPoolSpec.SKU, | ||
|
|
@@ -96,8 +97,10 @@ func (s *Service) Reconcile(ctx context.Context) error { | |
| // AKS will populate defaults and read-only values, which we want | ||
| // to strip/clean to match what we expect. | ||
|
|
||
| customHeaders := maps.FilterByKeyPrefix(s.scope.AgentPoolAnnotations(), azure.CustomHeaderPrefix) | ||
| if isCreate := azure.ResourceNotFound(err); isCreate { | ||
| err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile) | ||
| err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, | ||
| profile, customHeaders) | ||
| if err != nil && azure.ResourceNotFound(err) { | ||
| return azure.WithTransientError(errors.Wrap(err, "agent pool dependent resource does not exist yet"), 20*time.Second) | ||
| } else if err != nil { | ||
|
|
@@ -138,7 +141,8 @@ func (s *Service) Reconcile(ctx context.Context) error { | |
| diff := cmp.Diff(normalizedProfile, existingProfile) | ||
| 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, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we enable/disable features after node pool creation?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PR right now allows for updates, although not intentionally. If there is a non zero
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shysank added immutability check |
||
| profile, customHeaders) | ||
| if err != nil { | ||
| return errors.Wrap(err, "failed to create or update agent pool") | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,7 @@ import ( | |
| type Client interface { | ||
| Get(context.Context, string, string) (containerservice.ManagedCluster, error) | ||
| GetCredentials(context.Context, string, string) ([]byte, error) | ||
| CreateOrUpdate(context.Context, string, string, containerservice.ManagedCluster) (containerservice.ManagedCluster, error) | ||
| CreateOrUpdate(context.Context, string, string, containerservice.ManagedCluster, map[string]string) (containerservice.ManagedCluster, error) | ||
| Delete(context.Context, string, string) error | ||
| } | ||
|
|
||
|
|
@@ -78,11 +78,19 @@ func (ac *AzureClient) GetCredentials(ctx context.Context, resourceGroupName, na | |
| } | ||
|
|
||
| // CreateOrUpdate creates or updates a managed cluster. | ||
| func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, name string, cluster containerservice.ManagedCluster) (containerservice.ManagedCluster, error) { | ||
| func (ac *AzureClient) CreateOrUpdate(ctx context.Context, resourceGroupName, name string, cluster containerservice.ManagedCluster, headers map[string]string) (containerservice.ManagedCluster, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is another case where it won't work super well with the async interface once we move this func to async... @shysank @Jont828 @sonasingh46 For now I think it's fine to add it in here as-is but we'll need to have a way to expand ResourceSpecGetter when custom fields are needed like here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually for this one particularly @jackfrancis pointed out it would be something useful to have for all clients, so we could just add it as a generic capability of ResourceSpecGetter (ie.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right. Also, I had some ideas on this one that I pasted on slack sometime back. I will open up an issue/discussion -- highlighting the challenges that we faced and we can actually brainstorm/groom to get to some solution. |
||
| ctx, _, done := tele.StartSpanWithLogger(ctx, "managedclusters.AzureClient.CreateOrUpdate") | ||
| defer done() | ||
|
|
||
| future, err := ac.managedclusters.CreateOrUpdate(ctx, resourceGroupName, name, cluster) | ||
| preparer, err := ac.managedclusters.CreateOrUpdatePreparer(ctx, resourceGroupName, name, cluster) | ||
| if err != nil { | ||
| return containerservice.ManagedCluster{}, errors.Wrap(err, "failed to prepare operation") | ||
| } | ||
| for key, value := range headers { | ||
| preparer.Header.Add(key, value) | ||
| } | ||
|
|
||
| future, err := ac.managedclusters.CreateOrUpdateSender(preparer) | ||
| if err != nil { | ||
| return containerservice.ManagedCluster{}, errors.Wrap(err, "failed to begin operation") | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ import ( | |
| "k8s.io/klog/v2" | ||
| infrav1alpha4 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" | ||
| "sigs.k8s.io/cluster-api-provider-azure/azure" | ||
| "sigs.k8s.io/cluster-api-provider-azure/util/maps" | ||
| "sigs.k8s.io/cluster-api-provider-azure/util/tele" | ||
| clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" | ||
| ) | ||
|
|
@@ -42,6 +43,7 @@ var ( | |
| // ManagedClusterScope defines the scope interface for a managed cluster. | ||
| type ManagedClusterScope interface { | ||
| azure.ClusterDescriber | ||
| ManagedClusterAnnotations() map[string]string | ||
| ManagedClusterSpec() (azure.ManagedClusterSpec, error) | ||
| GetAllAgentPoolSpecs(ctx context.Context) ([]azure.AgentPoolSpec, error) | ||
| SetControlPlaneEndpoint(clusterv1.APIEndpoint) | ||
|
|
@@ -292,8 +294,9 @@ func (s *Service) Reconcile(ctx context.Context) error { | |
| } | ||
| } | ||
|
|
||
| customHeaders := maps.FilterByKeyPrefix(s.Scope.ManagedClusterAnnotations(), azure.CustomHeaderPrefix) | ||
| if isCreate { | ||
| managedCluster, err = s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroupName, managedClusterSpec.Name, managedCluster) | ||
| managedCluster, err = s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroupName, managedClusterSpec.Name, managedCluster, customHeaders) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create managed cluster, %w", err) | ||
| } | ||
|
|
@@ -322,7 +325,7 @@ func (s *Service) Reconcile(ctx context.Context) error { | |
| diff := computeDiffOfNormalizedClusters(managedCluster, existingMC) | ||
| 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar comment as above regarding enabling/disabling features after cluster creation?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shysank added validation webhook with immutability check to AzureManagedCluster as well. |
||
| if err != nil { | ||
| return fmt.Errorf("failed to update managed cluster, %w", err) | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Do we need this function? Can't we simply use
s.ControlPlane.Annotationsat places wherever required?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see why it is so. You basically implemented it as part of
ManagedMachinePoolScopeinterface.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular method implements
ManagedClusterScoperather thanManagedMachinePoolScope. This is done in the same spirit asManagedControlPlaneScope.ManagedClusterSpec().