From deddcf25959b3b44b5ec2cbc09815b950a1328dd Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 27 Jul 2023 21:09:18 -0400 Subject: [PATCH] Fixed upgrade_test.go Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 44 +---------- pkg/install/hypershift.go | 17 ----- pkg/install/upgrade.go | 53 ++++++++++++- pkg/install/upgrade_test.go | 143 ++++++++++++++++++++++++++---------- 4 files changed, 155 insertions(+), 102 deletions(-) diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go index 12803fca..0a740204 100644 --- a/pkg/install/expectedArgs.go +++ b/pkg/install/expectedArgs.go @@ -23,54 +23,12 @@ type expectedArg struct { argument string } -var ( - expected = []expectedConfig{ - { - objectName: util.HypershiftBucketSecretName, - objectType: corev1.Secret{}, - objectArgs: []expectedArg{ - {argument: "--oidc-storage-provider-s3-bucket-name={bucket}", shouldExist: true}, - {argument: "--oidc-storage-provider-s3-region={region}", shouldExist: true}, - {argument: "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials", shouldExist: true}, - }, - NoObjectArgs: []expectedArg{ - {argument: "--oidc-storage-provider-s3-bucket-name=", shouldExist: false}, - {argument: "--oidc-storage-provider-s3-region=", shouldExist: false}, - {argument: "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials", shouldExist: false}, - }, - deploymentName: util.HypershiftOperatorName, - }, - { - objectName: util.HypershiftPrivateLinkSecretName, - objectType: corev1.Secret{}, - objectArgs: []expectedArg{ - {argument: "--private-platform=AWS", shouldExist: true}, - {argument: "--private-platform=None", shouldExist: false}, - }, - NoObjectArgs: []expectedArg{ - {argument: "--private-platform=None", shouldExist: true}, - {argument: "--private-platform=AWS", shouldExist: false}, - }, - deploymentName: util.HypershiftOperatorName, - }, - { - objectName: util.HypershiftExternalDNSSecretName, - objectType: corev1.Secret{}, - objectArgs: []expectedArg{ - {argument: "--domain-filter={domain-filter}", shouldExist: true}, - {argument: "--provider={provider}", shouldExist: true}, - }, - deploymentName: util.HypershiftOperatorExternalDNSName, - }, - } -) - func (c *UpgradeController) getDeployment(operatorName string) (appsv1.Deployment, error) { deployment := &appsv1.Deployment{} nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: operatorName} err := c.spokeUncachedClient.Get(c.ctx, nsn, deployment) if err != nil { - c.log.Error(err, fmt.Sprintf("failed to get %s deployment: ", operatorName)) + c.log.Info(fmt.Sprintf("failed to get %s deployment", operatorName)) return *deployment, err } diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 637c4014..a41b1f6a 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -248,9 +248,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller return fmt.Errorf("hypershift-operator-oidc-provider-s3-credentials does not contain a bucket key") } - // if err := c.createOrUpdateAwsSpokeSecret(ctx, se, true); err != nil { - // return err - // } c.log.Info("oidc s3 bucket, region & credential arguments included") awsArgs := []string{ "--oidc-storage-provider-s3-bucket-name", bucketName, @@ -265,9 +262,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller } if privateLinkCreds { // if private link credentials is found, install hypershift with private secret options - // if err := c.createOrUpdateAwsSpokeSecret(ctx, spl, true); err != nil { - // return err - // } c.log.Info("private link region & credential arguments included") awsArgs := []string{ "--aws-private-secret", util.HypershiftPrivateLinkSecretName, @@ -282,17 +276,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller extDNSSecretKey := types.NamespacedName{Name: util.HypershiftExternalDNSSecretName, Namespace: c.clusterName} sExtDNS := &corev1.Secret{} if err := c.hubClient.Get(ctx, extDNSSecretKey, sExtDNS); err == nil { - // if awsPlatform { - // // For AWS DNS provider, users can specify either credentials or - // // aws-access-key-id and aws-secret-access-key - // if err := c.createOrUpdateAwsSpokeSecret(ctx, sExtDNS, false); err != nil { - // return err - // } - // } else { - // if err := c.createOrUpdateSpokeSecret(ctx, sExtDNS); err != nil { - // return err - // } - // } c.log.Info("external dns provider & domain-filter arguments included") awsArgs := []string{ diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index a80f644c..218794f7 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -72,7 +72,7 @@ func (c *UpgradeController) Start() { if err := c.syncHypershiftNS(); err != nil { c.log.Error(err, "failed to sync secrets in hypershift namespace with secrets in local-cluster namespace") } - c.populateExpectedArgs(&expected) + if c.startup || c.installfailed || c.installOptionsChanged() || c.upgradeImageCheck() { c.reinstallNeeded = true c.log.Info("hypershift operator re-installation is required") @@ -107,9 +107,52 @@ func (c *UpgradeController) installOptionsChanged() bool { // Create expected args based on secrets' existence and their values // Compare the expected args to the actual args // If they differ, reinstall - c.populateExpectedArgs(&expected) + expectedInstance := []expectedConfig{ + { + objectName: util.HypershiftBucketSecretName, + objectType: corev1.Secret{}, + objectArgs: []expectedArg{ + {argument: "--oidc-storage-provider-s3-bucket-name={bucket}", shouldExist: true}, + {argument: "--oidc-storage-provider-s3-region={region}", shouldExist: true}, + {argument: "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials", shouldExist: true}, + }, + NoObjectArgs: []expectedArg{ + {argument: "--oidc-storage-provider-s3-bucket-name=", shouldExist: false}, + {argument: "--oidc-storage-provider-s3-region=", shouldExist: false}, + {argument: "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials", shouldExist: false}, + }, + deploymentName: util.HypershiftOperatorName, + }, + { + objectName: util.HypershiftPrivateLinkSecretName, + objectType: corev1.Secret{}, + objectArgs: []expectedArg{ + {argument: "--private-platform=AWS", shouldExist: true}, + {argument: "--private-platform=None", shouldExist: false}, + }, + NoObjectArgs: []expectedArg{ + {argument: "--private-platform=None", shouldExist: true}, + {argument: "--private-platform=AWS", shouldExist: false}, + }, + deploymentName: util.HypershiftOperatorName, + }, + { + objectName: util.HypershiftExternalDNSSecretName, + objectType: corev1.Secret{}, + objectArgs: []expectedArg{ + {argument: "--domain-filter={domain-filter}", shouldExist: true}, + {argument: "--provider={provider}", shouldExist: true}, + }, + NoObjectArgs: []expectedArg{ + {argument: "--domain-filter={domain-filter}", shouldExist: false}, + {argument: "--provider={provider}", shouldExist: false}, + }, + deploymentName: util.HypershiftOperatorExternalDNSName, + }, + } + c.populateExpectedArgs(&expectedInstance) - for _, o := range expected { + for _, o := range expectedInstance { dep, err := c.getDeployment(o.deploymentName) if err != nil { continue @@ -119,12 +162,16 @@ func (c *UpgradeController) installOptionsChanged() bool { if err := c.hubClient.Get(context.TODO(), types.NamespacedName{Name: o.objectName, Namespace: c.clusterName}, &corev1.Secret{}); err == nil { if argMismatch(o.objectArgs, deploymentArgs) { + fmt.Println(deploymentArgs) c.log.Info(fmt.Sprintf("Mismatch between %s args and install options", o.objectName)) + fmt.Println(o.objectArgs) return true } } else { if argMismatch(o.NoObjectArgs, deploymentArgs) { + fmt.Println(deploymentArgs) c.log.Info(fmt.Sprintf("Mismatch between %s args and install options", o.objectName)) + fmt.Println(o.objectArgs) return true } } diff --git a/pkg/install/upgrade_test.go b/pkg/install/upgrade_test.go index 62be6bab..057b5e83 100644 --- a/pkg/install/upgrade_test.go +++ b/pkg/install/upgrade_test.go @@ -160,7 +160,31 @@ func TestBucketSecretChanges(t *testing.T) { "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{"--private-platform=None"}, + }}, + }, + }, + }, + } + controller.hubClient.Create(ctx, newBucketSecret) + controller.hubClient.Create(ctx, operatorDeployment) assert.Eventually(t, func() bool { theSecret := &corev1.Secret{} @@ -174,6 +198,12 @@ func TestBucketSecretChanges(t *testing.T) { }, 10*time.Second, 1*time.Second, "The bucket secret has changed. The hypershift operator needs to be re-installed") controller.Stop() + operatorDeployment.Spec.Template.Spec.Containers[0].Args = append(operatorDeployment.Spec.Template.Spec.Containers[0].Args, []string{ + "--oidc-storage-provider-s3-bucket-name=my-bucket", + "--oidc-storage-provider-s3-region=us-east-1", + "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials"}...) + controller.hubClient.Update(ctx, operatorDeployment) + controller.Start() assert.Eventually(t, func() bool { return !controller.reinstallNeeded @@ -187,8 +217,8 @@ func TestBucketSecretChanges(t *testing.T) { }, Data: map[string][]byte{ "bucket": []byte(`my-bucket`), - "region": []byte(`us-east-1`), - "credentials": []byte(`myNewCredential`), + "region": []byte(`us-east-2`), + "credentials": []byte(`myCredential`), }, } @@ -198,7 +228,7 @@ func TestBucketSecretChanges(t *testing.T) { theSecret := &corev1.Secret{} err := controller.hubClient.Get(ctx, types.NamespacedName{Namespace: controller.clusterName, Name: util.HypershiftBucketSecretName}, theSecret) if err == nil { - return string(theSecret.Data["credentials"]) == "myNewCredential" + return string(theSecret.Data["region"]) == "us-east-2" } return false }, 10*time.Second, 1*time.Second, "The bucket secret was updated successfully") @@ -223,6 +253,9 @@ func TestBucketSecretChanges(t *testing.T) { }, 10*time.Second, 1*time.Second, "The bucket secret was removed. The hypershift operator needs to be re-installed") controller.Stop() + operatorDeployment.Spec.Template.Spec.Containers[0].Args = []string{"--private-platform=None"} + controller.hubClient.Update(ctx, operatorDeployment) + controller.Start() assert.Eventually(t, func() bool { return !controller.reinstallNeeded @@ -265,6 +298,30 @@ func TestExtDnsSecretChanges(t *testing.T) { "credentials": []byte(`myCredential`), }, } + extDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + Kind: "Deployment", + APIVersion: "apps/v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "external-dns", + 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{"--private-platform=None"}, + }}, + }, + }, + }, + } + + controller.hubClient.Create(ctx, extDeployment) controller.hubClient.Create(ctx, newExtDnsSecret) assert.Eventually(t, func() bool { @@ -279,6 +336,11 @@ func TestExtDnsSecretChanges(t *testing.T) { }, 10*time.Second, 1*time.Second, "The external DNS secret has changed. The hypershift operator needs to be re-installed") controller.Stop() + extDeployment.Spec.Template.Spec.Containers[0].Args = append(extDeployment.Spec.Template.Spec.Containers[0].Args, []string{ + "--domain-filter=my.domain.filter", + "--provider=aws"}...) + controller.hubClient.Update(ctx, extDeployment) + controller.Start() assert.Eventually(t, func() bool { return !controller.reinstallNeeded @@ -291,10 +353,10 @@ func TestExtDnsSecretChanges(t *testing.T) { Namespace: controller.clusterName, }, Data: map[string][]byte{ - "domain-filter": []byte(`my.domain.filter`), + "domain-filter": []byte(`my.domain.filter2`), "provider": []byte(`aws`), "txt-owner-id": []byte(`my-txt-owner-id`), - "credentials": []byte(`myNewCredential`), + "credentials": []byte(`myCredential`), }, } @@ -304,7 +366,7 @@ func TestExtDnsSecretChanges(t *testing.T) { theSecret := &corev1.Secret{} err := controller.hubClient.Get(ctx, types.NamespacedName{Namespace: controller.clusterName, Name: util.HypershiftExternalDNSSecretName}, theSecret) if err == nil { - return string(theSecret.Data["credentials"]) == "myNewCredential" + return string(theSecret.Data["domain-filter"]) == "my.domain.filter2" } return false }, 10*time.Second, 1*time.Second, "The external DNS secret was updated successfully") @@ -329,6 +391,9 @@ func TestExtDnsSecretChanges(t *testing.T) { }, 10*time.Second, 1*time.Second, "The external DNS secret was removed. The hypershift operator needs to be re-installed") controller.Stop() + extDeployment.Spec.Template.Spec.Containers[0].Args = []string{"--private-platform=None"} + controller.hubClient.Update(ctx, extDeployment) + controller.Start() assert.Eventually(t, func() bool { return !controller.reinstallNeeded @@ -369,6 +434,32 @@ func TestPrivateLinkSecretChanges(t *testing.T) { "credentials": []byte(`my-credential-file`), }, } + + dp := &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{"--private-platform=None"}, + }}, + }, + }, + }, + } + controller.hubClient.Create(ctx, dp) + defer controller.hubClient.Delete(ctx, dp) + controller.hubClient.Create(ctx, newPrivateLinkSecret) assert.Eventually(t, func() bool { @@ -383,8 +474,10 @@ func TestPrivateLinkSecretChanges(t *testing.T) { }, 10*time.Second, 1*time.Second, "The private link secret has changed. The hypershift operator needs to be re-installed") controller.Stop() - //Add test to check successful installation + dp.Spec.Template.Spec.Containers[0].Args = []string{"--private-platform=AWS"} + controller.hubClient.Update(ctx, dp) + //Add test to check successful installation controller.Start() assert.Eventually(t, func() bool { return !controller.reinstallNeeded @@ -401,38 +494,6 @@ func TestPrivateLinkSecretChanges(t *testing.T) { controller.Stop() - changedPrivateLinkSecret := &corev1.Secret{ - ObjectMeta: v1.ObjectMeta{ - Name: util.HypershiftPrivateLinkSecretName, - Namespace: controller.clusterName, - }, - Data: map[string][]byte{ - "region": []byte(`us-west-1`), - "credentials": []byte(`my-credential-file`), - }, - } - - controller.hubClient.Update(ctx, changedPrivateLinkSecret) - - assert.Eventually(t, func() bool { - theSecret := &corev1.Secret{} - err := controller.hubClient.Get(ctx, types.NamespacedName{Namespace: controller.clusterName, Name: util.HypershiftPrivateLinkSecretName}, theSecret) - if err == nil { - return string(theSecret.Data["region"]) == "us-west-1" - } - return false - }, 10*time.Second, 1*time.Second, "The private link secret was updated successfully") - - controller.startup = false - controller.installfailed = false - controller.Start() - - assert.Eventually(t, func() bool { - return controller.reinstallNeeded - }, 10*time.Second, 1*time.Second, "The private link secret was updated. The hypershift operator needs to be re-installed") - - controller.Stop() - controller.hubClient.Delete(ctx, newPrivateLinkSecret) assert.Eventually(t, func() bool { @@ -449,6 +510,9 @@ func TestPrivateLinkSecretChanges(t *testing.T) { controller.Stop() + dp.Spec.Template.Spec.Containers[0].Args = []string{"--private-platform=None"} + controller.hubClient.Update(ctx, dp) + controller.startup = false controller.installfailed = false controller.Start() @@ -544,6 +608,7 @@ func TestInstallFlagChanges(t *testing.T) { Name: "nginx", Image: "nginx:1.14.2", Ports: []corev1.ContainerPort{{ContainerPort: 80}}, + Args: []string{"--private-platform=None"}, }}, }, },