Merge pull request #116390 from alexzielenski/kubectl/explain/openapiv3/on-by-default

kubectl explain: use openapiv3 by default

Kubernetes-commit: 900278dd415baf686a4964ce0c9b60782996c95c

pkg/cmd/explain/explain.go

@@ -68,10 +68,6 @@ type ExplainOptions struct {
 	Mapper meta.RESTMapper
 	Schema openapi.Resources
 
-	// Toggles whether the OpenAPI v3 template-based renderer should be used to show
-	// output.
-	EnableOpenAPIV3 bool
-
 	// Name of the template to use with the openapiv3 template renderer. If
 	// `EnableOpenAPIV3` is disabled, this does nothing
 	OutputFormat string
@@ -82,10 +78,9 @@ type ExplainOptions struct {
 
 func NewExplainOptions(parent string, streams genericclioptions.IOStreams) *ExplainOptions {
 	return &ExplainOptions{
-		IOStreams:       streams,
+		IOStreams:    streams,
-		CmdParent:       parent,
+		CmdParent:    parent,
-		EnableOpenAPIV3: cmdutil.ExplainOpenapiV3.IsEnabled(),
+		OutputFormat: plaintextTemplateName,
-		OutputFormat:    plaintextTemplateName,
 	}
 }
 
@@ -109,9 +104,7 @@ func NewCmdExplain(parent string, f cmdutil.Factory, streams genericclioptions.I
 	cmd.Flags().StringVar(&o.APIVersion, "api-version", o.APIVersion, "Get different explanations for particular API version (API group/version)")
 
 	// Only enable --output as a valid flag if the feature is enabled
-	if o.EnableOpenAPIV3 {
+	cmd.Flags().StringVar(&o.OutputFormat, "output", plaintextTemplateName, "Format in which to render the schema (plaintext, plaintext-openapiv2)")
-		cmd.Flags().StringVar(&o.OutputFormat, "output", plaintextTemplateName, "Format in which to render the schema (plaintext, plaintext-openapiv2)")
-	}
 
 	return cmd
 }
@@ -128,12 +121,10 @@ func (o *ExplainOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []
 		return err
 	}
 
-	// Only openapi v3 needs the openapiv3client
+	// Only openapi v3 needs the discovery client.
-	if o.EnableOpenAPIV3 {
+	o.OpenAPIV3Client, err = f.OpenAPIV3Client()
-		o.OpenAPIV3Client, err = f.OpenAPIV3Client()
+	if err != nil {
-		if err != nil {
+		return err
-			return err
-		}
 	}
 
 	o.args = args
@@ -172,39 +163,36 @@ func (o *ExplainOptions) Run() error {
 	}
 
 	// Fallback to openapiv2 implementation using special template name
-	if o.EnableOpenAPIV3 {
+	switch o.OutputFormat {
-		switch o.OutputFormat {
+	case plaintextOpenAPIV2TemplateName:
-		case plaintextOpenAPIV2TemplateName:
+		return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath)
+	case plaintextTemplateName:
+		// Check whether the server reponds to OpenAPIV3.
+		if _, err := o.OpenAPIV3Client.Paths(); err != nil {
+			// Use v2 renderer if server does not support v3
 			return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath)
-		case plaintextTemplateName:
-			// Check whether the server reponds to OpenAPIV3.
-			if _, err := o.OpenAPIV3Client.Paths(); err != nil {
-				// Use v2 renderer if server does not support v3
-				return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath)
-			}
-
-			fallthrough
-		default:
-			if len(o.APIVersion) > 0 {
-				apiVersion, err := schema.ParseGroupVersion(o.APIVersion)
-				if err != nil {
-					return err
-				}
-				fullySpecifiedGVR.Group = apiVersion.Group
-				fullySpecifiedGVR.Version = apiVersion.Version
-			}
-
-			return openapiv3explain.PrintModelDescription(
-				fieldsPath,
-				o.Out,
-				o.OpenAPIV3Client,
-				fullySpecifiedGVR,
-				o.Recursive,
-				o.OutputFormat,
-			)
 		}
+
+		fallthrough
+	default:
+		if len(o.APIVersion) > 0 {
+			apiVersion, err := schema.ParseGroupVersion(o.APIVersion)
+			if err != nil {
+				return err
+			}
+			fullySpecifiedGVR.Group = apiVersion.Group
+			fullySpecifiedGVR.Version = apiVersion.Version
+		}
+
+		return openapiv3explain.PrintModelDescription(
+			fieldsPath,
+			o.Out,
+			o.OpenAPIV3Client,
+			fullySpecifiedGVR,
+			o.Recursive,
+			o.OutputFormat,
+		)
 	}
-	return o.renderOpenAPIV2(fullySpecifiedGVR, fieldsPath)
 }
 
 func (o *ExplainOptions) renderOpenAPIV2(

pkg/cmd/explain/explain_test.go

@@ -22,7 +22,6 @@ import (
 	"regexp"
 	"testing"
 
-	"github.com/stretchr/testify/require"
 	"k8s.io/apimachinery/pkg/api/meta"
 	sptest "k8s.io/apimachinery/pkg/util/strategicpatch/testing"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
@@ -278,21 +277,3 @@ func runExplainTestCases(t *testing.T, cases []explainTestCase) {
 		buf.Reset()
 	}
 }
-
-func TestAlphaEnablement(t *testing.T) {
-	alphas := map[cmdutil.FeatureGate]string{
-		cmdutil.ExplainOpenapiV3: "output",
-	}
-	for feature, flag := range alphas {
-		f := cmdtesting.NewTestFactory()
-		defer f.Cleanup()
-
-		cmd := explain.NewCmdExplain("kubectl", f, genericclioptions.NewTestIOStreamsDiscard())
-		require.Nil(t, cmd.Flags().Lookup(flag), "flag %q should not be registered without the %q feature enabled", flag, feature)
-
-		cmdtesting.WithAlphaEnvs([]cmdutil.FeatureGate{feature}, t, func(t *testing.T) {
-			cmd := explain.NewCmdExplain("kubectl", f, genericclioptions.NewTestIOStreamsDiscard())
-			require.NotNil(t, cmd.Flags().Lookup(flag), "flag %q should be registered with the %q feature enabled", flag, feature)
-		})
-	}
-}

pkg/explain/v2/explain.go

@@ -18,6 +18,7 @@ package v2
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 
@@ -85,5 +86,12 @@ func printModelDescriptionWithGenerator(
 		return fmt.Errorf("failed to parse openapi schema for %s: %w", resourcePath, err)
 	}
 
-	return generator.Render(outputFormat, parsedV3Schema, gvr, fieldsPath, recursive, w)
+	err = generator.Render(outputFormat, parsedV3Schema, gvr, fieldsPath, recursive, w)
+
+	explainErr := explainError("")
+	if errors.As(err, &explainErr) {
+		return explainErr
+	}
+
+	return err
 }

pkg/explain/v2/funcs.go

@@ -29,8 +29,18 @@ import (
 	"k8s.io/kubectl/pkg/util/term"
 )
 
+type explainError string
+
+func (e explainError) Error() string {
+	return string(e)
+}
+
 func WithBuiltinTemplateFuncs(tmpl *template.Template) *template.Template {
 	return tmpl.Funcs(map[string]interface{}{
+		"throw": func(e string, args ...any) (string, error) {
+			errString := fmt.Sprintf(e, args...)
+			return "", explainError(errString)
+		},
 		"toJson": func(obj any) (string, error) {
 			res, err := json.Marshal(obj)
 			return string(res), err

pkg/explain/v2/templates/plaintext.tmpl

@@ -19,10 +19,10 @@
     {{- with include "schema" (dict "gvk" $gvk "Document" $.Document "FieldPath" $.FieldPath "Recursive" $.Recursive) -}}
         {{- . -}}
     {{- else -}}
-        error: GVK {{$gvk}} not found in OpenAPI schema
+        {{- throw "error: GVK %v not found in OpenAPI schema" $gvk -}}
     {{- end -}}
 {{- else -}}
-    error: GVR ({{$.GVR.String}}) not found in OpenAPI schema
+    {{- throw "error: GVR (%v) not found in OpenAPI schema" $.GVR.String -}}
 {{- end -}}
 {{- "\n" -}}
 
@@ -43,7 +43,8 @@ Takes dictionary as argument with keys:
             {{- with include "output" (set $ "schema" .) -}}
                 {{- . -}}
             {{- else -}}
-                error: field "{{$.FieldPath}}" does not exist
+                {{- $fieldName := (index $.FieldPath (sub (len $.FieldPath) 1)) -}}
+                {{- throw "error: field \"%v\" does not exist" $fieldName}}
             {{- end -}}
             {{- break -}}
         {{- end -}}
@@ -88,7 +89,7 @@ Takes dictionary as argument with keys:
         {{- $subschema := index $resolved.properties (first $.FieldPath) -}}
         {{- if eq 1 (len $.FieldPath) -}}
             {{- /* TODO: The original explain would say RESOURCE instead of FIELD here under some circumstances */ -}}
-            FIELD: {{first $.FieldPath}} <{{ template "typeGuess" (dict "schema" $subschema) }}>{{"\n"}}
+            FIELD: {{first $.FieldPath}} <{{ template "typeGuess" (dict "schema" $subschema "Document" $.Document) }}>{{"\n"}}
             {{- "\n" -}}
         {{- end -}}
         {{- template "output" (set $nextContext "history" (dict) "FieldPath" (slice $.FieldPath 1) "schema" $subschema ) -}}
@@ -149,7 +150,7 @@ Takes dictionary as argument with keys:
         {{- $nextContext := set $ "history" (set $.history $refString 1) -}}
         {{- $resolved := or (resolveRef $refString $.Document) $.schema -}}
         {{- range $k, $v := $resolved.properties -}}
-            {{- template "fieldDetail" (dict "name" $k "schema" $resolved "short" $.Recursive "level" $.level) -}}
+            {{- template "fieldDetail" (dict "name" $k "schema" $resolved "short" $.Recursive "level" $.level "Document" $.Document) -}}
             {{- if $.Recursive -}}
                 {{- /* Check if we already know about this element */}}
                 {{- template "fieldList" (set $nextContext "schema" $v "level" (add $.level 1)) -}}
@@ -174,12 +175,13 @@ Takes dictionary as argument with keys:
     name: name of field
     short: limit printing to a single-line summary
     level: indentation amount
+    Document: openapi document
 */ -}}
 {{- define "fieldDetail" -}}
     {{- $level := or $.level 0 -}}
     {{- $indentAmount := mul $level 2 -}}
     {{- $fieldSchema := index $.schema.properties $.name -}}
-    {{- $.name | indent $indentAmount -}}{{"\t"}}<{{ template "typeGuess" (dict "schema" $fieldSchema) }}>
+    {{- $.name | indent $indentAmount -}}{{"\t"}}<{{ template "typeGuess" (dict "schema" $fieldSchema "Document" $.Document) }}>
     {{- if contains $.schema.required $.name}} -required-{{- end -}}
     {{- "\n" -}}
     {{- if not $.short -}}
@@ -225,24 +227,35 @@ Takes dictionary as argument with keys:
 
 Takes dictionary as argument with keys:
     schema: openapiv3 JSON schema
+    Document: openapi document
 */ -}}
 {{- define "typeGuess" -}}
     {{- with $.schema -}}
         {{- if .items -}}
-            []{{template "typeGuess" (dict "schema" .items)}}
+            []{{template "typeGuess" (set $ "schema" .items)}}
         {{- else if .additionalProperties -}}
-            map[string]{{template "typeGuess" (dict "schema" .additionalProperties)}}
+            map[string]{{template "typeGuess" (set $ "schema" .additionalProperties)}}
         {{- else if and .allOf (not .properties) (eq (len .allOf) 1) -}}
             {{- /* If allOf has a single element and there are no direct
                 properties on the schema, defer to the allOf */ -}}
-            {{- template "typeGuess" (dict "schema" (first .allOf)) -}}
+            {{- template "typeGuess" (set $ "schema" (first .allOf)) -}}
         {{- else if index . "$ref"}}
             {{- /* Parse the #!/components/schemas/io.k8s.Kind string into just the Kind name */ -}}
             {{- $ref := index . "$ref" -}}
-            {{- $name := split $ref "/" | last -}}
+            {{- /* Look up ref schema to see primitive type. Only put the ref type name if it is an object. */ -}}
-            {{- or (split $name "." | last) "Object" -}}
+            {{- $refSchema := resolveRef $ref $.Document -}}
+            {{- if (or (not $refSchema) (or (not $refSchema.type) (eq $refSchema.type "object"))) -}}
+                {{- $name := split $ref "/" | last -}}
+                {{- or (split $name "." | last) "Object" -}}
+            {{- else if $refSchema.type -}}
+                {{- or $refSchema.type "Object" -}}
+            {{- else -}}
+                {{- or .type "Object" -}}
+            {{- end -}}
         {{- else -}}
-            {{- or .type "Object" -}}
+            {{/* Old explain used capitalized "Object". Just follow suit */}}
+            {{- if eq .type "object" -}}Object
+            {{- else -}}{{- or .type "Object" -}}{{- end -}}
         {{- end -}}
     {{- else -}}
         {{- fail "expected schema argument to subtemplate 'typeguess'" -}}

pkg/explain/v2/templates/plaintext_test.go

@@ -66,21 +66,21 @@ type testCase struct {
 }
 
 type check interface {
-	doCheck(output string) error
+	doCheck(output string, err error) error
 }
 
 type checkError string
 
-func (c checkError) doCheck(output string) error {
+func (c checkError) doCheck(output string, err error) error {
-	if !strings.Contains(output, "error: "+string(c)) {
+	if !strings.Contains(err.Error(), "error: "+string(c)) {
-		return fmt.Errorf("expected error: '%v' in string:\n%v", string(c), output)
+		return fmt.Errorf("expected error: '%v' in string:\n%v", string(c), err)
 	}
 	return nil
 }
 
 type checkContains string
 
-func (c checkContains) doCheck(output string) error {
+func (c checkContains) doCheck(output string, err error) error {
 	if !strings.Contains(output, string(c)) {
 		return fmt.Errorf("expected substring: '%v' in string:\n%v", string(c), output)
 	}
@@ -89,7 +89,7 @@ func (c checkContains) doCheck(output string) error {
 
 type checkEquals string
 
-func (c checkEquals) doCheck(output string) error {
+func (c checkEquals) doCheck(output string, err error) error {
 	if output != string(c) {
 		return fmt.Errorf("output is not equal to expectation:\n%v", cmp.Diff(string(c), output))
 	}
@@ -123,7 +123,7 @@ func TestPlaintext(t *testing.T) {
 				Recursive: false,
 			},
 			Checks: []check{
-				checkError("GVR (/, Resource=) not found in OpenAPI schema\n"),
+				checkError("GVR (/, Resource=) not found in OpenAPI schema"),
 			},
 		},
 		{
@@ -158,7 +158,7 @@ func TestPlaintext(t *testing.T) {
 				Recursive: false,
 			},
 			Checks: []check{
-				checkError(`field "[does not exist]" does not exist`),
+				checkError(`field "exist" does not exist`),
 			},
 		},
 		{
@@ -295,6 +295,30 @@ func TestPlaintext(t *testing.T) {
 				checkEquals("string"),
 			},
 		},
+		{
+			// Show that a ref to a primitive type uses the referred type's type
+			Name:        "PrimitiveRef",
+			Subtemplate: "typeGuess",
+			Context: map[string]any{
+				"schema": map[string]any{
+					"description": "a cool field",
+					"$ref":        "#/components/schemas/v1.Time",
+				},
+				"Document": map[string]any{
+					"components": map[string]any{
+						"schemas": map[string]any{
+							"v1.Time": map[string]any{
+								"type":   "string",
+								"format": "date-time",
+							},
+						},
+					},
+				},
+			},
+			Checks: []check{
+				checkEquals("string"),
+			},
+		},
 		{
 			// Shows that the typeguess template behaves correctly given an
 			// array with unknown items
@@ -310,6 +334,20 @@ func TestPlaintext(t *testing.T) {
 				checkEquals("array"),
 			},
 		},
+		{
+			// Shows that the typeguess puts Object tpye in title case
+			Name:        "ObjectTitle",
+			Subtemplate: "typeGuess",
+			Context: map[string]any{
+				"schema": map[string]any{
+					"description": "a cool field",
+					"type":        "object",
+				},
+			},
+			Checks: []check{
+				checkEquals("Object"),
+			},
+		},
 		{
 			// Shows that the typeguess template works with scalars
 			Name:        "ArrayOfScalar",
@@ -526,16 +564,17 @@ func TestPlaintext(t *testing.T) {
 
 		t.Run(testName, func(t *testing.T) {
 			buf := bytes.NewBuffer(nil)
+
+			var outputErr error
 			if len(tcase.Subtemplate) == 0 {
-				tmpl.Execute(buf, tcase.Context)
+				outputErr = tmpl.Execute(buf, tcase.Context)
 			} else {
-				tmpl.ExecuteTemplate(buf, tcase.Subtemplate, tcase.Context)
+				outputErr = tmpl.ExecuteTemplate(buf, tcase.Subtemplate, tcase.Context)
 			}
-			require.NoError(t, err)
 
 			output := buf.String()
 			for _, check := range tcase.Checks {
-				err = check.doCheck(output)
+				err = check.doCheck(output, outputErr)
 
 				if err != nil {
 					t.Log("test failed on output:\n" + output)