Skip to content

Commit 21f7eaa

Browse files
authored
Merge pull request kubernetes#130705 from aaron-prindle/validation-gen-add-metric-and-runtime-verification-upstream
[Declarative Validation] feat: add declarative validation metrics and associated runtime verification tests
2 parents 6b8341f + de904f8 commit 21f7eaa

File tree

8 files changed

+987
-21
lines changed

8 files changed

+987
-21
lines changed

pkg/apis/core/validation/validation_test.go

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ import (
3939
"k8s.io/apimachinery/pkg/util/sets"
4040
"k8s.io/apimachinery/pkg/util/validation"
4141
"k8s.io/apimachinery/pkg/util/validation/field"
42-
fldtest "k8s.io/apimachinery/pkg/util/validation/field/testing"
4342
"k8s.io/apimachinery/pkg/util/version"
4443
utilfeature "k8s.io/apiserver/pkg/util/feature"
4544
"k8s.io/component-base/featuregate"
@@ -16714,7 +16713,7 @@ func TestValidateReplicationControllerUpdate(t *testing.T) {
1671416713
tc.old.ObjectMeta.ResourceVersion = "1"
1671516714
tc.update.ObjectMeta.ResourceVersion = "1"
1671616715
errs := ValidateReplicationControllerUpdate(&tc.update, &tc.old, PodValidationOptions{})
16717-
matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring()
16716+
matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring()
1671816717
matcher.Test(t, tc.expectedErrs, errs)
1671916718
})
1672016719
}
@@ -16864,7 +16863,7 @@ func TestValidateReplicationController(t *testing.T) {
1686416863
for k, tc := range errorCases {
1686516864
t.Run(k, func(t *testing.T) {
1686616865
errs := ValidateReplicationController(&tc.input, PodValidationOptions{})
16867-
matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring()
16866+
matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().ByDetailSubstring()
1686816867
matcher.Test(t, tc.expectedErrs, errs)
1686916868
})
1687016869
}
@@ -20770,7 +20769,7 @@ func TestValidateEndpointsCreate(t *testing.T) {
2077020769
t.Run(k, func(t *testing.T) {
2077120770
errs := ValidateEndpointsCreate(&v.endpoints)
2077220771
// TODO: set .RequireOriginWhenInvalid() once metadata is done
20773-
matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin()
20772+
matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
2077420773
matcher.Test(t, v.expectedErrs, errs)
2077520774
})
2077620775
}
@@ -22767,7 +22766,7 @@ func TestValidateTopologySpreadConstraints(t *testing.T) {
2276722766
for _, tc := range testCases {
2276822767
t.Run(tc.name, func(t *testing.T) {
2276922768
errs := validateTopologySpreadConstraints(tc.constraints, fieldPath, tc.opts)
22770-
matcher := fldtest.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid()
22769+
matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid()
2277122770
matcher.Test(t, tc.wantFieldErrors, errs)
2277222771
})
2277322772
}

staging/src/k8s.io/apimachinery/pkg/runtime/scheme_test.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ import (
3535
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
3636
"k8s.io/apimachinery/pkg/util/sets"
3737
"k8s.io/apimachinery/pkg/util/validation/field"
38-
fieldtesting "k8s.io/apimachinery/pkg/util/validation/field/testing"
3938
)
4039

4140
type testConversions struct {
@@ -1089,7 +1088,7 @@ func TestRegisterValidate(t *testing.T) {
10891088
} else {
10901089
results = s.ValidateUpdate(ctx, tc.options, tc.object, tc.oldObject, tc.subresource...)
10911090
}
1092-
matcher := fieldtesting.ErrorMatcher{}.ByType().ByField().ByOrigin()
1091+
matcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin()
10931092
matcher.Test(t, tc.expected, results)
10941093
})
10951094
}
Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,16 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17-
package testing
17+
package field
1818

1919
import (
2020
"fmt"
2121
"reflect"
2222
"regexp"
2323
"strings"
24-
"testing"
25-
26-
field "k8s.io/apimachinery/pkg/util/validation/field"
2724
)
2825

29-
// ErrorMatcher is a helper for comparing field.Error objects.
26+
// ErrorMatcher is a helper for comparing Error objects.
3027
type ErrorMatcher struct {
3128
// TODO(thockin): consider whether type is ever NOT required, maybe just
3229
// assume it.
@@ -42,9 +39,9 @@ type ErrorMatcher struct {
4239
requireOriginWhenInvalid bool
4340
}
4441

45-
// Matches returns true if the two field.Error objects match according to the
42+
// Matches returns true if the two Error objects match according to the
4643
// configured criteria.
47-
func (m ErrorMatcher) Matches(want, got *field.Error) bool {
44+
func (m ErrorMatcher) Matches(want, got *Error) bool {
4845
if m.matchType && want.Type != got.Type {
4946
return false
5047
}
@@ -58,7 +55,7 @@ func (m ErrorMatcher) Matches(want, got *field.Error) bool {
5855
if want.Origin != got.Origin {
5956
return false
6057
}
61-
if m.requireOriginWhenInvalid && want.Type == field.ErrorTypeInvalid {
58+
if m.requireOriginWhenInvalid && want.Type == ErrorTypeInvalid {
6259
if want.Origin == "" || got.Origin == "" {
6360
return false
6461
}
@@ -72,7 +69,7 @@ func (m ErrorMatcher) Matches(want, got *field.Error) bool {
7269

7370
// Render returns a string representation of the specified Error object,
7471
// according to the criteria configured in the ErrorMatcher.
75-
func (m ErrorMatcher) Render(e *field.Error) string {
72+
func (m ErrorMatcher) Render(e *Error) string {
7673
buf := strings.Builder{}
7774

7875
comma := func() {
@@ -93,7 +90,7 @@ func (m ErrorMatcher) Render(e *field.Error) string {
9390
comma()
9491
buf.WriteString(fmt.Sprintf("Value=%v", e.BadValue))
9592
}
96-
if m.matchOrigin || m.requireOriginWhenInvalid && e.Type == field.ErrorTypeInvalid {
93+
if m.matchOrigin || m.requireOriginWhenInvalid && e.Type == ErrorTypeInvalid {
9794
comma()
9895
buf.WriteString(fmt.Sprintf("Origin=%q", e.Origin))
9996
}
@@ -170,17 +167,25 @@ func (m ErrorMatcher) ByDetailRegexp() ErrorMatcher {
170167
return m
171168
}
172169

170+
// TestIntf lets users pass a testing.T while not coupling this package to Go's
171+
// testing package.
172+
type TestIntf interface {
173+
Helper()
174+
Errorf(format string, args ...any)
175+
Logf(format string, args ...any)
176+
}
177+
173178
// Test compares two ErrorLists by the criteria configured in this matcher, and
174179
// fails the test if they don't match. If a given "want" error matches multiple
175180
// "got" errors, they will all be consumed. This might be OK (e.g. if there are
176181
// multiple errors on the same field from the same origin) or it might be an
177182
// insufficiently specific matcher, so these will be logged.
178-
func (m ErrorMatcher) Test(tb testing.TB, want, got field.ErrorList) {
183+
func (m ErrorMatcher) Test(tb TestIntf, want, got ErrorList) {
179184
tb.Helper()
180185

181186
remaining := got
182187
for _, w := range want {
183-
tmp := make(field.ErrorList, 0, len(remaining))
188+
tmp := make(ErrorList, 0, len(remaining))
184189
n := 0
185190
for _, g := range remaining {
186191
if m.Matches(w, g) {

staging/src/k8s.io/apiserver/pkg/registry/rest/validate.go

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ import (
2626
"k8s.io/apimachinery/pkg/util/sets"
2727
"k8s.io/apimachinery/pkg/util/validation/field"
2828
genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
29+
validationmetrics "k8s.io/apiserver/pkg/validation"
30+
"k8s.io/klog/v2"
2931
)
3032

3133
// ValidateDeclaratively validates obj against declarative validation tags
@@ -106,3 +108,212 @@ func parseSubresourcePath(subresourcePath string) ([]string, error) {
106108
parts := strings.Split(subresourcePath[1:], "/")
107109
return parts, nil
108110
}
111+
112+
// CompareDeclarativeErrorsAndEmitMismatches checks for mismatches between imperative and declarative validation
113+
// and logs + emits metrics when inconsistencies are found
114+
func CompareDeclarativeErrorsAndEmitMismatches(ctx context.Context, imperativeErrs, declarativeErrs field.ErrorList, takeover bool) {
115+
logger := klog.FromContext(ctx)
116+
mismatchDetails := gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs, takeover)
117+
for _, detail := range mismatchDetails {
118+
// Log information about the mismatch using contextual logger
119+
logger.Info(detail)
120+
121+
// Increment the metric for the mismatch
122+
validationmetrics.Metrics.IncDeclarativeValidationMismatchMetric()
123+
}
124+
}
125+
126+
// gatherDeclarativeValidationMismatches compares imperative and declarative validation errors
127+
// and returns detailed information about any mismatches found. Errors are compared via type, field, and origin
128+
func gatherDeclarativeValidationMismatches(imperativeErrs, declarativeErrs field.ErrorList, takeover bool) []string {
129+
var mismatchDetails []string
130+
// short circuit here to minimize allocs for usual case of 0 validation errors
131+
if len(imperativeErrs) == 0 && len(declarativeErrs) == 0 {
132+
return mismatchDetails
133+
}
134+
// recommendation based on takeover status
135+
recommendation := "This difference should not affect system operation since hand written validation is authoritative."
136+
if takeover {
137+
recommendation = "Consider disabling the DeclarativeValidationTakeover feature gate to keep data persisted in etcd consistent with prior versions of Kubernetes."
138+
}
139+
fuzzyMatcher := field.ErrorMatcher{}.ByType().ByField().ByOrigin().RequireOriginWhenInvalid()
140+
exactMatcher := field.ErrorMatcher{}.Exactly()
141+
142+
// Dedupe imperative errors of exact error matches as they are
143+
// not intended and come from (buggy) duplicate validation calls
144+
// This is necessary as without deduping we could get unmatched
145+
// imperative errors for cases that are correct (matching)
146+
dedupedImperativeErrs := field.ErrorList{}
147+
for _, err := range imperativeErrs {
148+
found := false
149+
for _, existingErr := range dedupedImperativeErrs {
150+
if exactMatcher.Matches(existingErr, err) {
151+
found = true
152+
break
153+
}
154+
}
155+
if !found {
156+
dedupedImperativeErrs = append(dedupedImperativeErrs, err)
157+
}
158+
}
159+
imperativeErrs = dedupedImperativeErrs
160+
161+
// Create a copy of declarative errors to track remaining ones
162+
remaining := make(field.ErrorList, len(declarativeErrs))
163+
copy(remaining, declarativeErrs)
164+
165+
// Match each "covered" imperative error to declarative errors.
166+
// We use a fuzzy matching approach to find corresponding declarative errors
167+
// for each imperative error marked as CoveredByDeclarative.
168+
// As matches are found, they're removed from the 'remaining' list.
169+
// They are removed from `remaining` with a "1:many" mapping: for a given
170+
// imperative error we mark as matched all matching declarative errors
171+
// This allows us to:
172+
// 1. Detect imperative errors that should have matching declarative errors but don't
173+
// 2. Identify extra declarative errors with no imperative counterpart
174+
// Both cases indicate issues with the declarative validation implementation.
175+
for _, iErr := range imperativeErrs {
176+
if !iErr.CoveredByDeclarative {
177+
continue
178+
}
179+
180+
tmp := make(field.ErrorList, 0, len(remaining))
181+
matchCount := 0
182+
183+
for _, dErr := range remaining {
184+
if fuzzyMatcher.Matches(iErr, dErr) {
185+
matchCount++
186+
} else {
187+
tmp = append(tmp, dErr)
188+
}
189+
}
190+
191+
if matchCount == 0 {
192+
mismatchDetails = append(mismatchDetails,
193+
fmt.Sprintf(
194+
"Unexpected difference between hand written validation and declarative validation error results, unmatched error(s) found %s. "+
195+
"This indicates an issue with declarative validation. %s",
196+
fuzzyMatcher.Render(iErr),
197+
recommendation,
198+
),
199+
)
200+
}
201+
202+
remaining = tmp
203+
}
204+
205+
// Any remaining unmatched declarative errors are considered "extra"
206+
for _, dErr := range remaining {
207+
mismatchDetails = append(mismatchDetails,
208+
fmt.Sprintf(
209+
"Unexpected difference between hand written validation and declarative validation error results, extra error(s) found %s. "+
210+
"This indicates an issue with declarative validation. %s",
211+
fuzzyMatcher.Render(dErr),
212+
recommendation,
213+
),
214+
)
215+
}
216+
217+
return mismatchDetails
218+
}
219+
220+
// createDeclarativeValidationPanicHandler returns a function with panic recovery logic
221+
// that will increment the panic metric and either log or append errors based on the takeover parameter.
222+
func createDeclarativeValidationPanicHandler(ctx context.Context, errs *field.ErrorList, takeover bool) func() {
223+
logger := klog.FromContext(ctx)
224+
return func() {
225+
if r := recover(); r != nil {
226+
// Increment the panic metric counter
227+
validationmetrics.Metrics.IncDeclarativeValidationPanicMetric()
228+
229+
const errorFmt = "panic during declarative validation: %v"
230+
if takeover {
231+
// If takeover is enabled, output as a validation error as authoritative validator panicked and validation should error
232+
*errs = append(*errs, field.InternalError(nil, fmt.Errorf(errorFmt, r)))
233+
} else {
234+
// if takeover not enabled, log the panic as an info message
235+
logger.Info(fmt.Sprintf(errorFmt, r))
236+
}
237+
}
238+
}
239+
}
240+
241+
// withRecover wraps a validation function with panic recovery logic.
242+
// It takes a validation function with the ValidateDeclaratively signature
243+
// and returns a function with the same signature.
244+
// The returned function will execute the wrapped function and handle any panics by
245+
// incrementing the panic metric, and logging an error message
246+
// if takeover=false, and adding a validation error if takeover=true.
247+
func withRecover(
248+
validateFunc func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) field.ErrorList,
249+
takeover bool,
250+
) func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) field.ErrorList {
251+
return func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object) (errs field.ErrorList) {
252+
defer createDeclarativeValidationPanicHandler(ctx, &errs, takeover)()
253+
254+
return validateFunc(ctx, options, scheme, obj)
255+
}
256+
}
257+
258+
// withRecoverUpdate wraps an update validation function with panic recovery logic.
259+
// It takes a validation function with the ValidateUpdateDeclaratively signature
260+
// and returns a function with the same signature.
261+
// The returned function will execute the wrapped function and handle any panics by
262+
// incrementing the panic metric, and logging an error message
263+
// if takeover=false, and adding a validation error if takeover=true.
264+
func withRecoverUpdate(
265+
validateUpdateFunc func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) field.ErrorList,
266+
takeover bool,
267+
) func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) field.ErrorList {
268+
return func(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object) (errs field.ErrorList) {
269+
defer createDeclarativeValidationPanicHandler(ctx, &errs, takeover)()
270+
271+
return validateUpdateFunc(ctx, options, scheme, obj, oldObj)
272+
}
273+
}
274+
275+
// ValidateDeclarativelyWithRecovery validates obj against declarative validation tags
276+
// with panic recovery logic. It uses the API version extracted from ctx and the
277+
// provided scheme for validation.
278+
//
279+
// The ctx MUST contain requestInfo, which determines the target API for
280+
// validation. The obj is converted to the API version using the provided scheme
281+
// before validation occurs. The scheme MUST have the declarative validation
282+
// registered for the requested resource/subresource.
283+
//
284+
// option should contain any validation options that the declarative validation
285+
// tags expect.
286+
//
287+
// takeover determines if panic recovery should return validation errors (true) or
288+
// just log warnings (false).
289+
//
290+
// Returns a field.ErrorList containing any validation errors. An internal error
291+
// is included if requestInfo is missing from the context, if version
292+
// conversion fails, or if a panic occurs during validation when
293+
// takeover is true.
294+
func ValidateDeclarativelyWithRecovery(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj runtime.Object, takeover bool) field.ErrorList {
295+
return withRecover(ValidateDeclaratively, takeover)(ctx, options, scheme, obj)
296+
}
297+
298+
// ValidateUpdateDeclarativelyWithRecovery validates obj and oldObj against declarative
299+
// validation tags with panic recovery logic. It uses the API version extracted from
300+
// ctx and the provided scheme for validation.
301+
//
302+
// The ctx MUST contain requestInfo, which determines the target API for
303+
// validation. The obj is converted to the API version using the provided scheme
304+
// before validation occurs. The scheme MUST have the declarative validation
305+
// registered for the requested resource/subresource.
306+
//
307+
// option should contain any validation options that the declarative validation
308+
// tags expect.
309+
//
310+
// takeover determines if panic recovery should return validation errors (true) or
311+
// just log warnings (false).
312+
//
313+
// Returns a field.ErrorList containing any validation errors. An internal error
314+
// is included if requestInfo is missing from the context, if version
315+
// conversion fails, or if a panic occurs during validation when
316+
// takeover is true.
317+
func ValidateUpdateDeclarativelyWithRecovery(ctx context.Context, options sets.Set[string], scheme *runtime.Scheme, obj, oldObj runtime.Object, takeover bool) field.ErrorList {
318+
return withRecoverUpdate(ValidateUpdateDeclaratively, takeover)(ctx, options, scheme, obj, oldObj)
319+
}

0 commit comments

Comments
 (0)