From c926be89aabd36396554c1678b024a1e5c193427 Mon Sep 17 00:00:00 2001 From: John Kyros Date: Fri, 21 Jun 2024 19:14:28 -0500 Subject: [PATCH] Make functionality tests wait for changes Previously the functionality tests that tested the log level changes were not waiting for rollouts to happen, so they were effectively not testing that the logLevel and audit flags were properly set on the deployment. Recent releases have changed that timing so these tests have started to fail. This changes the test such that they now wait for controller to process each of the changes (or not process each of the changes, depending on the test case). It does this by adding an additional deployment annotation for each test case, so the test case knows that the controller has processed the test case and it can check the values. This probably needs a more complete refactor at some point, but this at least makes the test dependable. The audit config tests don't work perfectly either, but that is harder to solve because the functionality they are testing is currently kind of broken, i.e. you can't currently deconfigure audit, so you can't ever reset the test to a "clean" state to try the next test case. For now I made them work by having them sequentially add the args without resetting. Signed-off-by: John Kyros --- .../keda/kedacontroller_controller_test.go | 166 ++++++++++++++---- 1 file changed, 132 insertions(+), 34 deletions(-) diff --git a/controllers/keda/kedacontroller_controller_test.go b/controllers/keda/kedacontroller_controller_test.go index 6f1b7a5f5..715f30be4 100644 --- a/controllers/keda/kedacontroller_controller_test.go +++ b/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) +}