-
Notifications
You must be signed in to change notification settings - Fork 425
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
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 |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -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 | ||
} | ||
|
||
|
@@ -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")) | ||
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. 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. 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 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 | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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"), | ||||||
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.
Suggested change
This should also be fixed for 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. Changed to use the capitalized variant. 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. 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 Adding The 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. Hmm you're definitely correct. I'm seeing some inconsistencies in the use of 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 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 | ||||||
} | ||||||
|
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.
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#L77There 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.
Thanks for pointing this out. I thought that the entire spec was immutable and did not check. Fixed.