-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Support dry run in admission webhooks #66936
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,22 @@ const ( | |
Fail FailurePolicyType = "Fail" | ||
) | ||
|
||
type SideEffectClass string | ||
|
||
const ( | ||
// SideEffectClassUnknown means that no information is known about the side effects of calling the webhook. | ||
// If a request with the dry-run attribute would trigger a call to this webhook, the request will instead fail. | ||
SideEffectClassUnknown SideEffectClass = "Unknown" | ||
// SideEffectClassNone means that calling the webhook will have no side effects. | ||
SideEffectClassNone SideEffectClass = "None" | ||
// SideEffectClassSome means that calling the webhook will possibly have side effects. | ||
// If a request with the dry-run attribute would trigger a call to this webhook, the request will instead fail. | ||
SideEffectClassSome SideEffectClass = "Some" | ||
// SideEffectClassNoneOnDryRun means that calling the webhook will possibly have side effects, but if the | ||
// request being reviewed has the dry-run attribute, the side effects will be suppressed. | ||
SideEffectClassNoneOnDryRun SideEffectClass = "NoneOnDryRun" | ||
) | ||
|
||
// +genclient | ||
// +genclient:nonNamespaced | ||
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object | ||
|
@@ -235,6 +251,15 @@ type Webhook struct { | |
// Default to the empty LabelSelector, which matches everything. | ||
// +optional | ||
NamespaceSelector *metav1.LabelSelector | ||
|
||
// SideEffects states whether this webhookk has side effects. | ||
// Acceptable values are: Unknown, None, Some, NoneOnDryRun | ||
// Webhooks with side effects MUST implement a reconciliation system, since a request may be | ||
// rejected by a future step in the admission change and the side effects therefore need to be undone. | ||
// Requests with the dryRun attribute will be auto-rejected if they match a webhook with | ||
// sideEffects == Unknown or Some. Defaults to Unknown. | ||
// +optional | ||
SideEffects *SideEffectClass | ||
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. did validation get added for this field? |
||
} | ||
|
||
// RuleWithOperations is a tuple of Operations and Resources. It is recommended to make | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,4 +35,9 @@ func SetDefaults_Webhook(obj *admissionregistrationv1beta1.Webhook) { | |
selector := metav1.LabelSelector{} | ||
obj.NamespaceSelector = &selector | ||
} | ||
if obj.SideEffects == nil { | ||
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. worth adding a TODO here (or in a "v1 graduation criteria" list if we have one) to revisit/remove this default and possibly make the field required when promoting to v1, so we don't forget |
||
// TODO: revisit/remove this default and possibly make the field required when promoting to v1 | ||
unknown := admissionregistrationv1beta1.SideEffectClassUnknown | ||
obj.SideEffects = &unknown | ||
} | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
And that the call must not have side effects.
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, what about something also indicating that a value of DryRun false doesn't guarantee that the object will be persisted, so any side effects should have a reconciliation mechanism, or is that redundant because it's already somewhere in the webhook documentation?
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 be documented, but feel free to state it again here if you think it makes it clearer.
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.
Okay, added this