az aro update CredentialsRequest hotfix (#3325)

* If the CredentialsRequest isn't found, retry until timeout instead of immediately erroring out

* `ensureCredentialsRequest` upon every `az aro update`

* Add an E2E test for the `az aro update` scenario where the ARO
operator's CredentialsRequest has been deleted

pkg/cluster/arooperator.go

@@ -61,6 +61,10 @@ func (m *manager) ensureAROOperatorRunningDesiredVersion(ctx context.Context) (b
 	return true, nil
 }
 
+func (m *manager) ensureCredentialsRequest(ctx context.Context) error {
+	return m.aroOperatorDeployer.CreateOrUpdateCredentialsRequest(ctx)
+}
+
 func (m *manager) renewMDSDCertificate(ctx context.Context) error {
 	return m.aroOperatorDeployer.RenewMDSDCertificate(ctx)
 }

pkg/cluster/condition.go

@@ -11,6 +11,7 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	consoleapi "github.com/openshift/console-operator/pkg/api"
 	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -101,7 +102,7 @@ func isOperatorAvailable(operator *configv1.ClusterOperator) bool {
 // is true if the CredentialsRequest has been reconciled within the past 5 minutes.
 // Checking for a change to the lastSyncCloudCredsSecretResourceVersion attribute of the CredentialRequest's status would be a neater way of checking
 // whether it was reconciled, but we would would have to save the value prior to updating the kube-system/azure-credentials Secret so that we'd have
-// and old value to compare to.
+// an old value to compare to.
 func (m *manager) aroCredentialsRequestReconciled(ctx context.Context) (bool, error) {
 	// If the CSP hasn't been updated, the CredentialsRequest does not need to be reconciled.
 	secret, err := m.servicePrincipalUpdated(ctx)
@@ -113,6 +114,11 @@ func (m *manager) aroCredentialsRequestReconciled(ctx context.Context) (bool, er
 
 	u, err := m.dynamiccli.Resource(CredentialsRequestGroupVersionResource).Namespace("openshift-cloud-credential-operator").Get(ctx, "openshift-azure-operator", metav1.GetOptions{})
 	if err != nil {
+		// If the CredentialsRequest is not found, it may have just recently been reconciled.
+		// Return nil to retry until we hit the condition timeout.
+		if kerrors.IsNotFound(err) {
+			return false, nil
+		}
 		return false, err
 	}
 

pkg/cluster/condition_test.go

@@ -5,6 +5,7 @@ package cluster
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -18,9 +19,11 @@ import (
 	"github.com/sirupsen/logrus"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
 	dynamicfake "k8s.io/client-go/dynamic/fake"
 	"k8s.io/client-go/kubernetes/fake"
 	"k8s.io/client-go/kubernetes/scheme"
+	ktesting "k8s.io/client-go/testing"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/env"
@@ -341,7 +344,7 @@ func TestAroCredentialsRequestReconciled(t *testing.T) {
 			want: true,
 		},
 		{
-			name: "Encounter error getting CredentialsRequest",
+			name: "CredentialsRequest not found",
 			kubernetescli: func() *fake.Clientset {
 				secret := getFakeAROSecret("aadClientId", "aadClientSecret")
 				return fake.NewSimpleClientset(&secret)
@@ -353,8 +356,27 @@ func TestAroCredentialsRequestReconciled(t *testing.T) {
 				ClientID:     "aadClientId",
 				ClientSecret: "aadClientSecretNew",
 			},
+			want: false,
+		},
+		{
+			name: "Encounter some other error getting the CredentialsRequest",
+			kubernetescli: func() *fake.Clientset {
+				secret := getFakeAROSecret("aadClientId", "aadClientSecret")
+				return fake.NewSimpleClientset(&secret)
+			},
+			dynamiccli: func() *dynamicfake.FakeDynamicClient {
+				dynamiccli := dynamicfake.NewSimpleDynamicClient(scheme.Scheme)
+				dynamiccli.PrependReactor("get", "*", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) {
+					return true, &cloudcredentialv1.CredentialsRequest{}, errors.New("Couldn't get CredentialsRequest for arbitrary reason")
+				})
+				return dynamiccli
+			},
+			spp: api.ServicePrincipalProfile{
+				ClientID:     "aadClientId",
+				ClientSecret: "aadClientSecretNew",
+			},
 			want:       false,
-			wantErrMsg: `credentialsrequests.cloudcredential.openshift.io "openshift-azure-operator" not found`,
+			wantErrMsg: "Couldn't get CredentialsRequest for arbitrary reason",
 		},
 		{
 			name: "CredentialsRequest is missing status.lastSyncTimestamp",

pkg/cluster/install.go

@@ -200,6 +200,7 @@ func (m *manager) Update(ctx context.Context) error {
 		steps.Action(m.configureAPIServerCertificate),
 		steps.Action(m.configureIngressCertificate),
 		steps.Action(m.renewMDSDCertificate),
+		steps.Action(m.ensureCredentialsRequest),
 		steps.Action(m.updateOpenShiftSecret),
 		steps.Condition(m.aroCredentialsRequestReconciled, 3*time.Minute, true),
 		steps.Action(m.updateAROSecret),

pkg/operator/deploy/deploy.go

@@ -50,6 +50,7 @@ var embeddedFiles embed.FS
 
 type Operator interface {
 	CreateOrUpdate(context.Context) error
+	CreateOrUpdateCredentialsRequest(context.Context) error
 	IsReady(context.Context) (bool, error)
 	Restart(context.Context, []string) error
 	IsRunningDesiredVersion(context.Context) (bool, error)
@@ -368,6 +369,28 @@ func (o *operator) CreateOrUpdate(ctx context.Context) error {
 	return nil
 }
 
+// CreateOrUpdateCredentialsRequest just creates/updates the ARO operator's CredentialsRequest
+// rather than doing all of the operator's associated Kubernetes resources.
+func (o *operator) CreateOrUpdateCredentialsRequest(ctx context.Context) error {
+	templ, err := template.ParseFS(embeddedFiles, "staticresources/credentialsrequest.yaml")
+	if err != nil {
+		return err
+	}
+
+	buff := &bytes.Buffer{}
+	err = templ.Execute(buff, nil)
+	if err != nil {
+		return err
+	}
+
+	crUnstructured, err := dynamichelper.DecodeUnstructured(buff.Bytes())
+	if err != nil {
+		return err
+	}
+
+	return o.dh.Ensure(ctx, crUnstructured)
+}
+
 func (o *operator) RenewMDSDCertificate(ctx context.Context) error {
 	key, cert := o.env.ClusterGenevaLoggingSecret()
 	gcsKeyBytes, err := utilpem.Encode(key)

test/e2e/setup.go

@@ -75,6 +75,7 @@ type clientSet struct {
 	HiveRestConfig     *rest.Config
 	Monitoring         monitoringclient.Interface
 	Kubernetes         kubernetes.Interface
+	Client             client.Client
 	MachineAPI         machineclient.Interface
 	MachineConfig      mcoclient.Interface
 	AROClusters        aroclient.Interface
@@ -277,6 +278,11 @@ func newClientSet(ctx context.Context) (*clientSet, error) {
 		return nil, err
 	}
 
+	controllerRuntimeClient, err := client.New(restconfig, client.Options{})
+	if err != nil {
+		return nil, err
+	}
+
 	monitoring, err := monitoringclient.NewForConfig(restconfig)
 	if err != nil {
 		return nil, err
@@ -358,6 +364,7 @@ func newClientSet(ctx context.Context) (*clientSet, error) {
 		RestConfig:         restconfig,
 		HiveRestConfig:     hiveRestConfig,
 		Kubernetes:         cli,
+		Client:             controllerRuntimeClient,
 		Monitoring:         monitoring,
 		MachineAPI:         machineapicli,
 		MachineConfig:      mcocli,

test/e2e/update.go

@@ -13,14 +13,42 @@ import (
 
 	mgmtnetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2020-08-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
+	cloudcredentialv1 "github.com/openshift/cloud-credential-operator/pkg/apis/cloudcredential/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 
 	mgmtredhatopenshift20220904 "github.com/Azure/ARO-RP/pkg/client/services/redhatopenshift/mgmt/2022-09-04/redhatopenshift"
 	mgmtredhatopenshift20230701preview "github.com/Azure/ARO-RP/pkg/client/services/redhatopenshift/mgmt/2023-07-01-preview/redhatopenshift"
 	"github.com/Azure/ARO-RP/pkg/util/stringutils"
 )
 
 var _ = Describe("Update clusters", func() {
+	It("must replace the ARO operator's CredentialsRequest if it has been deleted", func(ctx context.Context) {
+		crNamespacedName := types.NamespacedName{
+			Namespace: "openshift-cloud-credential-operator",
+			Name:      "openshift-azure-operator",
+		}
+
+		By("deleting the CredentialsRequest")
+		cr := &cloudcredentialv1.CredentialsRequest{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "openshift-cloud-credential-operator",
+				Name:      "openshift-azure-operator",
+			},
+		}
+		err := clients.Client.Delete(ctx, cr)
+		Expect(err).NotTo(HaveOccurred())
+
+		By("sending the PATCH request to update the cluster")
+		err = clients.OpenshiftClusters.UpdateAndWait(ctx, vnetResourceGroup, clusterName, mgmtredhatopenshift20220904.OpenShiftClusterUpdate{})
+		Expect(err).NotTo(HaveOccurred())
+
+		By("checking that the CredentialsRequest has been recreated")
+		cr = &cloudcredentialv1.CredentialsRequest{}
+		err = clients.Client.Get(ctx, crNamespacedName, cr)
+		Expect(err).NotTo(HaveOccurred())
+	})
+
 	It("must restart the aro-operator-master Deployment", func(ctx context.Context) {
 		By("saving the current revision of the aro-operator-master Deployment")
 		d, err := clients.Kubernetes.AppsV1().Deployments("openshift-azure-operator").Get(ctx, "aro-operator-master", metav1.GetOptions{})