diff --git a/pkg/cluster/arooperator.go b/pkg/cluster/arooperator.go index f8b3ef7a613..7afbb5b3c9d 100644 --- a/pkg/cluster/arooperator.go +++ b/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) } diff --git a/pkg/cluster/condition.go b/pkg/cluster/condition.go index a617403a283..5a4db152227 100644 --- a/pkg/cluster/condition.go +++ b/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 } diff --git a/pkg/cluster/condition_test.go b/pkg/cluster/condition_test.go index 412f9a9b9ba..51de91f4440 100644 --- a/pkg/cluster/condition_test.go +++ b/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", diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go index c08a823601a..f8195c99d2c 100644 --- a/pkg/cluster/install.go +++ b/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), diff --git a/pkg/operator/deploy/deploy.go b/pkg/operator/deploy/deploy.go index 59ebe8dd424..f41294e2780 100644 --- a/pkg/operator/deploy/deploy.go +++ b/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) diff --git a/pkg/util/mocks/operator/deploy/deploy.go b/pkg/util/mocks/operator/deploy/deploy.go index df12bfbb3b8..99ad4f2243b 100644 --- a/pkg/util/mocks/operator/deploy/deploy.go +++ b/pkg/util/mocks/operator/deploy/deploy.go @@ -48,6 +48,20 @@ func (mr *MockOperatorMockRecorder) CreateOrUpdate(arg0 interface{}) *gomock.Cal return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdate", reflect.TypeOf((*MockOperator)(nil).CreateOrUpdate), arg0) } +// CreateOrUpdateCredentialsRequest mocks base method. +func (m *MockOperator) CreateOrUpdateCredentialsRequest(arg0 context.Context) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateOrUpdateCredentialsRequest", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// CreateOrUpdateCredentialsRequest indicates an expected call of CreateOrUpdateCredentialsRequest. +func (mr *MockOperatorMockRecorder) CreateOrUpdateCredentialsRequest(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateOrUpdateCredentialsRequest", reflect.TypeOf((*MockOperator)(nil).CreateOrUpdateCredentialsRequest), arg0) +} + // IsReady mocks base method. func (m *MockOperator) IsReady(arg0 context.Context) (bool, error) { m.ctrl.T.Helper() diff --git a/test/e2e/setup.go b/test/e2e/setup.go index ded80fd8326..8d4840c4f59 100644 --- a/test/e2e/setup.go +++ b/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, diff --git a/test/e2e/update.go b/test/e2e/update.go index 123715deab3..b5ae061a678 100644 --- a/test/e2e/update.go +++ b/test/e2e/update.go @@ -13,7 +13,9 @@ 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" @@ -21,6 +23,32 @@ import ( ) 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{})