Skip to content

Commit

Permalink
Fix credential rotation failure (#3113)
Browse files Browse the repository at this point in the history 
* recreate azure-credentials if it's missing
* use apply instead of create/update
  • Loading branch information
@dem4gus
dem4gus authored Aug 28, 2023
1 parent 30c99ae commit bd89261
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 48 deletions.
56 changes: 18 additions & 38 deletions pkg/cluster/clusterserviceprincipal.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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",
Expand Down
81 changes: 71 additions & 10 deletions pkg/cluster/clusterserviceprincipal_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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) {
Expand All @@ -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
}

0 comments on commit bd89261

Please sign in to comment.