From c46947c79219a301c0eadbdbd4bcfb667a7d5180 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Wed, 7 Jun 2023 14:00:34 -0400 Subject: [PATCH 1/9] 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 bb26559092936f08fba68f26ff05f9e58b421fb2 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Wed, 19 Jul 2023 17:28:43 -0400 Subject: [PATCH 2/9] Sync secrets between local-cluster and hypershift ns Signed-off-by: Omar Farag --- pkg/install/hypershift.go | 14 +++++----- pkg/install/hypershift_test.go | 6 ++-- pkg/install/upgrade.go | 50 ++++++++++++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 10 deletions(-) diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 3ea66a5d..eae8a879 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -253,7 +253,7 @@ 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.createAwsSpokeSecret(ctx, se, true); err != nil { + if err := c.createOrUpdateAwsSpokeSecret(ctx, se, true); err != nil { return err } c.log.Info("oidc s3 bucket, region & credential arguments included") @@ -270,7 +270,7 @@ 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.createAwsSpokeSecret(ctx, spl, true); err != nil { + if err := c.createOrUpdateAwsSpokeSecret(ctx, spl, true); err != nil { return err } c.log.Info("private link region & credential arguments included") @@ -290,11 +290,11 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller if awsPlatform { // For AWS DNS provider, users can specify either credentials or // aws-access-key-id and aws-secret-access-key - if err := c.createAwsSpokeSecret(ctx, sExtDNS, false); err != nil { + if err := c.createOrUpdateAwsSpokeSecret(ctx, sExtDNS, false); err != nil { return err } } else { - if err := c.createSpokeSecret(ctx, sExtDNS); err != nil { + if err := c.createOrUpdateSpokeSecret(ctx, sExtDNS); err != nil { return err } } @@ -511,7 +511,7 @@ func getParamValue(s []string, e string) string { return "" } -func (c *UpgradeController) createAwsSpokeSecret(ctx context.Context, hubSecret *corev1.Secret, regionRequired bool) error { +func (c *UpgradeController) createOrUpdateAwsSpokeSecret(ctx context.Context, hubSecret *corev1.Secret, regionRequired bool) error { spokeSecret := hubSecret.DeepCopy() region := hubSecret.Data["region"] @@ -525,10 +525,10 @@ func (c *UpgradeController) createAwsSpokeSecret(ctx context.Context, hubSecret } } - return c.createSpokeSecret(ctx, spokeSecret) + return c.createOrUpdateSpokeSecret(ctx, spokeSecret) } -func (c *UpgradeController) createSpokeSecret(ctx context.Context, hubSecret *corev1.Secret) error { +func (c *UpgradeController) createOrUpdateSpokeSecret(ctx context.Context, hubSecret *corev1.Secret) error { spokeSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/install/hypershift_test.go b/pkg/install/hypershift_test.go index d6b13d51..21cb5a11 100644 --- a/pkg/install/hypershift_test.go +++ b/pkg/install/hypershift_test.go @@ -1418,7 +1418,7 @@ func TestCreateSpokeCredential(t *testing.T) { }, } - err := aCtrl.createAwsSpokeSecret(ctx, bucketSecret, true) + err := aCtrl.createOrUpdateAwsSpokeSecret(ctx, bucketSecret, true) assert.NotNil(t, err, "is not nil, when secret is not well formed") } @@ -1449,7 +1449,7 @@ func TestSpokeCredentialUpdated(t *testing.T) { }, } - err := aCtrl.createSpokeSecret(ctx, hubSecret) + err := aCtrl.createOrUpdateSpokeSecret(ctx, hubSecret) assert.Nil(t, err, "expected secret creation to succeed") spokeSecret := &corev1.Secret{ @@ -1465,7 +1465,7 @@ func TestSpokeCredentialUpdated(t *testing.T) { // now update the hub secret and propagate that change to spoke hubSecret.Data["credentials"] = []byte("February") - err = aCtrl.createSpokeSecret(ctx, hubSecret) + err = aCtrl.createOrUpdateSpokeSecret(ctx, hubSecret) assert.Nil(t, err, "expected updating secret to succeed") aCtrl.spokeUncachedClient.Get(ctx, ctrlClient.ObjectKeyFromObject(spokeSecret), spokeSecret) diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 540004e7..ef5d44b8 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -72,6 +72,9 @@ func (c *UpgradeController) Start() { c.log.Info(fmt.Sprintf("check if HyperShift operator re-installation is required (startup=%v, installfailed=%v)", c.startup, c.installfailed)) c.reinstallNeeded = false + if err := c.syncHypershiftNS(); err != nil { + c.log.Error(err, "failed to sync secrets in hypershift namespace with secrets in local-cluster namespace") + } if c.startup || c.installfailed || c.installOptionsChanged() || c.upgradeImageCheck() { c.reinstallNeeded = true c.log.Info("hypershift operator re-installation is required") @@ -175,3 +178,50 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap, } return false } + +func (c *UpgradeController) syncHypershiftNS() error { + //Sync secrets in local-cluster namespace with secrets in hypershift namespace + secrets := []string{"hypershift-operator-oidc-provider-s3-credentials", "hypershift-operator-private-link-credentials", "hypershift-operator-external-dns-credentials"} + awsPlatform := false + ctx := context.TODO() + + for s := range secrets { + if secrets[s] == "hypershift-operator-external-dns-credentials" { + + 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 + } + } + } else { + c.log.Info(fmt.Sprintf("external dns secret(%s) was not found", extDNSSecretKey)) + } + + } else { + + secretKey := types.NamespacedName{Name: secrets[s], Namespace: c.clusterName} + se := &corev1.Secret{} + if err := c.hubClient.Get(ctx, secretKey, se); err == nil { + awsPlatform = true + if err := c.createOrUpdateAwsSpokeSecret(ctx, se, true); err != nil { + return err + } + } else { + c.log.Info(fmt.Sprintf("secret(%s) not found on the hub.", secretKey)) + + } + + } + + } + return nil +} From 370e899dd0012944fef216a386bfdc8c0e31d939 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 20 Jul 2023 17:09:03 -0400 Subject: [PATCH 3/9] populate expected args Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 92 +++++++++++++++++++++++++++++++++++++ pkg/install/upgrade.go | 54 +++++++++++++++++++++- 2 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 pkg/install/expectedArgs.go diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go new file mode 100644 index 00000000..ee855b99 --- /dev/null +++ b/pkg/install/expectedArgs.go @@ -0,0 +1,92 @@ +package install + +import ( + "fmt" + + "github.com/stolostron/hypershift-addon-operator/pkg/util" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" +) + +type expectedConfig struct { + objectName string + deploymentName string + objectArgs []expectedArg + NoObjectArgs []expectedArg + objectType interface{} +} + +type expectedArg struct { + shouldExist bool + 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, + }, + { + objectName: util.HypershiftInstallFlagsCM, + objectType: corev1.ConfigMap{}, + objectArgs: []expectedArg{}, // fill out directly from config map + NoObjectArgs: []expectedArg{}, // fill out directly from config map + deploymentName: util.HypershiftOperatorName, + }, + } +) + +func stringToExpectedArg(toAdd []string) []expectedArg { + var result []expectedArg + for s := range toAdd { + result = append(result, expectedArg{shouldExist: true, argument: toAdd[s]}) + } + return result +} + +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)) + return *deployment, err + } + + return *deployment, nil +} \ No newline at end of file diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index ef5d44b8..7bbbc77b 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -127,7 +127,8 @@ func (c *UpgradeController) installOptionsChanged() bool { return true } - return false + return c.checkArgs() + } func (c *UpgradeController) getSecretFromHub(secretName string) (corev1.Secret, error) { @@ -223,5 +224,56 @@ func (c *UpgradeController) syncHypershiftNS() error { } } + return nil } + + +func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) error { + //anything with {key} gets replaced with the value of 'key' in the secret + tp := *toPopulate + for e := range tp { + if _, isCM := tp[e].objectType.(corev1.ConfigMap); isCM { + newInstallFlagsCM, err := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) + if err == nil { + tp[e].objectArgs = append(tp[e].objectArgs, stringToExpectedArg(c.buildOtherInstallFlags(newInstallFlagsCM))...) + } + } else { + + } + } + return nil +} + +func (c *UpgradeController) checkArgs() bool { + // Create expected args based on secrets' existence and their values + // Compare the expected args to the actual args + // If they differ, reinstall + + err := c.populateExpectedArgs(&expected) + + + for o := range expected { + operatorDeployment, err := c.getDeployment(expected[o].deploymentName) + if err != nil { + continue + } + + // switch expected[o].objectName { + // case util.HypershiftBucketSecretName: + + // case util.HypershiftPrivateLinkSecretName: + + // case util.HypershiftExternalDNSSecretName: + + // case util.HypershiftInstallFlagsCM: + + + // default: + // c.log.Info(fmt.Sprintf("unkown object (%s)", expected[o].objectName)) + + // } + } + + return false +} From 10022137c54c11714e9372df5766bce5ab3afc64 Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Mon, 24 Jul 2023 14:44:21 -0400 Subject: [PATCH 4/9] populate secret args Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 21 +++++++++++ pkg/install/hypershift.go | 34 ++++++++--------- pkg/install/upgrade.go | 75 ++++++++++++++++++++++++------------- 3 files changed, 88 insertions(+), 42 deletions(-) diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go index ee855b99..92d9ee39 100644 --- a/pkg/install/expectedArgs.go +++ b/pkg/install/expectedArgs.go @@ -2,6 +2,7 @@ package install import ( "fmt" + "strings" "github.com/stolostron/hypershift-addon-operator/pkg/util" appsv1 "k8s.io/api/apps/v1" @@ -89,4 +90,24 @@ func (c *UpgradeController) getDeployment(operatorName string) (appsv1.Deploymen } return *deployment, nil +} + +// Match {text} and remove it +// Returns matched text e.g. --oidc-storage-provider-s3-bucket-name={bucket} will become "--oidc-storage-provider-s3-bucket-name=" and return "bucket" +func matchAndTrim(s *string) string { + i := strings.Index(*s, "{") + if i >= 0 { + j := strings.Index(*s, "}") + if j >= 0 { + match := (*s)[i+1 : j] + *s = (*s)[:len(*s)-(len(match)+2)] + return match + } + } + return "" +} + +func getValueFromKey(secret corev1.Secret, key string) string { + value := secret.Data[key] + return string(value) } \ No newline at end of file diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index eae8a879..8595af7f 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -253,9 +253,9 @@ 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 - } + // 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, @@ -270,9 +270,9 @@ 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 - } + // 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, @@ -287,17 +287,17 @@ 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 - } - } + // 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 7bbbc77b..1e111f73 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -75,6 +75,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") @@ -127,7 +128,8 @@ func (c *UpgradeController) installOptionsChanged() bool { return true } - return c.checkArgs() + //return c.checkArgs() + return false } @@ -228,7 +230,6 @@ func (c *UpgradeController) syncHypershiftNS() error { return nil } - func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) error { //anything with {key} gets replaced with the value of 'key' in the secret tp := *toPopulate @@ -237,43 +238,67 @@ func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) e newInstallFlagsCM, err := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) if err == nil { tp[e].objectArgs = append(tp[e].objectArgs, stringToExpectedArg(c.buildOtherInstallFlags(newInstallFlagsCM))...) + c.log.Info("CONFIG MAP ARGUMENTS") + for _, a := range tp[e].objectArgs { + c.log.Info(fmt.Sprintf("CFMAP ARG %s",a.argument)) + } } } else { + secret, err := c.getSecretFromHub(tp[e].objectName) + if err != nil { + c.log.Info(fmt.Sprintf("secret %s is not present on the hub", tp[e].objectName)) + continue + } + for _, a := range tp[e].objectArgs { + key := matchAndTrim(&a.argument) + if key != "" { + value := getValueFromKey(secret, key) + a.argument += value + } + c.log.Info(fmt.Sprintf("ARG %s", a.argument)) + } + for _, a := range tp[e].NoObjectArgs { + key := matchAndTrim(&a.argument) + if key != "" { + value := getValueFromKey(secret, key) + a.argument += value + } + c.log.Info(fmt.Sprintf("n ARG %s", a.argument)) + } } } return nil } -func (c *UpgradeController) checkArgs() bool { - // Create expected args based on secrets' existence and their values - // Compare the expected args to the actual args - // If they differ, reinstall +// func (c *UpgradeController) checkArgs() bool { +// // Create expected args based on secrets' existence and their values +// // Compare the expected args to the actual args +// // If they differ, reinstall - err := c.populateExpectedArgs(&expected) +// err := c.populateExpectedArgs(&expected) - for o := range expected { - operatorDeployment, err := c.getDeployment(expected[o].deploymentName) - if err != nil { - continue - } - - // switch expected[o].objectName { - // case util.HypershiftBucketSecretName: +// for o := range expected { +// operatorDeployment, err := c.getDeployment(expected[o].deploymentName) +// if err != nil { +// continue +// } - // case util.HypershiftPrivateLinkSecretName: +// // switch expected[o].objectName { +// // case util.HypershiftBucketSecretName: - // case util.HypershiftExternalDNSSecretName: +// // case util.HypershiftPrivateLinkSecretName: - // case util.HypershiftInstallFlagsCM: +// // case util.HypershiftExternalDNSSecretName: +// // case util.HypershiftInstallFlagsCM: - // default: - // c.log.Info(fmt.Sprintf("unkown object (%s)", expected[o].objectName)) - - // } - } +// // default: +// // c.log.Info(fmt.Sprintf("unkown object (%s)", expected[o].objectName)) - return false -} +// // } +// } + +// return false +// } From eabc8ae22a5c0ba303a7dfc28bc5b80d175c864e Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Mon, 24 Jul 2023 16:49:31 -0400 Subject: [PATCH 5/9] reinstall when mismatch Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 36 +++++++++--- pkg/install/hypershift.go | 94 ++++++------------------------ pkg/install/hypershift_test.go | 2 - pkg/install/upgrade.go | 101 +++++++++------------------------ pkg/install/upgrade_test.go | 3 - 5 files changed, 73 insertions(+), 163 deletions(-) diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go index 92d9ee39..8bc7ed4d 100644 --- a/pkg/install/expectedArgs.go +++ b/pkg/install/expectedArgs.go @@ -62,13 +62,13 @@ var ( }, deploymentName: util.HypershiftOperatorExternalDNSName, }, - { - objectName: util.HypershiftInstallFlagsCM, - objectType: corev1.ConfigMap{}, - objectArgs: []expectedArg{}, // fill out directly from config map - NoObjectArgs: []expectedArg{}, // fill out directly from config map - deploymentName: util.HypershiftOperatorName, - }, + // { + // objectName: util.HypershiftInstallFlagsCM, + // objectType: corev1.ConfigMap{}, + // objectArgs: []expectedArg{}, // fill out directly from config map + // NoObjectArgs: []expectedArg{}, // fill out directly from config map + // deploymentName: util.HypershiftOperatorName, + // }, } ) @@ -110,4 +110,24 @@ func matchAndTrim(s *string) string { func getValueFromKey(secret corev1.Secret, key string) string { value := secret.Data[key] return string(value) -} \ No newline at end of file +} + +func argMismatch(args []expectedArg, deployArgs []string) bool { + for _, a := range args { + if argInList(a.argument, deployArgs) != a.shouldExist { + fmt.Printf("MISMATCH WITH ARG %s %t\n", a.argument, a.shouldExist) + return true + } + } + return false +} + +func argInList(arg string, list []string) bool { + for _, a := range list { + //fmt.Printf("comparing %s WITH %s\n", a, arg) + if strings.Contains(a, arg) { + return true + } + } + return false +} diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 8595af7f..34b96594 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -220,8 +220,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller oidcBucket = false } - // cache the bucket secret for comparison againt the hub's to detect any change - c.bucketSecret = *se //Attempt to retrieve private link creds privateSecretKey := types.NamespacedName{Name: util.HypershiftPrivateLinkSecretName, Namespace: c.clusterName} @@ -231,8 +229,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller privateLinkCreds = false } - // cache the private link secret for comparison againt the hub's to detect any change - c.privateLinkSecret = *spl //Platform is aws if either secret exists awsPlatform = oidcBucket || privateLinkCreds @@ -313,9 +309,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller c.log.Info(fmt.Sprintf("external dns secret(%s) was not found", extDNSSecretKey)) } - // cache the external DNS secret for comparison againt the hub's to detect any change - c.extDnsSecret = *sExtDNS - // Get the hypershift operator installation flags configmap from the hub installFlagsCM, _ := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) c.installFlagsConfigmap = installFlagsCM @@ -337,11 +330,7 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller // compare locally saved secrets to the hub secrets as well // If they are the same, skip re-install. if reinstallCheckRequired && - !(c.operatorImagesUpdated(im, *operatorDeployment) || - c.secretDataUpdated(util.HypershiftBucketSecretName, *se) || - c.secretDataUpdated(util.HypershiftPrivateLinkSecretName, *spl) || - c.secretDataUpdated(util.HypershiftExternalDNSSecretName, *sExtDNS) || - c.configmapDataUpdated(util.HypershiftInstallFlagsCM, installFlagsCM)) { + !(c.operatorImagesUpdated(im, *operatorDeployment)) { c.log.Info("no change in hypershift operator images, secrets and install flags, skipping hypershift operator installation") return nil } @@ -391,20 +380,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller // since the last successful installation metrics.InstallationOrUpgradeFailedCount.Set(0) - // Upon successful installation, save the secrets locally to check any changes on addon restart - if err := c.saveSecretLocally(ctx, se); err != nil { // S3 bucket secret - return err - } - if err := c.saveSecretLocally(ctx, spl); err != nil { // private link secret - return err - } - if err := c.saveSecretLocally(ctx, sExtDNS); err != nil { // external DNS secret - return err - } - if err := c.saveConfigmapLocally(ctx, &installFlagsCM); err != nil { // hypershift operator installation flags - return err - } - // Add label and update image pull secret in Hypershift deployment err = c.updateHyperShiftDeployment(ctx) if err != nil { @@ -548,41 +523,23 @@ func (c *UpgradeController) createOrUpdateSpokeSecret(ctx context.Context, hubSe return err } -func (c *UpgradeController) saveSecretLocally(ctx context.Context, hubSecret *corev1.Secret) error { - if hubSecret.Name != "" { - spokeSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: hubSecret.Name, - Namespace: c.addonNamespace, - }, - } - c.log.Info(fmt.Sprintf("save the secret (%s/%s) locally on cluster %s", c.addonNamespace, hubSecret.Name, c.clusterName)) - _, err := controllerutil.CreateOrUpdate(ctx, c.spokeUncachedClient, spokeSecret, func() error { - spokeSecret.Data = hubSecret.Data - return nil - }) - return err - } - return nil -} - -func (c *UpgradeController) saveConfigmapLocally(ctx context.Context, hubConfigmap *corev1.ConfigMap) error { - if hubConfigmap.Name != "" { - spokeConfigmap := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: hubConfigmap.Name, - Namespace: c.addonNamespace, - }, - } - c.log.Info(fmt.Sprintf("save the configmap (%s/%s) locally on cluster %s", c.addonNamespace, hubConfigmap.Name, c.clusterName)) - _, err := controllerutil.CreateOrUpdate(ctx, c.spokeUncachedClient, spokeConfigmap, func() error { - spokeConfigmap.Data = hubConfigmap.Data - return nil - }) - return err - } - return nil -} +// func (c *UpgradeController) saveConfigmapLocally(ctx context.Context, hubConfigmap *corev1.ConfigMap) error { +// if hubConfigmap.Name != "" { +// spokeConfigmap := &corev1.ConfigMap{ +// ObjectMeta: metav1.ObjectMeta{ +// Name: hubConfigmap.Name, +// Namespace: c.addonNamespace, +// }, +// } +// c.log.Info(fmt.Sprintf("save the configmap (%s/%s) locally on cluster %s", c.addonNamespace, hubConfigmap.Name, c.clusterName)) +// _, err := controllerutil.CreateOrUpdate(ctx, c.spokeUncachedClient, spokeConfigmap, func() error { +// spokeConfigmap.Data = hubConfigmap.Data +// return nil +// }) +// return err +// } +// return nil +// } func (c *UpgradeController) ensurePullSecret(ctx context.Context) error { mcePullSecret := &corev1.Secret{} @@ -842,21 +799,6 @@ func getContainerEnvVar(envVars []corev1.EnvVar, imageName string) string { return "" } -func (c *UpgradeController) secretDataUpdated(secretName string, secret corev1.Secret) bool { - c.log.Info(fmt.Sprintf("comparing hypershift operator installation secret(%s) to the locally saved secret", secretName)) - secretKey := types.NamespacedName{Name: secretName, Namespace: c.addonNamespace} - localSecret := &corev1.Secret{} - if err := c.spokeUncachedClient.Get(context.TODO(), secretKey, localSecret); err != nil && !apierrors.IsNotFound(err) { - c.log.Error(err, "failed to find secret:") // just log and continue - } - - if !reflect.DeepEqual(localSecret.Data, secret.Data) { // compare only the secret data - c.log.Info("the secret has changed") - return true - } - - return false -} func (c *UpgradeController) configmapDataUpdated(cmName string, cm corev1.ConfigMap) bool { c.log.Info(fmt.Sprintf("comparing hypershift operator installation flags configmap(%s) to the locally saved configmap", cmName)) diff --git a/pkg/install/hypershift_test.go b/pkg/install/hypershift_test.go index 21cb5a11..e63f8a16 100644 --- a/pkg/install/hypershift_test.go +++ b/pkg/install/hypershift_test.go @@ -1720,8 +1720,6 @@ func TestAWSPlatformDetection(t *testing.T) { clusterName: "cluster1", pullSecret: "pull-secret", hypershiftInstallExecutor: &HypershiftTestCliExecutor{}, - privateLinkSecret: corev1.Secret{}, - bucketSecret: corev1.Secret{}, } privateLinkSecret := &corev1.Secret{ diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 1e111f73..9b71fce6 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -31,9 +31,6 @@ type UpgradeController struct { hypershiftInstallExecutor HypershiftInstallExecutorInterface stopch chan struct{} ctx context.Context - bucketSecret corev1.Secret - extDnsSecret corev1.Secret - privateLinkSecret corev1.Secret imageOverrideConfigmap corev1.ConfigMap installFlagsConfigmap corev1.ConfigMap reinstallNeeded bool // this is used only for code test @@ -100,35 +97,33 @@ func (c *UpgradeController) Stop() { } func (c *UpgradeController) installOptionsChanged() bool { - // check for changes in AWS S3 bucket secret - newBucketSecret, err := c.getSecretFromHub(util.HypershiftBucketSecretName) - if err == nil && c.secretDataChanged(newBucketSecret, c.bucketSecret, util.HypershiftBucketSecretName) { - c.bucketSecret = newBucketSecret // save the new secret for the next cycle of comparison - return true - } + // 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) + + for _, o := range expected { + dep, err := c.getDeployment(o.deploymentName) + if err != nil { + continue + } - // check for changes in external DNS secret - newExtDnsSecret, err := c.getSecretFromHub(util.HypershiftExternalDNSSecretName) - if err == nil && c.secretDataChanged(newExtDnsSecret, c.extDnsSecret, util.HypershiftExternalDNSSecretName) { - c.extDnsSecret = newExtDnsSecret // save the new secret for the next cycle of comparison - return true - } + deploymentArgs := dep.Spec.Template.Spec.Containers[0].Args - // check for changes in AWS private link secret - newPrivateLinkSecret, err := c.getSecretFromHub(util.HypershiftPrivateLinkSecretName) - if err == nil && c.secretDataChanged(newPrivateLinkSecret, c.privateLinkSecret, util.HypershiftPrivateLinkSecretName) { - c.privateLinkSecret = newPrivateLinkSecret // save the new secret for the next cycle of comparison - return true - } + if err := c.hubClient.Get(context.TODO(), types.NamespacedName{Name: o.objectName, Namespace: c.clusterName}, &corev1.Secret{}); err == nil { + if argMismatch(o.objectArgs, deploymentArgs) { + c.log.Info(fmt.Sprintf("Mistmatch with %s args and install options, reinstalling", o.objectName)) + return true + } + } else { + if argMismatch(o.NoObjectArgs, deploymentArgs) { + c.log.Info(fmt.Sprintf("NONE Mistmatch with %s args and install options, reinstalling", o.objectName)) + return true + } + } - // check for changes in hypershift operator installation flags configmap - newInstallFlagsCM, err := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) - if err == nil && c.configmapDataChanged(newInstallFlagsCM, c.installFlagsConfigmap, util.HypershiftInstallFlagsCM) { - c.installFlagsConfigmap = newInstallFlagsCM // save the new configmap for the next cycle of comparison - return true } - //return c.checkArgs() return false } @@ -145,14 +140,6 @@ func (c *UpgradeController) getSecretFromHub(secretName string) (corev1.Secret, return *newSecret, nil } -func (c *UpgradeController) secretDataChanged(oldSecret, newSecret corev1.Secret, secretName string) bool { - if !reflect.DeepEqual(oldSecret.Data, newSecret.Data) { // compare only the secret data - c.log.Info(fmt.Sprintf("secret(%s) has changed", secretName)) - return true - } - return false -} - func (c *UpgradeController) upgradeImageCheck() bool { // Get the image override configmap from the hub and compare it to the controller's cached image override configmap newImageOverrideConfigmap, err := c.getConfigMapFromHub(util.HypershiftOverrideImagesCM) @@ -240,7 +227,7 @@ func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) e tp[e].objectArgs = append(tp[e].objectArgs, stringToExpectedArg(c.buildOtherInstallFlags(newInstallFlagsCM))...) c.log.Info("CONFIG MAP ARGUMENTS") for _, a := range tp[e].objectArgs { - c.log.Info(fmt.Sprintf("CFMAP ARG %s",a.argument)) + c.log.Info(fmt.Sprintf("CFMAP ARG %s", a.argument)) } } } else { @@ -249,56 +236,22 @@ func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) e c.log.Info(fmt.Sprintf("secret %s is not present on the hub", tp[e].objectName)) continue } - for _, a := range tp[e].objectArgs { + for i, a := range tp[e].objectArgs { key := matchAndTrim(&a.argument) if key != "" { value := getValueFromKey(secret, key) - a.argument += value + tp[e].objectArgs[i].argument = a.argument + value } - c.log.Info(fmt.Sprintf("ARG %s", a.argument)) } - for _, a := range tp[e].NoObjectArgs { + for i, a := range tp[e].NoObjectArgs { key := matchAndTrim(&a.argument) if key != "" { value := getValueFromKey(secret, key) - a.argument += value + tp[e].NoObjectArgs[i].argument = a.argument + value } - c.log.Info(fmt.Sprintf("n ARG %s", a.argument)) } } } return nil } - -// func (c *UpgradeController) checkArgs() bool { -// // Create expected args based on secrets' existence and their values -// // Compare the expected args to the actual args -// // If they differ, reinstall - -// err := c.populateExpectedArgs(&expected) - - -// for o := range expected { -// operatorDeployment, err := c.getDeployment(expected[o].deploymentName) -// if err != nil { -// continue -// } - -// // switch expected[o].objectName { -// // case util.HypershiftBucketSecretName: - -// // case util.HypershiftPrivateLinkSecretName: - -// // case util.HypershiftExternalDNSSecretName: - -// // case util.HypershiftInstallFlagsCM: - -// // default: -// // c.log.Info(fmt.Sprintf("unkown object (%s)", expected[o].objectName)) - -// // } -// } - -// return false -// } diff --git a/pkg/install/upgrade_test.go b/pkg/install/upgrade_test.go index 066f8527..62be6bab 100644 --- a/pkg/install/upgrade_test.go +++ b/pkg/install/upgrade_test.go @@ -147,7 +147,6 @@ func TestBucketSecretChanges(t *testing.T) { clusterName: "cluster1", pullSecret: "pull-secret", hypershiftInstallExecutor: &HypershiftTestCliExecutor{}, - bucketSecret: corev1.Secret{}, } newBucketSecret := &corev1.Secret{ @@ -252,7 +251,6 @@ func TestExtDnsSecretChanges(t *testing.T) { clusterName: "cluster1", pullSecret: "pull-secret", hypershiftInstallExecutor: &HypershiftTestCliExecutor{}, - extDnsSecret: corev1.Secret{}, } newExtDnsSecret := &corev1.Secret{ @@ -359,7 +357,6 @@ func TestPrivateLinkSecretChanges(t *testing.T) { clusterName: "cluster1", pullSecret: "pull-secret", hypershiftInstallExecutor: &HypershiftTestCliExecutor{}, - privateLinkSecret: corev1.Secret{}, } newPrivateLinkSecret := &corev1.Secret{ From 4da7d8769ddb0afaadd69c95c4b525c98f8d61fd Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Wed, 26 Jul 2023 15:37:06 -0400 Subject: [PATCH 6/9] Revert configmap comparison Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 17 ------------ pkg/install/hypershift.go | 45 +++++++++++++++--------------- pkg/install/upgrade.go | 55 ++++++++++++++++++------------------- 3 files changed, 49 insertions(+), 68 deletions(-) diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go index 8bc7ed4d..12803fca 100644 --- a/pkg/install/expectedArgs.go +++ b/pkg/install/expectedArgs.go @@ -62,24 +62,9 @@ var ( }, deploymentName: util.HypershiftOperatorExternalDNSName, }, - // { - // objectName: util.HypershiftInstallFlagsCM, - // objectType: corev1.ConfigMap{}, - // objectArgs: []expectedArg{}, // fill out directly from config map - // NoObjectArgs: []expectedArg{}, // fill out directly from config map - // deploymentName: util.HypershiftOperatorName, - // }, } ) -func stringToExpectedArg(toAdd []string) []expectedArg { - var result []expectedArg - for s := range toAdd { - result = append(result, expectedArg{shouldExist: true, argument: toAdd[s]}) - } - return result -} - func (c *UpgradeController) getDeployment(operatorName string) (appsv1.Deployment, error) { deployment := &appsv1.Deployment{} nsn := types.NamespacedName{Namespace: util.HypershiftOperatorNamespace, Name: operatorName} @@ -115,7 +100,6 @@ func getValueFromKey(secret corev1.Secret, key string) string { func argMismatch(args []expectedArg, deployArgs []string) bool { for _, a := range args { if argInList(a.argument, deployArgs) != a.shouldExist { - fmt.Printf("MISMATCH WITH ARG %s %t\n", a.argument, a.shouldExist) return true } } @@ -124,7 +108,6 @@ func argMismatch(args []expectedArg, deployArgs []string) bool { func argInList(arg string, list []string) bool { for _, a := range list { - //fmt.Printf("comparing %s WITH %s\n", a, arg) if strings.Contains(a, arg) { return true } diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index 34b96594..637c4014 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -220,7 +220,6 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller oidcBucket = false } - //Attempt to retrieve private link creds privateSecretKey := types.NamespacedName{Name: util.HypershiftPrivateLinkSecretName, Namespace: c.clusterName} spl := &corev1.Secret{} @@ -327,11 +326,10 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller args = append(args, "--image-refs", filepath.Join(os.TempDir(), util.HypershiftDownstreamOverride)) // compare installed operator images to the new image stream - // compare locally saved secrets to the hub secrets as well // If they are the same, skip re-install. if reinstallCheckRequired && - !(c.operatorImagesUpdated(im, *operatorDeployment)) { - c.log.Info("no change in hypershift operator images, secrets and install flags, skipping hypershift operator installation") + !(c.operatorImagesUpdated(im, *operatorDeployment) || c.configmapDataUpdated(util.HypershiftInstallFlagsCM, installFlagsCM)) { + c.log.Info("no change in hypershift operator images and install flags, skipping hypershift operator installation") return nil } } else { @@ -380,6 +378,10 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller // since the last successful installation metrics.InstallationOrUpgradeFailedCount.Set(0) + if err := c.saveConfigmapLocally(ctx, &installFlagsCM); err != nil { // hypershift operator installation flags + return err + } + // Add label and update image pull secret in Hypershift deployment err = c.updateHyperShiftDeployment(ctx) if err != nil { @@ -523,23 +525,23 @@ func (c *UpgradeController) createOrUpdateSpokeSecret(ctx context.Context, hubSe return err } -// func (c *UpgradeController) saveConfigmapLocally(ctx context.Context, hubConfigmap *corev1.ConfigMap) error { -// if hubConfigmap.Name != "" { -// spokeConfigmap := &corev1.ConfigMap{ -// ObjectMeta: metav1.ObjectMeta{ -// Name: hubConfigmap.Name, -// Namespace: c.addonNamespace, -// }, -// } -// c.log.Info(fmt.Sprintf("save the configmap (%s/%s) locally on cluster %s", c.addonNamespace, hubConfigmap.Name, c.clusterName)) -// _, err := controllerutil.CreateOrUpdate(ctx, c.spokeUncachedClient, spokeConfigmap, func() error { -// spokeConfigmap.Data = hubConfigmap.Data -// return nil -// }) -// return err -// } -// return nil -// } +func (c *UpgradeController) saveConfigmapLocally(ctx context.Context, hubConfigmap *corev1.ConfigMap) error { + if hubConfigmap.Name != "" { + spokeConfigmap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: hubConfigmap.Name, + Namespace: c.addonNamespace, + }, + } + c.log.Info(fmt.Sprintf("save the configmap (%s/%s) locally on cluster %s", c.addonNamespace, hubConfigmap.Name, c.clusterName)) + _, err := controllerutil.CreateOrUpdate(ctx, c.spokeUncachedClient, spokeConfigmap, func() error { + spokeConfigmap.Data = hubConfigmap.Data + return nil + }) + return err + } + return nil +} func (c *UpgradeController) ensurePullSecret(ctx context.Context) error { mcePullSecret := &corev1.Secret{} @@ -799,7 +801,6 @@ func getContainerEnvVar(envVars []corev1.EnvVar, imageName string) string { return "" } - func (c *UpgradeController) configmapDataUpdated(cmName string, cm corev1.ConfigMap) bool { c.log.Info(fmt.Sprintf("comparing hypershift operator installation flags configmap(%s) to the locally saved configmap", cmName)) cmKey := types.NamespacedName{Name: cmName, Namespace: c.addonNamespace} diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 9b71fce6..a80f644c 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -97,6 +97,13 @@ func (c *UpgradeController) Stop() { } func (c *UpgradeController) installOptionsChanged() bool { + + // check for changes in hypershift operator installation flags configmap + newInstallFlagsCM, err := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) + if err == nil && c.configmapDataChanged(newInstallFlagsCM, c.installFlagsConfigmap, util.HypershiftInstallFlagsCM) { + c.installFlagsConfigmap = newInstallFlagsCM // save the new configmap for the next cycle of comparison + return true + } // Create expected args based on secrets' existence and their values // Compare the expected args to the actual args // If they differ, reinstall @@ -112,12 +119,12 @@ 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) { - c.log.Info(fmt.Sprintf("Mistmatch with %s args and install options, reinstalling", o.objectName)) + c.log.Info(fmt.Sprintf("Mismatch between %s args and install options", o.objectName)) return true } } else { if argMismatch(o.NoObjectArgs, deploymentArgs) { - c.log.Info(fmt.Sprintf("NONE Mistmatch with %s args and install options, reinstalling", o.objectName)) + c.log.Info(fmt.Sprintf("Mismatch between %s args and install options", o.objectName)) return true } } @@ -221,37 +228,27 @@ func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) e //anything with {key} gets replaced with the value of 'key' in the secret tp := *toPopulate for e := range tp { - if _, isCM := tp[e].objectType.(corev1.ConfigMap); isCM { - newInstallFlagsCM, err := c.getConfigMapFromHub(util.HypershiftInstallFlagsCM) - if err == nil { - tp[e].objectArgs = append(tp[e].objectArgs, stringToExpectedArg(c.buildOtherInstallFlags(newInstallFlagsCM))...) - c.log.Info("CONFIG MAP ARGUMENTS") - for _, a := range tp[e].objectArgs { - c.log.Info(fmt.Sprintf("CFMAP ARG %s", a.argument)) - } - } - } else { - secret, err := c.getSecretFromHub(tp[e].objectName) - if err != nil { - c.log.Info(fmt.Sprintf("secret %s is not present on the hub", tp[e].objectName)) - continue + secret, err := c.getSecretFromHub(tp[e].objectName) + if err != nil { + c.log.Info(fmt.Sprintf("secret %s is not present on the hub", tp[e].objectName)) + continue + } + for i, a := range tp[e].objectArgs { + key := matchAndTrim(&a.argument) + if key != "" { + value := getValueFromKey(secret, key) + tp[e].objectArgs[i].argument = a.argument + value } - for i, a := range tp[e].objectArgs { - key := matchAndTrim(&a.argument) - if key != "" { - value := getValueFromKey(secret, key) - tp[e].objectArgs[i].argument = a.argument + value - } - } - for i, a := range tp[e].NoObjectArgs { - key := matchAndTrim(&a.argument) - if key != "" { - value := getValueFromKey(secret, key) - tp[e].NoObjectArgs[i].argument = a.argument + value - } + } + for i, a := range tp[e].NoObjectArgs { + key := matchAndTrim(&a.argument) + if key != "" { + value := getValueFromKey(secret, key) + tp[e].NoObjectArgs[i].argument = a.argument + value } } + } return nil } From deddcf25959b3b44b5ec2cbc09815b950a1328dd Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 27 Jul 2023 21:09:18 -0400 Subject: [PATCH 7/9] 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"}, }}, }, }, From 446e2802fc94617d5ee68b5288ea74157c506d2a Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Thu, 27 Jul 2023 23:09:39 -0400 Subject: [PATCH 8/9] Fix hypershift_test Signed-off-by: Omar Farag --- pkg/install/hypershift_test.go | 68 +++++++++++++++++----------------- 1 file changed, 33 insertions(+), 35 deletions(-) diff --git a/pkg/install/hypershift_test.go b/pkg/install/hypershift_test.go index e63f8a16..0b0c0e67 100644 --- a/pkg/install/hypershift_test.go +++ b/pkg/install/hypershift_test.go @@ -446,6 +446,9 @@ func TestRunHypershiftInstall(t *testing.T) { err = installHyperShiftOperator(t, ctx, aCtrl, true) assert.Nil(t, err, "is nil if install HyperShift is successful") + err = aCtrl.syncHypershiftNS() + assert.Nil(t, err, "is nil if sync NS is successful") + // Hypershift NS is created (with OIDc secret) err = aCtrl.spokeUncachedClient.Get(ctx, types.NamespacedName{Name: "hypershift"}, hypershiftNs) assert.Nil(t, err, "is nil if the hypershift namespace was created") @@ -461,19 +464,6 @@ func TestRunHypershiftInstall(t *testing.T) { assert.Nil(t, err, "is nil when oidc secret is found") assert.Equal(t, []byte(`myCredential`), oidcSecret.Data["credentials"], "the credentials should be equal if the copy was a success") - // Check hypershift-operator-oidc-provider-s3-credentials secret exists in the addon namespace - localOidcSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: util.HypershiftBucketSecretName, - Namespace: aCtrl.addonNamespace, - }, - } - err = aCtrl.spokeUncachedClient.Get(ctx, ctrlClient.ObjectKeyFromObject(localOidcSecret), localOidcSecret) - assert.Nil(t, err, "is nil when locally saved oidc secret is found") - assert.Equal(t, []byte(`myCredential`), localOidcSecret.Data["credentials"], "the credentials should be equal if the copy was a success") - assert.Equal(t, []byte(`my-bucket`), localOidcSecret.Data["bucket"], "the bucket should be equal if the copy was a success") - assert.Equal(t, []byte(`us-east-1`), localOidcSecret.Data["region"], "the resion should be equal if the copy was a success") - // Check hypershift-operator-private-link-credentials secret does NOT exist plSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -823,6 +813,9 @@ func TestRunHypershiftInstallPrivateLinkExternalDNS(t *testing.T) { defer deleteAllInstallJobs(ctx, aCtrl.spokeUncachedClient, aCtrl.addonNamespace) assert.Nil(t, err, "is nil if install HyperShift is successful") + err = aCtrl.syncHypershiftNS() + assert.Nil(t, err, "is nil if sync NS is successful") + // Check hypershift-operator-oidc-provider-s3-credentials secret exists oidcSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -881,28 +874,6 @@ func TestRunHypershiftInstallPrivateLinkExternalDNS(t *testing.T) { } } - // Check hypershift-operator-private-link-credentials secret exists in the addon namespace - localPrivateLinkSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: util.HypershiftPrivateLinkSecretName, - Namespace: aCtrl.addonNamespace, - }, - } - err = aCtrl.spokeUncachedClient.Get(ctx, ctrlClient.ObjectKeyFromObject(localPrivateLinkSecret), localPrivateLinkSecret) - assert.Nil(t, err, "is nil when locally saved private link secret is found") - assert.Equal(t, []byte(`us-east-1`), localPrivateLinkSecret.Data["region"], "the region should be equal if the copy was a success") - - // Check hypershift-operator-private-link-credentials secret exists in the addon namespace - localExtDnsSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: util.HypershiftExternalDNSSecretName, - Namespace: aCtrl.addonNamespace, - }, - } - err = aCtrl.spokeUncachedClient.Get(ctx, ctrlClient.ObjectKeyFromObject(localExtDnsSecret), localExtDnsSecret) - assert.Nil(t, err, "is nil when locally saved external DNS secret is found") - assert.Equal(t, []byte(`my.house.com`), localExtDnsSecret.Data["domain-filter"], "the domain-filter should be equal if the copy was a success") - // Cleanup err = aCtrl.RunHypershiftCmdWithRetires(ctx, 3, time.Second*10, aCtrl.RunHypershiftCleanup) assert.Nil(t, err, "is nil if cleanup is succcessful") @@ -1095,6 +1066,9 @@ func TestRunHypershiftInstallExternalDNSDifferentSecret(t *testing.T) { assert.Equal(t, float64(0), testutil.ToFloat64(metrics.InInstallationOrUpgradeBool)) assert.Equal(t, float64(0), testutil.ToFloat64(metrics.InstallationOrUpgradeFailedCount)) + err = aCtrl.syncHypershiftNS() + assert.Nil(t, err, "is nil if sync NS is successful") + // Check hypershift-operator-oidc-provider-s3-credentials secret exists oidcSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -1744,6 +1718,30 @@ func TestAWSPlatformDetection(t *testing.T) { }, } + 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, operatorDeployment) + controller.Start() assert.Eventually(t, func() bool { return !controller.awsPlatform From 8e1ecfd939dda37e4b934efc416eb1e53adefdbe Mon Sep 17 00:00:00 2001 From: Omar Farag Date: Fri, 28 Jul 2023 01:08:11 -0400 Subject: [PATCH 9/9] Fix sonar Signed-off-by: Omar Farag --- pkg/install/expectedArgs.go | 3 +- pkg/install/hypershift.go | 6 ++- pkg/install/upgrade.go | 77 ++++++++++++++++--------------------- test/e2e/install_test.go | 6 ++- 4 files changed, 44 insertions(+), 48 deletions(-) diff --git a/pkg/install/expectedArgs.go b/pkg/install/expectedArgs.go index 0a740204..9eba1b77 100644 --- a/pkg/install/expectedArgs.go +++ b/pkg/install/expectedArgs.go @@ -36,7 +36,8 @@ func (c *UpgradeController) getDeployment(operatorName string) (appsv1.Deploymen } // Match {text} and remove it -// Returns matched text e.g. --oidc-storage-provider-s3-bucket-name={bucket} will become "--oidc-storage-provider-s3-bucket-name=" and return "bucket" +// Returns matched text e.g. --oidc-storage-provider-s3-bucket-name={bucket} will become +// "--oidc-storage-provider-s3-bucket-name=" and return "bucket" func matchAndTrim(s *string) string { i := strings.Index(*s, "{") if i >= 0 { diff --git a/pkg/install/hypershift.go b/pkg/install/hypershift.go index a41b1f6a..86eba315 100644 --- a/pkg/install/hypershift.go +++ b/pkg/install/hypershift.go @@ -311,7 +311,8 @@ func (c *UpgradeController) runHypershiftInstall(ctx context.Context, controller // compare installed operator images to the new image stream // If they are the same, skip re-install. if reinstallCheckRequired && - !(c.operatorImagesUpdated(im, *operatorDeployment) || c.configmapDataUpdated(util.HypershiftInstallFlagsCM, installFlagsCM)) { + !(c.operatorImagesUpdated(im, *operatorDeployment) || + c.configmapDataUpdated(util.HypershiftInstallFlagsCM, installFlagsCM)) { c.log.Info("no change in hypershift operator images and install flags, skipping hypershift operator installation") return nil } @@ -471,7 +472,8 @@ func getParamValue(s []string, e string) string { return "" } -func (c *UpgradeController) createOrUpdateAwsSpokeSecret(ctx context.Context, hubSecret *corev1.Secret, regionRequired bool) error { +func (c *UpgradeController) createOrUpdateAwsSpokeSecret( + ctx context.Context, hubSecret *corev1.Secret, regionRequired bool) error { spokeSecret := hubSecret.DeepCopy() region := hubSecret.Data["region"] diff --git a/pkg/install/upgrade.go b/pkg/install/upgrade.go index 218794f7..be0d1a9a 100644 --- a/pkg/install/upgrade.go +++ b/pkg/install/upgrade.go @@ -114,12 +114,14 @@ func (c *UpgradeController) installOptionsChanged() bool { 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}, + {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}, + {argument: "--oidc-storage-provider-s3-credentials=/etc/oidc-storage-provider-s3-creds/credentials", + shouldExist: false}, }, deploymentName: util.HypershiftOperatorName, }, @@ -160,18 +162,17 @@ func (c *UpgradeController) installOptionsChanged() bool { deploymentArgs := dep.Spec.Template.Spec.Containers[0].Args - if err := c.hubClient.Get(context.TODO(), types.NamespacedName{Name: o.objectName, Namespace: c.clusterName}, &corev1.Secret{}); err == nil { + 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 } } @@ -225,45 +226,18 @@ func (c *UpgradeController) configmapDataChanged(oldCM, newCM corev1.ConfigMap, func (c *UpgradeController) syncHypershiftNS() error { //Sync secrets in local-cluster namespace with secrets in hypershift namespace - secrets := []string{"hypershift-operator-oidc-provider-s3-credentials", "hypershift-operator-private-link-credentials", "hypershift-operator-external-dns-credentials"} - awsPlatform := false + secrets := []string{util.HypershiftBucketSecretName, + util.HypershiftPrivateLinkSecretName, + util.HypershiftExternalDNSSecretName} ctx := context.TODO() for s := range secrets { - if secrets[s] == "hypershift-operator-external-dns-credentials" { - - 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 - } - } - } else { - c.log.Info(fmt.Sprintf("external dns secret(%s) was not found", extDNSSecretKey)) - } - - } else { - - secretKey := types.NamespacedName{Name: secrets[s], Namespace: c.clusterName} - se := &corev1.Secret{} - if err := c.hubClient.Get(ctx, secretKey, se); err == nil { - awsPlatform = true - if err := c.createOrUpdateAwsSpokeSecret(ctx, se, true); err != nil { - return err - } - } else { - c.log.Info(fmt.Sprintf("secret(%s) not found on the hub.", secretKey)) - - } - + secretKey := types.NamespacedName{Name: secrets[s], Namespace: c.clusterName} + se := &corev1.Secret{} + if err := c.hubClient.Get(ctx, secretKey, se); err != nil { + c.log.Info(fmt.Sprintf("secret(%s) not found on the hub.", secretKey)) + } else if err := c.createOrUpdateSecret(ctx, se); err != nil { + return err } } @@ -271,7 +245,7 @@ func (c *UpgradeController) syncHypershiftNS() error { return nil } -func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) error { +func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) { //anything with {key} gets replaced with the value of 'key' in the secret tp := *toPopulate for e := range tp { @@ -297,5 +271,20 @@ func (c *UpgradeController) populateExpectedArgs(toPopulate *[]expectedConfig) e } } +} + +func (c *UpgradeController) createOrUpdateSecret(ctx context.Context, secret *corev1.Secret) error { + if secret.Name == util.HypershiftExternalDNSSecretName && !c.awsPlatform { + if err := c.createOrUpdateSpokeSecret(ctx, secret); err != nil { + return err + } + } else { + c.awsPlatform = true + if err := c.createOrUpdateAwsSpokeSecret(ctx, secret, + secret.Name != util.HypershiftExternalDNSSecretName); err != nil { + return err + } + + } return nil } diff --git a/test/e2e/install_test.go b/test/e2e/install_test.go index ceead30f..c27cbecc 100644 --- a/test/e2e/install_test.go +++ b/test/e2e/install_test.go @@ -62,7 +62,11 @@ func createOIDCProviderSecret(ctx context.Context, client kubernetes.Interface, func deleteOIDCProviderSecret(ctx context.Context, client kubernetes.Interface, namespace string) error { ginkgo.By(fmt.Sprintf("Delete hypershift OIDC provider secret for %s", namespace)) - return client.CoreV1().Secrets(namespace).Delete(ctx, "hypershift-operator-oidc-provider-s3-credentials", metav1.DeleteOptions{}) + if err := client.CoreV1().Secrets(namespace).Delete(ctx, "hypershift-operator-oidc-provider-s3-credentials", metav1.DeleteOptions{}); err != nil { + ginkgo.By(fmt.Sprintf("Error deleting oidc credentials: %v", err.Error())) + return err + } + return nil } func getPodLogs(pod *corev1.Pod, container string) (string, error) {