From 94f0a6e6dddd5a6a5505a275a88bcca97d4511e6 Mon Sep 17 00:00:00 2001 From: Mihai Alexandrescu Date: Fri, 18 Aug 2023 18:59:41 +0300 Subject: [PATCH] refactor Apply-type functions --- tests/e2e/install_cluster.go | 3 +- tests/e2e/k8s.go | 80 +++++++++++++------- tests/e2e/kafkatopic_webhook.go | 129 ++++++++++++-------------------- 3 files changed, 101 insertions(+), 111 deletions(-) diff --git a/tests/e2e/install_cluster.go b/tests/e2e/install_cluster.go index a78b7d0000..4699e4a1e4 100644 --- a/tests/e2e/install_cluster.go +++ b/tests/e2e/install_cluster.go @@ -37,7 +37,8 @@ func requireCreatingKafkaCluster(kubectlOptions k8s.KubectlOptions, manifestPath By(fmt.Sprintf("KafkaCluster %s already exists\n", kafkaClusterName)) } else { By("Deploying a KafkaCluster") - applyK8sResourceManifest(kubectlOptions, manifestPath) + err := applyK8sResourceManifest(kubectlOptions, manifestPath) + Expect(err).NotTo(HaveOccurred()) } By("Verifying the KafkaCluster state") diff --git a/tests/e2e/k8s.go b/tests/e2e/k8s.go index e1c42cefbd..e8b07f4e9a 100644 --- a/tests/e2e/k8s.go +++ b/tests/e2e/k8s.go @@ -42,13 +42,58 @@ const ( // crdNamePrefix is the prefix of the CRD names when listed through kubectl. crdNamePrefix = "customresourcedefinition.apiextensions.k8s.io/" + + // Per the kubectl spec, "dry-run" strategy must be "none", "server", or "client". + // With current use cases in e2e tests, we only expect to use "server" so the other options are commented out. + dryRunStrategyArgServer string = "--dry-run=server" // submit server-side request without persisting the resource + //dryRunStrategyArgClient string = "--dry-run=client" // the resource is only validated locally but not sent to the Api-server. + //dryRunStrategyArgNone string = "--dry-run=none" // default value; submit server-side request and persist the resource + ) // applyK8sResourceManifests applies the specified manifest to the provided // kubectl context and namespace. -func applyK8sResourceManifest(kubectlOptions k8s.KubectlOptions, manifestPath string) { //nolint:unused // Note: this might come in handy for manual K8s resource operations. - By(fmt.Sprintf("Applying k8s manifest %s", manifestPath)) - k8s.KubectlApply(GinkgoT(), &kubectlOptions, manifestPath) +func applyK8sResourceManifest(kubectlOptions k8s.KubectlOptions, manifestPath string, extraArgs ...string) error { //nolint:unused // Note: this might come in handy for manual K8s resource operations. + args := []string{"apply", "-f", manifestPath} + logMsg := fmt.Sprintf("Applying k8s manifest from path %s", manifestPath) + logMsg, args = kubectlArgExtender(args, logMsg, "", "", kubectlOptions.Namespace, extraArgs) + By(logMsg) + + return k8s.RunKubectlE(GinkgoT(), &kubectlOptions, args...) +} + +// applyK8sResourceManifestFromString applies the specified manifest in string format to the provided +// kubectl context and namespace. +func applyK8sResourceManifestFromString(kubectlOptions k8s.KubectlOptions, manifest string, extraArgs ...string) error { + // Replicating terratest's k8s.KubectlApplyFromStringE but with the possibility of a variadic argument that allows options like --dry-run + // + // TODO: look for a different implementation for temp files because terratest's version uses the composite test name to generate + // the temp file name which, in our case, includes all the descriptive Ginkgo statements (which are by design quite verbose). + // That can lead to erroring out on temp file creation based on the file name being too long. + tmpfile, err := k8s.StoreConfigToTempFileE(GinkgoT(), manifest) + if err != nil { + return err + } + defer os.Remove(tmpfile) + + return applyK8sResourceManifest(kubectlOptions, tmpfile, extraArgs...) +} + +// applyK8sResourceFromTemplate generates manifest from the specified go-template based on values +// and applies the specified manifest to the provided kubectl context and namespace. +func applyK8sResourceFromTemplate(kubectlOptions k8s.KubectlOptions, templateFile string, values map[string]interface{}, extraArgs ...string) error { + By(fmt.Sprintf("Generating k8s manifest from template %s", templateFile)) + var manifest bytes.Buffer + rawTemplate, err := os.ReadFile(templateFile) + if err != nil { + return err + } + t := template.Must(template.New("template").Funcs(sprig.TxtFuncMap()).Parse(string(rawTemplate))) + err = t.Execute(&manifest, values) + if err != nil { + return err + } + return applyK8sResourceManifestFromString(kubectlOptions, manifest.String(), extraArgs...) } // isExistingK8SResource queries a Resource by it's kind, namespace and name and @@ -191,7 +236,10 @@ func installK8sCRD(kubectlOptions k8s.KubectlOptions, crd []byte, shouldBeValida createOrReplaceK8sResourcesFromManifest(kubectlOptions, "crd", object.GetName(), tempPath, shouldBeValidated) default: // Note: regular CRD. - applyK8sResourceManifest(kubectlOptions, tempPath) + err = applyK8sResourceManifest(kubectlOptions, tempPath) + if err != nil { + return errors.WrapIfWithDetails(err, "applying CRD failed", "crd", string(crd)) + } } return nil @@ -453,30 +501,6 @@ func deleteK8sResourceNoErrNotFound(kubectlOptions k8s.KubectlOptions, timeout t return err } -// applyK8sResourceManifestFromString applies the specified manifest in string format to the provided -// kubectl context and namespace. -func applyK8sResourceManifestFromString(kubectlOptions k8s.KubectlOptions, manifest string) error { - By(fmt.Sprintf("Applying k8s manifest\n%s", manifest)) - return k8s.KubectlApplyFromStringE(GinkgoT(), &kubectlOptions, manifest) -} - -// applyK8sResourceFromTemplate generates manifest from the specified go-template based on values -// and applies the specified manifest to the provided kubectl context and namespace. -func applyK8sResourceFromTemplate(kubectlOptions k8s.KubectlOptions, templateFile string, values map[string]interface{}) error { - By(fmt.Sprintf("Generating K8s manifest from template %s", templateFile)) - var manifest bytes.Buffer - rawTemplate, err := os.ReadFile(templateFile) - if err != nil { - return err - } - t := template.Must(template.New("template").Funcs(sprig.TxtFuncMap()).Parse(string(rawTemplate))) - err = t.Execute(&manifest, values) - if err != nil { - return err - } - return applyK8sResourceManifestFromString(kubectlOptions, manifest.String()) -} - // listK8sResourceKinds lists all of the available resource kinds on the K8s cluster // with the apiGroupSelector parameter the result can be narrowed by the resource group. // extraArgs can be any kubectl api-resources parameter. diff --git a/tests/e2e/kafkatopic_webhook.go b/tests/e2e/kafkatopic_webhook.go index 2729139dd9..f3e801f5c3 100644 --- a/tests/e2e/kafkatopic_webhook.go +++ b/tests/e2e/kafkatopic_webhook.go @@ -15,50 +15,13 @@ package e2e import ( - "bytes" - "fmt" - "os" "strings" - "text/template" - "github.com/Masterminds/sprig" "github.com/gruntwork-io/terratest/modules/k8s" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) -// TODO(mihalexa): move to k8s.go -const ( - dryRunStrategyNone string = "none" - dryRunStrategyClient string = "client" - dryRunStrategyServer string = "server" -) - -// TODO(mihalexa): move to k8s.go -// applyK8sResourceFromTemplateWithDryRun is copy of applyK8sResourceFromTemplate which calls a "--dry-run=" kubectl command -func applyK8sResourceFromTemplateWithDryRun(kubectlOptions k8s.KubectlOptions, templateFile string, values map[string]interface{}, dryRunStrategy string) (string, error) { - By(fmt.Sprintf("Generating K8s manifest from template %s for dry-run apply", templateFile)) - var manifest bytes.Buffer - rawTemplate, err := os.ReadFile(templateFile) - if err != nil { - return "", err - } - t := template.Must(template.New("template").Funcs(sprig.TxtFuncMap()).Parse(string(rawTemplate))) - err = t.Execute(&manifest, values) - if err != nil { - return "", err - } - - By("Replicating terratest's k8s.KubectlApplyFromStringE") - tmpfile, err := k8s.StoreConfigToTempFileE(GinkgoT(), manifest.String()) - if err != nil { - return "", err - } - defer os.Remove(tmpfile) - - return k8s.RunKubectlAndGetOutputE(GinkgoT(), &kubectlOptions, "apply", "-f", tmpfile, "--dry-run="+dryRunStrategy, "--output=yaml") -} - func testWebhooks() bool { return When("Testing webhooks", func() { // temporary section; to be refactored after kubeconfig injection PR @@ -88,7 +51,7 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { }) It("Test non-existent KafkaCluster", func() { - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -100,19 +63,20 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Namespace": kubectlOptions.Namespace, }, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) - // Example error: The KafkaTopic "topic-test-internal" is invalid: spec.clusterRef.name: Invalid value: "kafkaNOT": kafkaCluster 'kafkaNOT' in the namespace 'kafka' does not exist - Expect(len(strings.Split(output, "\n"))).To(Equal(1)) - Expect(output).To( + // Example error: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal" is invalid: spec.clusterRef.name: Invalid value: "kafkaNOT": kafkaCluster 'kafkaNOT' in the namespace 'kafka' does not exist + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(1)) + Expect(err.Error()).To( ContainSubstring("spec.clusterRef.name: Invalid value: %[1]q: kafkaCluster '%[1]s' in the namespace '%[2]s' does not exist", kafkaClusterName+"NOT", kubectlOptions.Namespace), ) }) It("Test 0 partitions and replicationFactor", func() { - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -121,15 +85,15 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Partition": "0", "ReplicationFactor": "0", }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal" is invalid: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal" is invalid: // * spec.partitions: Invalid value: 0: number of partitions must be larger than 0 (or set it to be -1 to use the broker's default) // * spec.replicationFactor: Invalid value: 0: replication factor must be larger than 0 (or set it to be -1 to use the broker's default) - Expect(len(strings.Split(output, "\n"))).To(Equal(3)) - Expect(output).To(And( + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(3)) + Expect(err.Error()).To(And( ContainSubstring("spec.partitions: Invalid value: %s: number of partitions must be larger than 0 (or set it to be -1 to use the broker's default)", "0"), ContainSubstring("spec.replicationFactor: Invalid value: %s: replication factor must be larger than 0 (or set it to be -1 to use the broker's default)", "0"), )) @@ -137,7 +101,7 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { // In the current validation webhook implementation, this case can only be encountered on a Create operation It("Test ReplicationFactor larger than number of brokers", func() { - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -145,13 +109,13 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "TopicName": testInternalTopicName, "ReplicationFactor": "10", }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal" is invalid: spec.replicationFactor: Invalid value: 10: replication factor is larger than the number of nodes in the kafka cluster (available brokers: 3) - Expect(len(strings.Split(output, "\n"))).To(Equal(1)) - Expect(output).To(And( + // error while running command: exit status 1; The KafkaTopic "topic-test-internal" is invalid: spec.replicationFactor: Invalid value: 10: replication factor is larger than the number of nodes in the kafka cluster (available brokers: 3) + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(1)) + Expect(err.Error()).To(And( ContainSubstring("spec.replicationFactor: Invalid value"), ContainSubstring("replication factor is larger than the number of nodes in the kafka cluster"), )) @@ -163,7 +127,7 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { It("Testing conflict on spec.name", func() { By("With managedBy koperator annotation") - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -172,19 +136,19 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Namespace": kubectlOptions.Namespace, "Annotations": []string{"managedBy: koperator"}, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal-different-cr-name" is invalid: spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' - Expect(len(strings.Split(output, "\n"))).To(Equal(1)) - Expect(output).To( + // error while running command: exit status 1; The KafkaTopic "topic-test-internal-different-cr-name" is invalid: spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(1)) + Expect(err.Error()).To( ContainSubstring("spec.name: Invalid value: %[1]q: kafkaTopic CR '%[1]s' in namesapce '%[2]s' is already referencing to Kafka topic '%[1]s'", testInternalTopicName, kubectlOptions.Namespace), ) By("Without managedBy koperator annotation") - output, err = applyK8sResourceFromTemplateWithDryRun( + err = applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -192,17 +156,17 @@ func testWebhookCreateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "TopicName": testInternalTopicName, "Namespace": kubectlOptions.Namespace, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal-different-cr-name" is invalid: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal-different-cr-name" is invalid: // * spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' // * spec.name: Invalid value: "topic-test-internal": topic "topic-test-internal" already exists on kafka cluster and it is not managed by Koperator, // if you want it to be managed by Koperator so you can modify its configurations through a KafkaTopic CR, // add this "managedBy: koperator" annotation to this KafkaTopic CR - Expect(len(strings.Split(output, "\n"))).To(Equal(5)) - Expect(output).To(And( + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(5)) + Expect(err.Error()).To(And( ContainSubstring("spec.name: Invalid value: %[1]q: kafkaTopic CR '%[1]s' in namesapce '%[2]s' is already referencing to Kafka topic '%[1]s'", testInternalTopicName, kubectlOptions.Namespace), ContainSubstring("spec.name: Invalid value: %[1]q: topic %[1]q already exists on kafka cluster and it is not managed by Koperator", @@ -226,7 +190,7 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { requireDeployingKafkaTopic(kubectlOptions, testInternalTopicName) It("Test non-existent KafkaCluster", func() { - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -238,12 +202,13 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Namespace": kubectlOptions.Namespace, }, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) - // Example error: The KafkaTopic "topic-test-internal" is invalid: spec.clusterRef.name: Invalid value: "kafkaNOT": kafkaCluster 'kafkaNOT' in the namespace 'kafka' does not exist - Expect(len(strings.Split(output, "\n"))).To(Equal(1)) - Expect(output).To( + // Example error: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal" is invalid: spec.clusterRef.name: Invalid value: "kafkaNOT": kafkaCluster 'kafkaNOT' in the namespace 'kafka' does not exist + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(1)) + Expect(err.Error()).To( ContainSubstring("spec.clusterRef.name: Invalid value: %[1]q: kafkaCluster '%[1]s' in the namespace '%[2]s' does not exist", kafkaClusterName+"NOT", kubectlOptions.Namespace), ) @@ -255,7 +220,7 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { // * spec.replicationFactor changed (not just decreased) // Consequently, an Update test for 0 values will automatically also cover the decreasing/changing scenarios. It("Test 0 values partitions and replicationFactor", func() { - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -264,17 +229,17 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Partition": "0", "ReplicationFactor": "0", }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal" is invalid: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal" is invalid: // * spec.partitions: Invalid value: 0: number of partitions must be larger than 0 (or set it to be -1 to use the broker's default) // * spec.replicationFactor: Invalid value: 0: replication factor must be larger than 0 (or set it to be -1 to use the broker's default) // * spec.partitions: Invalid value: 0: kafka does not support decreasing partition count on an existing topic (from 2 to 0) // * spec.replicationFactor: Invalid value: 0: kafka does not support changing the replication factor on an existing topic (from 2 to 0) - Expect(len(strings.Split(output, "\n"))).To(Equal(5)) - Expect(output).To(And( + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(5)) + Expect(err.Error()).To(And( ContainSubstring("spec.partitions: Invalid value: 0: number of partitions must be larger than 0"), ContainSubstring("spec.replicationFactor: Invalid value: 0: replication factor must be larger than 0"), ContainSubstring("spec.partitions: Invalid value: %s: kafka does not support decreasing partition count on an existing topic", "0"), @@ -284,7 +249,7 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { It("Testing conflict on spec.name", func() { By("With managedBy koperator annotation") - output, err := applyK8sResourceFromTemplateWithDryRun( + err := applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -293,19 +258,19 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "Namespace": kubectlOptions.Namespace, "Annotations": []string{"managedBy: koperator"}, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal-different-cr-name" is invalid: spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' - Expect(len(strings.Split(output, "\n"))).To(Equal(1)) - Expect(output).To( + // error while running command: exit status 1; The KafkaTopic "topic-test-internal-different-cr-name" is invalid: spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(1)) + Expect(err.Error()).To( ContainSubstring("spec.name: Invalid value: %q: kafkaTopic CR '%s' in namesapce '%s' is already referencing to Kafka topic '%s'", testInternalTopicName, testInternalTopicName, kubectlOptions.Namespace, testInternalTopicName), ) By("Without managedBy koperator annotation") - output, err = applyK8sResourceFromTemplateWithDryRun( + err = applyK8sResourceFromTemplate( kubectlOptions, kafkaTopicTemplate, map[string]interface{}{ @@ -313,17 +278,17 @@ func testWebhookUpdateKafkaTopic(kubectlOptions k8s.KubectlOptions) bool { "TopicName": testInternalTopicName, // same spec.name as the KafkaTopic deployed in the beginning of the Update test scenario "Namespace": kubectlOptions.Namespace, }, - dryRunStrategyServer, + dryRunStrategyArgServer, ) Expect(err).To(HaveOccurred()) // Example error: - // The KafkaTopic "topic-test-internal-different-cr-name" is invalid: + // error while running command: exit status 1; The KafkaTopic "topic-test-internal-different-cr-name" is invalid: // * spec.name: Invalid value: "topic-test-internal": kafkaTopic CR 'topic-test-internal' in namesapce 'kafka' is already referencing to Kafka topic 'topic-test-internal' // * spec.name: Invalid value: "topic-test-internal": topic "topic-test-internal" already exists on kafka cluster and it is not managed by Koperator, // if you want it to be managed by Koperator so you can modify its configurations through a KafkaTopic CR, // add this "managedBy: koperator" annotation to this KafkaTopic CR - Expect(len(strings.Split(output, "\n"))).To(Equal(5)) - Expect(output).To(And( + Expect(len(strings.Split(err.Error(), "\n"))).To(Equal(5)) + Expect(err.Error()).To(And( ContainSubstring("spec.name: Invalid value: %[1]q: kafkaTopic CR '%[1]s' in namesapce '%[2]s' is already referencing to Kafka topic '%[1]s'", testInternalTopicName, kubectlOptions.Namespace), ContainSubstring("spec.name: Invalid value: %[1]q: topic %[1]q already exists on kafka cluster and it is not managed by Koperator",