-
Notifications
You must be signed in to change notification settings - Fork 205
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
Bug 1876680: Reduce influence of webhooks and convert some errors to non-fatal #697
Bug 1876680: Reduce influence of webhooks and convert some errors to non-fatal #697
Conversation
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.
We should do a separate PR for k8s version bump.
For instance, openshift/client-go looks like it should be release-4.6 but we're using something much older.
return admission.Denied(err.Error()) | ||
ok, warnings, errs := h.webhookOperations(m, h.clusterID) | ||
if !ok { | ||
return responseWithWarnings(admission.Denied(errs.Error()), warnings) |
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 will result in a denial still?
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.
Indeed, the intention here is that within the webhookOperations
we should be able to define fatal and non-fatal errors.
Fatal errors are added to errs
, non fatal errors are added to warnings
.
If there are any fatal errors, we return and admission.Denied
as we currently do for any error.
If there are no fatal errors, we return admission.Allowed
as we currently do for no errors.
However, in both cases, we can also return non-fatal errors (warnings), which will allow the request to be allowed or denied as appropriate, but send those warnings back to the user even though it is accepted.
For a worked example, imagine everything is happy, as a user I try to set the disk size to 60gb, the validation logic thinks this should be 120gb or more so sets a warning, but there are no other errors, so it returns allowed with a warning disk size should be greater than 120gb
, the request to update from the user was accepted, but kubectl shows them this warning.
} | ||
|
||
return admission.Allowed("Machine valid") | ||
return responseWithWarnings(admission.Allowed("Machine valid"), warnings) |
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 seems confusing to me. We're saying it's valid, but also there may be warnings?
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.
+1
3ae6a68
to
0a3ceb2
Compare
func responseWithWarnings(response admission.Response, warnings []string) admission.Response { | ||
response.AdmissionResponse.Warnings = warnings | ||
return response | ||
} | ||
|
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 intend for this to be replaced by kubernetes-sigs/controller-runtime#1157 once it is merged
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.
may be add a TODO comment
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.
Since it's merged, and to keep the scope of this PR reduced, I'm gonna raise a follow up PR that we can keep open until a point at which we are happy to merge it rather than adding the todo
} | ||
if providerSpec.OSDisk.ManagedDisk.StorageAccountType == "" { | ||
errs = append(errs, field.Required(field.NewPath("providerSpec", "osDisk", "managedDisk", "storageAccountType"), "storageAccountType must be provided")) | ||
if providerSpec.OSDisk.DiskSizeGB <= 0 || providerSpec.OSDisk.DiskSizeGB >= 32768{ |
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.
move this into a constant 32768?
Missing space before {
workspaceWarnings, workspaceErrors := validateVSphereWorkspace(providerSpec.Workspace, field.NewPath("providerSpec", "workspace")) | ||
warnings = append(warnings, workspaceWarnings...) | ||
errs = append(errs, workspaceErrors...) | ||
|
||
errs = append(errs, validateVSphereNetwork(providerSpec.Network, field.NewPath("providerSpec", "network"))...) | ||
|
||
if providerSpec.NumCPUs != 0 && providerSpec.NumCPUs < minVSphereCPU { |
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.
shouldn't we remove the if providerSpec.NumCPUs != 0
check? so wouldn't providerSpec.NumCPUs < minVSphereCPU
be enough?
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.
Yep, good point, will fix
can we also drop |
We will enable this after openshift/machine-api-operator#697 get merged
/retitle Bug 1876680: Reduce influence of webhooks and convert some errors to non-fatal |
@JoelSpeed: This pull request references Bugzilla bug 1876680, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
In my testing, if there was no GCP Service Account then the node wouldn't join the cluster. Based on what I've done in this PR so far, I'd suggest making this a warning if changing at all, WDYT? |
0a3ceb2
to
50183c8
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The webhooks have been provided opinionated defaults and requiring these in validation. This may cause issues for existing users and as such, we need to reel back on these changes and ensure that we only error when a value is missing that would cause a Machine to not be created successfully
The webhooks have been provided opinionated defaults and requiring these in validation. This may cause issues for existing users and as such, we need to reel back on these changes and ensure that we only error when a value is missing that would cause a Machine to not be created successfully
The webhooks have been provided opinionated defaults and requiring these in validation. This may cause issues for existing users and as such, we need to reel back on these changes and ensure that we only error when a value is missing that would cause a Machine to not be created successfully
The webhooks have been provided opinionated defaults and requiring these in validation. This may cause issues for existing users and as such, we need to reel back on these changes and ensure that we only error when a value is missing that would cause a Machine to not be created successfully
50183c8
to
0b89f8d
Compare
We will enable this after openshift/machine-api-operator#697 get merged
/retest |
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 generally looks good to me, i'm withholding adding the label though because i couldn't till if there were still changes requested. happy to revisit if there are no more questions.
/retest |
/lgtm |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
@JoelSpeed: All pull requests linked via external trackers have merged: Bugzilla bug 1876680 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This should allow our validation/defaulting webhooks to respond with some of their validations as warnings rather than errors.
The webhooks have been provided opinionated defaults and requiring these in validation. This may cause issues for existing users and as such, we need to reel back on these changes and ensure that we only error when a value is missing that would cause a Machine to not be created successfully.
This PR should reduce the opinionation and ensure that we don't break any existing installations