docs: Add MAP management feature design document#4379
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new in-repo design document describing a proposed Gatekeeper feature to manage Kubernetes MutatingAdmissionPolicy resources via new wrapper CRDs (MAPTemplate / MAPConstraint), modeled after the existing VAP generation pattern.
Changes:
- Introduces a MAP management feature design doc (goals, API shape, controllers, scope sync, lifecycle, and rollout plan).
- Documents proposed CRDs and generated Kubernetes resources (MAP/MAPB + CRD).
- Outlines controller architecture, status/observability approach, and compatibility strategy.
| containers: object.spec.containers.map(c, | ||
| Object.spec.containers.item{ | ||
| name: c.name, | ||
| imagePullPolicy: params.spec.imagePullPolicy |
There was a problem hiding this comment.
The MAPTemplate example’s CEL uses params.spec.imagePullPolicy. In Gatekeeper’s existing VAP integration, variables.params is bound to params.spec.parameters (see pkg/drivers/k8scel/transform/cel_snippets.go), so examples that access params.spec.<field> are likely to be inconsistent with the intended param shape. Consider updating the example to reference parameters in a way that matches the planned binding (e.g., via variables.params.<field> or params.spec.parameters.<field>).
| imagePullPolicy: params.spec.imagePullPolicy | |
| imagePullPolicy: variables.params.imagePullPolicy |
| spec: | ||
| match: | ||
| namespaceSelector: | ||
| matchLabels: | ||
| environment: production | ||
| excludedNamespaces: ["kube-system"] | ||
| # Parameters accessed via params.spec.* in CEL | ||
| imagePullPolicy: "Always" | ||
|
|
There was a problem hiding this comment.
The MAPConstraint instance example puts imagePullPolicy directly under spec, and the note says parameters are accessed via params.spec.*. Existing Gatekeeper constraints conventionally store user-defined fields under spec.parameters (with spec.match reserved for match criteria). Adjusting the proposed schema/examples to use spec.parameters.imagePullPolicy will better match established constraint patterns and VAP param binding behavior.
|
|
||
| **Location**: `pkg/controller/maptemplate/` | ||
|
|
||
| **Operation Guard**: Only runs when `operations.HasGenerateOperations()` returns true. |
There was a problem hiding this comment.
This refers to operations.HasGenerateOperations(), but pkg/operations doesn’t define that helper (it has HasValidationOperations() and IsAssigned(op), with Generate being an Operation). Update the operation-guard examples to use operations.IsAssigned(operations.Generate) or explicitly note that HasGenerateOperations() would need to be added.
| **Operation Guard**: Only runs when `operations.HasGenerateOperations()` returns true. | |
| **Operation Guard**: Only runs when `operations.IsAssigned(operations.Generate)` returns true. |
| func (a *Adder) Add(mgr manager.Manager) error { | ||
| if !operations.HasGenerateOperations() { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The controller registration snippet also uses operations.HasGenerateOperations(), which doesn’t exist in the current pkg/operations API. For consistency with the existing controllers, use operations.IsAssigned(operations.Generate) (or propose adding a HasGenerateOperations() helper) in this example too.
| | Source | Injected As | | ||
| |--------|-------------| | ||
| | Config.spec.match.excludedNamespaces | matchCondition CEL expression | | ||
| | `--exempt-namespaces` flag | matchCondition CEL expression | |
There was a problem hiding this comment.
Gatekeeper’s namespace exemption CLI flag is --exempt-namespace (plus --exempt-namespace-prefix / --exempt-namespace-suffix), not --exempt-namespaces. Updating this row will avoid pointing readers to a non-existent flag.
| | `--exempt-namespaces` flag | matchCondition CEL expression | | |
| | `--exempt-namespace` / `--exempt-namespace-prefix` / `--exempt-namespace-suffix` flags | matchCondition CEL expression | |
|
|
||
| - [KEP-3962: Mutating Admission Policies](https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/3962-mutating-admission-policies) | ||
| - [Gatekeeper VAP Integration](https://open-policy-agent.github.io/gatekeeper/website/docs/validating-admission-policy/) | ||
| - [Existing VAP Transform Code](../pkg/drivers/k8scel/transform/make_vap_objects.go) |
There was a problem hiding this comment.
The relative link to ../pkg/drivers/k8scel/transform/make_vap_objects.go is broken from docs/design/ (it resolves to docs/pkg/..., which doesn’t exist). Update the link target (e.g., ../../pkg/drivers/k8scel/transform/make_vap_objects.go) or use an absolute GitHub URL so the reference works when browsing the repo.
| - [Existing VAP Transform Code](../pkg/drivers/k8scel/transform/make_vap_objects.go) | |
| - [Existing VAP Transform Code](../../pkg/drivers/k8scel/transform/make_vap_objects.go) |
| spec:/ | ||
| crd: | ||
| sp |
There was a problem hiding this comment.
Lines 357-359 contain incomplete YAML syntax. Line 357 has 'spec:/' (with a trailing slash instead of newline), and line 359 shows 'sp' which appears to be a fragment. This should be 'spec:' followed by proper YAML indentation, and likely 'spec:' (not 'sp').
| spec:/ | |
| crd: | |
| sp | |
| spec: | |
| crd: | |
| spec: |
3a2e07f to
2921c6c
Compare
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
2921c6c to
caf00e6
Compare
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
| spec:/ | ||
| crd: | ||
| sp |
There was a problem hiding this comment.
The YAML syntax in the MAPTemplate example is malformed. Line 427 has "spec:/" which should be "spec:" and line 429 has "sp" which appears to be an incomplete "spec:" field. This should be corrected to proper YAML syntax with "spec:" on line 427 and "spec:" under the "crd:" field (likely indented properly).
| spec:/ | |
| crd: | |
| sp | |
| spec: | |
| crd: | |
| spec: |
| MutationConstraint controller processes via `IfWatching(gvk, fn)` guard + | ||
| `EventPackerMapFunc`/`UnpackRequest` for GVK encoding. | ||
|
|
||
| **MutationConstraint re-triggering**: After MAP create/update/delete, MAPTemplate lists all |
There was a problem hiding this comment.
The documentation states that after MAP "create/update/delete" the MAPTemplate controller triggers MutationConstraint reconciliation (line 752), but this contradicts the earlier statement in lines 637-642 which correctly specifies that triggering occurs only after MAP "creation or deletion", NOT on update. The rationale given in lines 640-642 is that scope sync updates to MAP spec don't require MAPB re-generation since MAPBs reference the MAP by name. This inconsistency should be resolved by removing "update" from line 752.
| **MutationConstraint re-triggering**: After MAP create/update/delete, MAPTemplate lists all | |
| **MutationConstraint re-triggering**: After MAP create/delete, MAPTemplate lists all |
| @@ -0,0 +1,1550 @@ | |||
| # Design Document: MutatingAdmissionPolicy Management Feature | |||
There was a problem hiding this comment.
The PR description is essentially empty with only template placeholders. For a design document of this scope and importance (introducing a new major feature with MAPTemplate and MutationConstraint CRDs), the PR description should include a meaningful summary of what the design proposes, why it's needed, and any key decisions or open questions. This would help reviewers understand the context before diving into the detailed design document.
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
Signed-off-by: Jaydip Gabani <gabanijaydip@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 4 comments.
You can also share your feedback on Copilot code review. Take the survey.
| 5. Validate matchCondition names (reject gatekeeper_internal_* prefix in user conditions) | ||
| 6. Validate variable names (reject collisions with reserved names: params, anyObject, gatekeeper_* prefix) | ||
| 7. Perform CEL syntax validation using the Kubernetes CEL environment on all expressions | ||
| (mutations, matchConditions, variables). This provides Kubernetes-aware type checking |
There was a problem hiding this comment.
In this reconciliation step, the text says CEL compilation "provides Kubernetes-aware type checking", but earlier the doc explicitly states the controller does not do type-checking and only does syntax validation. Please align the wording here (e.g., "Kubernetes-aware syntax validation"), so readers don't assume semantic/type validation happens in-controller.
| (mutations, matchConditions, variables). This provides Kubernetes-aware type checking | |
| (mutations, matchConditions, variables). This provides Kubernetes-aware syntax validation |
| // scope. If omitted, the webhook config rules are used. Template rules | ||
| // that don't intersect with the webhook scope are rejected with an error | ||
| // in status.byPod[].errors rather than silently dropped. |
There was a problem hiding this comment.
This comment block has trailing whitespace and inconsistent indentation (the // lines are misaligned). Since this snippet is meant to be copy/paste-able reference, please clean up the whitespace/indentation to avoid readers copying formatting artifacts.
| // scope. If omitted, the webhook config rules are used. Template rules | |
| // that don't intersect with the webhook scope are rejected with an error | |
| // in status.byPod[].errors rather than silently dropped. | |
| // scope. If omitted, the webhook config rules are used. Template rules | |
| // that don't intersect with the webhook scope are rejected with an error | |
| // in status.byPod[].errors rather than silently dropped. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, using
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when the PR gets merged):Fixes #
Special notes for your reviewer: