Skip to content
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.

Sign up for GitHub

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

🐛 normalize version in managed control plane #818

Merged
merged 2 commits into from
Jul 28, 2020
+14 −3
Merged

🐛 normalize version in managed control plane #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f28c6f
:bug: normalize version in managed control plane
alexeldeib Jul 22, 2020
38303cf
:bug: normalize managed machinepool version too
alexeldeib Jul 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremanagedcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ spec:
version:
description: Version defines the desired Kubernetes version.
minLength: 2
pattern: ^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
type: string
required:
- defaultPoolRef
Expand Down
1 change: 0 additions & 1 deletion exp/api/v1alpha3/azuremanagedcontrolplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
type AzureManagedControlPlaneSpec struct {
// Version defines the desired Kubernetes version.
// +kubebuilder:validation:MinLength:=2
// +kubebuilder:validation:Pattern:=^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)([-0-9a-zA-Z_\.+]*)?$
CecileRobertMichon marked this conversation as resolved.
Show resolved Hide resolved
Version string `json:"version"`

// ResourceGroup is the name of the Azure resource group for this AKS Cluster.
Expand Down
10 changes: 9 additions & 1 deletion exp/controllers/azuremanagedmachinepool_reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package controllers
import (
"context"
"fmt"
"strings"

"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
"github.com/pkg/errors"
Expand Down Expand Up @@ -54,13 +55,20 @@ func newAzureManagedMachinePoolReconciler(scope *scope.ManagedControlPlaneScope)
// Reconcile reconciles all the services in pre determined order
func (r *azureManagedMachinePoolReconciler) Reconcile(ctx context.Context, scope *scope.ManagedControlPlaneScope) error {
scope.Logger.Info("reconciling machine pool")

var normalizedVersion *string
if scope.MachinePool.Spec.Template.Spec.Version != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this go in a webhook?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you suggesting a default version?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that way the normalized version appears in the spec and it's not too hidden from the user, same as https://github.com/CecileRobertMichon/cluster-api/blob/master/api/v1alpha3/machine_webhook.go#L60

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webhooks are not currently setup for AzureManged*. Can we open an issue to track this and pull in these changes?

Copy link
Contributor Author

@alexeldeib alexeldeib Jul 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#615
#612

I can add a defaulting webhook to AzureManagedMachinePool in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was part of the changes in kubernetes-sigs/cluster-api#3147, the Version isn't part of MachinePool spec, it's part of Machine spec which is embedded in the MachinePool spec as part of the template https://github.com/kubernetes-sigs/cluster-api/blob/master/exp/api/v1alpha3/machinepool_types.go#L44

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So actually do we even need this normalization here? Since the Machine webhook already takes care of it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For AzureManagedControlPlane Version is part of the object https://github.com/kubernetes-sigs/cluster-api-provider-azure/pull/818/files#diff-69a80dafa12ba6b9eeaf233d609e8f5eR29 so we could do the same normalization in a webhook, for AzureMachinePool I believe it's already taken care of by the Machine webhooks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so after some coffee and thinking through it with @devigned I think we need to:

  1. keep the AzureMachinePool normalization until we can move it into the capi MachinePool webhook
  2. Add a webhook normalization for AzureManagedControlPlane, either here or in a separate PR

Let's track whatever we don't get done here in issues so they don't get lost.

I take back what I said above, the Machine webhook doesn't do anything for MachinePool, only the schema validation is shared.

v := strings.TrimPrefix(*scope.MachinePool.Spec.Template.Spec.Version, "v")
normalizedVersion = &v
}

agentPoolSpec := &agentpools.Spec{
Name: scope.InfraMachinePool.Name,
ResourceGroup: scope.ControlPlane.Spec.ResourceGroup,
Cluster: scope.ControlPlane.Name,
SKU: scope.InfraMachinePool.Spec.SKU,
Replicas: 1,
Version: scope.MachinePool.Spec.Template.Spec.Version,
Version: normalizedVersion,
}

if scope.InfraMachinePool.Spec.OSDiskSizeGB != nil {
Expand Down
5 changes: 5 additions & 0 deletions exp/controllers/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,11 @@ func AzureManagedClusterToAzureManagedControlPlaneMapper(c client.Client, log lo
return nil
}

if cluster == nil {
log.Error(err, "cluster has not set owner ref yet")
return nil
}

ref := cluster.Spec.ControlPlaneRef
if ref == nil || ref.Name == "" {
return nil
Expand Down