Skip to content

Commit b52aefb

Browse files
njhensleymchmarny
andauthored
fix(validator): fail capability checks when declared deps missing (NVIDIA#2130)
Signed-off-by: Nathan Hensley <nhensley@nvidia.com> Co-authored-by: Mark Chmarny <mchmarny@users.noreply.github.com>
1 parent 77ab263 commit b52aefb

15 files changed

Lines changed: 1112 additions & 67 deletions

docs/contributor/validator.md

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,66 @@ validators.Run(map[string]validators.CheckFunc{
234234
| `/dev/termination-log` | Failure reason (≤ 4096 bytes), written on `return error` |
235235
| **stdout sentinel lines** | Structured/side-channel data — see [Stdout sentinels](#stdout-sentinels) |
236236

237+
### Capability applicability contract
238+
239+
A capability-gated conformance/performance check probes a live prerequisite
240+
(a Deployment, CRD, served API group, or non-empty list) before it runs, and
241+
turns that probe outcome into a verdict via `Capability.Require`
242+
(`validators/applicability.go`). A `Skip` (exit 2) is a claim that the
243+
capability is genuinely **inapplicable** — not a way to paper over a missing
244+
prerequisite or an infrastructure error. The dividing line is whether the
245+
resolved recipe *declares* the component that supplies the capability:
246+
`RecipeDeclares(ctx, component)` reports true only when that componentRef is
247+
present **and** enabled in `ctx.ValidationInput`.
248+
249+
`Require(ctx, probeErr, present)` resolves the fate from the probe outcome.
250+
`present` is consulted only on a clean read (`probeErr == nil`) — e.g. a `List`
251+
that returned zero items or a discovery call that did not serve the expected
252+
group. Probe errors are classified by `classifyCapabilityProbeError`:
253+
254+
| Probe outcome | recipe DECLARES | recipe does NOT declare |
255+
|---------------|-----------------|-------------------------|
256+
| clean read, present | `nil` (proceed) | `nil` (proceed) |
257+
| clean read, absent / empty | **FAIL** (`ErrCodeNotFound`) | **Skip** |
258+
| probe err: `NotFound` (incl. group-not-served) | **FAIL** (`ErrCodeNotFound`) | **Skip** |
259+
| probe err: `Forbidden` / `Unauthorized` (401/403) | **FAIL** (`ErrCodeUnauthorized`) — always | **FAIL** (`ErrCodeUnauthorized`) — always |
260+
| probe err: timeout / deadline | **FAIL** (`ErrCodeTimeout`) — always | **FAIL** (`ErrCodeTimeout`) — always |
261+
| probe err: transport (503 / conn reset / refused / HTTP2 lost) | **FAIL** (`ErrCodeUnavailable`) — always | **FAIL** (`ErrCodeUnavailable`) — always |
262+
| probe err: other / aggregated API discovery | **FAIL** (`ErrCodeInternal`) — always | **FAIL** (`ErrCodeInternal`) — always |
263+
264+
**Infrastructure errors never Skip**, even when the recipe does not declare the
265+
component: a missing RBAC grant, an apiserver timeout, a dropped connection, or
266+
an aggregated discovery failure is not evidence that the capability is
267+
inapplicable — it is evidence the validator *could not tell*, which must block
268+
the gate. Only a clean `NotFound` / empty result on a **non-declared**
269+
capability is Skip-eligible. (A `Forbidden` on a declared dependency maps to
270+
`ErrCodeUnauthorized` rather than `ErrCodeInternal` precisely so operators read
271+
"the validator cannot see it — grant it RBAC," not a blanket internal error.)
272+
273+
**Collection/list probes use `RequireList`.** When the probe is a `List` whose
274+
*empty result* (not an error) is what signals inapplicability — e.g.
275+
`detectPlatform` reading `Nodes().List` to classify the cloud — the caller passes
276+
the List error through `Capability.RequireList(err)` instead of `Require`. A List
277+
*error* is never Skip-eligible: even the rare `NotFound` shape on a collection
278+
endpoint is an apiserver/aggregation-layer anomaly, not the clean absence of a
279+
single object, so every List error blocks with a classified code and never a
280+
Skip. The empty-result inapplicability case is handled by the caller
281+
(`len(items) == 0`), keeping `Require`'s Skip path off the error branch entirely.
282+
283+
**#1327 standalone boundary.** A standalone validator run carries no recipe
284+
context: `pkg/validator/v1.ToValidationInput` leaves `ComponentRefs` empty, so
285+
`RecipeDeclares` returns false (it is nil-safe on a nil `Context` or nil
286+
`ValidationInput`). On that path a clean-absent probe still `Skip`s, which
287+
preserves the capability-driven automatic selection that #1327 introduced.
288+
Fail-closed fires **only once the recipe actually declares** the dependency —
289+
declaration is what converts "inapplicable, so Skip" into "promised but missing,
290+
so fail."
291+
292+
This closes the #2122 false-PASS: before this contract, a recipe that declared a
293+
capability whose prerequisite was absent — or whose probe hit an auth/timeout/
294+
transport/discovery error — reported `passed` (via a Skip that read as
295+
non-blocking), letting a broken or unauthorized cluster clear conformance.
296+
237297
### Stdout sentinels
238298

239299
A check runs inside a pod; the only channels that reach the orchestrator are the

validators/applicability.go

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
// Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
package validators
16+
17+
import (
18+
"fmt"
19+
20+
"github.com/NVIDIA/aicr/pkg/errors"
21+
"github.com/NVIDIA/aicr/validators/internal/allocmode"
22+
apierrors "k8s.io/apimachinery/pkg/api/errors"
23+
utilnet "k8s.io/apimachinery/pkg/util/net"
24+
)
25+
26+
// The capability applicability contract (#2122).
27+
//
28+
// A capability-gated conformance/performance check must Skip only when the
29+
// resolved recipe makes the capability INAPPLICABLE — i.e. the recipe does not
30+
// declare the component that supplies the capability. Once the recipe declares
31+
// the dependency, a missing prerequisite (Deployment/CRD/API/list result) or
32+
// any infrastructure error (RBAC denial, timeout, transport failure, API
33+
// discovery failure) must BLOCK the gate rather than masquerade as an
34+
// inapplicable Skip — otherwise a false-PASS lets a broken or unauthorized
35+
// cluster pass conformance.
36+
//
37+
// #1327 boundary: standalone validator runs carry no recipe context (empty
38+
// ComponentRefs; see pkg/validator/v1.ToValidationInput), so RecipeDeclares
39+
// returns false and capability-driven automatic selection keeps its existing
40+
// Skip behavior. Fail-closed fires ONLY once the recipe actually declares the
41+
// dependency.
42+
43+
// RecipeDeclares reports whether component is present AND enabled in the
44+
// resolved recipe's ComponentRefs (ctx.ValidationInput). A component that is
45+
// absent or explicitly disabled is not declared: it will not be deployed, so
46+
// its capability is genuinely inapplicable and a capability-gated check may
47+
// Skip. Nil-safe: a nil Context or nil ValidationInput reports false, which is
48+
// the standalone/no-recipe path that preserves capability-driven selection
49+
// (#1327).
50+
func RecipeDeclares(ctx *Context, component string) bool {
51+
if ctx == nil || ctx.ValidationInput == nil {
52+
return false
53+
}
54+
for _, ref := range ctx.ValidationInput.ComponentRefs {
55+
if ref.Name == component && ref.IsEnabled() {
56+
return true
57+
}
58+
}
59+
return false
60+
}
61+
62+
// Capability describes a recipe-declared capability whose live prerequisite a
63+
// check probes before proceeding. Require() turns the probe outcome into the
64+
// correct verdict (proceed / Skip / fail-closed) per the #2122 contract above.
65+
type Capability struct {
66+
// Component is the recipe componentRef name that supplies this capability
67+
// (e.g. "kai-scheduler"). RecipeDeclares(ctx, Component) decides whether the
68+
// recipe makes the capability applicable.
69+
Component string
70+
71+
// Subject names the probed prerequisite for diagnostics — the concrete
72+
// object/API the probe read (e.g. "kai-scheduler Deployment
73+
// kai-scheduler/kai-scheduler-default"). Used in the classified infra-error
74+
// messages so operators see exactly what could not be read.
75+
Subject string
76+
77+
// AbsentMsg is the actionable operator message emitted when the capability
78+
// is DECLARED but its prerequisite is cleanly missing (NotFound / empty
79+
// result). It should tell the operator how to remediate, e.g. "recipe
80+
// declares kai-scheduler but its Deployment is absent — apply the bundle or
81+
// check RBAC".
82+
AbsentMsg string
83+
84+
// InapplicableMsg is the Skip reason emitted when the capability is NOT
85+
// declared and is therefore genuinely inapplicable (e.g. "KAI scheduler not
86+
// found — cluster may use a different scheduler"). Optional: when empty a
87+
// generic reason is derived from Component and Subject.
88+
InapplicableMsg string
89+
}
90+
91+
// Require resolves a capability-gated check's fate from the outcome of probing
92+
// its live prerequisite:
93+
//
94+
// - probeErr is the error the probe returned (nil on a clean read).
95+
// - present reports whether the probe found the prerequisite. It is consulted
96+
// ONLY when probeErr is nil — e.g. a List that returned zero items, or a
97+
// discovery call that returned without the expected resource.
98+
//
99+
// Decision table (declared == RecipeDeclares(ctx, c.Component)):
100+
//
101+
// │ recipe DECLARES │ recipe does NOT declare
102+
// ────────────────────────────┼────────────────────────┼────────────────────────
103+
// clean read, present │ nil (proceed) │ nil (proceed)
104+
// clean read, absent/empty │ FAIL (NotFound) │ Skip
105+
// probe err: NotFound │ FAIL (NotFound) │ Skip
106+
// probe err: Forbidden/401 │ FAIL (Unauthorized) — always
107+
// probe err: timeout/deadline │ FAIL (Timeout) — always
108+
// probe err: transport/503 │ FAIL (Unavailable) — always
109+
// probe err: other/discovery │ FAIL (Internal) — always
110+
//
111+
// Infra errors (Forbidden, timeout, transport, API discovery) NEVER Skip, even
112+
// when the recipe does not declare the component: a missing RBAC grant or an
113+
// apiserver hiccup is not evidence that the capability is inapplicable. Only a
114+
// clean NotFound / empty result on a NON-declared capability may Skip (#2122).
115+
func (c Capability) Require(ctx *Context, probeErr error, present bool) error {
116+
declared := RecipeDeclares(ctx, c.Component)
117+
118+
if probeErr != nil {
119+
// A clean NotFound is the ONLY Skip-eligible probe error, and only when
120+
// the recipe does not declare the component. IsNotFound also covers the
121+
// "group-version not served" shape returned by discovery probes
122+
// (ServerResourcesForGroupVersion), which is the clean-absence signal
123+
// for an API-group capability.
124+
if apierrors.IsNotFound(probeErr) {
125+
if declared {
126+
return errors.Wrap(errors.ErrCodeNotFound, c.AbsentMsg, probeErr)
127+
}
128+
return Skip(c.inapplicableReason())
129+
}
130+
// Any non-NotFound probe error is an infrastructure failure and blocks
131+
// the gate regardless of declaration — a Forbidden/timeout/transport/
132+
// discovery error is not proof of inapplicability.
133+
return classifyCapabilityProbeError(probeErr, c.Subject)
134+
}
135+
136+
if present {
137+
return nil
138+
}
139+
if declared {
140+
return errors.New(errors.ErrCodeNotFound, c.AbsentMsg)
141+
}
142+
return Skip(c.inapplicableReason())
143+
}
144+
145+
// RequireList resolves a capability-gated check that probes a LIST/collection
146+
// endpoint whose EMPTY (non-error) result — not an error — is what signals
147+
// inapplicability. Unlike Require, a List *error* is never Skip-eligible: even a
148+
// NotFound on a collection endpoint is an apiserver/aggregation-layer anomaly,
149+
// not the clean absence of a single object, so every List error blocks with a
150+
// classified code (Forbidden→Unauthorized, deadline→Timeout, transport→
151+
// Unavailable, else Internal) and never masquerades as an inapplicable Skip.
152+
// Callers handle the empty-result inapplicability case themselves (e.g.
153+
// len(items) == 0 → "", nil). This closes the #2122 fail-open where routing a
154+
// List error through Require would Skip on the (rare but unenforced) NotFound
155+
// shape for an undeclared capability.
156+
func (c Capability) RequireList(probeErr error) error {
157+
if probeErr == nil {
158+
return nil
159+
}
160+
return classifyCapabilityProbeError(probeErr, c.Subject)
161+
}
162+
163+
// inapplicableReason returns the Skip reason for a non-declared capability,
164+
// preferring the caller-supplied InapplicableMsg and falling back to a message
165+
// derived from the component and subject.
166+
func (c Capability) inapplicableReason() string {
167+
if c.InapplicableMsg != "" {
168+
return c.InapplicableMsg
169+
}
170+
return fmt.Sprintf("%s not declared in recipe — %s is inapplicable", c.Component, c.Subject)
171+
}
172+
173+
// classifyCapabilityProbeError maps a NON-NotFound Kubernetes probe failure to
174+
// a blocking pkg/errors code that matches its cause, so operators get an
175+
// actionable classification instead of a blanket "internal error" — and so a
176+
// capability-gated check can never turn an infra error into a Skip. NotFound is
177+
// deliberately NOT handled here: it is the only Skip-eligible outcome and is
178+
// resolved by the caller. This intentionally distinguishes RBAC denials from
179+
// the shared allocmode.ClassifyK8sReadError (which folds Forbidden into
180+
// Internal): a denial on a declared dependency is "the validator cannot see
181+
// it", which ErrCodeUnauthorized names precisely.
182+
func classifyCapabilityProbeError(err error, subject string) error {
183+
switch {
184+
case apierrors.IsForbidden(err) || apierrors.IsUnauthorized(err):
185+
return errors.Wrap(errors.ErrCodeUnauthorized,
186+
fmt.Sprintf("not authorized to read %s — cannot prove the recipe-declared capability; grant the validator RBAC", subject),
187+
err)
188+
case allocmode.IsK8sTimeoutErr(err):
189+
return errors.Wrap(errors.ErrCodeTimeout, "timed out reading "+subject, err)
190+
case apierrors.IsServiceUnavailable(err) ||
191+
utilnet.IsConnectionReset(err) ||
192+
utilnet.IsConnectionRefused(err) ||
193+
utilnet.IsHTTP2ConnectionLost(err):
194+
return errors.Wrap(errors.ErrCodeUnavailable, "transport failure reading "+subject, err)
195+
default:
196+
// Anything else — including aggregated API-discovery failures
197+
// (*discovery.ErrGroupDiscoveryFailed) that are not NotFound — is an
198+
// ambiguous internal failure that must block, not skip.
199+
return errors.Wrap(errors.ErrCodeInternal, "failed to read "+subject, err)
200+
}
201+
}

0 commit comments

Comments
 (0)