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

Replace Paid SKU tier with Standard #4045

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Replace Paid SKU tier with Standard #4045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion api/v1beta1/azuremanagedcontrolplane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,14 +262,17 @@ type AddonProfile struct {
}

// AzureManagedControlPlaneSkuTier - Tier of a managed cluster SKU.
// +kubebuilder:validation:Enum=Free;Paid
// +kubebuilder:validation:Enum=Free;Paid;Standard
type AzureManagedControlPlaneSkuTier string

const (
// FreeManagedControlPlaneTier is the free tier of AKS without corresponding SLAs.
FreeManagedControlPlaneTier AzureManagedControlPlaneSkuTier = "Free"
// PaidManagedControlPlaneTier is the paid tier of AKS with corresponding SLAs.
// Deprecated. It has been replaced with StandardManagedControlPlaneTier.
PaidManagedControlPlaneTier AzureManagedControlPlaneSkuTier = "Paid"
// StandardManagedControlPlaneTier is the standard tier of AKS with corresponding SLAs.
StandardManagedControlPlaneTier AzureManagedControlPlaneSkuTier = "Standard"
)

// AKSSku - AKS SKU.
Expand Down
6 changes: 6 additions & 0 deletions api/v1beta1/azuremanagedcontrolplane_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,12 @@ func (mw *azureManagedControlPlaneWebhook) Default(ctx context.Context, obj runt
ctrl.Log.WithName("AzureManagedControlPlaneWebHookLogger").Error(err, "setDefaultSSHPublicKey failed")
}

// PaidManagedControlPlaneTier has been replaced with StandardManagedControlPlaneTier since v2023-02-01.
if m.Spec.SKU != nil && m.Spec.SKU.Tier == PaidManagedControlPlaneTier {
m.Spec.SKU.Tier = StandardManagedControlPlaneTier
ctrl.Log.WithName("AzureManagedControlPlaneWebHookLogger").Info("Paid SKU tier is deprecated and has been replaced by Standard")
Copy link
Contributor

Choose a reason for hiding this comment

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

We can even use a webhook warning to warn the user that "Paid" is deprecated and has been replaced by "Standard" now that those are available

I think what Cecile was referring to here was to include this message in the admission.Warnings returned by ValidateCreate. It looks like controller-runtime doesn't allow us to return those warnings from the defaulting webhook.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. However, if we're changing the value in the defaulting webhook, we'll never print the warning since the validation webhook runs after defaulting (so by the time we reach ValidateCreate we would have already changed the value).

I think the way it's implemented now is probably good enough, wdyt @nojnhuh ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like it wouldn't be too messy to do the validation in the webhook and the Paid->Standard translation further down the road, but I agree keeping everything in the defaulting webhook works just as well.

}

m.setDefaultNodeResourceGroupName()
m.setDefaultVirtualNetwork()
m.setDefaultSubnet()
Expand Down
2 changes: 1 addition & 1 deletion api/v1beta1/azuremanagedcontrolplane_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func TestDefaultingWebhook(t *testing.T) {
g.Expect(amcp.Spec.NodeResourceGroupName).To(Equal("fooNodeRg"))
g.Expect(amcp.Spec.VirtualNetwork.Name).To(Equal("fooVnetName"))
g.Expect(amcp.Spec.VirtualNetwork.Subnet.Name).To(Equal("fooSubnetName"))
g.Expect(amcp.Spec.SKU.Tier).To(Equal(PaidManagedControlPlaneTier))
g.Expect(amcp.Spec.SKU.Tier).To(Equal(StandardManagedControlPlaneTier))
g.Expect(*amcp.Spec.OIDCIssuerProfile.Enabled).To(BeTrue())

t.Logf("Testing amcp defaulting webhook with overlay")
Expand Down
1 change: 1 addition & 0 deletions config/crd/bases/infrastructure.cluster.x-k8s.io_azuremanagedcontrolplanes.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,7 @@ spec:
enum:
- Free
- Paid
- Standard
Copy link
Contributor

Choose a reason for hiding this comment

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

@CecileRobertMichon Would it make sense here for the API to accept all of Free, Standard, and Paid and translate Paid to Standard under the hood? That way other users wouldn't have to immediately change.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

We can even use a webhook warning to warn the user that "Paid" is deprecated and has been replaced by "Standard" now that those are available

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Updated validation and webhook. Added deprecation comment to PaidManagedControlPlaneTier.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also modified TestDefaultingWebhook to cover webhook SKU tier update.

type: string
required:
- tier
Expand Down
2 changes: 1 addition & 1 deletion docs/book/src/topics/managedcluster.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ spec:
networkPolicy: azure # or calico
networkPlugin: azure # or kubenet
sku:
tier: Free # or Paid
tier: Free # or Standard
addonProfiles:
- name: azureKeyvaultSecretsProvider
enabled: true
Expand Down