Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ACM-5994 - Detect changes from deployment instead of cached secrets #259

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

ACM-5994 - Detect changes from deployment instead of cached secrets #259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c46947c
add space, test PR
o-farag Jun 7, 2023
3076ef9
Merge branch 'stolostron:main' into main
o-farag Jun 29, 2023
4d3ba5b
Merge branch 'stolostron:main' into main
o-farag Jul 19, 2023
bb26559
Sync secrets between local-cluster and hypershift ns
o-farag Jul 19, 2023
370e899
populate expected args
o-farag Jul 20, 2023
1002213
populate secret args
o-farag Jul 24, 2023
eabc8ae
reinstall when mismatch
o-farag Jul 24, 2023
4da7d87
Revert configmap comparison
o-farag Jul 26, 2023
deddcf2
Fixed upgrade_test.go
o-farag Jul 28, 2023
446e280
Fix hypershift_test
o-farag Jul 28, 2023
8e1ecfd
Fix sonar
o-farag Jul 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions pkg/install/expectedArgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package install

import (
"fmt"
"strings"

"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
}

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.Info(fmt.Sprintf("failed to get %s deployment", operatorName))
return *deployment, err
}

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)
}

func argMismatch(args []expectedArg, deployArgs []string) bool {
for _, a := range args {
if argInList(a.argument, deployArgs) != a.shouldExist {
return true
}
}
return false
}

func argInList(arg string, list []string) bool {
for _, a := range list {
if strings.Contains(a, arg) {
return true
}
}
return false
}
82 changes: 5 additions & 77 deletions pkg/install/hypershift.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,9 +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}
spl := &corev1.Secret{}
Expand All @@ -231,8 +228,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
Expand All @@ -253,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.createAwsSpokeSecret(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,
Expand All @@ -270,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.createAwsSpokeSecret(ctx, spl, true); err != nil {
return err
}
c.log.Info("private link region & credential arguments included")
awsArgs := []string{
"--aws-private-secret", util.HypershiftPrivateLinkSecretName,
Expand All @@ -287,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.createAwsSpokeSecret(ctx, sExtDNS, false); err != nil {
return err
}
} else {
if err := c.createSpokeSecret(ctx, sExtDNS); err != nil {
return err
}
}

c.log.Info("external dns provider & domain-filter arguments included")
awsArgs := []string{
Expand All @@ -313,9 +291,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
Expand All @@ -334,15 +309,11 @@ 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.secretDataUpdated(util.HypershiftBucketSecretName, *se) ||
c.secretDataUpdated(util.HypershiftPrivateLinkSecretName, *spl) ||
c.secretDataUpdated(util.HypershiftExternalDNSSecretName, *sExtDNS) ||
c.configmapDataUpdated(util.HypershiftInstallFlagsCM, installFlagsCM)) {
c.log.Info("no change in hypershift operator images, secrets and install flags, skipping hypershift operator installation")
c.log.Info("no change in hypershift operator images and install flags, skipping hypershift operator installation")
return nil
}
} else {
Expand Down Expand Up @@ -391,16 +362,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
}
Expand Down Expand Up @@ -511,7 +472,8 @@ 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"]
Expand All @@ -525,10 +487,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{
Expand All @@ -548,24 +510,6 @@ func (c *UpgradeController) createSpokeSecret(ctx context.Context, hubSecret *co
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{
Expand Down Expand Up @@ -842,22 +786,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))
cmKey := types.NamespacedName{Name: cmName, Namespace: c.addonNamespace}
Expand Down
76 changes: 36 additions & 40 deletions pkg/install/hypershift_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -1418,7 +1392,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")

}
Expand Down Expand Up @@ -1449,7 +1423,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{
Expand All @@ -1465,7 +1439,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)
Expand Down Expand Up @@ -1720,8 +1694,6 @@ func TestAWSPlatformDetection(t *testing.T) {
clusterName: "cluster1",
pullSecret: "pull-secret",
hypershiftInstallExecutor: &HypershiftTestCliExecutor{},
privateLinkSecret: corev1.Secret{},
bucketSecret: corev1.Secret{},
}

privateLinkSecret := &corev1.Secret{
Expand All @@ -1746,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
Expand Down
Loading