From c46947c79219a301c0eadbdbd4bcfb667a7d5180 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Wed, 7 Jun 2023 14:00:34 -0400 Subject: [PATCH 1/6] add space, test PR Signed-off-by: Omar Farag --- test/e2e/install_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/e2e/install_test.go b/test/e2e/install_test.go index d263e0b3..ceead30f 100644 --- a/test/e2e/install_test.go +++ b/test/e2e/install_test.go @@ -179,7 +179,7 @@ var _ = ginkgo.Describe("Install", func() { for _, p := range podList.Items { if strings.HasPrefix(p.Name, "hypershift-addon-agent") { addonAgentPod = &p - ginkgo.By("Found addon agent pod" + p.Name) + ginkgo.By("Found addon agent pod " + p.Name) break } From 11ef1c65fee6dbd042982f0a85d9539326993edf Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 22 Jun 2023 13:14:00 -0400 Subject: [PATCH 2/6] Reinstall operator when deployment args change Signed-off-by: Omar Farag --- pkg/install/hypershift.go | 5 +++++ pkg/install/upgrade.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 3ea66a5d..5768e0c7 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -173,6 +173,11 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller return nil } + // Cache the operator deployment to check for change in args + if (operatorDeployment != nil) { + c.operatorDeployment = *operatorDeployment + } + // Initially set this to zero to indicate that the AWS S3 bucket secret is not used for the operator installation metrics.IsAWSS3BucketSecretConfigured.Set(0) diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 540004e7..a4a11edd 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -8,9 +8,11 @@ import ( "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" @@ -36,6 +38,7 @@ type UpgradeController struct { privateLinkSecret corev1.Secret imageOverrideConfigmap corev1.ConfigMap installFlagsConfigmap corev1.ConfigMap + operatorDeployment appsv1.Deployment reinstallNeeded bool // this is used only for code test awsPlatform bool // this is used only for code test startup bool // @@ -124,6 +127,12 @@ func (c *UpgradeController) installOptionsChanged() bool { return true } + // check for changes in hypershift operator deployment arguments + newOperatorDeployment, err := c.getDeployment() + if err == nil && c.operatorDeployment.ObjectMeta.Name != "" && c.deploymentArgsChanged(c.operatorDeployment, *newOperatorDeployment) { + c.operatorDeployment = *newOperatorDeployment + return true + } return false } @@ -175,3 +184,28 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap, } return false } + +func (c *UpgradeController) deploymentArgsChanged(oldDeployment, newDeployment appsv1.Deployment) bool { + // If args changed, we want to re-install the operator with new arguments + oldArgs := oldDeployment.Spec.Template.Spec.Containers[0].Args + newArgs := newDeployment.Spec.Template.Spec.Containers[0].Args + if !reflect.DeepEqual(oldArgs, newArgs) { + c.log.Info(fmt.Sprintf("the arguments of the deployment(%s) have changed", oldDeployment.Name)) + return true + } + return false +} + +func (c *UpgradeController) getDeployment() (*appsv1.Deployment, error) { + deployment := &appsv1.Deployment{} + nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: util.HypershiftOperatorName} + err := c.hubClient.Get(c.ctx, nsn, deployment) + if err != nil { + if !apierrors.IsNotFound(err) { + return nil, err + } + return nil, nil + } + + return deployment, nil +} From b2255f7b4ed82195de9c237bb454d573c5510489 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 22 Jun 2023 14:19:45 -0400 Subject: [PATCH 3/6] Change client, clean up Signed-off-by: Omar Farag --- pkg/install/hypershift.go | 2 +- pkg/install/upgrade.go | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 5768e0c7..757d406d 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -174,7 +174,7 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller } // Cache the operator deployment to check for change in args - if (operatorDeployment != nil) { + if operatorDeployment != nil { c.operatorDeployment = *operatorDeployment } diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index a4a11edd..976edcd7 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -8,7 +8,6 @@ import ( "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/util/wait" "sigs.k8s.io/controller-runtime/pkg/client" @@ -190,7 +189,7 @@ func (c *UpgradeController) deploymentArgsChanged(oldDeployment, newDeployment a oldArgs := oldDeployment.Spec.Template.Spec.Containers[0].Args newArgs := newDeployment.Spec.Template.Spec.Containers[0].Args if !reflect.DeepEqual(oldArgs, newArgs) { - c.log.Info(fmt.Sprintf("the arguments of the deployment(%s) have changed", oldDeployment.Name)) + c.log.Info(fmt.Sprintf("the arguments of the %s deployment have changed", oldDeployment.Name)) return true } return false @@ -199,9 +198,9 @@ func (c *UpgradeController) deploymentArgsChanged(oldDeployment, newDeployment a func (c *UpgradeController) getDeployment() (*appsv1.Deployment, error) { deployment := &appsv1.Deployment{} nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: util.HypershiftOperatorName} - err := c.hubClient.Get(c.ctx, nsn, deployment) + err := c.spokeUncachedClient.Get(c.ctx, nsn, deployment) if err != nil { - if !apierrors.IsNotFound(err) { + if !errors.IsNotFound(err) { return nil, err } return nil, nil From 8224158ec356a1563f45d554fff04db9d6ff5b8c Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Fri, 23 Jun 2023 15:08:06 -0400 Subject: [PATCH 4/6] reinstall operator if arg mismatch with secret Signed-off-by: Omar Farag --- pkg/install/hypershift.go | 5 ---- pkg/install/upgrade.go | 50 ++++++++++++++++++++++++--------------- 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 757d406d..3ea66a5d 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -173,11 +173,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller return nil } - // Cache the operator deployment to check for change in args - if operatorDeployment != nil { - c.operatorDeployment = *operatorDeployment - } - // Initially set this to zero to indicate that the AWS S3 bucket secret is not used for the operator installation metrics.IsAWSS3BucketSecretConfigured.Set(0) diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 976edcd7..013a94bf 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "strings" "time" "github.com/go-logr/logr" @@ -37,7 +38,6 @@ type UpgradeController struct { privateLinkSecret corev1.Secret imageOverrideConfigmap corev1.ConfigMap installFlagsConfigmap corev1.ConfigMap - operatorDeployment appsv1.Deployment reinstallNeeded bool // this is used only for code test awsPlatform bool // this is used only for code test startup bool // @@ -127,9 +127,8 @@ func (c *UpgradeController) installOptionsChanged() bool { } // check for changes in hypershift operator deployment arguments - newOperatorDeployment, err := c.getDeployment() - if err == nil && c.operatorDeployment.ObjectMeta.Name != "" && c.deploymentArgsChanged(c.operatorDeployment, *newOperatorDeployment) { - c.operatorDeployment = *newOperatorDeployment + operatorDeployment, err := c.getDeployment() + if err == nil && c.operatorArgMismatch(*operatorDeployment) { return true } return false @@ -184,27 +183,40 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap, return false } -func (c *UpgradeController) deploymentArgsChanged(oldDeployment, newDeployment appsv1.Deployment) bool { - // If args changed, we want to re-install the operator with new arguments - oldArgs := oldDeployment.Spec.Template.Spec.Containers[0].Args - newArgs := newDeployment.Spec.Template.Spec.Containers[0].Args - if !reflect.DeepEqual(oldArgs, newArgs) { - c.log.Info(fmt.Sprintf("the arguments of the %s deployment have changed", oldDeployment.Name)) - return true - } - return false -} - func (c *UpgradeController) getDeployment() (*appsv1.Deployment, error) { deployment := &appsv1.Deployment{} nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: util.HypershiftOperatorName} err := c.spokeUncachedClient.Get(c.ctx, nsn, deployment) if err != nil { - if !errors.IsNotFound(err) { - return nil, err - } - return nil, nil + return nil, err } return deployment, nil } + +func (c *UpgradeController) operatorArgMismatch(dep appsv1.Deployment) bool { + //If secret exists but operator does not have secret args, add secret args to deployment + + //Attempt to retrieve oidc bucket secret + oidcExists := false + bucketSecretKey := types.NamespacedName{Name: util.HypershiftBucketSecretName, Namespace: c.clusterName} + se := &corev1.Secret{} + if err := c.hubClient.Get(c.ctx, bucketSecretKey, se); err == nil { + oidcExists = true + } else { + return false + } + + args := dep.Spec.Template.Spec.Containers[0].Args + oidcArgExists := false + for arg := range args { + if strings.Contains(args[arg], "--oidc-storage-provider-s3-bucket-name") { + oidcArgExists = true + break + } + } + if oidcExists != oidcArgExists { + c.log.Info("hypershift operator has mismatch with oidc secrets, reinstalling operator") + } + return oidcExists != oidcArgExists +} From dee883bbbebba65136f3de1311dcfbd6bb331cdf Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Fri, 23 Jun 2023 17:08:19 -0400 Subject: [PATCH 5/6] added tests Signed-off-by: Omar Farag --- pkg/install/upgrade.go | 9 ++-- pkg/install/upgrade_test.go | 82 +++++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 013a94bf..fdca5f51 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -128,7 +128,7 @@ func (c *UpgradeController) installOptionsChanged() bool { // check for changes in hypershift operator deployment arguments operatorDeployment, err := c.getDeployment() - if err == nil && c.operatorArgMismatch(*operatorDeployment) { + if err == nil && c.operatorArgMismatch(operatorDeployment) { return true } return false @@ -183,15 +183,16 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap, return false } -func (c *UpgradeController) getDeployment() (*appsv1.Deployment, error) { +func (c *UpgradeController) getDeployment() (appsv1.Deployment, error) { deployment := &appsv1.Deployment{} nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: util.HypershiftOperatorName} err := c.spokeUncachedClient.Get(c.ctx, nsn, deployment) if err != nil { - return nil, err + c.log.Error(err, "failed to get operater deployment: ") + return *deployment, err } - return deployment, nil + return *deployment, nil } func (c *UpgradeController) operatorArgMismatch(dep appsv1.Deployment) bool { diff --git a/pkg/install/upgrade_test.go b/pkg/install/upgrade_test.go index 066f8527..f0094bc9 100644 --- a/pkg/install/upgrade_test.go +++ b/pkg/install/upgrade_test.go @@ -2,6 +2,7 @@ package install import ( "context" + "fmt" "testing" "time" @@ -614,3 +615,84 @@ func TestInstallFlagChanges(t *testing.T) { controller.Stop() } + +func TestDeploymentArgMismatch(t *testing.T) { + ctx := context.Background() + + zapLog, _ := zap.NewDevelopment() + client := initClient() + controller := &UpgradeController{ + spokeUncachedClient: client, + hubClient: client, + log: zapr.NewLogger(zapLog), + addonNamespace: "addon", + operatorImage: "my-test-image", + clusterName: "cluster1", + pullSecret: "pull-secret", + hypershiftInstallExecutor: &HypershiftTestCliExecutor{}, + bucketSecret: corev1.Secret{}, + } + + localOidcSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: util.HypershiftBucketSecretName, + Namespace: controller.clusterName, + }, + Data: map[string][]byte{ + "bucket": []byte(`my-bucket`), + "region": []byte(`us-east-1`), + "credentials": []byte(`myCredential`), + }, + } + operatorDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "operator", + Namespace: "hypershift", + }, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: "nginx", + Image: "nginx:1.14.2", + Ports: []corev1.ContainerPort{{ContainerPort: 80}}, + Args: []string{"sample-arg-not-oidc"}, + }}, + }, + }, + }, + } + + //Create deployment + controller.hubClient.Create(ctx, operatorDeployment) + defer controller.hubClient.Delete(ctx, operatorDeployment) + + // No oidc secret in deployment args nor does it exist - no mismatch - should not need reinstall + controller.Start() + assert.Eventually(t, func() bool { + fmt.Println(controller.reinstallNeeded) + return !controller.reinstallNeeded + }, 10*time.Second, 1*time.Second, "The oidc secret doesn't differ from operator args. The hypershift operator doesn't need to be re-installed") + controller.Stop() + + // create oidc secret + controller.hubClient.Create(ctx, localOidcSecret) + assert.Eventually(t, func() bool { + theSecret := &corev1.Secret{} + err := controller.hubClient.Get(ctx, types.NamespacedName{Namespace: controller.clusterName, Name: util.HypershiftBucketSecretName}, theSecret) + return err == nil + }, 10*time.Second, 1*time.Second, "The test oidc secret was created successfully") + + // oidc secret exists but not present in deployment args, should reinstall + controller.Start() + assert.Eventually(t, func() bool { + fmt.Println(controller.reinstallNeeded) + return controller.reinstallNeeded + }, 10*time.Second, 1*time.Second, "The oidc secret differs from operator args. The hypershift operator needs to be re-installed") + controller.Stop() + +} From ce8bc890e78dfb0152ce5c802e2daca58fb380c4 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Mon, 26 Jun 2023 13:58:14 -0400 Subject: [PATCH 6/6] fix tests Signed-off-by: Omar Farag --- pkg/install/upgrade_test.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/install/upgrade_test.go b/pkg/install/upgrade_test.go index f0094bc9..54b2915c 100644 --- a/pkg/install/upgrade_test.go +++ b/pkg/install/upgrade_test.go @@ -660,7 +660,6 @@ func TestDeploymentArgMismatch(t *testing.T) { Name: "nginx", Image: "nginx:1.14.2", Ports: []corev1.ContainerPort{{ContainerPort: 80}}, - Args: []string{"sample-arg-not-oidc"}, }}, }, }, @@ -671,13 +670,7 @@ func TestDeploymentArgMismatch(t *testing.T) { controller.hubClient.Create(ctx, operatorDeployment) defer controller.hubClient.Delete(ctx, operatorDeployment) - // No oidc secret in deployment args nor does it exist - no mismatch - should not need reinstall - controller.Start() - assert.Eventually(t, func() bool { - fmt.Println(controller.reinstallNeeded) - return !controller.reinstallNeeded - }, 10*time.Second, 1*time.Second, "The oidc secret doesn't differ from operator args. The hypershift operator doesn't need to be re-installed") - controller.Stop() + defer deleteAllInstallJobs(ctx, controller.spokeUncachedClient, controller.addonNamespace) // create oidc secret controller.hubClient.Create(ctx, localOidcSecret) @@ -687,12 +680,23 @@ func TestDeploymentArgMismatch(t *testing.T) { return err == nil }, 10*time.Second, 1*time.Second, "The test oidc secret was created successfully") + // Installing first time, reinstall needed + controller.Start() + assert.Eventually(t, func() bool { + return controller.reinstallNeeded + }, 10*time.Second, 1*time.Second, "First time install, \"reinstall\" needed") + controller.Stop() + + //Remove args from deployment + operatorDeployment.Spec.Template.Spec.Containers[0].Args = []string{"sample-arg-no-oidc"} + if err := controller.hubClient.Update(ctx, operatorDeployment); err != nil { + fmt.Println("could not update deployment") + } // oidc secret exists but not present in deployment args, should reinstall controller.Start() assert.Eventually(t, func() bool { - fmt.Println(controller.reinstallNeeded) return controller.reinstallNeeded - }, 10*time.Second, 1*time.Second, "The oidc secret differs from operator args. The hypershift operator needs to be re-installed") + }, 10*time.Second, 1*time.Second, "The oidc secret exists but not among operator args. The hypershift operator needs to be re-installed") controller.Stop() }