From b2c9b1f7295e9ef8e225b54c69f344e6c9697864 Mon Sep 17 00:00:00 2001 From: Haowei Cai Date: Wed, 28 Aug 2019 13:30:13 -0700 Subject: [PATCH 1/3] add a test to make sure the CRD OpenAPI path and defintion are protected from user-defined CRDs --- .../integration/master/kube_apiserver_test.go | 209 ++++++++++++++++++ 1 file changed, 209 insertions(+) diff --git a/test/integration/master/kube_apiserver_test.go b/test/integration/master/kube_apiserver_test.go index 29b84537dc3..4c5868ed01c 100644 --- a/test/integration/master/kube_apiserver_test.go +++ b/test/integration/master/kube_apiserver_test.go @@ -17,6 +17,7 @@ limitations under the License. package master import ( + "bytes" "encoding/json" "fmt" "net/http" @@ -26,18 +27,28 @@ import ( "testing" "time" + "github.com/go-openapi/spec" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions" + apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/registry/generic/registry" "k8s.io/client-go/kubernetes" "k8s.io/kube-aggregator/pkg/apis/apiregistration" kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing" + "k8s.io/kubernetes/test/integration/etcd" "k8s.io/kubernetes/test/integration/framework" ) +const ( + // testApiextensionsOverlapProbeString is a probe string which identifies whether + // a CRD change triggers an OpenAPI spec change + testApiextensionsOverlapProbeString = "testApiextensionsOverlapProbeField" +) + func TestRun(t *testing.T) { server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd()) defer server.TearDownFn() @@ -190,6 +201,204 @@ func TestOpenAPIDelegationChainPlumbing(t *testing.T) { } } +func TestOpenAPIApiextensionsOverlapProtection(t *testing.T) { + server := kubeapiservertesting.StartTestServerOrDie(t, nil, nil, framework.SharedEtcd()) + defer server.TearDownFn() + apiextensionsclient, err := apiextensionsclientset.NewForConfig(server.ClientConfig) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + crdPath, exist, err := getOpenAPIPath(apiextensionsclient, `/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/{name}`) + if err != nil { + t.Fatalf("unexpected error getting CRD OpenAPI path: %v", err) + } + if !exist { + t.Fatalf("unexpected error: apiextensions OpenAPI path doesn't exist") + } + + // Create a CRD that overlaps OpenAPI path with the CRD API + crd := &apiextensionsv1beta1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "customresourcedefinitions.apiextensions.k8s.io", + Annotations: map[string]string{"api-approved.kubernetes.io": "unapproved, test-only"}, + }, + Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ + Group: "apiextensions.k8s.io", + Version: "v1beta1", + Scope: apiextensionsv1beta1.ClusterScoped, + Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ + Plural: "customresourcedefinitions", + Singular: "customresourcedefinition", + Kind: "CustomResourceDefinition", + ListKind: "CustomResourceDefinitionList", + }, + Validation: &apiextensionsv1beta1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + testApiextensionsOverlapProbeString: {Type: "boolean"}, + }, + }, + }, + }, + } + etcd.CreateTestCRDs(t, apiextensionsclient, false, crd) + + // Create a probe CRD foo that triggers an OpenAPI spec change + if err := triggerSpecUpdateWithProbeCRD(t, apiextensionsclient, "foo"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Expect the CRD path to not change + path, _, err := getOpenAPIPath(apiextensionsclient, `/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/{name}`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + pathBytes, err := json.Marshal(path) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + crdPathBytes, err := json.Marshal(crdPath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !bytes.Equal(pathBytes, crdPathBytes) { + t.Fatalf("expected CRD OpenAPI path to not change, but got different results: want %q, got %q", string(crdPathBytes), string(pathBytes)) + } + + // Expect the orphan definition to be pruned from the spec + exist, err = specHasProbe(apiextensionsclient, testApiextensionsOverlapProbeString) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if exist { + t.Fatalf("unexpected error: orphan definition isn't pruned") + } + + // Create a CRD that overlaps OpenAPI definition with the CRD API + crd = &apiextensionsv1beta1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{ + Name: "customresourcedefinitions.apiextensions.apis.pkg.apiextensions-apiserver.k8s.io", + Annotations: map[string]string{"api-approved.kubernetes.io": "unapproved, test-only"}, + }, + Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ + Group: "apiextensions.apis.pkg.apiextensions-apiserver.k8s.io", + Version: "v1beta1", + Scope: apiextensionsv1beta1.ClusterScoped, + Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ + Plural: "customresourcedefinitions", + Singular: "customresourcedefinition", + Kind: "CustomResourceDefinition", + ListKind: "CustomResourceDefinitionList", + }, + Validation: &apiextensionsv1beta1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + testApiextensionsOverlapProbeString: {Type: "boolean"}, + }, + }, + }, + }, + } + etcd.CreateTestCRDs(t, apiextensionsclient, false, crd) + + // Create a probe CRD bar that triggers an OpenAPI spec change + if err := triggerSpecUpdateWithProbeCRD(t, apiextensionsclient, "bar"); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Expect the apiextensions definition to not change, since the overlapping definition will get renamed. + apiextensionsDefinition, exist, err := getOpenAPIDefinition(apiextensionsclient, `io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1beta1.CustomResourceDefinition`) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !exist { + t.Fatalf("unexpected error: apiextensions definition doesn't exist") + } + bytes, err := json.Marshal(apiextensionsDefinition) + if exist := strings.Contains(string(bytes), testApiextensionsOverlapProbeString); exist { + t.Fatalf("unexpected error: apiextensions definition gets overlapped") + } +} + +// triggerSpecUpdateWithProbeCRD creates a probe CRD with suffix in name, and waits until +// the path and definition for the probe CRD show up in the OpenAPI spec +func triggerSpecUpdateWithProbeCRD(t *testing.T, apiextensionsclient *apiextensionsclientset.Clientset, suffix string) error { + // Create a probe CRD that triggers OpenAPI spec change + name := fmt.Sprintf("integration-test-%s-crd", suffix) + kind := fmt.Sprintf("Integration-test-%s-crd", suffix) + group := "probe.test.com" + crd := &apiextensionsv1beta1.CustomResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: name + "s." + group}, + Spec: apiextensionsv1beta1.CustomResourceDefinitionSpec{ + Group: group, + Version: "v1", + Scope: apiextensionsv1beta1.ClusterScoped, + Names: apiextensionsv1beta1.CustomResourceDefinitionNames{ + Plural: name + "s", + Singular: name, + Kind: kind, + ListKind: kind + "List", + }, + }, + } + etcd.CreateTestCRDs(t, apiextensionsclient, false, crd) + + // Expect the probe CRD path to show up in the OpenAPI spec + // TODO(roycaihw): expose response header in rest client and utilize etag here + if err := wait.Poll(1*time.Second, wait.ForeverTestTimeout, func() (bool, error) { + _, exist, err := getOpenAPIPath(apiextensionsclient, fmt.Sprintf(`/apis/%s/v1/%ss/{name}`, group, name)) + if err != nil { + return false, err + } + return exist, nil + }); err != nil { + return fmt.Errorf("failed to observe probe CRD path in the spec: %v", err) + } + return nil +} + +func specHasProbe(clientset *apiextensionsclientset.Clientset, probe string) (bool, error) { + bs, err := clientset.RESTClient().Get().AbsPath("openapi", "v2").DoRaw() + if err != nil { + return false, err + } + return strings.Contains(string(bs), probe), nil +} + +func getOpenAPIPath(clientset *apiextensionsclientset.Clientset, path string) (spec.PathItem, bool, error) { + bs, err := clientset.RESTClient().Get().AbsPath("openapi", "v2").DoRaw() + if err != nil { + return spec.PathItem{}, false, err + } + s := spec.Swagger{} + if err := json.Unmarshal(bs, &s); err != nil { + return spec.PathItem{}, false, err + } + if s.SwaggerProps.Paths == nil { + return spec.PathItem{}, false, fmt.Errorf("unexpected empty path") + } + value, ok := s.SwaggerProps.Paths.Paths[path] + return value, ok, nil +} + +func getOpenAPIDefinition(clientset *apiextensionsclientset.Clientset, definition string) (spec.Schema, bool, error) { + bs, err := clientset.RESTClient().Get().AbsPath("openapi", "v2").DoRaw() + if err != nil { + return spec.Schema{}, false, err + } + s := spec.Swagger{} + if err := json.Unmarshal(bs, &s); err != nil { + return spec.Schema{}, false, err + } + if s.SwaggerProps.Definitions == nil { + return spec.Schema{}, false, fmt.Errorf("unexpected empty path") + } + value, ok := s.SwaggerProps.Definitions[definition] + return value, ok, nil +} + // return the unique endpoint IPs func getEndpointIPs(endpoints *corev1.Endpoints) []string { endpointMap := make(map[string]bool) From 81e00f0b7bcdfdb64936020f6b182f44edf89946 Mon Sep 17 00:00:00 2001 From: Haowei Cai Date: Wed, 28 Aug 2019 13:33:49 -0700 Subject: [PATCH 2/3] apiextensions: merge openapi spec ignore path conflict --- .../pkg/controller/openapi/builder/merge.go | 19 ++++++++++++++----- .../pkg/controller/openapi/controller.go | 6 +++++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go index f2cc8135b51..65324947d5b 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/merge.go @@ -18,12 +18,13 @@ package builder import ( "github.com/go-openapi/spec" + "k8s.io/kube-openapi/pkg/aggregator" ) // MergeSpecs aggregates all OpenAPI specs, reusing the metadata of the first, static spec as the basis. -// Later paths and definitions override earlier ones. None of the input is mutated, but input -// and output share data structures. -func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) *spec.Swagger { +// The static spec has the highest priority, and its paths and definitions won't get overlapped by +// user-defined CRDs. None of the input is mutated, but input and output share data structures. +func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) (*spec.Swagger, error) { // create shallow copy of staticSpec, but replace paths and definitions because we modify them. specToReturn := *staticSpec if staticSpec.Definitions != nil { @@ -41,11 +42,19 @@ func MergeSpecs(staticSpec *spec.Swagger, crdSpecs ...*spec.Swagger) *spec.Swagg } } + crdSpec := &spec.Swagger{} for _, s := range crdSpecs { - mergeSpec(&specToReturn, s) + // merge specs without checking conflicts, since the naming controller prevents + // conflicts between user-defined CRDs + mergeSpec(crdSpec, s) } - return &specToReturn + // The static spec has the highest priority. Resolve conflicts to prevent user-defined + // CRDs potentially overlapping the built-in apiextensions API + if err := aggregator.MergeSpecsIgnorePathConflict(&specToReturn, crdSpec); err != nil { + return nil, err + } + return &specToReturn, nil } // mergeSpec copies paths and definitions from source to dest, mutating dest, but not source. diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go index 4d54b411f3d..a192add33a3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/controller.go @@ -218,7 +218,11 @@ func (c *Controller) updateSpecLocked() error { crdSpecs = append(crdSpecs, s) } } - return c.openAPIService.UpdateSpec(builder.MergeSpecs(c.staticSpec, crdSpecs...)) + mergedSpec, err := builder.MergeSpecs(c.staticSpec, crdSpecs...) + if err != nil { + return fmt.Errorf("failed to merge specs: %v", err) + } + return c.openAPIService.UpdateSpec(mergedSpec) } func (c *Controller) addCustomResourceDefinition(obj interface{}) { From 2a10b0dc3158d714214c7364de0277f8146dd202 Mon Sep 17 00:00:00 2001 From: Haowei Cai Date: Wed, 28 Aug 2019 13:34:45 -0700 Subject: [PATCH 3/3] generated --- staging/src/k8s.io/apiextensions-apiserver/go.mod | 1 + staging/src/k8s.io/apiextensions-apiserver/go.sum | 2 ++ .../pkg/controller/openapi/builder/BUILD | 1 + 3 files changed, 4 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/go.mod b/staging/src/k8s.io/apiextensions-apiserver/go.mod index 242cf2e4193..83aabe025c3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/go.mod +++ b/staging/src/k8s.io/apiextensions-apiserver/go.mod @@ -7,6 +7,7 @@ go 1.12 require ( github.com/coreos/etcd v3.3.15+incompatible github.com/emicklei/go-restful v2.9.5+incompatible + github.com/ghodss/yaml v0.0.0-20180820084758-c7ce16629ff4 // indirect github.com/go-openapi/errors v0.19.2 github.com/go-openapi/spec v0.19.2 github.com/go-openapi/strfmt v0.19.0 diff --git a/staging/src/k8s.io/apiextensions-apiserver/go.sum b/staging/src/k8s.io/apiextensions-apiserver/go.sum index 1a9d8eaeebe..2443461b99f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/go.sum +++ b/staging/src/k8s.io/apiextensions-apiserver/go.sum @@ -64,6 +64,8 @@ github.com/evanphx/json-patch v4.2.0+incompatible/go.mod h1:50XU6AFN0ol/bzJsmQLi github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I= github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo= github.com/ghodss/yaml v0.0.0-20150909031657-73d445a93680/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= +github.com/ghodss/yaml v0.0.0-20180820084758-c7ce16629ff4 h1:bRzFpEzvausOAt4va+I/22BZ1vXDtERngp0BNYDKej0= +github.com/ghodss/yaml v0.0.0-20180820084758-c7ce16629ff4/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04= github.com/globalsign/mgo v0.0.0-20180905125535-1ca0a4f7cbcb/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8 h1:DujepqpGd1hyOd7aW59XpK7Qymp8iy83xq74fLr21is= github.com/globalsign/mgo v0.0.0-20181015135952-eeefdecb41b8/go.mod h1:xkRDCp4j0OGD1HRkm4kmhM+pmpv3AKq5SU7GMg4oO/Q= diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD index 32405eed904..aed96f7fe35 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/openapi/builder/BUILD @@ -27,6 +27,7 @@ go_library( "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//vendor/github.com/emicklei/go-restful:go_default_library", "//vendor/github.com/go-openapi/spec:go_default_library", + "//vendor/k8s.io/kube-openapi/pkg/aggregator:go_default_library", "//vendor/k8s.io/kube-openapi/pkg/builder:go_default_library", "//vendor/k8s.io/kube-openapi/pkg/common:go_default_library", "//vendor/k8s.io/kube-openapi/pkg/util:go_default_library",