Merge pull request #117796 from seans3/fallback-invalid-doc-fix

QueryParamVerifier falls back on invalid v3 document

staging/src/k8s.io/cli-runtime/pkg/resource/fallback_query_param_verifier.go

@@ -17,7 +17,6 @@ limitations under the License.
 package resource
 
 import (
-	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/klog/v2"
 )
@@ -44,12 +43,16 @@ func NewFallbackQueryParamVerifier(primary Verifier, secondary Verifier) Verifie
 // HasSupport returns an error if the passed GVK does not support the
 // query param (fieldValidation), as determined by the primary and
 // secondary OpenAPI endpoints. The primary endoint is checked first,
-// but if it not found, the secondary attempts to determine support.
+// but if there is an error retrieving the OpenAPI V3 document, the
-// If the GVK supports the query param, nil is returned.
+// secondary attempts to determine support. If the GVK supports the query param,
+// nil is returned.
 func (f *fallbackQueryParamVerifier) HasSupport(gvk schema.GroupVersionKind) error {
 	err := f.primary.HasSupport(gvk)
-	if errors.IsNotFound(err) {
+	// If an error was returned from the primary OpenAPI endpoint,
-		klog.V(7).Infoln("openapi v3 endpoint not found...falling back to legacy")
+	// we fallback to check the secondary OpenAPI endpoint for
+	// any error *except* "paramUnsupportedError".
+	if err != nil && !IsParamUnsupportedError(err) {
+		klog.V(7).Infof("openapi v3 error...falling back to legacy: %s", err)
 		err = f.secondary.HasSupport(gvk)
 	}
 	return err

staging/src/k8s.io/cli-runtime/pkg/resource/fallback_query_param_verifier_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resource
 
 import (
+	"fmt"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -32,7 +33,6 @@ func TestFallbackQueryParamVerifier_PrimaryNoFallback(t *testing.T) {
 		crds             []schema.GroupKind      // CRDFinder returns these CRD's
 		gvk              schema.GroupVersionKind // GVK whose OpenAPI spec is checked
 		queryParam       VerifiableQueryParam    // Usually "fieldValidation"
-		primaryError     error
 		expectedSupports bool
 	}{
 		"Field validation query param is supported for batch/v1/Job, primary verifier": {
@@ -123,7 +123,8 @@ func TestFallbackQueryParamVerifier_PrimaryNoFallback(t *testing.T) {
 	for tn, tc := range tests {
 		t.Run(tn, func(t *testing.T) {
 			primary := createFakeV3Verifier(tc.crds, root, tc.queryParam)
-			secondary := createFakeLegacyVerifier(tc.crds, &fakeSchema, tc.queryParam)
+			// secondary verifier should not be called.
+			secondary := &failingVerifier{name: "secondary", t: t}
 			verifier := NewFallbackQueryParamVerifier(primary, secondary)
 			err := verifier.HasSupport(tc.gvk)
 			if tc.expectedSupports && err != nil {
@@ -151,6 +152,29 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "Job",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
+			expectedSupports: true,
+		},
+		"Field validation query param is supported for batch/v1/Job, invalid v3 document error": {
+			crds: []schema.GroupKind{},
+			gvk: schema.GroupVersionKind{
+				Group:   "batch",
+				Version: "v1",
+				Kind:    "Job",
+			},
+			queryParam:       QueryParamFieldValidation,
+			primaryError:     fmt.Errorf("Invalid OpenAPI V3 document"),
+			expectedSupports: true,
+		},
+		"Field validation query param is supported for batch/v1/Job, timeout error": {
+			crds: []schema.GroupKind{},
+			gvk: schema.GroupVersionKind{
+				Group:   "batch",
+				Version: "v1",
+				Kind:    "Job",
+			},
+			queryParam:       QueryParamFieldValidation,
+			primaryError:     fmt.Errorf("timeout"),
 			expectedSupports: true,
 		},
 		"Field validation query param supported for core/v1/Namespace, secondary verifier": {
@@ -161,6 +185,7 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "Namespace",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
 			expectedSupports: true,
 		},
 		"Field validation unsupported for unknown GVK, secondary verifier": {
@@ -171,6 +196,18 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "Uknown",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
+			expectedSupports: false,
+		},
+		"Field validation unsupported for unknown GVK, invalid document causes secondary verifier": {
+			crds: []schema.GroupKind{},
+			gvk: schema.GroupVersionKind{
+				Group:   "bad",
+				Version: "v1",
+				Kind:    "Uknown",
+			},
+			queryParam:       QueryParamFieldValidation,
+			primaryError:     fmt.Errorf("Invalid OpenAPI V3 document"),
 			expectedSupports: false,
 		},
 		"Unknown query param unsupported (for all GVK's), secondary verifier": {
@@ -181,6 +218,7 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "Deployment",
 			},
 			queryParam:       "UnknownQueryParam",
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
 			expectedSupports: false,
 		},
 		"Field validation query param supported for found CRD, secondary verifier": {
@@ -197,6 +235,7 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "ExampleCRD",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
 			expectedSupports: true,
 		},
 		"Field validation query param unsupported for missing CRD, secondary verifier": {
@@ -213,6 +252,7 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "ExampleCRD",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
 			expectedSupports: false,
 		},
 		"List GVK is specifically unsupported": {
@@ -223,16 +263,17 @@ func TestFallbackQueryParamVerifier_SecondaryFallback(t *testing.T) {
 				Kind:    "List",
 			},
 			queryParam:       QueryParamFieldValidation,
+			primaryError:     errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found"),
 			expectedSupports: false,
 		},
 	}
 
 	// Primary OpenAPI client always returns "NotFound" error, so secondary verifier is used.
 	fakeOpenAPIClient := openapitest.NewFakeClient()
-	fakeOpenAPIClient.ForcedErr = errors.NewNotFound(schema.GroupResource{}, "OpenAPI V3 endpoint not found")
 	root := openapi3.NewRoot(fakeOpenAPIClient)
 	for tn, tc := range tests {
 		t.Run(tn, func(t *testing.T) {
+			fakeOpenAPIClient.ForcedErr = tc.primaryError
 			primary := createFakeV3Verifier(tc.crds, root, tc.queryParam)
 			secondary := createFakeLegacyVerifier(tc.crds, &fakeSchema, tc.queryParam)
 			verifier := NewFallbackQueryParamVerifier(primary, secondary)
@@ -269,3 +310,14 @@ func createFakeLegacyVerifier(crds []schema.GroupKind, fakeSchema discovery.Open
 		queryParam:    queryParam,
 	}
 }
+
+// failingVerifier always crashes when called; implements Verifier
+type failingVerifier struct {
+	name string
+	t    *testing.T
+}
+
+func (c *failingVerifier) HasSupport(gvk schema.GroupVersionKind) error {
+	c.t.Fatalf("%s verifier should not be called", c.name)
+	return nil
+}

staging/src/k8s.io/cli-runtime/pkg/resource/query_param_verifier_v3.go

@@ -17,6 +17,8 @@ limitations under the License.
 package resource
 
 import (
+	"fmt"
+
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/openapi"
@@ -62,10 +64,7 @@ func (v *queryParamVerifierV3) HasSupport(gvk schema.GroupVersionKind) error {
 	}
 	gvSpec, err := v.root.GVSpec(gvk.GroupVersion())
 	if err == nil {
-		if supports := supportsQueryParamV3(gvSpec, gvk, v.queryParam); supports {
+		return supportsQueryParamV3(gvSpec, gvk, v.queryParam)
-			return nil
-		}
-		return NewParamUnsupportedError(gvk, v.queryParam)
 	}
 	if _, isErr := err.(*openapi3.GroupVersionNotFoundError); !isErr {
 		return err
@@ -78,9 +77,7 @@ func (v *queryParamVerifierV3) HasSupport(gvk schema.GroupVersionKind) error {
 			// If error retrieving Namespace spec, propagate error.
 			return err
 		}
-		if supports := supportsQueryParamV3(namespaceSpec, namespaceGVK, v.queryParam); supports {
+		return supportsQueryParamV3(namespaceSpec, namespaceGVK, v.queryParam)
-			return nil
-		}
 	}
 	return NewParamUnsupportedError(gvk, v.queryParam)
 }
@@ -103,11 +100,13 @@ func hasGVKExtensionV3(extensions spec.Extensions, gvk schema.GroupVersionKind)
 
 // supportsQueryParam is a method that let's us look in the OpenAPI if the
 // specific group-version-kind supports the specific query parameter for
-// the PATCH end-point. Returns true if the query param is supported by the
+// the PATCH end-point. Returns nil if the passed GVK supports the passed
-// spec for the passed GVK; false otherwise.
+// query parameter; otherwise, a "paramUnsupportedError" is returned (except
-func supportsQueryParamV3(doc *spec3.OpenAPI, gvk schema.GroupVersionKind, queryParam VerifiableQueryParam) bool {
+// when an invalid document error is returned when an invalid OpenAPI V3
+// is passed in).
+func supportsQueryParamV3(doc *spec3.OpenAPI, gvk schema.GroupVersionKind, queryParam VerifiableQueryParam) error {
 	if doc == nil || doc.Paths == nil {
-		return false
+		return fmt.Errorf("Invalid OpenAPI V3 document")
 	}
 	for _, path := range doc.Paths.Paths {
 		// If operation is not PATCH, then continue.
@@ -123,10 +122,10 @@ func supportsQueryParamV3(doc *spec3.OpenAPI, gvk schema.GroupVersionKind, query
 		// for the PATCH operation.
 		for _, param := range op.OperationProps.Parameters {
 			if param.ParameterProps.Name == string(queryParam) {
-				return true
+				return nil
 			}
 		}
-		return false
+		return NewParamUnsupportedError(gvk, queryParam)
 	}
-	return false
+	return NewParamUnsupportedError(gvk, queryParam)
 }

staging/src/k8s.io/cli-runtime/pkg/resource/query_param_verifier_v3_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package resource
 
 import (
+	"strings"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -141,24 +142,18 @@ func TestInvalidOpenAPIV3Document(t *testing.T) {
 	tests := map[string]struct {
 		spec *spec3.OpenAPI
 	}{
-		"nil document correctly returns Unsupported error": {
+		"nil document returns error": {
 			spec: nil,
 		},
-		"empty document correctly returns Unsupported error": {
+		"empty document returns error": {
 			spec: &spec3.OpenAPI{},
 		},
-		"minimal document correctly returns Unsupported error": {
+		"minimal document returns error": {
 			spec: &spec3.OpenAPI{
 				Version: "openapi 3.0.0",
 				Paths:   nil,
 			},
 		},
-		"document with empty Paths correctly returns Unsupported error": {
-			spec: &spec3.OpenAPI{
-				Version: "openapi 3.0.0",
-				Paths:   &spec3.Paths{},
-			},
-		},
 	}
 
 	gvk := schema.GroupVersionKind{
@@ -177,8 +172,8 @@ func TestInvalidOpenAPIV3Document(t *testing.T) {
 				queryParam: QueryParamFieldValidation,
 			}
 			err := verifier.HasSupport(gvk)
-			if err == nil {
+			if !strings.Contains(err.Error(), "Invalid OpenAPI V3 document") {
-				t.Errorf("Expected not supports error, but none received.")
+				t.Errorf("Expected invalid document error, but none received.")
 			}
 		})
 	}