From ab3bbd4f35669dc264f9ca2560b1144ef035445f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nils=20Gustav=20Str=C3=A5b=C3=B8?= <65334626+nilsgstrabo@users.noreply.github.com> Date: Tue, 20 Aug 2024 10:45:38 +0200 Subject: [PATCH 1/3] 1159 bug git ssh keys secret unexpectedly regenerated (#1169) * initial commit of secret refactoring * revert change unrelated to the issue * to advantage of optimisitc locking when saving deploy key secret and configmap * define const for known_hosts key * update lint step to scan from root * ignore linter deprecation warning for kube.ApplySecret * extract extract of deploy key to separate function * update chart --- .github/workflows/pr.yml | 3 +- charts/radix-operator/Chart.yaml | 4 +- pkg/apis/alert/secrets.go | 2 +- pkg/apis/application/application_test.go | 5 +- pkg/apis/application/secrets.go | 202 ++++++++++-------- pkg/apis/applicationconfig/build_secret.go | 4 +- pkg/apis/applicationconfig/imagehubsecret.go | 2 +- pkg/apis/defaults/commonsecrets.go | 3 + pkg/apis/defaults/configmaps.go | 2 - pkg/apis/deployment/externaldns.go | 2 +- .../deployment/oauthproxyresourcemanager.go | 2 +- pkg/apis/deployment/secretrefs.go | 4 +- pkg/apis/deployment/secrets.go | 6 +- pkg/apis/deployment/volumemount.go | 4 +- pkg/apis/kube/config_maps.go | 33 +-- pkg/apis/kube/config_maps_test.go | 42 ---- pkg/apis/kube/secrets.go | 21 ++ pkg/apis/test/utils.go | 67 +++++- 18 files changed, 228 insertions(+), 180 deletions(-) diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 2a7bf1aee..d24b14450 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -77,8 +77,7 @@ jobs: - name: golangci-lint uses: golangci/golangci-lint-action@v6 with: - version: v1.58.2 - working-directory: './radix-operator' + version: v1.59.1 verify-code-generation: name: Verify Code Generation diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml index daf3be918..2cacc4a0a 100644 --- a/charts/radix-operator/Chart.yaml +++ b/charts/radix-operator/Chart.yaml @@ -1,7 +1,7 @@ apiVersion: v2 name: radix-operator -version: 1.37.5 -appVersion: 1.57.13 +version: 1.37.6 +appVersion: 1.57.14 kubeVersion: ">=1.24.0" description: Radix Operator keywords: diff --git a/pkg/apis/alert/secrets.go b/pkg/apis/alert/secrets.go index 3128d1ced..ff3a3ab9f 100644 --- a/pkg/apis/alert/secrets.go +++ b/pkg/apis/alert/secrets.go @@ -31,7 +31,7 @@ func (syncer *alertSyncer) createOrUpdateSecret(ctx context.Context) error { syncer.setSecretCommonProps(secret) - _, err = syncer.kubeUtil.ApplySecret(ctx, ns, secret) + _, err = syncer.kubeUtil.ApplySecret(ctx, ns, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret return err } diff --git a/pkg/apis/application/application_test.go b/pkg/apis/application/application_test.go index f50c4fdb7..c443da6bc 100644 --- a/pkg/apis/application/application_test.go +++ b/pkg/apis/application/application_test.go @@ -123,8 +123,9 @@ func TestOnSync_RegistrationCreated_AppNamespaceWithResourcesCreated(t *testing. assert.Equal(t, 1, len(appAdminRoleBinding.Subjects)) secrets, _ := client.CoreV1().Secrets("any-app-app").List(context.Background(), metav1.ListOptions{}) - assert.Equal(t, 1, len(secrets.Items)) - assert.Equal(t, "git-ssh-keys", secrets.Items[0].Name) + secretNames := slice.Map(secrets.Items, func(s corev1.Secret) string { return s.Name }) + expectedSecrets := []string{defaults.GitPrivateKeySecretName, defaults.AzureACRServicePrincipleBuildahSecretName, defaults.AzureACRServicePrincipleSecretName, defaults.AzureACRTokenPasswordAppRegistrySecretName} + assert.ElementsMatch(t, expectedSecrets, secretNames) serviceAccounts, _ := client.CoreV1().ServiceAccounts("any-app-app").List(context.Background(), metav1.ListOptions{}) assert.Equal(t, 2, len(serviceAccounts.Items)) diff --git a/pkg/apis/application/secrets.go b/pkg/apis/application/secrets.go index c2bbc34ac..73ceabac7 100644 --- a/pkg/apis/application/secrets.go +++ b/pkg/apis/application/secrets.go @@ -15,97 +15,149 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +const ( + knownHostsSecretKey = "known_hosts" +) + // ApplySecretsForPipelines creates secrets needed by pipeline to run func (app *Application) applySecretsForPipelines(ctx context.Context) error { - radixRegistration := app.registration - log.Ctx(ctx).Debug().Msg("Apply secrets for pipelines") - buildNamespace := utils.GetAppNamespace(radixRegistration.Name) - err := app.applyGitDeployKeyToBuildNamespace(ctx, buildNamespace) - if err != nil { - return err + if err := app.applyGitDeployKeyToBuildNamespace(ctx); err != nil { + return fmt.Errorf("failed to apply pipeline git deploy keys: %w", err) } - err = app.applyServicePrincipalACRSecretToBuildNamespace(ctx, buildNamespace) - if err != nil { - log.Ctx(ctx).Warn().Msgf("Failed to apply service principle acr secrets (%s, %s) to namespace %s", defaults.AzureACRServicePrincipleSecretName, defaults.AzureACRServicePrincipleBuildahSecretName, buildNamespace) + + if err := app.applyServicePrincipalACRSecretToBuildNamespace(ctx); err != nil { + return fmt.Errorf("failed to apply pipeline service principal ACR secrets: %w", err) } + return nil } -func (app *Application) applyGitDeployKeyToBuildNamespace(ctx context.Context, namespace string) error { - radixRegistration := app.registration - secret, err := app.kubeutil.GetSecret(ctx, namespace, defaults.GitPrivateKeySecretName) +func (app *Application) applyGitDeployKeyToBuildNamespace(ctx context.Context) error { + currentSecret, desiredSecret, derivedPublicKey, err := app.getCurrentAndDesiredGitPrivateDeployKeySecret(ctx) if err != nil { - if !k8serrors.IsNotFound(err) { + return err + } + + currentCm, desiredCm, err := app.getCurrentAndDesiredGitPublicDeployKeyConfigMap(ctx, derivedPublicKey) + if err != nil { + return err + } + + if currentSecret != nil { + if _, err := app.kubeutil.UpdateSecret(ctx, currentSecret, desiredSecret); err != nil { + return err + } + } else { + if _, err := app.kubeutil.CreateSecret(ctx, desiredSecret.Namespace, desiredSecret); err != nil { return err } } - deployKey := &utils.DeployKey{} - - privateKeyExists := app.gitPrivateKeyExists(secret) // checking if private key exists - if privateKeyExists { - deployKey, err = deriveKeyPairFromSecret(secret) // if private key exists, retrieve public key from secret - if err != nil { + if currentCm != nil { + if _, err = app.kubeutil.UpdateConfigMap(ctx, currentCm, desiredCm); err != nil { return err } } else { - // if private key secret does not exist, check if RR has private key - deployKeyString := radixRegistration.Spec.DeployKey - if deployKeyString == "" { - // if RR does not have private key, generate new key pair - deployKey, err = utils.GenerateDeployKey() - if err != nil { - return err - } - } else { - // if RR has private key, retrieve both public and private key from RR - deployKey.PublicKey = radixRegistration.Spec.DeployKeyPublic - deployKey.PrivateKey = radixRegistration.Spec.DeployKey + if _, err := app.kubeutil.CreateConfigMap(ctx, desiredCm.Namespace, desiredCm); err != nil { + return err } } - // create corresponding secret with private key - secret, err = app.createNewGitDeployKey(ctx, namespace, deployKey.PrivateKey, radixRegistration) + return nil +} + +func (app *Application) getCurrentAndDesiredGitPrivateDeployKeySecret(ctx context.Context) (current, desired *corev1.Secret, derivedPublicKey string, err error) { + namespace := utils.GetAppNamespace(app.registration.Name) + // Cannot assign `current` directly from GetSecret since kube client returns a non-nil value even when an error is returned + currentInternal, err := app.kubeutil.GetSecret(ctx, namespace, defaults.GitPrivateKeySecretName) if err != nil { - return err + if !k8serrors.IsNotFound(err) { + return nil, nil, "", err + } + desired = &corev1.Secret{ + Type: "Opaque", + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.GitPrivateKeySecretName, + Namespace: namespace, + }, + } + } else { + desired = currentInternal.DeepCopy() + current = currentInternal } - _, err = app.kubeutil.ApplySecret(ctx, namespace, secret) + + knownHostsSecret, err := app.kubeutil.GetSecret(ctx, corev1.NamespaceDefault, "radix-known-hosts-git") if err != nil { - return err + return nil, nil, "", fmt.Errorf("failed to get known hosts secret: %w", err) } - newCm := app.createGitPublicKeyConfigMap(namespace, deployKey.PublicKey) - _, err = app.kubeutil.CreateConfigMap(ctx, namespace, newCm) + deployKey, err := getExistingOrGenerateNewDeployKey(current, app.registration) if err != nil { - if k8serrors.IsAlreadyExists(err) { - existingCm, err := app.kubeutil.GetConfigMap(ctx, namespace, defaults.GitPublicKeyConfigMapName) - if err != nil { - return err - } - err = app.kubeutil.ApplyConfigMap(ctx, namespace, existingCm, newCm) - if err != nil { - return err - } - } else { - return err - } + return nil, nil, "", fmt.Errorf("failed to get deploy key: %w", err) } - return nil + desired.ObjectMeta.Labels = labels.ForApplicationName(app.registration.Name) // Required when restoring with Velero. We only restore secrets with the "radix-app" label + desired.Data = map[string][]byte{ + defaults.GitPrivateKeySecretKey: []byte(deployKey.PrivateKey), + knownHostsSecretKey: knownHostsSecret.Data[knownHostsSecretKey], + } + + return current, desired, deployKey.PublicKey, nil +} + +func getExistingOrGenerateNewDeployKey(fromSecret *corev1.Secret, fromRadixRegistration *v1.RadixRegistration) (*utils.DeployKey, error) { + switch { + case fromSecret != nil && secretHasGitPrivateDeployKey(fromSecret): + privateKey := fromSecret.Data[defaults.GitPrivateKeySecretKey] + keypair, err := utils.DeriveDeployKeyFromPrivateKey(string(privateKey)) + if err != nil { + return nil, fmt.Errorf("failed to parse deploy key from existing secret: %w", err) + } + return keypair, nil + case len(fromRadixRegistration.Spec.DeployKey) > 0: + return &utils.DeployKey{ + PrivateKey: fromRadixRegistration.Spec.DeployKey, + PublicKey: fromRadixRegistration.Spec.DeployKeyPublic, + }, nil + default: + keypair, err := utils.GenerateDeployKey() + if err != nil { + return nil, fmt.Errorf("failed to generate new git deploy key: %w", err) + } + return keypair, nil + } } -func deriveKeyPairFromSecret(secret *corev1.Secret) (*utils.DeployKey, error) { - privateKey := string(secret.Data[defaults.GitPrivateKeySecretKey]) - deployKey, err := utils.DeriveDeployKeyFromPrivateKey(privateKey) +func (app *Application) getCurrentAndDesiredGitPublicDeployKeyConfigMap(ctx context.Context, publicKey string) (current, desired *corev1.ConfigMap, err error) { + namespace := utils.GetAppNamespace(app.registration.Name) + // Cannot assign `current` directly from GetConfigMap since kube client returns a non-nil value even when an error is returned + currentInternal, err := app.kubeutil.GetConfigMap(ctx, namespace, defaults.GitPublicKeyConfigMapName) if err != nil { - return nil, err + if !k8serrors.IsNotFound(err) { + return nil, nil, err + } + desired = &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.GitPublicKeyConfigMapName, + Namespace: namespace, + }, + } + } else { + desired = currentInternal.DeepCopy() + current = currentInternal } - return deployKey, nil + + desired.Data = map[string]string{ + defaults.GitPublicKeyConfigMapKey: publicKey, + } + + return current, desired, nil } -func (app *Application) applyServicePrincipalACRSecretToBuildNamespace(ctx context.Context, buildNamespace string) error { +func (app *Application) applyServicePrincipalACRSecretToBuildNamespace(ctx context.Context) error { + buildNamespace := utils.GetAppNamespace(app.registration.Name) servicePrincipalSecretForBuild, err := app.createNewServicePrincipalACRSecret(ctx, buildNamespace, defaults.AzureACRServicePrincipleSecretName) if err != nil { return err @@ -122,7 +174,7 @@ func (app *Application) applyServicePrincipalACRSecretToBuildNamespace(ctx conte } for _, secret := range []*corev1.Secret{servicePrincipalSecretForBuild, servicePrincipalSecretForBuildahBuild, tokenSecretForAppRegistry} { - _, err = app.kubeutil.ApplySecret(ctx, buildNamespace, secret) + _, err = app.kubeutil.ApplySecret(ctx, buildNamespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } @@ -131,28 +183,6 @@ func (app *Application) applyServicePrincipalACRSecretToBuildNamespace(ctx conte return nil } -func (app *Application) createNewGitDeployKey(ctx context.Context, namespace, deployKey string, registration *v1.RadixRegistration) (*corev1.Secret, error) { - knownHostsSecret, err := app.kubeutil.GetSecret(ctx, corev1.NamespaceDefault, "radix-known-hosts-git") - if err != nil { - return nil, fmt.Errorf("failed to get known hosts secret: %w", err) - } - - secret := corev1.Secret{ - Type: "Opaque", - ObjectMeta: metav1.ObjectMeta{ - Name: defaults.GitPrivateKeySecretName, - Namespace: namespace, - // Required when restoring with Velero. We only restore secrets with the "radix-app" label - Labels: labels.ForApplicationName(registration.GetName()), - }, - Data: map[string][]byte{ - defaults.GitPrivateKeySecretKey: []byte(deployKey), - "known_hosts": knownHostsSecret.Data["known_hosts"], - }, - } - return &secret, nil -} - func (app *Application) createNewServicePrincipalACRSecret(ctx context.Context, namespace, secretName string) (*corev1.Secret, error) { servicePrincipalSecret, err := app.kubeutil.GetSecret(ctx, corev1.NamespaceDefault, secretName) if err != nil { @@ -175,19 +205,9 @@ func (app *Application) createNewServicePrincipalACRSecret(ctx context.Context, return &secret, nil } -func (app *Application) gitPrivateKeyExists(secret *corev1.Secret) bool { +func secretHasGitPrivateDeployKey(secret *corev1.Secret) bool { if secret == nil { return false } return len(strings.TrimSpace(string(secret.Data[defaults.GitPrivateKeySecretKey]))) > 0 } -func (app *Application) createGitPublicKeyConfigMap(namespace string, key string) *corev1.ConfigMap { - // Create a configmap with the public key - cm := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: defaults.GitPublicKeyConfigMapName, - Namespace: namespace, - }, Data: map[string]string{defaults.GitPublicKeyConfigMapKey: key}} - - return cm -} diff --git a/pkg/apis/applicationconfig/build_secret.go b/pkg/apis/applicationconfig/build_secret.go index cb38a0530..d5e63c66a 100644 --- a/pkg/apis/applicationconfig/build_secret.go +++ b/pkg/apis/applicationconfig/build_secret.go @@ -62,7 +62,7 @@ func (app *ApplicationConfig) initializeBuildSecret(ctx context.Context, namespa } secret := getBuildSecretForData(app.config.Name, namespace, name, data) - _, err := app.kubeutil.ApplySecret(ctx, namespace, secret) + _, err := app.kubeutil.ApplySecret(ctx, namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } @@ -82,7 +82,7 @@ func (app *ApplicationConfig) updateBuildSecret(ctx context.Context, namespace, secret = getBuildSecretForData(app.config.Name, namespace, name, secret.Data) } - _, err = app.kubeutil.ApplySecret(ctx, namespace, secret) + _, err = app.kubeutil.ApplySecret(ctx, namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } diff --git a/pkg/apis/applicationconfig/imagehubsecret.go b/pkg/apis/applicationconfig/imagehubsecret.go index 04fe8257a..75f80c2cc 100644 --- a/pkg/apis/applicationconfig/imagehubsecret.go +++ b/pkg/apis/applicationconfig/imagehubsecret.go @@ -107,7 +107,7 @@ func ApplyPrivateImageHubSecret(ctx context.Context, kubeutil *kube.Kube, ns, ap secret.Data[corev1.DockerConfigJsonKey] = secretValue } - _, err := kubeutil.ApplySecret(ctx, ns, &secret) + _, err := kubeutil.ApplySecret(ctx, ns, &secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return fmt.Errorf("failed to create private image hub secrets in namespace %s: %w", ns, err) } diff --git a/pkg/apis/defaults/commonsecrets.go b/pkg/apis/defaults/commonsecrets.go index 2be7e9344..958901e8a 100644 --- a/pkg/apis/defaults/commonsecrets.go +++ b/pkg/apis/defaults/commonsecrets.go @@ -9,4 +9,7 @@ const ( // AzureACRTokenPasswordAppRegistrySecretName name of the secret containing Cache ACR authentication information, consumed by buildah AzureACRTokenPasswordAppRegistrySecretName = "radix-app-registry" + + GitPrivateKeySecretName = "git-ssh-keys" + GitPrivateKeySecretKey = "id_rsa" ) diff --git a/pkg/apis/defaults/configmaps.go b/pkg/apis/defaults/configmaps.go index 4e82a9484..a52cc46ff 100644 --- a/pkg/apis/defaults/configmaps.go +++ b/pkg/apis/defaults/configmaps.go @@ -11,6 +11,4 @@ const ( RadixGitChangedChangedRadixConfig = "git-changed-radixconfig" GitPublicKeyConfigMapName = "git-ssh-public-key" GitPublicKeyConfigMapKey = "public_key" - GitPrivateKeySecretName = "git-ssh-keys" - GitPrivateKeySecretKey = "id_rsa" ) diff --git a/pkg/apis/deployment/externaldns.go b/pkg/apis/deployment/externaldns.go index b7fc19306..2d2729cf7 100644 --- a/pkg/apis/deployment/externaldns.go +++ b/pkg/apis/deployment/externaldns.go @@ -230,7 +230,7 @@ func (deploy *Deployment) createOrUpdateExternalDnsTlsSecret(ctx context.Context return err } - _, err = deploy.kubeutil.ApplySecret(ctx, ns, &secret) + _, err = deploy.kubeutil.ApplySecret(ctx, ns, &secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } diff --git a/pkg/apis/deployment/oauthproxyresourcemanager.go b/pkg/apis/deployment/oauthproxyresourcemanager.go index ca87ae21d..602ebd64b 100644 --- a/pkg/apis/deployment/oauthproxyresourcemanager.go +++ b/pkg/apis/deployment/oauthproxyresourcemanager.go @@ -468,7 +468,7 @@ func (o *oauthProxyResourceManager) createOrUpdateSecret(ctx context.Context, co } } - _, err = o.kubeutil.ApplySecret(ctx, o.rd.Namespace, secret) + _, err = o.kubeutil.ApplySecret(ctx, o.rd.Namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret return err } diff --git a/pkg/apis/deployment/secretrefs.go b/pkg/apis/deployment/secretrefs.go index 7d1257961..5fa0fbb36 100644 --- a/pkg/apis/deployment/secretrefs.go +++ b/pkg/apis/deployment/secretrefs.go @@ -31,7 +31,7 @@ func (deploy *Deployment) createSecretRefs(ctx context.Context, namespace string } if credsSecret != nil && !isOwnerReference(credsSecret.ObjectMeta, secretProviderClass.ObjectMeta) { credsSecret.ObjectMeta.OwnerReferences = append(credsSecret.ObjectMeta.OwnerReferences, getOwnerReferenceOfSecretProviderClass(secretProviderClass)) - _, err = deploy.kubeutil.ApplySecret(ctx, namespace, credsSecret) + _, err = deploy.kubeutil.ApplySecret(ctx, namespace, credsSecret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return nil, err } @@ -89,7 +89,7 @@ func (deploy *Deployment) getOrCreateAzureKeyVaultCredsSecret(ctx context.Contex if err != nil { if errors.IsNotFound(err) { secret = buildAzureKeyVaultCredentialsSecret(appName, componentName, secretName, azKeyVaultName) - return deploy.kubeutil.ApplySecret(ctx, namespace, secret) + return deploy.kubeutil.ApplySecret(ctx, namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret } return nil, err } diff --git a/pkg/apis/deployment/secrets.go b/pkg/apis/deployment/secrets.go index 9fd790dc2..926d96584 100644 --- a/pkg/apis/deployment/secrets.go +++ b/pkg/apis/deployment/secrets.go @@ -273,7 +273,7 @@ func (deploy *Deployment) createOrUpdateComponentSecret(ctx context.Context, ns, return err } - _, err = deploy.kubeutil.ApplySecret(ctx, ns, &secret) + _, err = deploy.kubeutil.ApplySecret(ctx, ns, &secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } @@ -325,7 +325,7 @@ func (deploy *Deployment) createClientCertificateSecret(ctx context.Context, ns, data["ca.crt"] = defaultValue secret.Data = data - _, err := deploy.kubeutil.ApplySecret(ctx, ns, &secret) + _, err := deploy.kubeutil.ApplySecret(ctx, ns, &secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret return err } @@ -345,7 +345,7 @@ func (deploy *Deployment) removeOrphanedSecrets(ctx context.Context, ns, secretN } if orphanRemoved { - _, err = deploy.kubeutil.ApplySecret(ctx, ns, secret) + _, err = deploy.kubeutil.ApplySecret(ctx, ns, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } diff --git a/pkg/apis/deployment/volumemount.go b/pkg/apis/deployment/volumemount.go index ee8eda963..1d5109919 100644 --- a/pkg/apis/deployment/volumemount.go +++ b/pkg/apis/deployment/volumemount.go @@ -489,7 +489,7 @@ func (deploy *Deployment) createOrUpdateVolumeMountsSecrets(ctx context.Context, blobfusecredsSecret.Data = data - _, err := deploy.kubeutil.ApplySecret(ctx, namespace, &blobfusecredsSecret) + _, err := deploy.kubeutil.ApplySecret(ctx, namespace, &blobfusecredsSecret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } @@ -517,7 +517,7 @@ func (deploy *Deployment) createOrUpdateCsiAzureVolumeMountsSecrets(ctx context. secret.Data = data - _, err := deploy.kubeutil.ApplySecret(ctx, namespace, &secret) + _, err := deploy.kubeutil.ApplySecret(ctx, namespace, &secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret if err != nil { return err } diff --git a/pkg/apis/kube/config_maps.go b/pkg/apis/kube/config_maps.go index ef3ae9b9c..56af46eed 100644 --- a/pkg/apis/kube/config_maps.go +++ b/pkg/apis/kube/config_maps.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "github.com/equinor/radix-operator/pkg/apis/utils/slice" "github.com/rs/zerolog/log" @@ -15,13 +16,6 @@ import ( "k8s.io/apimachinery/pkg/util/strategicpatch" ) -// CreateConfigMap Create config map -func (kubeutil *Kube) CreateConfigMap(ctx context.Context, namespace string, configMap *corev1.ConfigMap) (*corev1.ConfigMap, error) { - return kubeutil.kubeClient.CoreV1().ConfigMaps(namespace).Create(ctx, - configMap, - metav1.CreateOptions{}) -} - // GetConfigMap Gets config map by name func (kubeutil *Kube) GetConfigMap(ctx context.Context, namespace, name string) (*corev1.ConfigMap, error) { return kubeutil.kubeClient.CoreV1().ConfigMaps(namespace).Get(ctx, name, metav1.GetOptions{}) @@ -42,15 +36,24 @@ func (kubeutil *Kube) ListEnvVarsMetadataConfigMaps(ctx context.Context, namespa return kubeutil.ListConfigMapsWithSelector(ctx, namespace, getEnvVarsMetadataConfigMapSelector().String()) } -// UpdateConfigMap Update config-maps -func (kubeutil *Kube) UpdateConfigMap(ctx context.Context, namespace string, configMaps ...*corev1.ConfigMap) error { - for _, configMap := range configMaps { - _, err := kubeutil.kubeClient.CoreV1().ConfigMaps(namespace).Update(ctx, configMap, metav1.UpdateOptions{}) - if err != nil { - return err - } +// CreateConfigMap Create config map +func (kubeutil *Kube) CreateConfigMap(ctx context.Context, namespace string, configMap *corev1.ConfigMap) (*corev1.ConfigMap, error) { + created, err := kubeutil.kubeClient.CoreV1().ConfigMaps(namespace).Create(ctx, configMap, metav1.CreateOptions{}) + log.Ctx(ctx).Info().Msgf("Created config map %s/%s", created.Namespace, created.Name) + return created, err +} + +// UpdateConfigMap updates the `modified` configmap. +// If `original` is set, the two configmaps are compared, and the secret is only updated if they are not equal. +func (kubeutil *Kube) UpdateConfigMap(ctx context.Context, original, modified *corev1.ConfigMap) (*corev1.ConfigMap, error) { + if original != nil && reflect.DeepEqual(original, modified) { + log.Ctx(ctx).Debug().Msgf("No need to update configmap %s/%s", modified.Namespace, modified.Name) + return modified, nil } - return nil + + updated, err := kubeutil.kubeClient.CoreV1().ConfigMaps(modified.Namespace).Update(ctx, modified, metav1.UpdateOptions{}) + log.Ctx(ctx).Info().Msgf("Updated configmap %s/%s", updated.Namespace, updated.Name) + return updated, err } // DeleteConfigMap Deletes config-maps diff --git a/pkg/apis/kube/config_maps_test.go b/pkg/apis/kube/config_maps_test.go index d41af9620..f56094cc1 100644 --- a/pkg/apis/kube/config_maps_test.go +++ b/pkg/apis/kube/config_maps_test.go @@ -143,48 +143,6 @@ func Test_GetConfigMap(t *testing.T) { }) } -func Test_UpdateConfigMap(t *testing.T) { - t.Run("Update not existing config-map", func(t *testing.T) { - t.Parallel() - testEnv := getConfigMapTestEnv() - namespace := "some-namespace" - name := "some-name" - testConfigMap := corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: name}, - Data: map[string]string{"key1": "value1", "key2": "value2"}, - } - - err := testEnv.kubeUtil.UpdateConfigMap(context.Background(), namespace, &testConfigMap) - - assert.NotNil(t, err) - assert.Equal(t, "configmaps \"some-name\" not found", err.Error()) - }) - - t.Run("Update existing config-map", func(t *testing.T) { - t.Parallel() - testEnv := getConfigMapTestEnv() - namespace := "some-namespace" - name := "some-name" - _, _ = testEnv.kubeclient.CoreV1().ConfigMaps(namespace).Create(context.Background(), &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: name}, - Data: map[string]string{"key1": "value1", "key2": "value2"}, - }, metav1.CreateOptions{}) - - testConfigMap := corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: name}, - Data: map[string]string{"key2": "value2changed", "key3": "value3"}, - } - err := testEnv.kubeUtil.UpdateConfigMap(context.Background(), namespace, &testConfigMap) - require.NoError(t, err) - - configMap, err := testEnv.kubeUtil.GetConfigMap(context.Background(), namespace, name) - require.NoError(t, err) - assert.Equal(t, name, configMap.ObjectMeta.Name) - assert.Equal(t, namespace, configMap.ObjectMeta.Namespace) - assert.True(t, radixutils.EqualStringMaps(testConfigMap.Data, configMap.Data)) - }) -} - func Test_ApplyConfigMap(t *testing.T) { namespace := "some-namespace" name := "some-name" diff --git a/pkg/apis/kube/secrets.go b/pkg/apis/kube/secrets.go index 00cb3a2cc..ba680576e 100644 --- a/pkg/apis/kube/secrets.go +++ b/pkg/apis/kube/secrets.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "strings" "github.com/equinor/radix-common/utils" @@ -43,6 +44,26 @@ func (kubeutil *Kube) ListSecretExistsForLabels(ctx context.Context, namespace s return list.Items, nil } +func (kubeutil *Kube) CreateSecret(ctx context.Context, namespace string, secret *corev1.Secret) (*corev1.Secret, error) { + created, err := kubeutil.kubeClient.CoreV1().Secrets(namespace).Create(ctx, secret, metav1.CreateOptions{}) + log.Ctx(ctx).Info().Msgf("Created secret %s/%s", created.Namespace, created.Name) + return created, err +} + +// UpdateSecret updates the `modified` secret. +// If `original` is set, the two secrets are compared, and the secret is only updated if they are not equal. +func (kubeutil *Kube) UpdateSecret(ctx context.Context, original, modified *corev1.Secret) (*corev1.Secret, error) { + if original != nil && reflect.DeepEqual(original, modified) { + log.Ctx(ctx).Debug().Msgf("No need to update secret %s/%s", modified.Namespace, modified.Name) + return modified, nil + } + + updated, err := kubeutil.kubeClient.CoreV1().Secrets(modified.Namespace).Update(ctx, modified, metav1.UpdateOptions{}) + log.Ctx(ctx).Info().Msgf("Updated secret %s/%s", updated.Namespace, updated.Name) + return updated, err +} + +// Deprecated: ApplySecret is not safe to use because it does not use the resourceVersion of the supplied secret when updating. Use UpdateSecret or CreateSecret instead. // ApplySecret Creates or updates secret to namespace func (kubeutil *Kube) ApplySecret(ctx context.Context, namespace string, secret *corev1.Secret) (savedSecret *corev1.Secret, err error) { secretName := secret.GetName() diff --git a/pkg/apis/test/utils.go b/pkg/apis/test/utils.go index 6a6ac202c..be897bf10 100644 --- a/pkg/apis/test/utils.go +++ b/pkg/apis/test/utils.go @@ -2,6 +2,7 @@ package test import ( "context" + "errors" "os" "time" @@ -16,7 +17,7 @@ import ( "github.com/rs/zerolog/log" "github.com/rs/zerolog/pkgerrors" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" + kubeerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/client-go/kubernetes" @@ -69,7 +70,7 @@ func (tu *Utils) ApplyRegistration(registrationBuilder utils.RegistrationBuilder _, err := tu.radixclient.RadixV1().RadixRegistrations().Create(context.Background(), rr, metav1.CreateOptions{}) if err != nil { - if errors.IsAlreadyExists(err) { + if kubeerrors.IsAlreadyExists(err) { return tu.ApplyRegistrationUpdate(registrationBuilder) } return rr, err @@ -109,7 +110,7 @@ func (tu *Utils) ApplyApplication(applicationBuilder utils.ApplicationBuilder) ( appNamespace := CreateAppNamespace(tu.client, ra.GetName()) _, err := tu.radixclient.RadixV1().RadixApplications(appNamespace).Create(context.Background(), ra, metav1.CreateOptions{}) if err != nil { - if errors.IsAlreadyExists(err) { + if kubeerrors.IsAlreadyExists(err) { return tu.ApplyApplicationUpdate(applicationBuilder) } @@ -148,7 +149,7 @@ func (tu *Utils) ApplyApplicationUpdate(applicationBuilder utils.ApplicationBuil } else { rr, err = tu.radixclient.RadixV1().RadixRegistrations().Get(context.Background(), ra.GetName(), metav1.GetOptions{}) } - if err != nil && !errors.IsNotFound(err) { + if err != nil && !kubeerrors.IsNotFound(err) { return ra, err } @@ -255,7 +256,7 @@ func (tu *Utils) ApplyEnvironment(environmentBuilder utils.EnvironmentBuilder) ( newRe, err := tu.radixclient.RadixV1().RadixEnvironments().Create(context.Background(), re, metav1.CreateOptions{}) if err != nil { - if errors.IsAlreadyExists(err) { + if kubeerrors.IsAlreadyExists(err) { return tu.ApplyEnvironmentUpdate(environmentBuilder) } return nil, err @@ -304,7 +305,8 @@ func SetRequiredEnvironmentVariables() { func (tu *Utils) CreateClusterPrerequisites(clustername, egressIps, subscriptionId string) error { SetRequiredEnvironmentVariables() - if _, err := tu.client.CoreV1().Secrets(corev1.NamespaceDefault).Create( + var errs []error + _, err := tu.client.CoreV1().Secrets(corev1.NamespaceDefault).Create( context.Background(), &corev1.Secret{ Type: "Opaque", @@ -316,11 +318,10 @@ func (tu *Utils) CreateClusterPrerequisites(clustername, egressIps, subscription "known_hosts": []byte("abcd"), }, }, - metav1.CreateOptions{}); err != nil { - return err - } + metav1.CreateOptions{}) + errs = append(errs, err) - _, err := tu.client.CoreV1().ConfigMaps(corev1.NamespaceDefault).Create( + _, err = tu.client.CoreV1().ConfigMaps(corev1.NamespaceDefault).Create( context.Background(), &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -334,7 +335,51 @@ func (tu *Utils) CreateClusterPrerequisites(clustername, egressIps, subscription }, }, metav1.CreateOptions{}) - return err + errs = append(errs, err) + + _, err = tu.client.CoreV1().Secrets(corev1.NamespaceDefault).Create( + context.Background(), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.AzureACRServicePrincipleSecretName, + Namespace: corev1.NamespaceDefault, + }, + StringData: map[string]string{ + "sp_credentials.json": "{}", + }, + }, + metav1.CreateOptions{}) + errs = append(errs, err) + + _, err = tu.client.CoreV1().Secrets(corev1.NamespaceDefault).Create( + context.Background(), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.AzureACRServicePrincipleBuildahSecretName, + Namespace: corev1.NamespaceDefault, + }, + StringData: map[string]string{ + "sp_credentials.json": "{}", + }, + }, + metav1.CreateOptions{}) + errs = append(errs, err) + + _, err = tu.client.CoreV1().Secrets(corev1.NamespaceDefault).Create( + context.Background(), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.AzureACRTokenPasswordAppRegistrySecretName, + Namespace: corev1.NamespaceDefault, + }, + StringData: map[string]string{ + "sp_credentials.json": "{}", + }, + }, + metav1.CreateOptions{}) + errs = append(errs, err) + + return errors.Join(errs...) } // CreateAppNamespace Helper method to creat app namespace From dfb7481e086b0d9ccc4747314c4839240a760cde Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Tue, 20 Aug 2024 16:09:24 +0200 Subject: [PATCH 2/3] fix-unnecessary-radix-diployment-calls (#1171) * Remove unnecessary RD calls * Remove unnecessary RD calls --- charts/radix-operator/Chart.yaml | 4 +-- pkg/apis/job/job.go | 32 +++++++++----------- pkg/apis/kube/namespaces.go | 50 +++++++++++++++++++++++++++++++ pkg/apis/kube/radix_deployment.go | 18 +++++++++++ 4 files changed, 84 insertions(+), 20 deletions(-) diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml index 2cacc4a0a..00d9550a9 100644 --- a/charts/radix-operator/Chart.yaml +++ b/charts/radix-operator/Chart.yaml @@ -1,7 +1,7 @@ apiVersion: v2 name: radix-operator -version: 1.37.6 -appVersion: 1.57.14 +version: 1.37.7 +appVersion: 1.57.15 kubeVersion: ">=1.24.0" description: Radix Operator keywords: diff --git a/pkg/apis/job/job.go b/pkg/apis/job/job.go index 291a95b3a..8eb37a0e6 100644 --- a/pkg/apis/job/job.go +++ b/pkg/apis/job/job.go @@ -279,16 +279,16 @@ func (job *Job) setStatusOfJob(ctx context.Context) error { return err } log.Ctx(ctx).Debug().Msgf("Got %d RadixJob steps", len(steps)) - environments, err := job.getJobEnvironments(ctx) + jobStatusCondition, err := job.getJobConditionFromJobStatus(ctx, pipelineJob.Status) if err != nil { return err } - jobStatusCondition, err := job.getJobConditionFromJobStatus(ctx, pipelineJob.Status) + + environments, err := job.getJobEnvironments(ctx) if err != nil { return err } - - err = job.updateRadixJobStatusWithMetrics(ctx, job.radixJob, job.originalRadixJobCondition, func(currStatus *v1.RadixJobStatus) { + if err = job.updateRadixJobStatusWithMetrics(ctx, job.radixJob, job.originalRadixJobCondition, func(currStatus *v1.RadixJobStatus) { log.Ctx(ctx).Debug().Msgf("Update RadixJob status with %d steps and the condition %s", len(steps), jobStatusCondition) currStatus.Steps = steps currStatus.Condition = jobStatusCondition @@ -300,13 +300,11 @@ func (job *Job) setStatusOfJob(ctx context.Context) error { if len(environments) > 0 { currStatus.TargetEnvs = environments } - }) - if err != nil { + }); err != nil { return err } if isJobConditionDone(jobStatusCondition) { - err = job.setNextJobToRunning(ctx) - if err != nil { + if err = job.setNextJobToRunning(ctx); err != nil { return err } } @@ -679,19 +677,17 @@ func getJobStep(podName, containerName string, containerStatus *corev1.Container } func (job *Job) getJobEnvironments(ctx context.Context) ([]string, error) { - deploymentsLinkedToJob, err := job.radixclient.RadixV1().RadixDeployments(corev1.NamespaceAll).List( - ctx, - metav1.ListOptions{LabelSelector: job.getRadixJobNameLabelSelector()}) + radixDeployments, err := job.kubeutil.GetRadixDeploymentsForApp(ctx, job.radixJob.Spec.AppName, job.getRadixJobNameLabelSelector()) if err != nil { return nil, err } - - var environments []string - for _, deployment := range deploymentsLinkedToJob.Items { - environments = append(environments, deployment.Spec.Environment) - } - - return environments, nil + environmentsMap := slice.Reduce(radixDeployments, make(map[string]struct{}), func(acc map[string]struct{}, rd v1.RadixDeployment) map[string]struct{} { + if rd.GetLabels()[kube.RadixJobNameLabel] == job.radixJob.Name { + acc[rd.Spec.Environment] = struct{}{} + } + return acc + }) + return maps.GetKeysFromMap(environmentsMap), nil } func (job *Job) updateRadixJobStatusWithMetrics(ctx context.Context, savingRadixJob *v1.RadixJob, originalRadixJobCondition v1.RadixJobCondition, changeStatusFunc func(currStatus *v1.RadixJobStatus)) error { diff --git a/pkg/apis/kube/namespaces.go b/pkg/apis/kube/namespaces.go index 2c548f839..60c76b3e7 100644 --- a/pkg/apis/kube/namespaces.go +++ b/pkg/apis/kube/namespaces.go @@ -5,10 +5,14 @@ import ( "encoding/json" "fmt" + "github.com/equinor/radix-common/utils/slice" "github.com/rs/zerolog/log" corev1 "k8s.io/api/core/v1" k8errs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" + kubelabels "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/strategicpatch" ) @@ -84,3 +88,49 @@ func (kubeutil *Kube) getNamespace(ctx context.Context, name string) (*corev1.Na return namespace, nil } + +// ListNamespacesWithSelector List namespaces with selector +func (kubeutil *Kube) ListNamespacesWithSelector(ctx context.Context, labelSelectorString string) ([]*corev1.Namespace, error) { + if kubeutil.NamespaceLister != nil { + selector, err := labels.Parse(labelSelectorString) + if err != nil { + return nil, err + } + return kubeutil.NamespaceLister.List(selector) + } + + list, err := kubeutil.kubeClient.CoreV1().Namespaces().List(ctx, metav1.ListOptions{LabelSelector: labelSelectorString}) + if err != nil { + return nil, err + } + + return slice.PointersOf(list.Items).([]*corev1.Namespace), nil + +} + +// GetEnvNamespacesForApp Get all env namespaces for an application +func (kubeutil *Kube) GetEnvNamespacesForApp(ctx context.Context, appName string) ([]*corev1.Namespace, error) { + return kubeutil.ListNamespacesWithSelector(ctx, envNamespacesLabelForApp(appName).String()) +} + +func envNamespacesLabelForApp(appName string) kubelabels.Selector { + return labels.NewSelector(). + Add(*requirementRadixAppNameLabel(appName)). + Add(*requirementNotRadixAppNamespaceLabel()) +} + +func requirementRadixAppNameLabel(appName string) *labels.Requirement { + requirement, err := kubelabels.NewRequirement(RadixAppLabel, selection.Equals, []string{appName}) + if err != nil { + panic(err) + } + return requirement +} + +func requirementNotRadixAppNamespaceLabel() *labels.Requirement { + requirement, err := kubelabels.NewRequirement(RadixEnvLabel, selection.NotEquals, []string{"app"}) + if err != nil { + panic(err) + } + return requirement +} diff --git a/pkg/apis/kube/radix_deployment.go b/pkg/apis/kube/radix_deployment.go index 589829421..11443c3d8 100644 --- a/pkg/apis/kube/radix_deployment.go +++ b/pkg/apis/kube/radix_deployment.go @@ -63,3 +63,21 @@ func (kubeutil *Kube) GetActiveDeployment(ctx context.Context, namespace string) } return nil, nil } + +// GetRadixDeploymentsForApp Get all Radix deployments for an application +func (kubeutil *Kube) GetRadixDeploymentsForApp(ctx context.Context, appName string, labelSelector string) ([]v1.RadixDeployment, error) { + envNamespaces, err := kubeutil.GetEnvNamespacesForApp(ctx, appName) + if err != nil { + return nil, err + } + var radixDeployments []v1.RadixDeployment + for _, namespace := range envNamespaces { + radixDeploymentList, err := kubeutil.RadixClient().RadixV1().RadixDeployments(namespace.GetName()).List(ctx, + metav1.ListOptions{LabelSelector: labelSelector}) + if err != nil { + return nil, err + } + radixDeployments = append(radixDeployments, radixDeploymentList.Items...) + } + return radixDeployments, nil +} From 52aa6dac77166c83c15d7ca5eb264ee5ddbf0810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nils=20Gustav=20Str=C3=A5b=C3=B8?= <65334626+nilsgstrabo@users.noreply.github.com> Date: Wed, 21 Aug 2024 09:23:01 +0200 Subject: [PATCH 3/3] Remove calls to deprecated kube.ApplySecret (#1173) * refactor alert syncer to not use ApplySecret * refactor application build secrets to not use ApplySecret * update chart version --- charts/radix-operator/Chart.yaml | 4 +- pkg/apis/alert/secrets.go | 44 ++++--- pkg/apis/applicationconfig/build_secret.go | 132 +++++++++------------ pkg/apis/applicationconfig/role.go | 6 +- pkg/apis/kube/config_maps.go | 6 + pkg/apis/kube/secrets.go | 6 + 6 files changed, 106 insertions(+), 92 deletions(-) diff --git a/charts/radix-operator/Chart.yaml b/charts/radix-operator/Chart.yaml index 00d9550a9..c0938d092 100644 --- a/charts/radix-operator/Chart.yaml +++ b/charts/radix-operator/Chart.yaml @@ -1,7 +1,7 @@ apiVersion: v2 name: radix-operator -version: 1.37.7 -appVersion: 1.57.15 +version: 1.37.8 +appVersion: 1.57.16 kubeVersion: ">=1.24.0" description: Radix Operator keywords: diff --git a/pkg/apis/alert/secrets.go b/pkg/apis/alert/secrets.go index ff3a3ab9f..efc55025c 100644 --- a/pkg/apis/alert/secrets.go +++ b/pkg/apis/alert/secrets.go @@ -5,37 +5,51 @@ import ( "fmt" "github.com/equinor/radix-operator/pkg/apis/kube" - v1 "k8s.io/api/core/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func (syncer *alertSyncer) createOrUpdateSecret(ctx context.Context) error { - secretName, ns := GetAlertSecretName(syncer.radixAlert.Name), syncer.radixAlert.Namespace + current, desired, err := syncer.getCurrentAndDesiredAlertSecret(ctx) + if err != nil { + return err + } - secret, err := syncer.kubeUtil.GetSecret(ctx, ns, secretName) - if err != nil && !errors.IsNotFound(err) { + if current != nil { + _, err = syncer.kubeUtil.UpdateSecret(ctx, current, desired) return err } - if secret == nil { - secret = &v1.Secret{ - Type: v1.SecretType("Opaque"), + _, err = syncer.kubeUtil.CreateSecret(ctx, desired.Namespace, desired) + return err +} + +func (syncer *alertSyncer) getCurrentAndDesiredAlertSecret(ctx context.Context) (current, desired *corev1.Secret, err error) { + secretName, ns := GetAlertSecretName(syncer.radixAlert.Name), syncer.radixAlert.Namespace + currentInternal, err := syncer.kubeUtil.GetSecret(ctx, ns, secretName) + if err != nil { + if !errors.IsNotFound(err) { + return nil, nil, err + } + desired = &corev1.Secret{ + Type: corev1.SecretType("Opaque"), ObjectMeta: metav1.ObjectMeta{ - Name: secretName, + Name: secretName, + Namespace: ns, }, } } else { - syncer.removedOrphanedSecretKeys(secret) + desired = currentInternal.DeepCopy() + current = currentInternal } - syncer.setSecretCommonProps(secret) - - _, err = syncer.kubeUtil.ApplySecret(ctx, ns, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret - return err + syncer.removedOrphanedSecretKeys(desired) + syncer.setSecretCommonProps(desired) + return current, desired, nil } -func (syncer *alertSyncer) setSecretCommonProps(secret *v1.Secret) { +func (syncer *alertSyncer) setSecretCommonProps(secret *corev1.Secret) { secret.OwnerReferences = syncer.getOwnerReference() labels := map[string]string{} @@ -45,7 +59,7 @@ func (syncer *alertSyncer) setSecretCommonProps(secret *v1.Secret) { secret.Labels = labels } -func (syncer *alertSyncer) removedOrphanedSecretKeys(secret *v1.Secret) { +func (syncer *alertSyncer) removedOrphanedSecretKeys(secret *corev1.Secret) { expectedKeys := map[string]interface{}{} // Secret keys related to receiver configuration diff --git a/pkg/apis/applicationconfig/build_secret.go b/pkg/apis/applicationconfig/build_secret.go index d5e63c66a..6657fef79 100644 --- a/pkg/apis/applicationconfig/build_secret.go +++ b/pkg/apis/applicationconfig/build_secret.go @@ -2,99 +2,99 @@ package applicationconfig import ( "context" + "fmt" + "slices" - commonUtils "github.com/equinor/radix-common/utils" "github.com/equinor/radix-operator/pkg/apis/defaults" "github.com/equinor/radix-operator/pkg/apis/kube" "github.com/equinor/radix-operator/pkg/apis/utils" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) func (app *ApplicationConfig) syncBuildSecrets(ctx context.Context) error { - appNamespace := utils.GetAppNamespace(app.config.Name) - isSecretExist := app.kubeutil.SecretExists(ctx, appNamespace, defaults.BuildSecretsName) - - if app.config.Spec.Build == nil { - if isSecretExist { - // Delete build secret - err := app.kubeutil.DeleteSecret(ctx, appNamespace, defaults.BuildSecretsName) - if err != nil { - return err - } + if app.config.Spec.Build == nil || len(app.config.Spec.Build.Secrets) == 0 { + if err := app.garbageCollectBuildSecrets(ctx); err != nil { + return fmt.Errorf("failed to garbage collect build secret: %w", err) } - err := garbageCollectAccessToBuildSecrets(ctx, app) - if err != nil { - return err + + if err := app.garbageCollectAccessToBuildSecrets(ctx); err != nil { + return fmt.Errorf("failed to garbage collect access to build secret: %w", err) } } else { - buildSecrets := app.config.Spec.Build.Secrets - if !isSecretExist { - // Create build secret - err := app.initializeBuildSecret(ctx, appNamespace, defaults.BuildSecretsName, buildSecrets) - if err != nil { - return err + currentSecret, desiredSecret, err := app.getCurrentAndDesiredBuildSecret(ctx) + if err != nil { + return fmt.Errorf("failed get current and desired build secret: %w", err) + } + + if currentSecret != nil { + if _, err := app.kubeutil.UpdateSecret(ctx, currentSecret, desiredSecret); err != nil { + return fmt.Errorf("failed to update build secret: %w", err) } } else { - // Update build secret if there is any change - err := app.updateBuildSecret(ctx, appNamespace, defaults.BuildSecretsName, buildSecrets) - if err != nil { - return err + if _, err := app.kubeutil.CreateSecret(ctx, desiredSecret.Namespace, desiredSecret); err != nil { + return fmt.Errorf("failed to create build secret: %w", err) } } - // Grant access to build secret (RBAC) - err := app.grantAccessToBuildSecrets(ctx, appNamespace) - if err != nil { - return err + if err := app.grantAccessToBuildSecrets(ctx); err != nil { + return fmt.Errorf("failed to grant access to build secret: %w", err) } } return nil } -func (app *ApplicationConfig) initializeBuildSecret(ctx context.Context, namespace, name string, buildSecrets []string) error { - data := make(map[string][]byte) - defaultValue := []byte(defaults.BuildSecretDefaultData) - - for _, buildSecret := range buildSecrets { - data[buildSecret] = defaultValue - } - - secret := getBuildSecretForData(app.config.Name, namespace, name, data) - _, err := app.kubeutil.ApplySecret(ctx, namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret +func (app *ApplicationConfig) getCurrentAndDesiredBuildSecret(ctx context.Context) (current, desired *corev1.Secret, err error) { + ns := utils.GetAppNamespace(app.config.Name) + currentInternal, err := app.kubeutil.GetSecret(ctx, ns, defaults.BuildSecretsName) if err != nil { - return err + if !k8serrors.IsNotFound(err) { + return nil, nil, err + } + desired = &corev1.Secret{ + Type: corev1.SecretTypeOpaque, + ObjectMeta: metav1.ObjectMeta{ + Name: defaults.BuildSecretsName, + Namespace: ns, + }, + } + } else { + desired = currentInternal.DeepCopy() + current = currentInternal } - return nil -} -func (app *ApplicationConfig) updateBuildSecret(ctx context.Context, namespace, name string, buildSecrets []string) error { - secret, err := app.kubeutil.GetSecret(ctx, namespace, name) - if err != nil { - return err + desired.Labels = map[string]string{ + kube.RadixAppLabel: app.config.Name, } - orphanRemoved := removeOrphanedSecrets(secret, buildSecrets) - secretAppended := appendSecrets(secret, buildSecrets) - if !orphanRemoved && !secretAppended { - // Secret definition may have changed, but not data - secret = getBuildSecretForData(app.config.Name, namespace, name, secret.Data) - } + setBuildSecretData(desired, app.config.Spec.Build.Secrets) + + return current, desired, nil +} - _, err = app.kubeutil.ApplySecret(ctx, namespace, secret) //nolint:staticcheck // must be updated to use UpdateSecret or CreateSecret +func (app *ApplicationConfig) garbageCollectBuildSecrets(ctx context.Context) error { + secret, err := app.kubeutil.GetSecret(ctx, utils.GetAppNamespace(app.config.Name), defaults.BuildSecretsName) if err != nil { + if k8serrors.IsNotFound(err) { + return nil + } return err } + return app.kubeutil.DeleteSecret(ctx, secret.Namespace, secret.Name) +} - return nil +func setBuildSecretData(buildSecret *corev1.Secret, buildSecretKeys []string) { + removeOrphanedBuildSecretKeys(buildSecret, buildSecretKeys) + appendMissingBuildSecretKeys(buildSecret, buildSecretKeys) } -func removeOrphanedSecrets(buildSecrets *corev1.Secret, secrets []string) bool { +func removeOrphanedBuildSecretKeys(buildSecret *corev1.Secret, buildSecretKeys []string) bool { orphanRemoved := false - for secretName := range buildSecrets.Data { - if !commonUtils.ContainsString(secrets, secretName) { - delete(buildSecrets.Data, secretName) + for secretName := range buildSecret.Data { + if !slices.Contains(buildSecretKeys, secretName) { + delete(buildSecret.Data, secretName) orphanRemoved = true } } @@ -102,7 +102,7 @@ func removeOrphanedSecrets(buildSecrets *corev1.Secret, secrets []string) bool { return orphanRemoved } -func appendSecrets(buildSecrets *corev1.Secret, secrets []string) bool { +func appendMissingBuildSecretKeys(buildSecrets *corev1.Secret, buildSecretKeys []string) bool { defaultValue := []byte(defaults.BuildSecretDefaultData) if buildSecrets.Data == nil { @@ -111,7 +111,7 @@ func appendSecrets(buildSecrets *corev1.Secret, secrets []string) bool { } secretAppended := false - for _, secretName := range secrets { + for _, secretName := range buildSecretKeys { if _, ok := buildSecrets.Data[secretName]; !ok { buildSecrets.Data[secretName] = defaultValue secretAppended = true @@ -120,17 +120,3 @@ func appendSecrets(buildSecrets *corev1.Secret, secrets []string) bool { return secretAppended } - -func getBuildSecretForData(appName, namespace, name string, data map[string][]byte) *corev1.Secret { - return &corev1.Secret{ - Type: corev1.SecretTypeOpaque, - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - Labels: map[string]string{ - kube.RadixAppLabel: appName, - }, - }, - Data: data, - } -} diff --git a/pkg/apis/applicationconfig/role.go b/pkg/apis/applicationconfig/role.go index b8b6e5396..851371655 100644 --- a/pkg/apis/applicationconfig/role.go +++ b/pkg/apis/applicationconfig/role.go @@ -13,7 +13,8 @@ import ( "k8s.io/apimachinery/pkg/api/errors" ) -func (app *ApplicationConfig) grantAccessToBuildSecrets(ctx context.Context, namespace string) error { +func (app *ApplicationConfig) grantAccessToBuildSecrets(ctx context.Context) error { + namespace := utils.GetAppNamespace(app.config.Name) err := app.grantPipelineAccessToSecret(ctx, namespace, defaults.BuildSecretsName) if err != nil { return err @@ -93,7 +94,8 @@ func (app *ApplicationConfig) garbageCollectAccessToBuildSecretsForRole(ctx cont return nil } -func garbageCollectAccessToBuildSecrets(ctx context.Context, app *ApplicationConfig) error { + +func (app *ApplicationConfig) garbageCollectAccessToBuildSecrets(ctx context.Context) error { appNamespace := utils.GetAppNamespace(app.config.Name) for _, roleName := range []string{ getPipelineRoleNameToSecret(defaults.BuildSecretsName), diff --git a/pkg/apis/kube/config_maps.go b/pkg/apis/kube/config_maps.go index 56af46eed..147e47918 100644 --- a/pkg/apis/kube/config_maps.go +++ b/pkg/apis/kube/config_maps.go @@ -39,6 +39,9 @@ func (kubeutil *Kube) ListEnvVarsMetadataConfigMaps(ctx context.Context, namespa // CreateConfigMap Create config map func (kubeutil *Kube) CreateConfigMap(ctx context.Context, namespace string, configMap *corev1.ConfigMap) (*corev1.ConfigMap, error) { created, err := kubeutil.kubeClient.CoreV1().ConfigMaps(namespace).Create(ctx, configMap, metav1.CreateOptions{}) + if err != nil { + return nil, err + } log.Ctx(ctx).Info().Msgf("Created config map %s/%s", created.Namespace, created.Name) return created, err } @@ -52,6 +55,9 @@ func (kubeutil *Kube) UpdateConfigMap(ctx context.Context, original, modified *c } updated, err := kubeutil.kubeClient.CoreV1().ConfigMaps(modified.Namespace).Update(ctx, modified, metav1.UpdateOptions{}) + if err != nil { + return nil, err + } log.Ctx(ctx).Info().Msgf("Updated configmap %s/%s", updated.Namespace, updated.Name) return updated, err } diff --git a/pkg/apis/kube/secrets.go b/pkg/apis/kube/secrets.go index ba680576e..45925dc21 100644 --- a/pkg/apis/kube/secrets.go +++ b/pkg/apis/kube/secrets.go @@ -46,6 +46,9 @@ func (kubeutil *Kube) ListSecretExistsForLabels(ctx context.Context, namespace s func (kubeutil *Kube) CreateSecret(ctx context.Context, namespace string, secret *corev1.Secret) (*corev1.Secret, error) { created, err := kubeutil.kubeClient.CoreV1().Secrets(namespace).Create(ctx, secret, metav1.CreateOptions{}) + if err != nil { + return nil, err + } log.Ctx(ctx).Info().Msgf("Created secret %s/%s", created.Namespace, created.Name) return created, err } @@ -59,6 +62,9 @@ func (kubeutil *Kube) UpdateSecret(ctx context.Context, original, modified *core } updated, err := kubeutil.kubeClient.CoreV1().Secrets(modified.Namespace).Update(ctx, modified, metav1.UpdateOptions{}) + if err != nil { + return nil, err + } log.Ctx(ctx).Info().Msgf("Updated secret %s/%s", updated.Namespace, updated.Name) return updated, err }