-
Notifications
You must be signed in to change notification settings - Fork 5
feat(vm): validate cloud-init userdata #1937
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
base: main
Are you sure you want to change the base?
Conversation
d5074c3 to
150f8a7
Compare
|
Workflow has started. The target step completed with status: failure. |
150f8a7 to
4c1d8d4
Compare
Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
d831e4b to
298e709
Compare
Signed-off-by: Roman Sysoev <roman.sysoev@flant.com>
298e709 to
c907d80
Compare
| return nil | ||
| } | ||
|
|
||
| func (p *ProvisioningService) validateSysprepSecret(_ *corev1.Secret) error { |
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.
Overengineering. Code that does nothing. Remove everything unnecessary
Keep It Simple
| } | ||
|
|
||
| func (p *ProvisioningService) ValidateUserDataLen(userData string) error { | ||
| if userData == "" { |
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.
trim it before validation
| validate := len(checkKeys) == 0 | ||
| for _, key := range checkKeys { | ||
| if _, ok := secret.Data[key]; ok { | ||
| validate = true |
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.
The variable is named incorrectly, and you can actually get rid of it
Keep It Simple
| err := p.reader.Get(ctx, key, secret) | ||
| if err != nil { | ||
| if k8serrors.IsNotFound(err) { | ||
| return SecretNotFoundError(key.String()) |
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.
Overengineering. The error looks cool, but you’re not using its advantages. You’re just putting it into a condition afterward. Just return a simple fmt.Errorf
Keep It Simple
| provisioningService *service.ProvisioningService | ||
| } | ||
|
|
||
| func NewProvisioningValidator(provisioningService *service.ProvisioningService) *ProvisioningValidator { |
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.
It looks like you can use CEL rules instead
| reader client.Reader | ||
| } | ||
|
|
||
| func NewProvisioningService(reader client.Reader) *ProvisioningService { |
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.
Why is this here and not inside the vm directory?
|
|
||
| func (p *ProvisioningService) validateCloudInitSecret(secret *corev1.Secret) error { | ||
| if !p.hasOneOfKeys(secret, cloudInitCheckKeys...) { | ||
| return fmt.Errorf("the secret should have one of data fields %v: %w", cloudInitCheckKeys, ErrSecretIsNotValid) |
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.
"the specified provisioning secret %q must have at least one of the following fields: %v: %w"
| builder.Status(metav1.ConditionFalse). | ||
| Reason(vmcondition.ReasonProvisioningNotReady). | ||
| Message(service.CapitalizeFirstLetter(err.Error())) | ||
| return nil |
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 is a bug. It’s not the user’s fault. Shouldn’t this be validated at the CRD level?
| case errors.Is(err, service.ErrSecretIsNotValid): | ||
| builder.Status(metav1.ConditionFalse). | ||
| Reason(vmcondition.ReasonProvisioningNotReady). | ||
| Message(fmt.Sprintf("Invalid secret %q: %s", secretKey.String(), err.Error())) |
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.
It should already be clear from err.Error() that the secret failed validation. No need to duplicate this "Invalid secret"
| } else { | ||
| err := h.provisioningService.ValidateUserDataLen(p.UserData) | ||
| if err != nil { | ||
| errMsg := fmt.Errorf("failed to validate userdata length: %w", err) |
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.
err = fmt.Errorf("failed to validate userdata length: %w", err)
No need to create one more variable
Description
Cloud-init userdata in the virtual machine specification should not be empty or exceed 2048 KB.
Why do we need it, and what problem does it solve?
These changes are required to prevent the virtual machine from entering a pending status due to incorrect cloud-init userdata.
What is the expected result?
Webhook prevents virtual machine creation or update with incorrect cloud-init userdata and Provisioning Handler return an error if resource was created with invalidated data.
Checklist
Changelog entries