From bd8926136e13ab7814b93ec566e259fbfd2b3189 Mon Sep 17 00:00:00 2001 From: Jason Healy Date: Mon, 28 Aug 2023 15:23:42 -0400 Subject: [PATCH] Fix credential rotation failure (#3113) * recreate azure-credentials if it's missing * use apply instead of create/update --- pkg/cluster/clusterserviceprincipal.go | 56 +++++--------- pkg/cluster/clusterserviceprincipal_test.go | 81 ++++++++++++++++++--- 2 files changed, 89 insertions(+), 48 deletions(-) diff --git a/pkg/cluster/clusterserviceprincipal.go b/pkg/cluster/clusterserviceprincipal.go index 30ff6b2117d..bdcac38065a 100644 --- a/pkg/cluster/clusterserviceprincipal.go +++ b/pkg/cluster/clusterserviceprincipal.go @@ -12,9 +12,11 @@ import ( "github.com/ghodss/yaml" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + applyv1 "k8s.io/client-go/applyconfigurations/core/v1" "k8s.io/client-go/util/retry" "github.com/Azure/ARO-RP/pkg/util/arm" + "github.com/Azure/ARO-RP/pkg/util/clusterauthorizer" "github.com/Azure/ARO-RP/pkg/util/rbac" "github.com/Azure/ARO-RP/pkg/util/stringutils" ) @@ -173,50 +175,28 @@ func (m *manager) updateAROSecret(ctx context.Context) error { } func (m *manager) updateOpenShiftSecret(ctx context.Context) error { - var changed bool + resourceGroupID := m.doc.OpenShiftCluster.Properties.ClusterProfile.ResourceGroupID spp := m.doc.OpenShiftCluster.Properties.ServicePrincipalProfile - err := retry.RetryOnConflict(retry.DefaultRetry, func() error { - //data: - // azure_client_id: secret_id - // azure_client_secret: secret_value - // azure_tenant_id: tenant_id - secret, err := m.kubernetescli.CoreV1().Secrets("kube-system").Get(ctx, "azure-credentials", metav1.GetOptions{}) - if err != nil { - return err - } - - if string(secret.Data["azure_client_id"]) != spp.ClientID { - secret.Data["azure_client_id"] = []byte(spp.ClientID) - changed = true - } - - if string(secret.Data["azure_client_secret"]) != string(spp.ClientSecret) { - secret.Data["azure_client_secret"] = []byte(spp.ClientSecret) - changed = true - } - - if string(secret.Data["azure_tenant_id"]) != m.subscriptionDoc.Subscription.Properties.TenantID { - secret.Data["azure_tenant_id"] = []byte(m.subscriptionDoc.Subscription.Properties.TenantID) - changed = true - } + //data: + // azure_client_id: secret_id + // azure_client_secret: secret_value + // azure_tenant_id: tenant_id + desiredData := map[string][]byte{ + "azure_subscription_id": []byte(m.subscriptionDoc.ID), + "azure_resource_prefix": []byte(m.doc.OpenShiftCluster.Properties.InfraID), + "azure_resourcegroup": []byte(resourceGroupID[strings.LastIndex(resourceGroupID, "/")+1:]), + "azure_region": []byte(m.doc.OpenShiftCluster.Location), + "azure_client_id": []byte(spp.ClientID), + "azure_client_secret": []byte(spp.ClientSecret), + "azure_tenant_id": []byte(m.subscriptionDoc.Subscription.Properties.TenantID), + } - if changed { - _, err = m.kubernetescli.CoreV1().Secrets("kube-system").Update(ctx, secret, metav1.UpdateOptions{}) - if err != nil { - return err - } - } - return nil - }) + secretApplyConfig := applyv1.Secret(clusterauthorizer.AzureCredentialSecretName, clusterauthorizer.AzureCredentialSecretNameSpace).WithData(desiredData) + _, err := m.kubernetescli.CoreV1().Secrets(clusterauthorizer.AzureCredentialSecretNameSpace).Apply(ctx, secretApplyConfig, metav1.ApplyOptions{FieldManager: "aro-rp", Force: true}) if err != nil { return err } - // return early if not changed - if !changed { - return nil - } - // restart cloud credentials operator to trigger rotation err = m.kubernetescli.CoreV1().Pods("openshift-cloud-credential-operator").DeleteCollection(ctx, metav1.DeleteOptions{}, metav1.ListOptions{ LabelSelector: "app=cloud-credential-operator", diff --git a/pkg/cluster/clusterserviceprincipal_test.go b/pkg/cluster/clusterserviceprincipal_test.go index d3ffe6ee905..dfaab85bee1 100644 --- a/pkg/cluster/clusterserviceprincipal_test.go +++ b/pkg/cluster/clusterserviceprincipal_test.go @@ -18,8 +18,12 @@ import ( operatorfake "github.com/openshift/client-go/operator/clientset/versioned/fake" "github.com/sirupsen/logrus" 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/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/fake" + ktesting "k8s.io/client-go/testing" "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/util/arm" @@ -297,9 +301,13 @@ func getFakeOpenShiftSecret() corev1.Secret { name := "azure-credentials" namespace := "kube-system" data := map[string][]byte{ - "azure_client_id": []byte("azure_client_id_value"), - "azure_client_secret": []byte("azure_client_secret_value"), - "azure_tenant_id": []byte("azure_tenant_id_value"), + "azure_subscription_id": {}, + "azure_resource_prefix": {}, + "azure_resourcegroup": {}, + "azure_region": {}, + "azure_client_id": []byte("azure_client_id_value"), + "azure_client_secret": []byte("azure_client_secret_value"), + "azure_tenant_id": []byte("azure_tenant_id_value"), } return corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -327,7 +335,7 @@ func TestUpdateOpenShiftSecret(t *testing.T) { name: "noop", kubernetescli: func() *fake.Clientset { secret := getFakeOpenShiftSecret() - return fake.NewSimpleClientset(&secret) + return cliWithApply(&secret) }, doc: api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ @@ -352,7 +360,7 @@ func TestUpdateOpenShiftSecret(t *testing.T) { name: "update secret", kubernetescli: func() *fake.Clientset { secret := getFakeOpenShiftSecret() - return fake.NewSimpleClientset(&secret) + return cliWithApply(&secret) }, doc: api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ @@ -379,7 +387,7 @@ func TestUpdateOpenShiftSecret(t *testing.T) { name: "update tenant", kubernetescli: func() *fake.Clientset { secret := getFakeOpenShiftSecret() - return fake.NewSimpleClientset(&secret) + return cliWithApply(&secret) }, doc: api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ @@ -403,16 +411,28 @@ func TestUpdateOpenShiftSecret(t *testing.T) { }, }, { - name: "not found - no fail", + name: "recreate secret when not found", kubernetescli: func() *fake.Clientset { - return fake.NewSimpleClientset() + return cliWithApply() }, doc: api.OpenShiftCluster{ Properties: api.OpenShiftClusterProperties{ - ServicePrincipalProfile: api.ServicePrincipalProfile{}, + ServicePrincipalProfile: api.ServicePrincipalProfile{ + ClientID: "azure_client_id_value", + ClientSecret: "azure_client_secret_value", + }, + }, + }, + subscriptionDoc: api.SubscriptionDocument{ + Subscription: &api.Subscription{ + Properties: &api.SubscriptionProperties{ + TenantID: "azure_tenant_id_value", + }, }, }, - wantErr: `secrets "azure-credentials" not found`, + expect: func() corev1.Secret { + return getFakeOpenShiftSecret() + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -438,3 +458,44 @@ func TestUpdateOpenShiftSecret(t *testing.T) { }) } } + +// The current kubernetes testing client does not propery handle Apply actions so we reimplement it here. +// See https://github.com/kubernetes/client-go/issues/1184 for more details. +func cliWithApply(object ...runtime.Object) *fake.Clientset { + fc := fake.NewSimpleClientset(object...) + fc.PrependReactor("patch", "secrets", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + pa := action.(ktesting.PatchAction) + if pa.GetPatchType() == types.ApplyPatchType { + // Apply patches are supposed to upsert, but fake client fails if the object doesn't exist, + // if an apply patch occurs for a secret that doesn't yet exist, create it. + // However, we already hold the fakeclient lock, so we can't use the front door. + rfunc := ktesting.ObjectReaction(fc.Tracker()) + _, obj, err := rfunc( + ktesting.NewGetAction(pa.GetResource(), pa.GetNamespace(), pa.GetName()), + ) + if kerrors.IsNotFound(err) || obj == nil { + _, _, _ = rfunc( + ktesting.NewCreateAction( + pa.GetResource(), + pa.GetNamespace(), + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: pa.GetName(), + Namespace: pa.GetNamespace(), + }, + }, + ), + ) + } + return rfunc(ktesting.NewPatchAction( + pa.GetResource(), + pa.GetNamespace(), + pa.GetName(), + types.StrategicMergePatchType, + pa.GetPatch())) + } + return false, nil, nil + }, + ) + return fc +}