From cd0b46874a6b9005187cecb55e885177d1294615 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 11:06:45 +0200 Subject: [PATCH 01/13] apiextensions: add storage race TODO in CRD conversion e2e test --- test/e2e/apimachinery/crd_conversion_webhook.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/e2e/apimachinery/crd_conversion_webhook.go b/test/e2e/apimachinery/crd_conversion_webhook.go index 485b19b002..07f2d3dbc3 100644 --- a/test/e2e/apimachinery/crd_conversion_webhook.go +++ b/test/e2e/apimachinery/crd_conversion_webhook.go @@ -40,6 +40,7 @@ import ( "github.com/onsi/ginkgo" "github.com/onsi/gomega" + // ensure libs have a chance to initialize _ "github.com/stretchr/testify/assert" ) @@ -386,6 +387,8 @@ func testCRListConversion(f *framework.Framework, testCrd *crd.TestCrd) { // After changing a CRD, the resources for versions will be re-created that can be result in // cancelled connection (e.g. "grpc connection closed" or "context canceled"). // Just retrying fixes that. + // + // TODO: we have to wait for the storage version to become effective. Storage version changes are not instant. for i := 0; i < 5; i++ { _, err = customResourceClients["v1"].Create(crInstance, metav1.CreateOptions{}) if err == nil { From 74e62b9a77b07902f35a5b78340e4a92a0f4bf66 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sat, 25 May 2019 16:03:56 +0200 Subject: [PATCH 02/13] apiextensions: fix npe in subresource test --- .../test/integration/subresources_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go index d770f42591..6f3cf72b49 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/subresources_test.go @@ -534,7 +534,7 @@ func TestValidateOnlyStatus(t *testing.T) { } createdNoxuInstance, err = noxuResourceClient.UpdateStatus(createdNoxuInstance, metav1.UpdateOptions{}) if err != nil { - t.Errorf("unexpected error: %v", err) + t.Fatalf("unexpected error: %v", err) } // update with .status.num = 15, expecting an error From 0cafec6608bd46b371702cdaa2a081be81607a32 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 11:17:04 +0200 Subject: [PATCH 03/13] apiextensions: make integration test server flags available to test code --- .../test/integration/fixtures/server.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go index 0a3d6444e9..d2f487051f 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures/server.go @@ -31,7 +31,7 @@ import ( ) // StartDefaultServer starts a test server. -func StartDefaultServer(t servertesting.Logger) (func(), *rest.Config, *options.CustomResourceDefinitionsServerOptions, error) { +func StartDefaultServer(t servertesting.Logger, flags ...string) (func(), *rest.Config, *options.CustomResourceDefinitionsServerOptions, error) { // create kubeconfig which will not actually be used. But authz/authn needs it to startup. fakeKubeConfig, err := ioutil.TempFile("", "kubeconfig") fakeKubeConfig.WriteString(` @@ -55,15 +55,16 @@ users: `) fakeKubeConfig.Close() - s, err := servertesting.StartTestServer(t, nil, []string{ + s, err := servertesting.StartTestServer(t, nil, append([]string{ "--etcd-prefix", uuid.New(), "--etcd-servers", strings.Join(IntegrationEtcdServers(), ","), "--authentication-skip-lookup", "--authentication-kubeconfig", fakeKubeConfig.Name(), "--authorization-kubeconfig", fakeKubeConfig.Name(), "--kubeconfig", fakeKubeConfig.Name(), - "--disable-admission-plugins", "NamespaceLifecycle,MutatingAdmissionWebhook,ValidatingAdmissionWebhook", - }, nil) + "--disable-admission-plugins", "NamespaceLifecycle,MutatingAdmissionWebhook,ValidatingAdmissionWebhook"}, + flags..., + ), nil) if err != nil { os.Remove(fakeKubeConfig.Name()) return nil, nil, nil, err @@ -78,8 +79,8 @@ users: } // StartDefaultServerWithClients starts a test server and returns clients for it. -func StartDefaultServerWithClients(t servertesting.Logger) (func(), clientset.Interface, dynamic.Interface, error) { - tearDown, config, _, err := StartDefaultServer(t) +func StartDefaultServerWithClients(t servertesting.Logger, extraFlags ...string) (func(), clientset.Interface, dynamic.Interface, error) { + tearDown, config, _, err := StartDefaultServer(t, extraFlags...) if err != nil { return nil, nil, nil, err } From 5b5fea050243dd7b84bcf1066dc988d546e3379a Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sat, 25 May 2019 17:08:04 +0200 Subject: [PATCH 04/13] apiextensions: make conversion webhook test server more compositional --- .../test/integration/conversion_test.go | 31 ++++++-- .../test/integration/convert/BUILD | 2 +- .../test/integration/convert/webhook.go | 74 +++++-------------- 3 files changed, 43 insertions(+), 64 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go index 2006895277..0dcf06698e 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go @@ -22,6 +22,7 @@ import ( "net/http" "reflect" "strings" + "sync" "testing" "time" @@ -36,6 +37,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/uuid" + "k8s.io/apimachinery/pkg/util/wait" etcd3watcher "k8s.io/apiserver/pkg/storage/etcd3" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" @@ -131,7 +133,8 @@ func TestWebhookConverter(t *testing.T) { for _, test := range tests { t.Run(test.group, func(t *testing.T) { - tearDown, webhookClientConfig, webhookWaitReady, err := convert.StartConversionWebhookServerWithWaitReady(test.handler) + upCh, handler := closeOnCall(test.handler) + tearDown, webhookClientConfig, err := convert.StartConversionWebhookServer(handler) if err != nil { t.Fatal(err) } @@ -140,12 +143,17 @@ func TestWebhookConverter(t *testing.T) { ctc.setConversionWebhook(t, webhookClientConfig) defer ctc.removeConversionWebhook(t) - err = webhookWaitReady(30*time.Second, func() error { - // the marker's storage version is v1beta2, so a v1beta1 read always triggers conversion + // wait until new webhook is called the first time + if err := wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { _, err := ctc.versionedClient(marker.GetNamespace(), "v1beta1").Get(marker.GetName(), metav1.GetOptions{}) - return err - }) - if err != nil { + select { + case <-upCh: + return true, nil + default: + t.Logf("Waiting for webhook to become effective, getting marker object: %v", err) + return false, nil + } + }); err != nil { t.Fatal(err) } @@ -562,3 +570,14 @@ func newConversionMultiVersionFixture(namespace, name, version string) *unstruct }, } } + +func closeOnCall(h http.Handler) (chan struct{}, http.Handler) { + ch := make(chan struct{}) + once := sync.Once{} + return ch, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + once.Do(func() { + close(ch) + }) + h.ServeHTTP(w, r) + }) +} diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD b/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD index eafa4b428f..9be2cb0b60 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD @@ -10,7 +10,7 @@ go_library( "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go index d829ac2bb5..3a119f0ab1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go @@ -25,91 +25,51 @@ import ( "net/http" "net/http/httptest" "strings" - "sync" "testing" "time" + "k8s.io/apimachinery/pkg/util/wait" + apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/util/uuid" ) -// WaitReadyFunc calls triggerConversionFn periodically and waits until it detects that the webhook -// conversion server has handled at least 1 conversion request or the timeout is exceeded, in which -// case an error is returned. -type WaitReadyFunc func(timeout time.Duration, triggerConversionFn func() error) error - -// StartConversionWebhookServerWithWaitReady starts an http server with the provided handler and returns the WebhookClientConfig -// needed to configure a CRD to use this conversion webhook as its converter. -// It also returns a WaitReadyFunc to be called after the CRD is configured to wait until the conversion webhook handler -// accepts at least one conversion request. If the server fails to start, an error is returned. -// WaitReady is useful when changing the conversion webhook config of an existing CRD because the update does not take effect immediately. -func StartConversionWebhookServerWithWaitReady(handler http.Handler) (func(), *apiextensionsv1beta1.WebhookClientConfig, WaitReadyFunc, error) { - var once sync.Once - handlerReadyC := make(chan struct{}) - readyNotifyHandler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - once.Do(func() { - close(handlerReadyC) - }) - handler.ServeHTTP(w, r) - }) - - tearDown, webhookConfig, err := StartConversionWebhookServer(readyNotifyHandler) - if err != nil { - return nil, nil, nil, fmt.Errorf("error starting webhook server: %v", err) - } - - waitReady := func(timeout time.Duration, triggerConversionFn func() error) error { - var err error - for { - select { - case <-handlerReadyC: - return nil - case <-time.After(timeout): - return fmt.Errorf("Timed out waiting for CRD webhook converter update, last trigger conversion error: %v", err) - case <-time.After(100 * time.Millisecond): - err = triggerConversionFn() - } - } - } - return tearDown, webhookConfig, waitReady, err -} - // StartConversionWebhookServer starts an http server with the provided handler and returns the WebhookClientConfig // needed to configure a CRD to use this conversion webhook as its converter. func StartConversionWebhookServer(handler http.Handler) (func(), *apiextensionsv1beta1.WebhookClientConfig, error) { - // Use a unique path for each webhook server. This ensures that all conversion requests - // received by the handler are intended for it; if a WebhookClientConfig other than this one - // is applied in the api server, conversion requests will not reach the handler (if they - // reach the server they will be returned at 404). This helps prevent tests that require a - // specific conversion webhook from accidentally using a different one, which could otherwise - // cause a test to flake or pass when it should fail. Since updating the conversion client - // config of a custom resource definition does not take effect immediately, this is needed - // by the WaitReady returned StartConversionWebhookServerWithWaitReady to detect when a - // conversion client config change in the api server has taken effect. - path := fmt.Sprintf("/conversionwebhook-%s", uuid.NewUUID()) roots := x509.NewCertPool() if !roots.AppendCertsFromPEM(localhostCert) { - return nil, nil, fmt.Errorf("Failed to append Cert from PEM") + return nil, nil, fmt.Errorf("failed to append Cert from PEM") } cert, err := tls.X509KeyPair(localhostCert, localhostKey) if err != nil { - return nil, nil, fmt.Errorf("Failed to build cert with error: %+v", err) + return nil, nil, fmt.Errorf("failed to build cert with error: %+v", err) } + webhookMux := http.NewServeMux() - webhookMux.Handle(path, handler) + webhookMux.Handle("/convert", handler) webhookServer := httptest.NewUnstartedServer(webhookMux) webhookServer.TLS = &tls.Config{ RootCAs: roots, Certificates: []tls.Certificate{cert}, } webhookServer.StartTLS() - endpoint := webhookServer.URL + path + endpoint := webhookServer.URL + "/convert" webhookConfig := &apiextensionsv1beta1.WebhookClientConfig{ CABundle: localhostCert, URL: &endpoint, } + + // StartTLS returns immediately, there is a small chance of a race to avoid. + if err := wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { + _, err := webhookServer.Client().Get(webhookServer.URL) // even a 404 is fine + return err == nil, nil + }); err != nil { + webhookServer.Close() + return nil, nil, err + } + return webhookServer.Close, webhookConfig, nil } From 743f0e6cc260b6e2524dc552e1981797fc729e1c Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sat, 25 May 2019 17:09:58 +0200 Subject: [PATCH 05/13] apiextensions: always fix storage version after tests --- .../apiextensions-apiserver/test/integration/conversion_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go index 0dcf06698e..d2cb4f1202 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go @@ -160,7 +160,7 @@ func TestWebhookConverter(t *testing.T) { for i, checkFn := range test.checks { name := fmt.Sprintf("check-%d", i) t.Run(name, func(t *testing.T) { - ctc.setAndWaitStorageVersion(t, "v1beta2") + defer ctc.setAndWaitStorageVersion(t, "v1beta2") ctc.namespace = fmt.Sprintf("webhook-conversion-%s-%s", test.group, name) checkFn(t, ctc) }) From 8a6d14afc8914fa6a4f705429d0e78db9e2043db Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 12:08:40 +0200 Subject: [PATCH 06/13] apiextensions: fix non-trivial conversion integration test This adds a third version v1alpha1 which has the same schema as v1beta1. Moreover, v1beta1 becomes the storage version. Hence, we can do noop webhook conversion from v1alpha1 to v1beta1 and back. --- .../test/integration/conversion_test.go | 235 +++++++++++------- 1 file changed, 143 insertions(+), 92 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go index d2cb4f1202..0691c03609 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go @@ -26,6 +26,8 @@ import ( "testing" "time" + "github.com/google/go-cmp/cmp" + "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/apiextensions-apiserver/pkg/cmd/server/options" serveroptions "k8s.io/apiextensions-apiserver/pkg/cmd/server/options" @@ -64,12 +66,12 @@ func TestWebhookConverter(t *testing.T) { { group: "noop-converter", handler: convert.NewObjectConverterWebhookHandler(t, noopConverter), - checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions), + checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1")), // no v1beta2 as the schema differs }, { group: "nontrivial-converter", handler: convert.NewObjectConverterWebhookHandler(t, nontrivialConverter), - checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions), + checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2")), }, { group: "empty-response", @@ -126,7 +128,7 @@ func TestWebhookConverter(t *testing.T) { defer ctcTearDown() // read only object to read at a different version than stored when we need to force conversion - marker, err := ctc.versionedClient("marker", "v1beta2").Create(newConversionMultiVersionFixture("marker", "marker", "v1beta2"), metav1.CreateOptions{}) + marker, err := ctc.versionedClient("marker", "v1beta1").Create(newConversionMultiVersionFixture("marker", "marker", "v1beta1"), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } @@ -145,7 +147,7 @@ func TestWebhookConverter(t *testing.T) { // wait until new webhook is called the first time if err := wait.PollImmediate(time.Millisecond*100, wait.ForeverTestTimeout, func() (bool, error) { - _, err := ctc.versionedClient(marker.GetNamespace(), "v1beta1").Get(marker.GetName(), metav1.GetOptions{}) + _, err := ctc.versionedClient(marker.GetNamespace(), "v1alpha1").Get(marker.GetName(), metav1.GetOptions{}) select { case <-upCh: return true, nil @@ -160,7 +162,7 @@ func TestWebhookConverter(t *testing.T) { for i, checkFn := range test.checks { name := fmt.Sprintf("check-%d", i) t.Run(name, func(t *testing.T) { - defer ctc.setAndWaitStorageVersion(t, "v1beta2") + defer ctc.setAndWaitStorageVersion(t, "v1beta1") ctc.namespace = fmt.Sprintf("webhook-conversion-%s-%s", test.group, name) checkFn(t, ctc) }) @@ -172,11 +174,11 @@ func TestWebhookConverter(t *testing.T) { func validateStorageVersion(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace - for _, version := range []string{"v1beta1", "v1beta2"} { - t.Run(version, func(t *testing.T) { - name := "storageversion-" + version - client := ctc.versionedClient(ns, version) - obj, err := client.Create(newConversionMultiVersionFixture(ns, name, version), metav1.CreateOptions{}) + for _, version := range ctc.crd.Spec.Versions { + t.Run(version.Name, func(t *testing.T) { + name := "storageversion-" + version.Name + client := ctc.versionedClient(ns, version.Name) + obj, err := client.Create(newConversionMultiVersionFixture(ns, name, version.Name), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } @@ -194,66 +196,64 @@ func validateStorageVersion(t *testing.T, ctc *conversionTestContext) { // validateMixedStorageVersions ensures that identical custom resources written at different storage versions // are readable and remain the same. -func validateMixedStorageVersions(t *testing.T, ctc *conversionTestContext) { - ns := ctc.namespace +func validateMixedStorageVersions(versions ...string) func(t *testing.T, ctc *conversionTestContext) { + return func(t *testing.T, ctc *conversionTestContext) { + ns := ctc.namespace + clients := ctc.versionedClients(ns) - v1client := ctc.versionedClient(ns, "v1beta1") - v2client := ctc.versionedClient(ns, "v1beta2") - clients := map[string]dynamic.ResourceInterface{"v1beta1": v1client, "v1beta2": v2client} - versions := []string{"v1beta1", "v1beta2"} + // Create CRs at all storage versions + objNames := []string{} + for _, version := range versions { + ctc.setAndWaitStorageVersion(t, version) - // Create CRs at all storage versions - objNames := []string{} - for _, version := range versions { - ctc.setAndWaitStorageVersion(t, version) - - name := "stored-at-" + version - obj, err := clients[version].Create(newConversionMultiVersionFixture(ns, name, version), metav1.CreateOptions{}) - if err != nil { - t.Fatal(err) - } - objNames = append(objNames, obj.GetName()) - } - - // Ensure copies of an object have the same fields and values at each custom resource definition version regardless of storage version - for clientVersion, client := range clients { - t.Run(clientVersion, func(t *testing.T) { - o1, err := client.Get(objNames[0], metav1.GetOptions{}) + name := "mixedstorage-stored-as-" + version + obj, err := clients[version].Create(newConversionMultiVersionFixture(ns, name, version), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } - for _, objName := range objNames[1:] { - o2, err := client.Get(objName, metav1.GetOptions{}) + objNames = append(objNames, obj.GetName()) + } + + // Ensure copies of an object have the same fields and values at each custom resource definition version regardless of storage version + for clientVersion, client := range clients { + t.Run(clientVersion, func(t *testing.T) { + o1, err := client.Get(objNames[0], metav1.GetOptions{}) if err != nil { t.Fatal(err) } + for _, objName := range objNames[1:] { + o2, err := client.Get(objName, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } - // ignore metadata for comparison purposes - delete(o1.Object, "metadata") - delete(o2.Object, "metadata") - if !reflect.DeepEqual(o1.Object, o2.Object) { - t.Errorf("Expected custom resource to be same regardless of which storage version is used but got %+v != %+v", o1, o2) + // ignore metadata for comparison purposes + delete(o1.Object, "metadata") + delete(o2.Object, "metadata") + if !reflect.DeepEqual(o1.Object, o2.Object) { + t.Errorf("Expected custom resource to be same regardless of which storage version is used to create, but got: %s", cmp.Diff(o1, o2)) + } } - } - }) + }) + } } } func validateServed(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace - for _, version := range []string{"v1beta1", "v1beta2"} { - t.Run(version, func(t *testing.T) { - name := "served-" + version - client := ctc.versionedClient(ns, version) - obj, err := client.Create(newConversionMultiVersionFixture(ns, name, version), metav1.CreateOptions{}) + for _, version := range ctc.crd.Spec.Versions { + t.Run(version.Name, func(t *testing.T) { + name := "served-" + version.Name + client := ctc.versionedClient(ns, version.Name) + obj, err := client.Create(newConversionMultiVersionFixture(ns, name, version.Name), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } - ctc.setServed(t, version, false) - ctc.waitForServed(t, version, false, client, obj) - ctc.setServed(t, version, true) - ctc.waitForServed(t, version, true, client, obj) + ctc.setServed(t, version.Name, false) + ctc.waitForServed(t, version.Name, false, client, obj) + ctc.setServed(t, version.Name, true) + ctc.waitForServed(t, version.Name, true, client, obj) }) } } @@ -261,11 +261,10 @@ func validateServed(t *testing.T, ctc *conversionTestContext) { func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc *conversionTestContext) { return func(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace - v1client := ctc.versionedClient(ns, "v1beta1") - v2client := ctc.versionedClient(ns, "v1beta2") + clients := ctc.versionedClients(ns) var err error - // storage version is v1beta2, so this skips conversion - obj, err := v2client.Create(newConversionMultiVersionFixture(ns, id, "v1beta2"), metav1.CreateOptions{}) + // storage version is v1beta1, so this skips conversion + obj, err := clients["v1beta1"].Create(newConversionMultiVersionFixture(ns, id, "v1beta1"), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } @@ -273,19 +272,19 @@ func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc * t.Run(verb, func(t *testing.T) { switch verb { case "get": - _, err = v1client.Get(obj.GetName(), metav1.GetOptions{}) + _, err = clients["v1beta2"].Get(obj.GetName(), metav1.GetOptions{}) case "list": - _, err = v1client.List(metav1.ListOptions{}) + _, err = clients["v1beta2"].List(metav1.ListOptions{}) case "create": - _, err = v1client.Create(newConversionMultiVersionFixture(ns, id, "v1beta1"), metav1.CreateOptions{}) + _, err = clients["v1beta2"].Create(newConversionMultiVersionFixture(ns, id, "v1beta2"), metav1.CreateOptions{}) case "update": - _, err = v1client.Update(obj, metav1.UpdateOptions{}) + _, err = clients["v1beta2"].Update(obj, metav1.UpdateOptions{}) case "patch": - _, err = v1client.Patch(obj.GetName(), types.MergePatchType, []byte(`{"metadata":{"annotations":{"patch":"true"}}}`), metav1.PatchOptions{}) + _, err = clients["v1beta2"].Patch(obj.GetName(), types.MergePatchType, []byte(`{"metadata":{"annotations":{"patch":"true"}}}`), metav1.PatchOptions{}) case "delete": - err = v1client.Delete(obj.GetName(), &metav1.DeleteOptions{}) + err = clients["v1beta2"].Delete(obj.GetName(), &metav1.DeleteOptions{}) case "deletecollection": - err = v1client.DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}) + err = clients["v1beta2"].DeleteCollection(&metav1.DeleteOptions{}, metav1.ListOptions{}) } if err == nil { @@ -300,11 +299,11 @@ func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc * t.Run(fmt.Sprintf("%s-%s", subresource, verb), func(t *testing.T) { switch verb { case "create": - _, err = v1client.Create(newConversionMultiVersionFixture(ns, id, "v1beta1"), metav1.CreateOptions{}, subresource) + _, err = clients["v1beta2"].Create(newConversionMultiVersionFixture(ns, id, "v1beta2"), metav1.CreateOptions{}, subresource) case "update": - _, err = v1client.Update(obj, metav1.UpdateOptions{}, subresource) + _, err = clients["v1beta2"].Update(obj, metav1.UpdateOptions{}, subresource) case "patch": - _, err = v1client.Patch(obj.GetName(), types.MergePatchType, []byte(`{"metadata":{"annotations":{"patch":"true"}}}`), metav1.PatchOptions{}, subresource) + _, err = clients["v1beta2"].Patch(obj.GetName(), types.MergePatchType, []byte(`{"metadata":{"annotations":{"patch":"true"}}}`), metav1.PatchOptions{}, subresource) } if err == nil { @@ -321,12 +320,12 @@ func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc * func noopConverter(desiredAPIVersion string, obj runtime.RawExtension) (runtime.RawExtension, error) { u := &unstructured.Unstructured{Object: map[string]interface{}{}} if err := json.Unmarshal(obj.Raw, u); err != nil { - return runtime.RawExtension{}, fmt.Errorf("Fail to deserialize object: %s with error: %v", string(obj.Raw), err) + return runtime.RawExtension{}, fmt.Errorf("failed to deserialize object: %s with error: %v", string(obj.Raw), err) } u.Object["apiVersion"] = desiredAPIVersion raw, err := json.Marshal(u) if err != nil { - return runtime.RawExtension{}, fmt.Errorf("Fail to serialize object: %v with error: %v", u, err) + return runtime.RawExtension{}, fmt.Errorf("failed to serialize object: %v with error: %v", u, err) } return runtime.RawExtension{Raw: raw}, nil } @@ -354,26 +353,32 @@ func failureResponseConverter(message string) func(review apiextensionsv1beta1.C func nontrivialConverter(desiredAPIVersion string, obj runtime.RawExtension) (runtime.RawExtension, error) { u := &unstructured.Unstructured{Object: map[string]interface{}{}} if err := json.Unmarshal(obj.Raw, u); err != nil { - return runtime.RawExtension{}, fmt.Errorf("Fail to deserialize object: %s with error: %v", string(obj.Raw), err) + return runtime.RawExtension{}, fmt.Errorf("failed to deserialize object: %s with error: %v", string(obj.Raw), err) } - currentAPIVersion := u.Object["apiVersion"] - if currentAPIVersion == "v1beta2" && desiredAPIVersion == "v1beta1" { + currentAPIVersion := u.GetAPIVersion() + + if currentAPIVersion == "stable.example.com/v1beta2" && (desiredAPIVersion == "stable.example.com/v1alpha1" || desiredAPIVersion == "stable.example.com/v1beta1") { u.Object["num"] = u.Object["numv2"] u.Object["content"] = u.Object["contentv2"] delete(u.Object, "numv2") delete(u.Object, "contentv2") - } - if currentAPIVersion == "v1beta1" && desiredAPIVersion == "v1beta2" { + } else if (currentAPIVersion == "stable.example.com/v1alpha1" || currentAPIVersion == "stable.example.com/v1beta1") && desiredAPIVersion == "stable.example.com/v1beta2" { u.Object["numv2"] = u.Object["num"] u.Object["contentv2"] = u.Object["content"] delete(u.Object, "num") delete(u.Object, "content") + } else if currentAPIVersion == "stable.example.com/v1alpha1" && desiredAPIVersion == "stable.example.com/v1beta1" { + // same schema + } else if currentAPIVersion == "stable.example.com/v1beta1" && desiredAPIVersion == "stable.example.com/v1alpha1" { + // same schema + } else if currentAPIVersion != desiredAPIVersion { + return runtime.RawExtension{}, fmt.Errorf("cannot convert from %s to %s", currentAPIVersion, desiredAPIVersion) } u.Object["apiVersion"] = desiredAPIVersion raw, err := json.Marshal(u) if err != nil { - return runtime.RawExtension{}, fmt.Errorf("Fail to serialize object: %v with error: %v", u, err) + return runtime.RawExtension{}, fmt.Errorf("failed to serialize object: %v with error: %v", u, err) } return runtime.RawExtension{Raw: raw}, nil } @@ -406,6 +411,14 @@ func (c *conversionTestContext) versionedClient(ns string, version string) dynam return newNamespacedCustomResourceVersionedClient(ns, c.dynamicClient, c.crd, version) } +func (c *conversionTestContext) versionedClients(ns string) map[string]dynamic.ResourceInterface { + ret := map[string]dynamic.ResourceInterface{} + for _, v := range c.crd.Spec.Versions { + ret[v.Name] = newNamespacedCustomResourceVersionedClient(ns, c.dynamicClient, c.crd, v.Name) + } + return ret +} + func (c *conversionTestContext) setConversionWebhook(t *testing.T, webhookClientConfig *apiextensionsv1beta1.WebhookClientConfig) { crd, err := c.apiExtensionsClient.ApiextensionsV1beta1().CustomResourceDefinitions().Get(c.crd.Name, metav1.GetOptions{}) if err != nil { @@ -440,15 +453,18 @@ func (c *conversionTestContext) removeConversionWebhook(t *testing.T) { } func (c *conversionTestContext) setAndWaitStorageVersion(t *testing.T, version string) { - c.setStorageVersion(t, "v1beta2") + c.setStorageVersion(t, version) - client := c.versionedClient("probe", "v1beta2") + // create probe object. Version should be the default one to avoid webhook calls during test setup. + client := c.versionedClient("probe", "v1beta1") name := fmt.Sprintf("probe-%v", uuid.NewUUID()) - storageProbe, err := client.Create(newConversionMultiVersionFixture("probe", name, "v1beta2"), metav1.CreateOptions{}) + storageProbe, err := client.Create(newConversionMultiVersionFixture("probe", name, "v1beta1"), metav1.CreateOptions{}) if err != nil { t.Fatal(err) } - c.waitForStorageVersion(t, "v1beta2", c.versionedClient(storageProbe.GetNamespace(), "v1beta2"), storageProbe) + + // update object continuously and wait for etcd to have the target storage version. + c.waitForStorageVersion(t, version, c.versionedClient(storageProbe.GetNamespace(), "v1beta1"), storageProbe) err = client.Delete(name, &metav1.DeleteOptions{}) if err != nil { @@ -462,7 +478,7 @@ func (c *conversionTestContext) setStorageVersion(t *testing.T, version string) t.Fatal(err) } for i, v := range crd.Spec.Versions { - crd.Spec.Versions[i].Storage = (v.Name == version) + crd.Spec.Versions[i].Storage = v.Name == version } crd, err = c.apiExtensionsClient.ApiextensionsV1beta1().CustomResourceDefinitions().Update(crd) if err != nil { @@ -472,13 +488,16 @@ func (c *conversionTestContext) setStorageVersion(t *testing.T, version string) } func (c *conversionTestContext) waitForStorageVersion(t *testing.T, version string, versionedClient dynamic.ResourceInterface, obj *unstructured.Unstructured) *unstructured.Unstructured { - c.etcdObjectReader.WaitForStorageVersion(version, obj.GetNamespace(), obj.GetName(), 30*time.Second, func() { - var err error - obj, err = versionedClient.Update(obj, metav1.UpdateOptions{}) - if err != nil { + if err := c.etcdObjectReader.WaitForStorageVersion(version, obj.GetNamespace(), obj.GetName(), 30*time.Second, func() { + if _, err := versionedClient.Patch(obj.GetName(), types.MergePatchType, []byte(`{}`), metav1.PatchOptions{}); err != nil { t.Fatalf("failed to update object: %v", err) } - }) + }); err != nil { + t.Fatalf("failed waiting for storage version %s: %v", version, err) + } + + t.Logf("Effective storage version: %s", version) + return obj } @@ -531,14 +550,22 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{ Scope: apiextensionsv1beta1.NamespaceScoped, Versions: []apiextensionsv1beta1.CustomResourceDefinitionVersion{ { + // storage version, same schema as v1alpha1 Name: "v1beta1", Served: true, + Storage: true, + }, + { + // same schema as v1beta1 + Name: "v1alpha1", + Served: true, Storage: false, }, { + // different schema than v1beta1 and v1alpha1 Name: "v1beta2", Served: true, - Storage: true, + Storage: false, }, }, Subresources: &apiextensionsv1beta1.CustomResourceSubresources{ @@ -552,7 +579,7 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{ } func newConversionMultiVersionFixture(namespace, name, version string) *unstructured.Unstructured { - return &unstructured.Unstructured{ + u := &unstructured.Unstructured{ Object: map[string]interface{}{ "apiVersion": "stable.example.com/" + version, "kind": "MultiVersion", @@ -560,15 +587,39 @@ func newConversionMultiVersionFixture(namespace, name, version string) *unstruct "namespace": namespace, "name": name, }, - "content": map[string]interface{}{ - "key": "value", - }, - "num": map[string]interface{}{ - "num1": 1, - "num2": 1000000, - }, }, } + + switch version { + case "v1alpha1": + u.Object["content"] = map[string]interface{}{ + "key": "value", + } + u.Object["num"] = map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + } + case "v1beta1": + u.Object["content"] = map[string]interface{}{ + "key": "value", + } + u.Object["num"] = map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + } + case "v1beta2": + u.Object["contentv2"] = map[string]interface{}{ + "key": "value", + } + u.Object["numv2"] = map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + } + default: + panic(fmt.Sprintf("unknown version %s", version)) + } + + return u } func closeOnCall(h http.Handler) (chan struct{}, http.Handler) { From b83be574354d8b6ac3e11308614e84def4fff05d Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sat, 25 May 2019 17:52:22 +0200 Subject: [PATCH 07/13] apiextensions: port conversion e2e tests to integration --- .../test/integration/conversion_test.go | 107 +++++++++++++++++- 1 file changed, 106 insertions(+), 1 deletion(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go index 0691c03609..c631d89eeb 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/apimachinery/pkg/util/wait" etcd3watcher "k8s.io/apiserver/pkg/storage/etcd3" @@ -71,7 +72,7 @@ func TestWebhookConverter(t *testing.T) { { group: "nontrivial-converter", handler: convert.NewObjectConverterWebhookHandler(t, nontrivialConverter), - checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2")), + checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList), }, { group: "empty-response", @@ -258,6 +259,64 @@ func validateServed(t *testing.T, ctc *conversionTestContext) { } } +func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) { + ns := ctc.namespace + + for _, createVersion := range ctc.crd.Spec.Versions { + t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) { + name := "converted-" + createVersion.Name + client := ctc.versionedClient(ns, createVersion.Name) + if _, err := client.Create(newConversionMultiVersionFixture(ns, name, createVersion.Name), metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + + for _, getVersion := range ctc.crd.Spec.Versions { + client := ctc.versionedClient(ns, getVersion.Name) + obj, err := client.Get(name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + verifyMultiVersionObject(t, getVersion.Name, obj) + } + }) + } +} + +func validateNonTrivialConvertedList(t *testing.T, ctc *conversionTestContext) { + ns := ctc.namespace + "-list" + + names := sets.String{} + for _, createVersion := range ctc.crd.Spec.Versions { + name := "converted-" + createVersion.Name + client := ctc.versionedClient(ns, createVersion.Name) + if _, err := client.Create(newConversionMultiVersionFixture(ns, name, createVersion.Name), metav1.CreateOptions{}); err != nil { + t.Fatal(err) + } + names.Insert(name) + } + + for _, listVersion := range ctc.crd.Spec.Versions { + t.Run(fmt.Sprintf("listing objects as %s", listVersion.Name), func(t *testing.T) { + client := ctc.versionedClient(ns, listVersion.Name) + obj, err := client.List(metav1.ListOptions{}) + if err != nil { + t.Fatal(err) + } + if len(obj.Items) != len(ctc.crd.Spec.Versions) { + t.Fatal("unexpected number of items") + } + foundNames := sets.String{} + for _, u := range obj.Items { + foundNames.Insert(u.GetName()) + verifyMultiVersionObject(t, listVersion.Name, &u) + } + if !foundNames.Equal(names) { + t.Errorf("unexpected set of returned items: %s", foundNames.Difference(names)) + } + }) + } +} + func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc *conversionTestContext) { return func(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace @@ -622,6 +681,52 @@ func newConversionMultiVersionFixture(namespace, name, version string) *unstruct return u } +func verifyMultiVersionObject(t *testing.T, v string, obj *unstructured.Unstructured) { + j := runtime.DeepCopyJSON(obj.Object) + + if expected := "stable.example.com/" + v; obj.GetAPIVersion() != expected { + t.Errorf("unexpected apiVersion %q, expected %q", obj.GetAPIVersion(), expected) + return + } + + delete(j, "metadata") + delete(j, "apiVersion") + delete(j, "kind") + + var expected = map[string]map[string]interface{}{ + "v1alpha1": { + "content": map[string]interface{}{ + "key": "value", + }, + "num": map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + }, + }, + "v1beta1": { + "content": map[string]interface{}{ + "key": "value", + }, + "num": map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + }, + }, + "v1beta2": { + "contentv2": map[string]interface{}{ + "key": "value", + }, + "numv2": map[string]interface{}{ + "num1": int64(1), + "num2": int64(1000000), + }, + }, + } + if !reflect.DeepEqual(expected[v], j) { + t.Errorf("unexpected %s object: %s", v, cmp.Diff(expected[v], j)) + } +} + func closeOnCall(h http.Handler) (chan struct{}, http.Handler) { ch := make(chan struct{}) once := sync.Once{} From faa24b2b382cc97580d5bbd248066770b466cc70 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sat, 25 May 2019 22:27:52 +0200 Subject: [PATCH 08/13] apiextensions: move conversion tests into own package --- .../test/integration/conversion/BUILD | 60 +++++++++++++++++++ .../{ => conversion}/conversion_test.go | 21 ++++--- .../{convert => conversion}/webhook.go | 2 +- 3 files changed, 73 insertions(+), 10 deletions(-) create mode 100644 staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD rename staging/src/k8s.io/apiextensions-apiserver/test/integration/{ => conversion}/conversion_test.go (97%) rename staging/src/k8s.io/apiextensions-apiserver/test/integration/{convert => conversion}/webhook.go (99%) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD new file mode 100644 index 0000000000..2dff89b680 --- /dev/null +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD @@ -0,0 +1,60 @@ +package(default_visibility = ["//visibility:public"]) + +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") + +go_test( + name = "go_default_test", + srcs = ["conversion_test.go"], + embed = [":go_default_library"], + tags = ["integration"], + deps = [ + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures:go_default_library", + "//staging/src/k8s.io/apiextensions-apiserver/test/integration/storage:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/storage/etcd3:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", + "//staging/src/k8s.io/client-go/dynamic:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", + ], +) + +go_library( + name = "go_default_library", + srcs = ["webhook.go"], + importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/test/integration/conversion", + importpath = "k8s.io/apiextensions-apiserver/test/integration/conversion", + visibility = ["//visibility:public"], + deps = [ + "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", + ], +) + +filegroup( + name = "package-srcs", + srcs = glob(["**"]), + tags = ["automanaged"], + visibility = ["//visibility:private"], +) + +filegroup( + name = "all-srcs", + srcs = [":package-srcs"], + tags = ["automanaged"], + visibility = ["//visibility:public"], +) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go similarity index 97% rename from staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go rename to staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index c631d89eeb..90cf41d003 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package integration +package conversion import ( "encoding/json" @@ -47,7 +47,6 @@ import ( featuregatetesting "k8s.io/component-base/featuregate/testing" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - "k8s.io/apiextensions-apiserver/test/integration/convert" "k8s.io/apiextensions-apiserver/test/integration/fixtures" "k8s.io/apiextensions-apiserver/test/integration/storage" ) @@ -66,22 +65,22 @@ func TestWebhookConverter(t *testing.T) { }{ { group: "noop-converter", - handler: convert.NewObjectConverterWebhookHandler(t, noopConverter), + handler: NewObjectConverterWebhookHandler(t, noopConverter), checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1")), // no v1beta2 as the schema differs }, { group: "nontrivial-converter", - handler: convert.NewObjectConverterWebhookHandler(t, nontrivialConverter), + handler: NewObjectConverterWebhookHandler(t, nontrivialConverter), checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList), }, { group: "empty-response", - handler: convert.NewReviewWebhookHandler(t, emptyResponseConverter), + handler: NewReviewWebhookHandler(t, emptyResponseConverter), checks: checks(expectConversionFailureMessage("empty-response", "expected 1 converted objects")), }, { group: "failure-message", - handler: convert.NewReviewWebhookHandler(t, failureResponseConverter("custom webhook conversion error")), + handler: NewReviewWebhookHandler(t, failureResponseConverter("custom webhook conversion error")), checks: checks(expectConversionFailureMessage("failure-message", "custom webhook conversion error")), }, } @@ -137,7 +136,7 @@ func TestWebhookConverter(t *testing.T) { for _, test := range tests { t.Run(test.group, func(t *testing.T) { upCh, handler := closeOnCall(test.handler) - tearDown, webhookClientConfig, err := convert.StartConversionWebhookServer(handler) + tearDown, webhookClientConfig, err := StartConversionWebhookServer(handler) if err != nil { t.Fatal(err) } @@ -467,13 +466,17 @@ type conversionTestContext struct { } func (c *conversionTestContext) versionedClient(ns string, version string) dynamic.ResourceInterface { - return newNamespacedCustomResourceVersionedClient(ns, c.dynamicClient, c.crd, version) + gvr := schema.GroupVersionResource{Group: c.crd.Spec.Group, Version: version, Resource: c.crd.Spec.Names.Plural} + if c.crd.Spec.Scope != apiextensionsv1beta1.ClusterScoped { + return c.dynamicClient.Resource(gvr).Namespace(ns) + } + return c.dynamicClient.Resource(gvr) } func (c *conversionTestContext) versionedClients(ns string) map[string]dynamic.ResourceInterface { ret := map[string]dynamic.ResourceInterface{} for _, v := range c.crd.Spec.Versions { - ret[v.Name] = newNamespacedCustomResourceVersionedClient(ns, c.dynamicClient, c.crd, v.Name) + ret[v.Name] = c.versionedClient(ns, v.Name) } return ret } diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go similarity index 99% rename from staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go rename to staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go index 3a119f0ab1..e1dcfb84a3 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/webhook.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/webhook.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package convert +package conversion import ( "crypto/tls" From ee4f1847f5cd5199685aed66cee6ba394d4b2d25 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sun, 26 May 2019 00:27:13 +0200 Subject: [PATCH 09/13] Update generated files --- .../test/integration/BUILD | 7 +---- .../test/integration/convert/BUILD | 29 ------------------- 2 files changed, 1 insertion(+), 35 deletions(-) delete mode 100644 staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/BUILD b/staging/src/k8s.io/apiextensions-apiserver/test/integration/BUILD index c8e993dd5f..252307c8e0 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/BUILD @@ -12,7 +12,6 @@ go_test( "apply_test.go", "basic_test.go", "change_test.go", - "conversion_test.go", "finalization_test.go", "objectmeta_test.go", "pruning_test.go", @@ -32,9 +31,7 @@ go_test( "//staging/src/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset/scheme:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/cmd/server/options:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/pkg/features:go_default_library", - "//staging/src/k8s.io/apiextensions-apiserver/test/integration/convert:go_default_library", "//staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures:go_default_library", - "//staging/src/k8s.io/apiextensions-apiserver/test/integration/storage:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", @@ -45,13 +42,11 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/types:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/json:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/uuid:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/yaml:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/watch:go_default_library", "//staging/src/k8s.io/apiserver/pkg/endpoints/request:go_default_library", "//staging/src/k8s.io/apiserver/pkg/features:go_default_library", - "//staging/src/k8s.io/apiserver/pkg/storage/etcd3:go_default_library", "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", "//staging/src/k8s.io/client-go/dynamic:go_default_library", "//staging/src/k8s.io/client-go/rest:go_default_library", @@ -76,7 +71,7 @@ filegroup( name = "all-srcs", srcs = [ ":package-srcs", - "//staging/src/k8s.io/apiextensions-apiserver/test/integration/convert:all-srcs", + "//staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion:all-srcs", "//staging/src/k8s.io/apiextensions-apiserver/test/integration/fixtures:all-srcs", "//staging/src/k8s.io/apiextensions-apiserver/test/integration/storage:all-srcs", ], diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD b/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD deleted file mode 100644 index 9be2cb0b60..0000000000 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/convert/BUILD +++ /dev/null @@ -1,29 +0,0 @@ -load("@io_bazel_rules_go//go:def.bzl", "go_library") - -go_library( - name = "go_default_library", - srcs = ["webhook.go"], - importmap = "k8s.io/kubernetes/vendor/k8s.io/apiextensions-apiserver/test/integration/convert", - importpath = "k8s.io/apiextensions-apiserver/test/integration/convert", - visibility = ["//visibility:public"], - deps = [ - "//staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", - "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library", - ], -) - -filegroup( - name = "package-srcs", - srcs = glob(["**"]), - tags = ["automanaged"], - visibility = ["//visibility:private"], -) - -filegroup( - name = "all-srcs", - srcs = [":package-srcs"], - tags = ["automanaged"], - visibility = ["//visibility:public"], -) From d87de1a90359291d1c9d7ff68cc927ddc9837f61 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 18:01:31 +0200 Subject: [PATCH 10/13] apiextensions: prune all GVKs in serializer --- .../pkg/apiserver/customresource_handler.go | 45 +++++++++++-------- 1 file changed, 27 insertions(+), 18 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go index c9088ad496..b977de6521 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go @@ -493,6 +493,23 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource statusScopes := map[string]*handlers.RequestScope{} scaleScopes := map[string]*handlers.RequestScope{} + structuralSchemas := map[string]*structuralschema.Structural{} + for _, v := range crd.Spec.Versions { + val, err := apiextensions.GetSchemaForVersion(crd, v.Name) + if err != nil { + utilruntime.HandleError(err) + return nil, fmt.Errorf("the server could not properly serve the CR schema") + } + if val == nil { + continue + } + structuralSchemas[v.Name], err = structuralschema.NewStructural(val.OpenAPIV3Schema) + if *crd.Spec.PreserveUnknownFields == false && err != nil { + utilruntime.HandleError(err) + return nil, fmt.Errorf("the server could not properly serve the CR schema") // validation should avoid this + } + } + for _, v := range crd.Spec.Versions { safeConverter, unsafeConverter, err := r.converterFactory.NewConverter(crd) if err != nil { @@ -529,14 +546,6 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource return nil, fmt.Errorf("unexpected nil spec.preserveUnknownFields in the CustomResourceDefinition") } - var structuralSchema *structuralschema.Structural - if validationSchema != nil { - structuralSchema, err = structuralschema.NewStructural(validationSchema.OpenAPIV3Schema) - if *crd.Spec.PreserveUnknownFields == false && err != nil { - return nil, err // validation should avoid this - } - } - var statusSpec *apiextensions.CustomResourceSubresourceStatus var statusValidator *validate.SchemaValidator subresources, err := apiextensions.GetSubresourcesForVersion(crd, v.Name) @@ -591,7 +600,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource converter: safeConverter, decoderVersion: schema.GroupVersion{Group: crd.Spec.Group, Version: v.Name}, encoderVersion: schema.GroupVersion{Group: crd.Spec.Group, Version: storageVersion}, - structuralSchema: structuralSchema, + structuralSchemas: structuralSchemas, structuralSchemaGK: kind.GroupKind(), preserveUnknownFields: *crd.Spec.PreserveUnknownFields, }, @@ -619,7 +628,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource typer: typer, creator: creator, converter: safeConverter, - structuralSchema: structuralSchema, + structuralSchemas: structuralSchemas, structuralSchemaGK: kind.GroupKind(), preserveUnknownFields: *crd.Spec.PreserveUnknownFields, }, @@ -676,7 +685,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensions.CustomResource statusScope.Serializer = unstructuredNegotiatedSerializer{ typer: typer, creator: creator, converter: safeConverter, - structuralSchema: structuralSchema, + structuralSchemas: structuralSchemas, structuralSchemaGK: kind.GroupKind(), preserveUnknownFields: *crd.Spec.PreserveUnknownFields, } @@ -715,7 +724,7 @@ type unstructuredNegotiatedSerializer struct { creator runtime.ObjectCreater converter runtime.ObjectConvertor - structuralSchema *structuralschema.Structural + structuralSchemas map[string]*structuralschema.Structural // by version structuralSchemaGK schema.GroupKind preserveUnknownFields bool } @@ -750,7 +759,7 @@ func (s unstructuredNegotiatedSerializer) EncoderForVersion(encoder runtime.Enco } func (s unstructuredNegotiatedSerializer) DecoderToVersion(decoder runtime.Decoder, gv runtime.GroupVersioner) runtime.Decoder { - d := schemaCoercingDecoder{delegate: decoder, validator: unstructuredSchemaCoercer{structuralSchema: s.structuralSchema, structuralSchemaGK: s.structuralSchemaGK, preserveUnknownFields: s.preserveUnknownFields}} + d := schemaCoercingDecoder{delegate: decoder, validator: unstructuredSchemaCoercer{structuralSchemas: s.structuralSchemas, structuralSchemaGK: s.structuralSchemaGK, preserveUnknownFields: s.preserveUnknownFields}} return versioning.NewDefaultingCodecForScheme(Scheme, nil, d, nil, gv) } @@ -842,7 +851,7 @@ type crdConversionRESTOptionsGetter struct { converter runtime.ObjectConvertor encoderVersion schema.GroupVersion decoderVersion schema.GroupVersion - structuralSchema *structuralschema.Structural + structuralSchemas map[string]*structuralschema.Structural // by version structuralSchemaGK schema.GroupKind preserveUnknownFields bool } @@ -853,12 +862,12 @@ func (t crdConversionRESTOptionsGetter) GetRESTOptions(resource schema.GroupReso d := schemaCoercingDecoder{delegate: ret.StorageConfig.Codec, validator: unstructuredSchemaCoercer{ // drop invalid fields while decoding old CRs (before we haven't had any ObjectMeta validation) dropInvalidMetadata: true, - structuralSchema: t.structuralSchema, + structuralSchemas: t.structuralSchemas, structuralSchemaGK: t.structuralSchemaGK, preserveUnknownFields: t.preserveUnknownFields, }} c := schemaCoercingConverter{delegate: t.converter, validator: unstructuredSchemaCoercer{ - structuralSchema: t.structuralSchema, + structuralSchemas: t.structuralSchemas, structuralSchemaGK: t.structuralSchemaGK, preserveUnknownFields: t.preserveUnknownFields, }} @@ -950,7 +959,7 @@ func (v schemaCoercingConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, type unstructuredSchemaCoercer struct { dropInvalidMetadata bool - structuralSchema *structuralschema.Structural + structuralSchemas map[string]*structuralschema.Structural structuralSchemaGK schema.GroupKind preserveUnknownFields bool } @@ -976,7 +985,7 @@ func (v *unstructuredSchemaCoercer) apply(u *unstructured.Unstructured) error { return err } if !v.preserveUnknownFields && gv.Group == v.structuralSchemaGK.Group && kind == v.structuralSchemaGK.Kind { - structuralpruning.Prune(u.Object, v.structuralSchema) + structuralpruning.Prune(u.Object, v.structuralSchemas[gv.Version]) } // restore meta fields, starting clean From 26366255fcecac01ee7b6e8eb69a175307ba5f39 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 22:56:16 +0200 Subject: [PATCH 11/13] apiextensions: run conversion tests also with preserveUnknownFields=false --- .../integration/conversion/conversion_test.go | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index 90cf41d003..228b7fdcd1 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -45,6 +45,7 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/dynamic" featuregatetesting "k8s.io/component-base/featuregate/testing" + "k8s.io/utils/pointer" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" "k8s.io/apiextensions-apiserver/test/integration/fixtures" @@ -58,6 +59,14 @@ func checks(checkers ...Checker) []Checker { } func TestWebhookConverter(t *testing.T) { + testWebhookConverter(t, false) +} + +func TestWebhookConverterWithPruning(t *testing.T) { + testWebhookConverter(t, true) +} + +func testWebhookConverter(t *testing.T, pruning bool) { tests := []struct { group string handler http.Handler @@ -111,6 +120,7 @@ func TestWebhookConverter(t *testing.T) { defer tearDown() crd := multiVersionFixture.DeepCopy() + crd.Spec.PreserveUnknownFields = pointer.BoolPtr(!pruning) RESTOptionsGetter := serveroptions.NewCRDRESTOptionsGetter(*options.RecommendedOptions.Etcd) restOptions, err := RESTOptionsGetter.GetRESTOptions(schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural}) @@ -616,18 +626,78 @@ var multiVersionFixture = &apiextensionsv1beta1.CustomResourceDefinition{ Name: "v1beta1", Served: true, Storage: true, + Schema: &apiextensionsv1beta1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "content": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "key": {Type: "string"}, + }, + }, + "num": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "num1": {Type: "integer"}, + "num2": {Type: "integer"}, + }, + }, + }, + }, + }, }, { // same schema as v1beta1 Name: "v1alpha1", Served: true, Storage: false, + Schema: &apiextensionsv1beta1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "content": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "key": {Type: "string"}, + }, + }, + "num": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "num1": {Type: "integer"}, + "num2": {Type: "integer"}, + }, + }, + }, + }, + }, }, { // different schema than v1beta1 and v1alpha1 Name: "v1beta2", Served: true, Storage: false, + Schema: &apiextensionsv1beta1.CustomResourceValidation{ + OpenAPIV3Schema: &apiextensionsv1beta1.JSONSchemaProps{ + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "contentv2": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "key": {Type: "string"}, + }, + }, + "numv2": { + Type: "object", + Properties: map[string]apiextensionsv1beta1.JSONSchemaProps{ + "num1": {Type: "integer"}, + "num2": {Type: "integer"}, + }, + }, + }, + }, + }, }, }, Subresources: &apiextensionsv1beta1.CustomResourceSubresources{ From 33e95bc185f016affd210744b012e0686ebaa8e8 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Fri, 24 May 2019 23:28:26 +0200 Subject: [PATCH 12/13] apiextensions: add conversion pruning tests --- .../test/integration/conversion/BUILD | 1 + .../integration/conversion/conversion_test.go | 96 ++++++++++++++++++- .../test/integration/storage/objectreader.go | 14 +++ 3 files changed, 106 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD index 2dff89b680..4468e4bff4 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/BUILD @@ -28,6 +28,7 @@ go_test( "//staging/src/k8s.io/client-go/dynamic:go_default_library", "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", "//vendor/github.com/google/go-cmp/cmp:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go index 228b7fdcd1..192f0b9aad 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/conversion/conversion_test.go @@ -80,7 +80,7 @@ func testWebhookConverter(t *testing.T, pruning bool) { { group: "nontrivial-converter", handler: NewObjectConverterWebhookHandler(t, nontrivialConverter), - checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList), + checks: checks(validateStorageVersion, validateServed, validateMixedStorageVersions("v1alpha1", "v1beta1", "v1beta2"), validateNonTrivialConverted, validateNonTrivialConvertedList, validateStoragePruning), }, { group: "empty-response", @@ -275,10 +275,24 @@ func validateNonTrivialConverted(t *testing.T, ctc *conversionTestContext) { t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) { name := "converted-" + createVersion.Name client := ctc.versionedClient(ns, createVersion.Name) - if _, err := client.Create(newConversionMultiVersionFixture(ns, name, createVersion.Name), metav1.CreateOptions{}); err != nil { + + fixture := newConversionMultiVersionFixture(ns, name, createVersion.Name) + if !*ctc.crd.Spec.PreserveUnknownFields { + if err := unstructured.SetNestedField(fixture.Object, "foo", "garbage"); err != nil { + t.Fatal(err) + } + } + if _, err := client.Create(fixture, metav1.CreateOptions{}); err != nil { t.Fatal(err) } + // verify that the right, pruned version is in storage + obj, err := ctc.etcdObjectReader.GetStoredCustomResource(ns, name) + if err != nil { + t.Fatal(err) + } + verifyMultiVersionObject(t, "v1beta1", obj) + for _, getVersion := range ctc.crd.Spec.Versions { client := ctc.versionedClient(ns, getVersion.Name) obj, err := client.Get(name, metav1.GetOptions{}) @@ -298,7 +312,14 @@ func validateNonTrivialConvertedList(t *testing.T, ctc *conversionTestContext) { for _, createVersion := range ctc.crd.Spec.Versions { name := "converted-" + createVersion.Name client := ctc.versionedClient(ns, createVersion.Name) - if _, err := client.Create(newConversionMultiVersionFixture(ns, name, createVersion.Name), metav1.CreateOptions{}); err != nil { + fixture := newConversionMultiVersionFixture(ns, name, createVersion.Name) + if !*ctc.crd.Spec.PreserveUnknownFields { + if err := unstructured.SetNestedField(fixture.Object, "foo", "garbage"); err != nil { + t.Fatal(err) + } + } + _, err := client.Create(fixture, metav1.CreateOptions{}) + if err != nil { t.Fatal(err) } names.Insert(name) @@ -326,6 +347,67 @@ func validateNonTrivialConvertedList(t *testing.T, ctc *conversionTestContext) { } } +func validateStoragePruning(t *testing.T, ctc *conversionTestContext) { + if *ctc.crd.Spec.PreserveUnknownFields { + return + } + + ns := ctc.namespace + + for _, createVersion := range ctc.crd.Spec.Versions { + t.Run(fmt.Sprintf("getting objects created as %s", createVersion.Name), func(t *testing.T) { + name := "storagepruning-" + createVersion.Name + client := ctc.versionedClient(ns, createVersion.Name) + + fixture := newConversionMultiVersionFixture(ns, name, createVersion.Name) + if err := unstructured.SetNestedField(fixture.Object, "foo", "garbage"); err != nil { + t.Fatal(err) + } + _, err := client.Create(fixture, metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + + // verify that the right, pruned version is in storage + obj, err := ctc.etcdObjectReader.GetStoredCustomResource(ns, name) + if err != nil { + t.Fatal(err) + } + verifyMultiVersionObject(t, "v1beta1", obj) + + // add garbage and set a label + if err := unstructured.SetNestedField(obj.Object, "foo", "garbage"); err != nil { + t.Fatal(err) + } + labels := obj.GetLabels() + if labels == nil { + labels = map[string]string{} + } + labels["mutated"] = "true" + obj.SetLabels(labels) + if err := ctc.etcdObjectReader.SetStoredCustomResource(ns, name, obj); err != nil { + t.Fatal(err) + } + + for _, getVersion := range ctc.crd.Spec.Versions { + client := ctc.versionedClient(ns, getVersion.Name) + obj, err := client.Get(name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + + // check that the direct mutation in etcd worked + labels := obj.GetLabels() + if labels["mutated"] != "true" { + t.Errorf("expected object %s in version %s to have label 'mutated=true'", name, getVersion.Name) + } + + verifyMultiVersionObject(t, getVersion.Name, obj) + } + }) + } +} + func expectConversionFailureMessage(id, message string) func(t *testing.T, ctc *conversionTestContext) { return func(t *testing.T, ctc *conversionTestContext) { ns := ctc.namespace @@ -763,11 +845,11 @@ func verifyMultiVersionObject(t *testing.T, v string, obj *unstructured.Unstruct } delete(j, "metadata") - delete(j, "apiVersion") - delete(j, "kind") var expected = map[string]map[string]interface{}{ "v1alpha1": { + "apiVersion": "stable.example.com/v1alpha1", + "kind": "MultiVersion", "content": map[string]interface{}{ "key": "value", }, @@ -777,6 +859,8 @@ func verifyMultiVersionObject(t *testing.T, v string, obj *unstructured.Unstruct }, }, "v1beta1": { + "apiVersion": "stable.example.com/v1beta1", + "kind": "MultiVersion", "content": map[string]interface{}{ "key": "value", }, @@ -786,6 +870,8 @@ func verifyMultiVersionObject(t *testing.T, v string, obj *unstructured.Unstruct }, }, "v1beta2": { + "apiVersion": "stable.example.com/v1beta2", + "kind": "MultiVersion", "contentv2": map[string]interface{}{ "key": "value", }, diff --git a/staging/src/k8s.io/apiextensions-apiserver/test/integration/storage/objectreader.go b/staging/src/k8s.io/apiextensions-apiserver/test/integration/storage/objectreader.go index a6e21fd624..d784458ce9 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/test/integration/storage/objectreader.go +++ b/staging/src/k8s.io/apiextensions-apiserver/test/integration/storage/objectreader.go @@ -84,6 +84,20 @@ func (s *EtcdObjectReader) GetStoredCustomResource(ns, name string) (*unstructur return u, nil } +// SetStoredCustomResource writes the storage representation of a custom resource to etcd. +func (s *EtcdObjectReader) SetStoredCustomResource(ns, name string, obj *unstructured.Unstructured) error { + bs, err := obj.MarshalJSON() + if err != nil { + return err + } + + key := path.Join("/", s.storagePrefix, s.crd.Spec.Group, s.crd.Spec.Names.Plural, ns, name) + if _, err := s.etcdClient.KV.Put(context.Background(), key, string(bs)); err != nil { + return fmt.Errorf("error setting storage object %s, %s from etcd at key %s: %v", ns, name, key, err) + } + return nil +} + // GetEtcdClients returns an initialized clientv3.Client and clientv3.KV. func GetEtcdClients(config storagebackend.TransportConfig) (*clientv3.Client, clientv3.KV, error) { tlsInfo := transport.TLSInfo{ From 3ccb26c3bb7c2fd995292cf6664ea23d5d25ae4b Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Sun, 26 May 2019 09:51:02 +0200 Subject: [PATCH 13/13] apiextensions: fix metrics double registration during tests --- .../pkg/apiserver/conversion/converter.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go index 3ad5672034..d9827ca4cb 100644 --- a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go +++ b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go @@ -35,14 +35,15 @@ type CRConverterFactory struct { // webhookConverterFactory is the factory for webhook converters. // This field should not be used if CustomResourceWebhookConversion feature is disabled. webhookConverterFactory *webhookConverterFactory - converterMetricFactory *converterMetricFactory } +// converterMetricFactorySingleton protects us from reregistration of metrics on repeated +// apiextensions-apiserver runs. +var converterMetricFactorySingleton = newConverterMertricFactory() + // NewCRConverterFactory creates a new CRConverterFactory func NewCRConverterFactory(serviceResolver webhook.ServiceResolver, authResolverWrapper webhook.AuthenticationInfoResolverWrapper) (*CRConverterFactory, error) { - converterFactory := &CRConverterFactory{ - converterMetricFactory: newConverterMertricFactory(), - } + converterFactory := &CRConverterFactory{} if utilfeature.DefaultFeatureGate.Enabled(apiextensionsfeatures.CustomResourceWebhookConversion) { webhookConverterFactory, err := newWebhookConverterFactory(serviceResolver, authResolverWrapper) if err != nil { @@ -72,7 +73,7 @@ func (m *CRConverterFactory) NewConverter(crd *apiextensions.CustomResourceDefin if err != nil { return nil, nil, err } - converter, err = m.converterMetricFactory.addMetrics("webhook", crd.Name, converter) + converter, err = converterMetricFactorySingleton.addMetrics("webhook", crd.Name, converter) if err != nil { return nil, nil, err }