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.

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

Add DisableExtensionOperations field #4792

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions api/v1beta1/azuremachine_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,12 @@ type AzureMachineSpec struct {
// +optional
DNSServers []string `json:"dnsServers,omitempty"`

// DisableExtensionOperations specifies whether extension operations should be disabled on the virtual machine.
// Use this setting only if VMExtensions are not supported by your image, as it disables CAPZ bootstrapping extension used for detecting Kubernetes bootstrap failure.
// This may only be set to True when no extensions are configured on the virtual machine.
// +optional
DisableExtensionOperations *bool `json:"disableExtensionOperations,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking this field should be immutable since it doesn't make too much sense to disable extensions after the VM is already provisioned. Can you add an immutability check in ValidateUpdate? https://github.com/willie-yao/cluster-api-provider-azure/blob/5243ae7d5dae9483280e73da2904a3568fbf8839/api/v1beta1/azuremachine_webhook.go#L77

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out. I thought that the entire spec was immutable and did not check. Fixed.


// VMExtensions specifies a list of extensions to be added to the virtual machine.
// +optional
VMExtensions []VMExtension `json:"vmExtensions,omitempty"`
Expand Down
16 changes: 16 additions & 0 deletions api/v1beta1/azuremachine_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/google/uuid"
"golang.org/x/crypto/ssh"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/utils/ptr"
azureutil "sigs.k8s.io/cluster-api-provider-azure/util/azure"
)

Expand Down Expand Up @@ -71,6 +72,10 @@ func ValidateAzureMachineSpec(spec AzureMachineSpec) field.ErrorList {
allErrs = append(allErrs, errs...)
}

if errs := ValidateVMExtensions(spec.DisableExtensionOperations, spec.VMExtensions, field.NewPath("vmExtensions")); len(errs) > 0 {
allErrs = append(allErrs, errs...)
}

return allErrs
}

Expand Down Expand Up @@ -471,3 +476,14 @@ func ValidateCapacityReservationGroupID(capacityReservationGroupID *string, fldP

return allErrs
}

// ValidateVMExtensions validates the VMExtensions spec.
func ValidateVMExtensions(disableExtensionOperations *bool, vmExtensions []VMExtension, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if ptr.Deref(disableExtensionOperations, false) && len(vmExtensions) > 0 {
allErrs = append(allErrs, field.Forbidden(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "VMExtensions"), "VMExtensions must be empty when DisableExtensionOperations is true"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Judging by the logic here, it seems like this check is unnecessary. If the goal is to disable the default extension, a flag to explicitly do that would be better. That way, it's still possible to specify additional extensions without using the default one, and avoid confusion about the two conflicting fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent of disableExtensionOperations is to disable all VMExtension for the machine. This check is run in webhook to reject the machine template. Setting disableExtensionOperations true and configuring VMExtension would at best result with no VMExtension deplyoed (unexpteced behavior) and at worst machine failing to provision.

}

return allErrs
}
7 changes: 7 additions & 0 deletions api/v1beta1/azuremachine_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,13 @@ func (mw *azureMachineWebhook) ValidateUpdate(ctx context.Context, oldObj, newOb
allErrs = append(allErrs, err)
}

if err := webhookutils.ValidateImmutable(
field.NewPath("spec", "disableExtensionOperations"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
field.NewPath("spec", "disableExtensionOperations"),
field.NewPath("spec", "DisableExtensionOperations"),

This should also be fixed for capacityReservationGroupID on line 210. Sorry for not catching this in the previous PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to use the capitalized variant. capacityReservationGroupID should be fixed in its own PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why with a capital? This breaks Kube API conventions. The path is supposed to take the json serialisation representation of the field name, so a camelCase is correct.

Adding DisableExtensionOperations with a capital means the error that comes back prints an incorrect JSON path

The capacityReservationGroupID is correct and should not be changed

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm you're definitely correct. I'm seeing some inconsistencies in the use of field.NewPath() in CAPZ and that should definitely be fixed. Apologies for the confusion there!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This PR is changed back to camelCase.

old.Spec.DisableExtensionOperations,
m.Spec.DisableExtensionOperations); err != nil {
allErrs = append(allErrs, err)
}

if len(allErrs) == 0 {
return nil, nil
}
Expand Down
63 changes: 63 additions & 0 deletions api/v1beta1/azuremachine_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,16 @@ func TestAzureMachine_ValidateCreate(t *testing.T) {
machine: createMachineWithCapacityReservaionGroupID("invalid-capacity-group-id"),
wantErr: true,
},
{
name: "azuremachine with DisableExtensionOperations true and without VMExtensions",
machine: createMachineWithDisableExtenionOperations(),
wantErr: false,
},
{
name: "azuremachine with DisableExtensionOperations true and with VMExtension",
machine: createMachineWithDisableExtenionOperationsAndHasExtension(),
wantErr: true,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
Expand Down Expand Up @@ -778,6 +788,34 @@ func TestAzureMachine_ValidateUpdate(t *testing.T) {
},
wantErr: true,
},
{
name: "invalidTest: azuremachine.spec.disableExtensionOperations is immutable",
oldMachine: &AzureMachine{
Spec: AzureMachineSpec{
DisableExtensionOperations: ptr.To(true),
},
},
newMachine: &AzureMachine{
Spec: AzureMachineSpec{
DisableExtensionOperations: ptr.To(false),
},
},
wantErr: true,
},
{
name: "validTest: azuremachine.spec.disableExtensionOperations is immutable",
oldMachine: &AzureMachine{
Spec: AzureMachineSpec{
DisableExtensionOperations: ptr.To(true),
},
},
newMachine: &AzureMachine{
Spec: AzureMachineSpec{
DisableExtensionOperations: ptr.To(true),
},
},
wantErr: false,
},
{
name: "validTest: azuremachine.spec.networkInterfaces is immutable",
oldMachine: &AzureMachine{
Expand Down Expand Up @@ -1154,3 +1192,28 @@ func createMachineWithCapacityReservaionGroupID(capacityReservationGroupID strin
},
}
}

func createMachineWithDisableExtenionOperationsAndHasExtension() *AzureMachine {
return &AzureMachine{
Spec: AzureMachineSpec{
SSHPublicKey: validSSHPublicKey,
OSDisk: validOSDisk,
DisableExtensionOperations: ptr.To(true),
VMExtensions: []VMExtension{{
Name: "test-extension",
Publisher: "test-publiher",
Version: "v0.0.1-test",
}},
},
}
}

func createMachineWithDisableExtenionOperations() *AzureMachine {
return &AzureMachine{
Spec: AzureMachineSpec{
SSHPublicKey: validSSHPublicKey,
OSDisk: validOSDisk,
DisableExtensionOperations: ptr.To(true),
},
}
}
5 changes: 5 additions & 0 deletions api/v1beta1/azuremachinetemplate_webhook.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/utils/ptr"
"sigs.k8s.io/cluster-api/util/topology"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/webhook"
Expand Down Expand Up @@ -85,6 +86,10 @@ func (r *AzureMachineTemplate) ValidateCreate(ctx context.Context, obj runtime.O
}
}

if ptr.Deref(r.Spec.Template.Spec.DisableExtensionOperations, false) && len(r.Spec.Template.Spec.VMExtensions) > 0 {
allErrs = append(allErrs, field.Forbidden(field.NewPath("AzureMachineTemplate", "spec", "template", "spec", "VMExtensions"), "VMExtensions must be empty when DisableExtensionOperations is true"))
}

if len(allErrs) == 0 {
return nil, nil
}
Expand Down
10 changes: 10 additions & 0 deletions api/v1beta1/azuremachinetemplate_webhook_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,16 @@ func TestAzureMachineTemplate_ValidateCreate(t *testing.T) {
machineTemplate: createAzureMachineTemplateFromMachine(createMachineWithRoleAssignmentName()),
wantErr: true,
},
{
name: "azuremachinetemplate with DisableExtensionOperations true and without VMExtensions",
machineTemplate: createAzureMachineTemplateFromMachine(createMachineWithDisableExtenionOperations()),
wantErr: false,
},
{
name: "azuremachinetempalte with DisableExtensionOperations true and with VMExtension",
machineTemplate: createAzureMachineTemplateFromMachine(createMachineWithDisableExtenionOperationsAndHasExtension()),
wantErr: true,
},
{
name: "azuremachinetemplate without RoleAssignmentName",
machineTemplate: createAzureMachineTemplateFromMachine(createMachineWithoutRoleAssignmentName()),
Expand Down
5 changes: 5 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions azure/scope/machine.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@
SpotVMOptions: m.AzureMachine.Spec.SpotVMOptions,
SecurityProfile: m.AzureMachine.Spec.SecurityProfile,
DiagnosticsProfile: m.AzureMachine.Spec.Diagnostics,
DisableExtensionOperations: ptr.Deref(m.AzureMachine.Spec.DisableExtensionOperations, false),

Check warning on line 181 in azure/scope/machine.go

View check run for this annotation

Codecov / codecov/patch

azure/scope/machine.go#L181

Added line #L181 was not covered by tests
AdditionalTags: m.AdditionalTags(),
AdditionalCapabilities: m.AzureMachine.Spec.AdditionalCapabilities,
CapacityReservationGroupID: m.GetCapacityReservationGroupID(),
Expand Down Expand Up @@ -374,6 +375,10 @@

// VMExtensionSpecs returns the VM extension specs.
func (m *MachineScope) VMExtensionSpecs() []azure.ResourceSpecGetter {
if ptr.Deref(m.AzureMachine.Spec.DisableExtensionOperations, false) {
return []azure.ResourceSpecGetter{}
}

var extensionSpecs = []azure.ResourceSpecGetter{}
for _, extension := range m.AzureMachine.Spec.VMExtensions {
extensionSpecs = append(extensionSpecs, &vmextensions.VMExtensionSpec{
Expand Down
38 changes: 38 additions & 0 deletions azure/scope/machine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,44 @@ func TestMachineScope_VMExtensionSpecs(t *testing.T) {
},
},
},
{
name: "If OS type is Linux and cloud is AzurePublicCloud and DisableExtensionOperations is true, it returns empty",
machineScope: MachineScope{
Machine: &clusterv1.Machine{},
AzureMachine: &infrav1.AzureMachine{
ObjectMeta: metav1.ObjectMeta{
Name: "machine-name",
},
Spec: infrav1.AzureMachineSpec{
DisableExtensionOperations: ptr.To(true),
OSDisk: infrav1.OSDisk{
OSType: "Linux",
},
},
},
ClusterScoper: &ClusterScope{
AzureClients: AzureClients{
EnvironmentSettings: auth.EnvironmentSettings{
Environment: azureautorest.Environment{
Name: azureautorest.PublicCloud.Name,
},
},
},
AzureCluster: &infrav1.AzureCluster{
Spec: infrav1.AzureClusterSpec{
ResourceGroup: "my-rg",
AzureClusterClassSpec: infrav1.AzureClusterClassSpec{
Location: "westus",
},
},
},
},
cache: &MachineCache{
VMSKU: resourceskus.SKU{},
},
},
want: []azure.ResourceSpecGetter{},
},
{
name: "If OS type is Linux and cloud is not AzurePublicCloud, it returns empty",
machineScope: MachineScope{
Expand Down
8 changes: 5 additions & 3 deletions azure/services/virtualmachines/spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ type VMSpec struct {
AdditionalTags infrav1.Tags
AdditionalCapabilities *infrav1.AdditionalCapabilities
DiagnosticsProfile *infrav1.Diagnostics
DisableExtensionOperations bool
CapacityReservationGroupID string
SKU resourceskus.SKU
Image *infrav1.Image
Expand Down Expand Up @@ -263,9 +264,10 @@ func (s *VMSpec) generateOSProfile() (*armcompute.OSProfile, error) {
}

osProfile := &armcompute.OSProfile{
ComputerName: ptr.To(s.Name),
AdminUsername: ptr.To(azure.DefaultUserName),
CustomData: ptr.To(s.BootstrapData),
ComputerName: ptr.To(s.Name),
AdminUsername: ptr.To(azure.DefaultUserName),
CustomData: ptr.To(s.BootstrapData),
AllowExtensionOperations: ptr.To(!s.DisableExtensionOperations),
}

switch s.OSDisk.OSType {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,12 @@ spec:
- storageAccountType
type: object
type: object
disableExtensionOperations:
description: |-
DisableExtensionOperations specifies whether extension operations should be disabled on the virtual machine.
Use this setting only if VMExtensions are not supported by your image, as it disables CAPZ bootstrapping extension used for detecting Kubernetes bootstrap failure.
This may only be set to True when no extensions are configured on the virtual machine.
type: boolean
dnsServers:
description: DNSServers adds a list of DNS Server IP addresses to
the VM NICs.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,12 @@ spec:
- storageAccountType
type: object
type: object
disableExtensionOperations:
description: |-
DisableExtensionOperations specifies whether extension operations should be disabled on the virtual machine.
Use this setting only if VMExtensions are not supported by your image, as it disables CAPZ bootstrapping extension used for detecting Kubernetes bootstrap failure.
This may only be set to True when no extensions are configured on the virtual machine.
type: boolean
dnsServers:
description: DNSServers adds a list of DNS Server IP addresses
to the VM NICs.
Expand Down