From 1f38c932c8d96634ac066723d91d6225063eb864 Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 11:06:53 -0400 Subject: [PATCH 1/7] rotate the openshift-config pull-secret on acr token rotation --- pkg/cluster/acrtoken.go | 47 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index e731252b91d..a28953f4919 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -5,6 +5,8 @@ package cluster import ( "context" + kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -15,6 +17,8 @@ import ( "github.com/Azure/ARO-RP/pkg/util/pullsecret" ) +var pullSecretName = types.NamespacedName{Name: "pull-secret", Namespace: "openshift-config"} + func (m *manager) ensureACRToken(ctx context.Context) error { if m.env.IsLocalDevelopmentMode() { return nil @@ -108,6 +112,49 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error { if err != nil { return err } + err = m.rotateOpenShiftConfigSecret(ctx, pullSecret.Data[corev1.DockerConfigJsonKey]) + if err != nil { + return err + } return nil } + +func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDockerConfigJson []byte) error { + openshiftConfigSecret, err := m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Get(ctx, pullSecretName.Name, metav1.GetOptions{}) + if err != nil { + return err + } + updatedSecret := corev1.Secret{} + + recreate := openshiftConfigSecret == nil || + (openshiftConfigSecret.Type != corev1.SecretTypeDockerConfigJson || openshiftConfigSecret.Data == nil) || + (openshiftConfigSecret.Immutable != nil && *openshiftConfigSecret.Immutable) + + if recreate { + updatedSecret = corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pullSecretName.Name, + Namespace: pullSecretName.Namespace, + }, + Type: corev1.SecretTypeDockerConfigJson, + Data: make(map[string][]byte), + } + } else { + updatedSecret = *openshiftConfigSecret.DeepCopy() + } + + updatedSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson + + if recreate { + err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Delete(ctx, pullSecretName.Name, metav1.DeleteOptions{}) + if err != nil && !kerrors.IsNotFound(err) { + return err + } + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, &updatedSecret, metav1.CreateOptions{}) + } else { + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, &updatedSecret, metav1.UpdateOptions{}) + } + + return err +} From 584f7735df4d3ef0c133427d936021859a39830c Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 11:30:24 -0400 Subject: [PATCH 2/7] refactor to reduce use of conditions --- pkg/cluster/acrtoken.go | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index a28953f4919..f1a786058e4 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -125,36 +125,30 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker if err != nil { return err } - updatedSecret := corev1.Secret{} - recreate := openshiftConfigSecret == nil || + recreationOfSecretRequired := openshiftConfigSecret == nil || (openshiftConfigSecret.Type != corev1.SecretTypeDockerConfigJson || openshiftConfigSecret.Data == nil) || (openshiftConfigSecret.Immutable != nil && *openshiftConfigSecret.Immutable) - if recreate { - updatedSecret = corev1.Secret{ + if recreationOfSecretRequired { + recreatedSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: pullSecretName.Name, Namespace: pullSecretName.Namespace, }, Type: corev1.SecretTypeDockerConfigJson, - Data: make(map[string][]byte), + Data: map[string][]byte{corev1.DockerConfigJsonKey: encodedDockerConfigJson}, } - } else { - updatedSecret = *openshiftConfigSecret.DeepCopy() - } - - updatedSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson - - if recreate { err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Delete(ctx, pullSecretName.Name, metav1.DeleteOptions{}) if err != nil && !kerrors.IsNotFound(err) { return err } - _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, &updatedSecret, metav1.CreateOptions{}) - } else { - _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, &updatedSecret, metav1.UpdateOptions{}) + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, recreatedSecret, metav1.CreateOptions{}) + return err } + openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, openshiftConfigSecret, metav1.UpdateOptions{}) + return err } From 9ea5c627ce172255a8ea6affe36ad3c6adc62f54 Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 13:04:53 -0400 Subject: [PATCH 3/7] adds retries to delete/create/update on the openshift-config pull secret --- pkg/cluster/acrtoken.go | 33 ++++++++++++++++++++++++++------- 1 file changed, 26 insertions(+), 7 deletions(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index f1a786058e4..88cceb5c1f0 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -5,11 +5,14 @@ package cluster import ( "context" - kerrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/types" + "time" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/operator" @@ -139,16 +142,32 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker Type: corev1.SecretTypeDockerConfigJson, Data: map[string][]byte{corev1.DockerConfigJsonKey: encodedDockerConfigJson}, } - err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Delete(ctx, pullSecretName.Name, metav1.DeleteOptions{}) + + err := retryOperation(func() error { + return m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Delete(ctx, pullSecretName.Name, metav1.DeleteOptions{}) + }) if err != nil && !kerrors.IsNotFound(err) { return err } - _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, recreatedSecret, metav1.CreateOptions{}) - return err + return retryOperation(func() error { + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, recreatedSecret, metav1.CreateOptions{}) + return err + }) } openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson - _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, openshiftConfigSecret, metav1.UpdateOptions{}) - return err + return retryOperation(func() error { + _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, openshiftConfigSecret, metav1.UpdateOptions{}) + return err + }) +} + +func retryOperation(retryable func() error) error { + return retry.OnError(wait.Backoff{ + Steps: 10, + Duration: 2 * time.Second, + }, func(err error) bool { + return kerrors.IsBadRequest(err) || kerrors.IsInternalError(err) || kerrors.IsServerTimeout(err) + }, retryable) } From ca6c4cd6af90d1920aa0b047474908c307946cdc Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 13:27:45 -0400 Subject: [PATCH 4/7] merge existing data with new token --- pkg/cluster/acrtoken.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index 88cceb5c1f0..15a7087a5cf 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -155,7 +155,8 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker }) } - openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson + mergedPullSecretData, _, err := pullsecret.Merge(string(openshiftConfigSecret.Data[corev1.DockerConfigJsonKey]), string(encodedDockerConfigJson)) + openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = []byte(mergedPullSecretData) return retryOperation(func() error { _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, openshiftConfigSecret, metav1.UpdateOptions{}) From aeea7eb6e0dc7e1ccf8e6471696e85de1821c305 Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 14:38:34 -0400 Subject: [PATCH 5/7] continue forward if openshiftConfigSecret is missing --- pkg/cluster/acrtoken.go | 2 +- pkg/util/clusterdata/worker_profile.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index 15a7087a5cf..7c5ebdc4896 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -125,7 +125,7 @@ func (m *manager) rotateACRTokenPassword(ctx context.Context) error { func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDockerConfigJson []byte) error { openshiftConfigSecret, err := m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Get(ctx, pullSecretName.Name, metav1.GetOptions{}) - if err != nil { + if err != nil && !kerrors.IsNotFound(err) { return err } diff --git a/pkg/util/clusterdata/worker_profile.go b/pkg/util/clusterdata/worker_profile.go index 455bce06888..e69d04a2710 100644 --- a/pkg/util/clusterdata/worker_profile.go +++ b/pkg/util/clusterdata/worker_profile.go @@ -103,6 +103,7 @@ func (ce machineClientEnricher) Enrich( defer oc.Lock.Unlock() oc.Properties.WorkerProfilesStatus = workerProfiles + return nil } From 6750ea5b36769531eda63648e0ef8c55c60f3595 Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 14:57:12 -0400 Subject: [PATCH 6/7] attempts to merge original and new even when recreating --- pkg/cluster/acrtoken.go | 14 ++++++++++++++ pkg/util/clusterdata/worker_profile.go | 1 - 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index 7c5ebdc4896..951c229100d 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -149,12 +149,26 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker if err != nil && !kerrors.IsNotFound(err) { return err } + + // attempt to merge if we can, defaults to the created pull secret + if openshiftConfigSecret != nil && openshiftConfigSecret.Data != nil { + previousConfigData, previousConfigDataExists := openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] + if previousConfigDataExists { + mergedPullSecretData, _, err := pullsecret.Merge(string(previousConfigData), string(encodedDockerConfigJson)) + if err == nil { + recreatedSecret.Data[corev1.DockerConfigJsonKey] = []byte(mergedPullSecretData) + } else { + m.log.Error("Could not merge openshift config pull secret, overriding with new acr token", err) + } + } + } return retryOperation(func() error { _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Create(ctx, recreatedSecret, metav1.CreateOptions{}) return err }) } + // update flow mergedPullSecretData, _, err := pullsecret.Merge(string(openshiftConfigSecret.Data[corev1.DockerConfigJsonKey]), string(encodedDockerConfigJson)) openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = []byte(mergedPullSecretData) diff --git a/pkg/util/clusterdata/worker_profile.go b/pkg/util/clusterdata/worker_profile.go index e69d04a2710..455bce06888 100644 --- a/pkg/util/clusterdata/worker_profile.go +++ b/pkg/util/clusterdata/worker_profile.go @@ -103,7 +103,6 @@ func (ce machineClientEnricher) Enrich( defer oc.Lock.Unlock() oc.Properties.WorkerProfilesStatus = workerProfiles - return nil } From 33de1ab9219f7c4f905efcbb493fedce3f7597d1 Mon Sep 17 00:00:00 2001 From: Spencer Amann Date: Thu, 21 Sep 2023 15:04:39 -0400 Subject: [PATCH 7/7] fallback to new pull secret if we cant merge --- pkg/cluster/acrtoken.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/cluster/acrtoken.go b/pkg/cluster/acrtoken.go index 951c229100d..b039771d6a8 100644 --- a/pkg/cluster/acrtoken.go +++ b/pkg/cluster/acrtoken.go @@ -170,7 +170,11 @@ func (m *manager) rotateOpenShiftConfigSecret(ctx context.Context, encodedDocker // update flow mergedPullSecretData, _, err := pullsecret.Merge(string(openshiftConfigSecret.Data[corev1.DockerConfigJsonKey]), string(encodedDockerConfigJson)) - openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = []byte(mergedPullSecretData) + if err == nil { + openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = []byte(mergedPullSecretData) + } else { + openshiftConfigSecret.Data[corev1.DockerConfigJsonKey] = encodedDockerConfigJson + } return retryOperation(func() error { _, err = m.kubernetescli.CoreV1().Secrets(pullSecretName.Namespace).Update(ctx, openshiftConfigSecret, metav1.UpdateOptions{})