-
Notifications
You must be signed in to change notification settings - Fork 40.2k
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
KEP-3488: Implement Enforcement Actions and Audit Annotations #115973
Conversation
/sig api-machinery |
/triage accepted |
d8c042d
to
ea772d8
Compare
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/interface.go
Outdated
Show resolved
Hide resolved
staging/src/k8s.io/apiserver/pkg/admission/plugin/validatingadmissionpolicy/policy_decision.go
Outdated
Show resolved
Hide resolved
ea772d8
to
d5537b1
Compare
This PR may require API review. If so, when the changes are ready, complete the pre-review checklist and request an API review. Status of requested reviews is tracked in the API Review project. |
@cici37 Feedback applied, thanks! |
/retest |
Does anything stop me from abusing this to write a well-known annotation key, or to try to fool someone into thinking I did? |
924a506
to
1d8bda2
Compare
Rebased and API review feedback applied! |
staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/compile.go
Outdated
Show resolved
Hide resolved
@@ -70,9 +70,6 @@ func (a *evaluationActivation) Parent() interpreter.Activation { | |||
|
|||
// Compile compiles the cel expressions defined in the ExpressionAccessors into a Filter | |||
func (c *filterCompiler) Compile(expressionAccessors []ExpressionAccessor, hasParam bool) Filter { | |||
if len(expressionAccessors) == 0 { | |||
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.
Why to remove this nil check?
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.
Now that we have both validations
and auditAnnotations
, it is possible for one of the calls to Compile to have an empty slice. We could write special case cost to avoid the compile call, but it is less error prone to make the compile calls safe by not returning an error here.
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.
Here is not really a err handling but rather a short circus. I am ok with either since it is nit and not a blocker any way :)
@@ -165,6 +170,8 @@ func (c *celAdmissionController) Run(stopCh <-chan struct{}) { | |||
wg.Wait() | |||
} | |||
|
|||
const maxAuditAnnotationValueLength = 10 * 1024 |
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 this is different than the number in validation?
Also, maybe have a comment to remind keeping it same as it is in validation if it should be the same.
A following up PR might be add this to celconfig file we used in #115747
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.
An expressions that concatenate strings will often produce a longer value than the expression, so the expression limit is set at 5kb and the annotation value limits is set at 10k.
type validationFailureValue struct { | ||
Message string `json:"message"` | ||
Policy string `json:"policy"` | ||
Binding string `json:"binding"` |
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.
Maybe use PolicyName
and BindingName
here since we only assign name not the entire policy/binding?
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 was trying to save space since I'm already concerned with the data volume produced by these audit annotation values. It should be clear enough? Let me know if you feel strongly about it.
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.
Just thought we have Policy/Binding all over and it's easily confused in terms of code maintenance.
It is not a merging blocker though :)
for i, evalResult := range auditAnnotationEvalResults { | ||
var auditAnnotationResult = &auditAnnotationResults[i] | ||
// TODO: move this to generics | ||
validation, ok := evalResult.ExpressionAccessor.(*AuditAnnotationCondition) |
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.
Is that possible that evalResult.ExpressionAccessor
is nil and evalResult.Error
saved the err?
Looks at the code path checks evalResult.ExpressionAccessor.(*AuditAnnotationCondition) before evalResult.Error
and the error might be ignored?
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 the same order as for validations
above. This !ok
check is a failure that should never be reached if the code is working as expected (and will go away when we switch to using generics here).
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.
A couple nits/suggestions, but API types, validation, and docs lgtm.
Once the compile/admission/impl bits are reviewed/approved, tag me back in and I can approve.
5df742e
to
5d7aa42
Compare
staging/src/k8s.io/apiserver/pkg/admission/plugin/cel/filter_test.go
Outdated
Show resolved
Hide resolved
Looks like the tests messages need to be updated with the code updates. Couple nits. Overall LGTM :) |
7157751
to
012ef09
Compare
Feedback applied and commits squashed to minimal set (with codegen separated out) |
012ef09
to
c2b3871
Compare
/lgtm /test pull-kubernetes-e2e-kind-ipv6 |
LGTM label has been added. Git tree hash: fbbfb21bd33f40a55ccdbfc530ec543351172d47
|
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cici37, jpbetz, liggitt 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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Implements the Enforcement Actions and Audit Annotations sections of CEL for Admission Control.
Special notes for your reviewer:
Adding
validationActions
as a required field is a intentional (alpha stability) breaking change. We want developers setting this explicitly going forward.Something we missed when writing the KEP: If multiple bindings for a policy match the same resource, I'd decided to have all the unique audit annotation values produced by the
auditAnnotations
concatenated into a comma-separated value. For simple cases where all the bindings produce the same audit annotation value, this results in that single value being published to the audit event. For cases where bindings produce different values, the values are concatenated into a comma-separated list so no information is lost. Better ideas on how to best combine the values into a single audit annotation value are welcome.Deviations from the KEP:
{policyName}/validation_failure
to thevalidation.policy.admission.k8s.io/validation_failure
to adhere better to the conventions of audit annotations.{ValidatingPolicyDefinition name}/{auditAnnotation key}
. We had wanted to offer a way to prevent a validating or mutating webhook from producing the exact same audit annotation as a ValidatingPolicyDefinition, but the structure of audit annotation keys makes this awkward because only one/
is allowed and there is a length restriction on audit annotation keys.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: