Merge pull request #233 from jkyros/make-loglevel-test-work

Fix functionality (audit, loglevel) tests to actually wait for changes

Author: joelsmith

controllers/keda/kedacontroller_controller_test.go

@@ -69,7 +69,7 @@ var _ = Describe("Deploying KedaController manifest", func() {
 		})
 
 		AfterEach(func() {
-			manifest, err = changeAttribute(manifest, "namespace", namespace, scheme)
+			manifest, err = changeAttribute(manifest, "namespace", namespace, scheme, "")
 			Expect(err).To(BeNil())
 
 			Expect(manifest.Delete()).Should(Succeed())
@@ -97,7 +97,7 @@ var _ = Describe("Deploying KedaController manifest", func() {
 
 			It("Should not deploy KedaController", func() {
 
-				manifest, err = changeAttribute(manifest, "namespace", changedNamespace, scheme)
+				manifest, err = changeAttribute(manifest, "namespace", changedNamespace, scheme, "")
 				Expect(err).To(BeNil())
 
 				Expect(manifest.Apply()).Should(Succeed())
@@ -137,10 +137,28 @@ var _ = Describe("Testing functionality", func() {
 		)
 
 		BeforeEach(func() {
+			By("Applying a manifest that defaults operator logLevel back to info")
 			scheme = k8sManager.GetScheme()
 			manifest, err = createManifest(kedaManifestFilepath, k8sClient)
 			Expect(err).To(BeNil())
+			manifest, err = changeAttribute(manifest, "logLevel", "info", scheme, "default")
+			Expect(err).To(BeNil())
 			Expect(manifest.Apply()).Should(Succeed())
+
+			By("Waiting for the operator deployment to reflect the changes")
+			Eventually(func() error {
+				return deploymentHasRolledOut(deploymentName, namespace, "default")
+			}, timeout, interval).Should(Succeed())
+
+			By("Checking to make sure loglevel is set back to info")
+			u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
+			Expect(err).To(BeNil())
+			err = scheme.Convert(u, dep, nil)
+			Expect(err).To(BeNil())
+
+			arg, err = getDepArg(dep, logLevelPrefix, containerName)
+			Expect(err).To(BeNil())
+			Expect(arg).To(Equal("info"))
 		})
 
 		Context("When changing \"--zap-log-level\"", func() {
@@ -160,6 +178,8 @@ var _ = Describe("Testing functionality", func() {
 					initialLogLevel: "error",
 					actualLogLevel:  "error",
 				},
+				// the default in the sample kedacontroller manifest is "info", so "info" in this list can also mean "make sure it doesn't change",
+				// it is supposed to ignore these two cases because the values are invalid.
 				{
 					initialLogLevel: "",
 					actualLogLevel:  "info",
@@ -171,27 +191,33 @@ var _ = Describe("Testing functionality", func() {
 			}
 
 			for _, variant := range variants {
-				It(fmt.Sprintf("Should change it, initialLoglevel='%s', actualLoglevel='%s'",
-					variant.initialLogLevel, variant.actualLogLevel), func() {
-
-					manifest, err = changeAttribute(manifest, "logLevel", variant.initialLogLevel, scheme)
-					_ = manifest.Apply()
-
-					Eventually(func() error {
-						_, err = getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
-						return err
-					}, timeout, interval).Should(Succeed())
-
-					u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
-					Expect(err).To(BeNil())
-					err = scheme.Convert(u, dep, nil)
-					Expect(err).To(BeNil())
-
-					arg, err = getDepArg(dep, logLevelPrefix, containerName)
-					Expect(err).To(BeNil())
-					Expect(arg).To(Equal(variant.actualLogLevel))
-				})
+				caseName := fmt.Sprintf("Should change it, initialLoglevel='%s', actualLoglevel='%s'", variant.initialLogLevel, variant.actualLogLevel)
+				It(caseName,
+					func() {
+						By(fmt.Sprintf("Setting operator loglevel to %s in kedaController manifest", variant.initialLogLevel))
+						manifest, err = changeAttribute(manifest, "logLevel", variant.initialLogLevel, scheme, caseName)
+						Expect(err).To(BeNil())
+						err = manifest.Apply()
+						Expect(err).To(BeNil())
+
+						By("Waiting for the operator deployment to reflect the changes")
+						Eventually(func() error {
+							return deploymentHasRolledOut(deploymentName, namespace, caseName)
+						}, timeout, interval).Should(Succeed())
+
+						By("Checking to make sure the log level is " + variant.actualLogLevel)
+						u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
+						Expect(err).To(BeNil())
+						err = scheme.Convert(u, dep, nil)
+						Expect(err).To(BeNil())
+
+						arg, err = getDepArg(dep, logLevelPrefix, containerName)
+						Expect(err).To(BeNil())
+						Expect(arg).To(Equal(variant.actualLogLevel))
+
+					})
 			}
+
 		})
 	})
 
@@ -219,10 +245,28 @@ var _ = Describe("Testing functionality", func() {
 		)
 
 		BeforeEach(func() {
+			By("Applying a manifest that defaults operator logLevel back to info")
 			scheme = k8sManager.GetScheme()
 			manifest, err = createManifest(kedaManifestFilepath, k8sClient)
 			Expect(err).To(BeNil())
+			manifest, err = changeAttribute(manifest, "logLevel", "info", scheme, "default")
+			Expect(err).To(BeNil())
 			Expect(manifest.Apply()).Should(Succeed())
+
+			By("Waiting for the operator deployment to reflect the changes")
+			Eventually(func() error {
+				return deploymentHasRolledOut(deploymentName, namespace, "default")
+			}, timeout, interval).Should(Succeed())
+
+			By("Checking to make sure loglevel is set back to info")
+			u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
+			Expect(err).To(BeNil())
+			err = scheme.Convert(u, dep, nil)
+			Expect(err).To(BeNil())
+
+			arg, err = getDepArg(dep, logLevelPrefix, containerName)
+			Expect(err).To(BeNil())
+			Expect(arg).To(Equal("info"))
 		})
 
 		Context("When changing \"--zap-log-level\"", func() {
@@ -242,6 +286,8 @@ var _ = Describe("Testing functionality", func() {
 					initialLogLevel: "error",
 					actualLogLevel:  "error",
 				},
+				// the default in the sample kedacontroller manifest is "info", so "info" in this list can also mean "make sure it doesn't change",
+				// it is supposed to ignore these two cases because the values are invalid.
 				{
 					initialLogLevel: "",
 					actualLogLevel:  "info",
@@ -253,17 +299,21 @@ var _ = Describe("Testing functionality", func() {
 			}
 
 			for _, variant := range variants {
-				It(fmt.Sprintf("Should change it, initialLoglevel='%s', actualLoglevel='%s'",
-					variant.initialLogLevel, variant.actualLogLevel), func() {
+				caseName := fmt.Sprintf("Should change it, initialLoglevel='%s', actualLoglevel='%s'", variant.initialLogLevel, variant.actualLogLevel)
 
-					manifest, err = changeAttribute(manifest, "logLevel", variant.initialLogLevel, scheme)
-					_ = manifest.Apply()
+				It(caseName, func() {
+					By(fmt.Sprintf("Setting admission loglevel to %s in kedaController manifest", variant.initialLogLevel))
+					manifest, err = changeAttribute(manifest, "logLevel-admission", variant.initialLogLevel, scheme, caseName)
+					Expect(err).To(BeNil())
+					err = manifest.Apply()
+					Expect(err).To(BeNil())
 
+					By("Waiting for the admission deployment to reflect the changes")
 					Eventually(func() error {
-						_, err = getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
-						return err
+						return deploymentHasRolledOut(deploymentName, namespace, caseName)
 					}, timeout, interval).Should(Succeed())
 
+					By("Checking to make sure the log level is " + variant.actualLogLevel)
 					u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
 					Expect(err).To(BeNil())
 					err = scheme.Convert(u, dep, nil)
@@ -298,6 +348,10 @@ var _ = Describe("Testing audit flags", func() {
 	)
 
 	When("Manipulating parameters", func() {
+
+		// TODO(jkyros): come back to refactor this test, it doesn't proeprly reset between test cases, and
+		// if it did, it would be failing, because it's currently impossible to deconfigure audit because of
+		// how our manifestival code only operates if the settings are different from default/empty.
 		BeforeEach(func() {
 			scheme = k8sManager.GetScheme()
 			dep = &appsv1.Deployment{}
@@ -317,7 +371,7 @@ var _ = Describe("Testing audit flags", func() {
 					value:    "json",
 				},
 				{
-					argument: "auditMaxAge=",
+					argument: "auditMaxAge",
 					prefix:   "--audit-log-maxage=",
 					value:    "1",
 				},
@@ -333,13 +387,15 @@ var _ = Describe("Testing audit flags", func() {
 				},
 			}
 			for _, variant := range vars {
+				caseName := fmt.Sprintf("adds '%s' with value '%s'", variant.argument, variant.value)
+				It(caseName, func() {
+					manifest, err := changeAttribute(manifest, variant.argument, variant.value, scheme, caseName)
+					Expect(err).To(BeNil())
 
-				It(fmt.Sprintf("adds '%s' with value '%s'", variant.argument, variant.value), func() {
-					manifest, err := changeAttribute(manifest, variant.argument, variant.value, scheme)
 					Expect(manifest.Apply()).To(Succeed())
 					Eventually(func() error {
-						_, err = getObject(ctx, "Deployment", metricsServerName, namespace, k8sClient)
-						return err
+						return deploymentHasRolledOut(metricsServerName, namespace, caseName)
+
 					}, timeout, interval).Should(Succeed())
 					u, err := getObject(ctx, "Deployment", metricsServerName, namespace, k8sClient)
 					Expect(err).To(BeNil())
@@ -369,18 +425,41 @@ func getDepArg(dep *appsv1.Deployment, prefix string, containerName string) (str
 	return "", errors.New("Could not find a container: " + containerName)
 }
 
-func changeAttribute(manifest mf.Manifest, attr string, value string, scheme *runtime.Scheme) (mf.Manifest, error) {
+func changeAttribute(manifest mf.Manifest, attr string, value string, scheme *runtime.Scheme, annotation string) (mf.Manifest, error) {
 	transformer := func(u *unstructured.Unstructured) error {
 		kedaControllerInstance := &kedav1alpha1.KedaController{}
 		if err := scheme.Convert(u, kedaControllerInstance, nil); err != nil {
 			return err
 		}
 
+		// Annotations might be nil, so we need to make sure we account for that
+		if kedaControllerInstance.Spec.Operator.DeploymentAnnotations == nil {
+			kedaControllerInstance.Spec.Operator.DeploymentAnnotations = make(map[string]string)
+		}
+
+		if kedaControllerInstance.Spec.AdmissionWebhooks.DeploymentAnnotations == nil {
+			kedaControllerInstance.Spec.AdmissionWebhooks.DeploymentAnnotations = make(map[string]string)
+		}
+		if kedaControllerInstance.Spec.MetricsServer.DeploymentAnnotations == nil {
+			kedaControllerInstance.Spec.MetricsServer.DeploymentAnnotations = make(map[string]string)
+		}
+		// When we push through an attribute change, we also set an annotation matching the test case
+		// so we can be sure the controller reacted to our kedaControllerInstance updates and we have "our"
+		// changes, not just some changes that might be from a previous test case
+		kedaControllerInstance.Spec.Operator.DeploymentAnnotations["testCase"] = annotation
+		kedaControllerInstance.Spec.AdmissionWebhooks.DeploymentAnnotations["testCase"] = annotation
+		kedaControllerInstance.Spec.MetricsServer.DeploymentAnnotations["testCase"] = annotation
+
 		switch attr {
 		case "namespace":
 			kedaControllerInstance.Namespace = value
 		case "logLevel":
 			kedaControllerInstance.Spec.Operator.LogLevel = value
+		// TODO(jkyros): this breaks pattern with the rest of these cases but multiple operands have
+		// the same field, but we kind of bolted the admission tests on here without doing a refactor and
+		// this makes it work for now
+		case "logLevel-admission":
+			kedaControllerInstance.Spec.AdmissionWebhooks.LogLevel = value
 		// metricsServer audit arguments
 		case "auditLogFormat":
 			kedaControllerInstance.Spec.MetricsServer.AuditConfig.LogFormat = value
@@ -398,3 +477,22 @@ func changeAttribute(manifest mf.Manifest, attr string, value string, scheme *ru
 
 	return manifest.Transform(transformer)
 }
+
+// deploymentHasRolledOut waits for the specified deployment to possess the specified annotation
+//
+//nolint:unparam
+func deploymentHasRolledOut(deploymentName string, namespace string, deploymentAnnotation string) error {
+	u, err := getObject(ctx, "Deployment", deploymentName, namespace, k8sClient)
+	if err != nil {
+		return err
+	}
+	// The default manifest has no annotation, so I'm not checking whether it's present, only the value,
+	// because the default case will not have the annotation, and we want that to be okay in the case where we
+	// apply the default.
+	testcase := u.GetAnnotations()["testCase"]
+	if deploymentAnnotation == testcase {
+		By("Observing that the test case annotation is now: " + testcase)
+		return nil
+	}
+	return fmt.Errorf("Deployment has not rolled out, annotation is still '%s'", testcase)
+}