Skip to content

Commit d544bc9

Browse files
myasnikovdaniilmyasnikovdaniil
committed
docs(operator): pin the admission order by rule, not plugin index
The scan's justification for treating a VerticalPodAutoscaler as harmless rested on LimitRanger being "position 6 of AllOrderedPlugins" and MutatingAdmissionWebhook being "35". Both numbers are properties of one release rather than of the ordering, and they had already drifted out from under the comment. Under k8s 1.35, which this tree builds against, LimitRanger is the 6th entry but MutatingAdmissionWebhook is the 33rd; index 35 is ResourceQuota and the 35th entry is ValidatingAdmissionWebhook, neither of which mutates, so the sentence named the wrong plugin under either counting convention. In 1.31 the same plugin sat at 30, and SecurityContextDeny being dropped in 1.35 is what moved LimitRanger. Restate it on the invariant the list asserts about itself -- webhook, resourcequota and deny plugins must go at the end -- with LimitRanger compiled in above that marker and every plugin that can rewrite a container's resources below it. That survives a release bump where an index does not, and it covers MutatingAdmissionPolicy as well, a second mutator that the positional wording predated and never accounted for. Record what the order buys, which the old wording left implied. Run the other way round, a plugin mutating before LimitRanger would write a request into a container that has no limit yet, LimitRanger would then default the limit to the configured ceiling, and core validation would reject the fully mutated pod. The scan can see none of that -- the request is in no template and the pod is never admitted -- so the order is the only thing keeping VPA out of the residual gap this comment already records for controlledValues: RequestsOnly. Fix the matching rationale on the live-pod case in TestFindRequestAboveDefaultLimit, which still claimed the pod's request was raised by a webhook after the template was rendered. The ordering rules that out; what the case actually rests on is a pod admitted before the LimitRange existed, or one its controller builds from a custom resource no template in the scan can show. No behaviour change, and no new test: a container that carries its own memory limit is already pinned out of the scan's scope for a template, in TestFindRequestAboveDefaultLimit, and for a live pod, in TestReconcileSystemDefaultsLimitRangeAppliesWhenLargeRequestCarriesItsOwnLimit. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent b123c3e commit d544bc9

2 files changed

Lines changed: 33 additions & 11 deletions

File tree

‎internal/operator/package_reconciler.go‎

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,13 +1027,31 @@ type memoryRequestBlocker struct {
10271027
// because LimitRanger never touches it.
10281028
//
10291029
// A VerticalPodAutoscaler is not how that happens, though an earlier version of this
1030-
// comment said it was. LimitRanger is position 6 of AllOrderedPlugins and
1031-
// MutatingAdmissionWebhook is 35, so the LimitRange has already defaulted the container's
1032-
// memory limit before VPA's webhook is called; VPA then rescales the limit in proportion to
1033-
// the request it writes (GetProportionalLimit, under the default controlledValues:
1034-
// RequestsAndLimits), so request and limit stay consistent and the pod is admitted. The
1035-
// monitoring VPAs cap at maxAllowed anyway — 8Gi for vmselect and vmstorage, 6G for
1036-
// vmagent — and VPA never recommends above that cap.
1030+
// comment said it was. Every plugin that can rewrite a container's resources runs after
1031+
// LimitRanger has defaulted them, so by the time VPA's webhook is called the container
1032+
// already carries a memory limit, and VPA rescales that limit in proportion to the request
1033+
// it writes (GetProportionalLimit, under the default controlledValues: RequestsAndLimits).
1034+
// Request and limit stay consistent and the pod is admitted. The monitoring VPAs cap at
1035+
// maxAllowed anyway — 8Gi for vmselect and vmstorage, 6G for vmagent — and VPA never
1036+
// recommends above that cap.
1037+
//
1038+
// That ordering is structural, and this comment used to pin it the wrong way round, by
1039+
// quoting each plugin's numeric position in AllOrderedPlugins. Those positions shift every
1040+
// release as plugins are added and removed, and they had already shifted out from under the
1041+
// numbers written here; a number nobody rechecks is worse than no number at all. What
1042+
// actually holds is the rule the list states about itself — webhook, resourcequota and deny
1043+
// plugins must go at the end. LimitRanger is compiled in above that marker, while both
1044+
// plugins that can rewrite resources, MutatingAdmissionWebhook and MutatingAdmissionPolicy,
1045+
// sit below it. That survives a release bump where an index does not.
1046+
//
1047+
// Run the other way round the order would matter a great deal, which is the reason to write
1048+
// it down rather than leave it implied. A plugin mutating before LimitRanger would write a
1049+
// request into a container that has no limit yet; LimitRanger would then default the limit
1050+
// to the configured ceiling, and the request-not-above-limit check that core validation
1051+
// applies to the fully mutated object would reject the pod. This scan would see none of it:
1052+
// the request appears in no template, and the pod that would have carried it is never
1053+
// admitted. That is the blind spot recorded below for controlledValues: RequestsOnly,
1054+
// generalised from one opt-in setting to every VPA in cozy-monitoring.
10371055
//
10381056
// What does happen is a request that is already in the spec when it reaches admission, with
10391057
// no limit beside it. Most realistically that is an operator lowering

‎internal/operator/package_reconciler_test.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1119,10 +1119,14 @@ func TestFindRequestAboveDefaultLimit(t *testing.T) {
11191119
},
11201120
},
11211121
{
1122-
// The mirror case, and why live pods are still scanned: the VPA
1123-
// admission webhook writes a recommendation into the pod's requests
1124-
// after the template has been rendered, so the pod can ask for more
1125-
// than any template in the namespace admits to.
1122+
// The mirror case, and why live pods are still scanned. Not because
1123+
// a webhook raised the request after the template was rendered:
1124+
// LimitRanger defaults the limit before any webhook or policy plugin
1125+
// runs, so a pod one of them touched arrives here already carrying a
1126+
// limit and is skipped. What is left is a pod admitted before the
1127+
// LimitRange existed, or one its controller built from a custom
1128+
// resource no template in this scan can show — either can ask for
1129+
// more than any template in the namespace admits to.
11261130
name: "a request raised on the live pod outranks its template",
11271131
objects: []client.Object{
11281132
systemDeployment("vmselect", 2, "5Gi", ""),

0 commit comments

Comments
 (0)