From 397f107a0886999e0d45955dfe460112edd9ac5e Mon Sep 17 00:00:00 2001 From: knight42 Date: Thu, 26 Mar 2020 16:07:06 +0800 Subject: [PATCH] fix(kubectl): explain crds with the same resource name with builtin objects --- .../k8s.io/kubectl/pkg/cmd/explain/explain.go | 13 ++---- .../src/k8s.io/kubectl/pkg/explain/explain.go | 12 +++-- .../kubectl/pkg/explain/explain_test.go | 23 +++++----- test/e2e/apimachinery/crd_publish_openapi.go | 46 ++++++++++++++++++- 4 files changed, 69 insertions(+), 25 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/explain/explain.go b/staging/src/k8s.io/kubectl/pkg/cmd/explain/explain.go index 97817fcd5f7..7d7b5202936 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/explain/explain.go +++ b/staging/src/k8s.io/kubectl/pkg/cmd/explain/explain.go @@ -123,19 +123,14 @@ func (o *ExplainOptions) Run(args []string) error { // TODO: After we figured out the new syntax to separate group and resource, allow // the users to use it in explain (kubectl explain ). // Refer to issue #16039 for why we do this. Refer to PR #15808 that used "/" syntax. - inModel, fieldsPath, err := explain.SplitAndParseResourceRequest(args[0], o.Mapper) + fullySpecifiedGVR, fieldsPath, err := explain.SplitAndParseResourceRequest(args[0], o.Mapper) if err != nil { return err } - // TODO: We should deduce the group for a resource by discovering the supported resources at server. - fullySpecifiedGVR, groupResource := schema.ParseResourceArg(inModel) - gvk := schema.GroupVersionKind{} - if fullySpecifiedGVR != nil { - gvk, _ = o.Mapper.KindFor(*fullySpecifiedGVR) - } + gvk, _ := o.Mapper.KindFor(fullySpecifiedGVR) if gvk.Empty() { - gvk, err = o.Mapper.KindFor(groupResource.WithVersion("")) + gvk, err = o.Mapper.KindFor(fullySpecifiedGVR.GroupResource().WithVersion("")) if err != nil { return err } @@ -151,7 +146,7 @@ func (o *ExplainOptions) Run(args []string) error { schema := o.Schema.LookupResource(gvk) if schema == nil { - return fmt.Errorf("Couldn't find resource for %q", gvk) + return fmt.Errorf("couldn't find resource for %q", gvk) } return explain.PrintModelDescription(fieldsPath, o.Out, schema, gvk, recursive) diff --git a/staging/src/k8s.io/kubectl/pkg/explain/explain.go b/staging/src/k8s.io/kubectl/pkg/explain/explain.go index 816628d4254..9d0e904bb9a 100644 --- a/staging/src/k8s.io/kubectl/pkg/explain/explain.go +++ b/staging/src/k8s.io/kubectl/pkg/explain/explain.go @@ -20,9 +20,10 @@ import ( "io" "strings" + "k8s.io/kube-openapi/pkg/util/proto" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/kube-openapi/pkg/util/proto" ) type fieldsPrinter interface { @@ -43,10 +44,13 @@ func splitDotNotation(model string) (string, []string) { } // SplitAndParseResourceRequest separates the users input into a model and fields -func SplitAndParseResourceRequest(inResource string, mapper meta.RESTMapper) (string, []string, error) { +func SplitAndParseResourceRequest(inResource string, mapper meta.RESTMapper) (schema.GroupVersionResource, []string, error) { inResource, fieldsPath := splitDotNotation(inResource) - inResource, _ = mapper.ResourceSingularizer(inResource) - return inResource, fieldsPath, nil + gvr, err := mapper.ResourceFor(schema.GroupVersionResource{Resource: inResource}) + if err != nil { + return schema.GroupVersionResource{}, nil, err + } + return gvr, fieldsPath, nil } // PrintModelDescription prints the description of a specific model or dot path. diff --git a/staging/src/k8s.io/kubectl/pkg/explain/explain_test.go b/staging/src/k8s.io/kubectl/pkg/explain/explain_test.go index eef15f72ea2..efd389b31b5 100644 --- a/staging/src/k8s.io/kubectl/pkg/explain/explain_test.go +++ b/staging/src/k8s.io/kubectl/pkg/explain/explain_test.go @@ -21,37 +21,38 @@ import ( "testing" "k8s.io/apimachinery/pkg/api/meta/testrestmapper" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/kubectl/pkg/scheme" ) func TestSplitAndParseResourceRequest(t *testing.T) { tests := []struct { name string - inresource string + inResource string - expectedInResource string + expectedGVR schema.GroupVersionResource expectedFieldsPath []string expectedErr bool }{ { name: "no trailing period", - inresource: "field1.field2.field3", + inResource: "pods.field2.field3", - expectedInResource: "field1", + expectedGVR: schema.GroupVersionResource{Resource: "pods", Version: "v1"}, expectedFieldsPath: []string{"field2", "field3"}, }, { name: "trailing period with correct fieldsPath", - inresource: "field1.field2.field3.", + inResource: "service.field2.field3.", - expectedInResource: "field1", + expectedGVR: schema.GroupVersionResource{Resource: "services", Version: "v1"}, expectedFieldsPath: []string{"field2", "field3"}, }, { name: "trailing period with incorrect fieldsPath", - inresource: "field1.field2.field3.", + inResource: "node.field2.field3.", - expectedInResource: "field1", + expectedGVR: schema.GroupVersionResource{Resource: "nodes", Version: "v1"}, expectedFieldsPath: []string{"field2", "field3", ""}, expectedErr: true, }, @@ -60,13 +61,13 @@ func TestSplitAndParseResourceRequest(t *testing.T) { mapper := testrestmapper.TestOnlyStaticRESTMapper(scheme.Scheme, scheme.Scheme.PrioritizedVersionsAllGroups()...) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotInResource, gotFieldsPath, err := SplitAndParseResourceRequest(tt.inresource, mapper) + gotGVR, gotFieldsPath, err := SplitAndParseResourceRequest(tt.inResource, mapper) if err != nil { t.Errorf("unexpected error: %v", err) } - if !reflect.DeepEqual(tt.expectedInResource, gotInResource) && !tt.expectedErr { - t.Errorf("%s: expected inresource: %s, got: %s", tt.name, tt.expectedInResource, gotInResource) + if !reflect.DeepEqual(tt.expectedGVR, gotGVR) && !tt.expectedErr { + t.Errorf("%s: expected inResource: %s, got: %s", tt.name, tt.expectedGVR, gotGVR) } if !reflect.DeepEqual(tt.expectedFieldsPath, gotFieldsPath) && !tt.expectedErr { diff --git a/test/e2e/apimachinery/crd_publish_openapi.go b/test/e2e/apimachinery/crd_publish_openapi.go index 7409b2f7abb..591a7f043df 100644 --- a/test/e2e/apimachinery/crd_publish_openapi.go +++ b/test/e2e/apimachinery/crd_publish_openapi.go @@ -68,6 +68,12 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI [Privileged:ClusterAdmin]", fu framework.Failf("%v", err) } + customServiceShortName := "ksvc" + crdSvc, err := setupCRDWithShortName(f, schemaCustomService, "service", customServiceShortName, "v1alpha1") + if err != nil { + framework.Failf("%v", err) + } + meta := fmt.Sprintf(metaPattern, crd.Crd.Spec.Names.Kind, crd.Crd.Spec.Group, crd.Crd.Spec.Versions[0].Name, "test-foo") ns := fmt.Sprintf("--namespace=%v", f.Namespace.Name) @@ -120,6 +126,11 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI [Privileged:ClusterAdmin]", fu framework.Failf("%v", err) } + ginkgo.By("kubectl explain works for CR with the same resource name as built-in object") + if err := verifyKubectlExplain(f.Namespace.Name, customServiceShortName+".spec", `(?s)DESCRIPTION:.*Specification of CustomService.*FIELDS:.*dummy.*.*Dummy property`); err != nil { + framework.Failf("%v", err) + } + ginkgo.By("kubectl explain works to return error when explain is called on property that doesn't exist") if _, err := framework.RunKubectl(f.Namespace.Name, "explain", crd.Crd.Spec.Names.Plural+".spec.bars2"); err == nil || !strings.Contains(err.Error(), `field "bars2" does not exist`) { framework.Failf("unexpected no error when explaining property that doesn't exist: %v", err) @@ -128,6 +139,9 @@ var _ = SIGDescribe("CustomResourcePublishOpenAPI [Privileged:ClusterAdmin]", fu if err := cleanupCRD(f, crd); err != nil { framework.Failf("%v", err) } + if err := cleanupCRD(f, crdSvc); err != nil { + framework.Failf("%v", err) + } }) /* @@ -476,7 +490,24 @@ func setupCRD(f *framework.Framework, schema []byte, groupSuffix string, version return setupCRDAndVerifySchema(f, schema, expect, groupSuffix, versions...) } +func setupCRDWithShortName(f *framework.Framework, schema []byte, groupSuffix, shortName string, versions ...string) (*crd.TestCrd, error) { + expect := schema + if schema == nil { + // to be backwards compatible, we expect CRD controller to treat + // CRD with nil schema specially and publish an empty schema + expect = []byte(`type: object`) + } + setShortName := func(crd *apiextensionsv1.CustomResourceDefinition) { + crd.Spec.Names.ShortNames = []string{shortName} + } + return setupCRDAndVerifySchemaWithOptions(f, schema, expect, groupSuffix, versions, setShortName) +} + func setupCRDAndVerifySchema(f *framework.Framework, schema, expect []byte, groupSuffix string, versions ...string) (*crd.TestCrd, error) { + return setupCRDAndVerifySchemaWithOptions(f, schema, expect, groupSuffix, versions) +} + +func setupCRDAndVerifySchemaWithOptions(f *framework.Framework, schema, expect []byte, groupSuffix string, versions []string, options ...crd.Option) (*crd.TestCrd, error) { group := fmt.Sprintf("%s-test-%s.example.com", f.BaseName, groupSuffix) if len(versions) == 0 { return nil, fmt.Errorf("require at least one version for CRD") @@ -489,7 +520,7 @@ func setupCRDAndVerifySchema(f *framework.Framework, schema, expect []byte, grou } } - crd, err := crd.CreateMultiVersionTestCRD(f, group, func(crd *apiextensionsv1.CustomResourceDefinition) { + options = append(options, func(crd *apiextensionsv1.CustomResourceDefinition) { var apiVersions []apiextensionsv1.CustomResourceDefinitionVersion for i, version := range versions { version := apiextensionsv1.CustomResourceDefinitionVersion{ @@ -514,6 +545,7 @@ func setupCRDAndVerifySchema(f *framework.Framework, schema, expect []byte, grou } crd.Spec.Versions = apiVersions }) + crd, err := crd.CreateMultiVersionTestCRD(f, group, options...) if err != nil { return nil, fmt.Errorf("failed to create CRD: %v", err) } @@ -728,6 +760,18 @@ properties: pattern: in-tree|out-of-tree type: string`) +var schemaCustomService = []byte(`description: CustomService CRD for Testing +type: object +properties: + spec: + description: Specification of CustomService + type: object + properties: + dummy: + description: Dummy property. + type: string +`) + var schemaWaldo = []byte(`description: Waldo CRD for Testing type: object properties: