From 82f693a19a6b9d6d9f3c4a1e413ab21fe7d53ee3 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 17 Apr 2025 10:29:57 +0100 Subject: [PATCH 1/5] trivial: Use errors.New rather than fmt.Errorf Where fmt.Errorf is called with a single string parameter, it is faster and arguably more correct to call errors.New instead. Signed-off-by: Stephen Finucane --- pkg/operator/secretannotator/openstack/reconciler.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/operator/secretannotator/openstack/reconciler.go b/pkg/operator/secretannotator/openstack/reconciler.go index 9cb3d70e0f..eb8a5e2eaa 100644 --- a/pkg/operator/secretannotator/openstack/reconciler.go +++ b/pkg/operator/secretannotator/openstack/reconciler.go @@ -18,6 +18,7 @@ package openstack import ( "context" + "errors" "fmt" "time" @@ -134,7 +135,7 @@ func (r *ReconcileCloudCredSecret) Reconcile(ctx context.Context, request reconc } if conflict { r.Logger.Error("configuration conflict between legacy configmap and operator config") - return reconcile.Result{}, fmt.Errorf("configuration conflict") + return reconcile.Result{}, errors.New("configuration conflict") } if mode == operatorv1.CloudCredentialsModeManual { r.Logger.Info("operator in disabled / manual mode") @@ -146,7 +147,7 @@ func (r *ReconcileCloudCredSecret) Reconcile(ctx context.Context, request reconc default: const msg = "OpenStack only supports Passthrough mode" r.Logger.Error(msg) - return reconcile.Result{}, fmt.Errorf(msg) + return reconcile.Result{}, errors.New(msg) } secret := &corev1.Secret{} From 852f4f6f1e044f013b1918d7b5024e0a8b04bd19 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Thu, 17 Apr 2025 10:27:00 +0100 Subject: [PATCH 2/5] openstack: Sync cacert from CCM to root credential There's some rework needed around CCM and the docs to get users to start using the new location of the CA cert. That is not going to happen in 4.19, so for now we opt to simply sync from the old place to the new place and leave the existing docs in place. In a future release, we can fully remove the old place (with a release note) and remove this syncer. Signed-off-by: Stephen Finucane --- .../secretannotator/openstack/reconciler.go | 19 ++++++++++++++++++- .../openstack/reconciler_test.go | 12 ++++++++++-- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/pkg/operator/secretannotator/openstack/reconciler.go b/pkg/operator/secretannotator/openstack/reconciler.go index eb8a5e2eaa..e677a13278 100644 --- a/pkg/operator/secretannotator/openstack/reconciler.go +++ b/pkg/operator/secretannotator/openstack/reconciler.go @@ -163,13 +163,30 @@ func (r *ReconcileCloudCredSecret) Reconcile(ctx context.Context, request reconc return reconcile.Result{}, err } + // Sync the cacert from its legacy location (the 'ca-bundle.pem' key of the + // 'openshift-config / cloud-provider-config' CM) to the new place, if present. + // TODO(stephenfin): Remove this syncer in a future release once CCM no longer + // relies on the legacy place during bootstrapping. + config := &corev1.ConfigMap{} + err = r.RootCredClient.Get(context.Background(), types.NamespacedName{Namespace: "openshift-config", Name: "cloud-provider-config"}, config) + if err != nil { + r.Logger.Debugf("cloud provider config not found: %v", err) + return reconcile.Result{}, err + } + + cacertUpdated := false + if ccmCACert := config.Data["ca-bundle.pem"]; ccmCACert != cacert { + cacert = ccmCACert + cacertUpdated = true + } + clouds, cloudsUpdated, err := r.fixInvalidCACertFile(clouds) if err != nil { r.Logger.WithError(err).Error("errored checking clouds.yaml") return reconcile.Result{}, err } - if cloudsUpdated { + if cloudsUpdated || cacertUpdated { openstack.SetRootCloudCredentialsSecretData(secret, clouds, cacert) err := r.RootCredClient.Update(context.TODO(), secret) if err != nil { diff --git a/pkg/operator/secretannotator/openstack/reconciler_test.go b/pkg/operator/secretannotator/openstack/reconciler_test.go index e738918037..32aab010c0 100644 --- a/pkg/operator/secretannotator/openstack/reconciler_test.go +++ b/pkg/operator/secretannotator/openstack/reconciler_test.go @@ -108,6 +108,14 @@ func TestReconcileCloudCredSecret_Reconcile(t *testing.T) { }, } + ccmConfig := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-provider-config", + Namespace: "openshift-config", + }, + Data: map[string]string{}, + } + /* Test parsing of CCO configuration and the resulting annotation of the root secret. Most of this is boilerplate behaviour. @@ -181,7 +189,7 @@ func TestReconcileCloudCredSecret_Reconcile(t *testing.T) { secret := testSecret(fmt.Sprintf(cloudsWithCACert, correctCACertFile)) existing := append(tc.existing, infra, testOperatorConfig(tc.mode)) fakeClient := fake.NewClientBuilder().WithRuntimeObjects(existing...).Build() - fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret).Build() + fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret, ccmConfig).Build() r := &ReconcileCloudCredSecret{ Client: fakeClient, @@ -270,7 +278,7 @@ func TestReconcileCloudCredSecret_Reconcile(t *testing.T) { t.Run(tc.name, func(t *testing.T) { secret := testSecret(tc.cloudsYAML) fakeClient := fake.NewClientBuilder().WithRuntimeObjects(infra, passthrough).Build() - fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret).Build() + fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret, ccmConfig).Build() t.Logf("clouds.yaml: %s", tc.cloudsYAML) r := &ReconcileCloudCredSecret{ From d90783dbc3e2ff894bd56f2b948907f8184ef346 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 21 May 2025 12:46:18 +0100 Subject: [PATCH 3/5] manifests: Rename existing Role, RoleBinding manifests Reflect their respective namespaces. Signed-off-by: Stephen Finucane --- ...nfig-role-binding.yaml => 01-config-managed-role-binding.yaml} | 0 manifests/{01-config-role.yaml => 01-config-managed-role.yaml} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename manifests/{01-config-role-binding.yaml => 01-config-managed-role-binding.yaml} (100%) rename manifests/{01-config-role.yaml => 01-config-managed-role.yaml} (100%) diff --git a/manifests/01-config-role-binding.yaml b/manifests/01-config-managed-role-binding.yaml similarity index 100% rename from manifests/01-config-role-binding.yaml rename to manifests/01-config-managed-role-binding.yaml diff --git a/manifests/01-config-role.yaml b/manifests/01-config-managed-role.yaml similarity index 100% rename from manifests/01-config-role.yaml rename to manifests/01-config-managed-role.yaml From 633534a3e2415156f5a555118360a3a5d4931f50 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 21 May 2025 12:48:46 +0100 Subject: [PATCH 4/5] manifests: Add Role, RoleBinding for openshift-config/cloud-provider-config CM Signed-off-by: Stephen Finucane --- manifests/01-config-role-binding.yaml | 17 +++++++++++++++++ manifests/01-config-role.yaml | 20 ++++++++++++++++++++ 2 files changed, 37 insertions(+) create mode 100644 manifests/01-config-role-binding.yaml create mode 100644 manifests/01-config-role.yaml diff --git a/manifests/01-config-role-binding.yaml b/manifests/01-config-role-binding.yaml new file mode 100644 index 0000000000..3f79cbbfa6 --- /dev/null +++ b/manifests/01-config-role-binding.yaml @@ -0,0 +1,17 @@ +kind: RoleBinding +apiVersion: rbac.authorization.k8s.io/v1 +metadata: + name: cloud-credential-operator + namespace: openshift-config + annotations: + capability.openshift.io/name: CloudCredential + include.release.openshift.io/ibm-cloud-managed: "true" + include.release.openshift.io/self-managed-high-availability: "true" +subjects: +- kind: ServiceAccount + name: cloud-credential-operator + namespace: openshift-cloud-credential-operator +roleRef: + kind: Role + apiGroup: rbac.authorization.k8s.io + name: cloud-credential-operator-role diff --git a/manifests/01-config-role.yaml b/manifests/01-config-role.yaml new file mode 100644 index 0000000000..a914914416 --- /dev/null +++ b/manifests/01-config-role.yaml @@ -0,0 +1,20 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: cloud-credential-operator-role + namespace: openshift-config + annotations: + capability.openshift.io/name: CloudCredential + include.release.openshift.io/ibm-cloud-managed: "true" + include.release.openshift.io/self-managed-high-availability: "true" +rules: +- apiGroups: + - "" + resources: + - configmaps + resourceNames: + - cloud-provider-config + verbs: + - get + - list + - watch From d5af20beb55c046da0d8cc27d958657a4b3b3eea Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Wed, 28 May 2025 14:55:25 +0100 Subject: [PATCH 5/5] openstack: Use a "live client" for fetching config map Unlike the other clients, this one does not use caching (which requires the operator have cluster-wide access to config maps). This is the same thing done for AWS. Signed-off-by: Stephen Finucane --- pkg/operator/secretannotator/openstack/reconciler.go | 6 +++++- pkg/operator/secretannotator/openstack/reconciler_test.go | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/operator/secretannotator/openstack/reconciler.go b/pkg/operator/secretannotator/openstack/reconciler.go index e677a13278..fed4db31a6 100644 --- a/pkg/operator/secretannotator/openstack/reconciler.go +++ b/pkg/operator/secretannotator/openstack/reconciler.go @@ -53,6 +53,7 @@ func NewReconciler(c client.Client, mgr manager.Manager) reconcile.Reconciler { r := &ReconcileCloudCredSecret{ Client: c, RootCredClient: mgr.GetClient(), + LiveClient: utils.LiveClient(mgr), Logger: log.WithField("controller", constants.SecretAnnotatorControllerName), } @@ -106,6 +107,7 @@ var _ reconcile.Reconciler = &ReconcileCloudCredSecret{} type ReconcileCloudCredSecret struct { Client client.Client RootCredClient client.Client + LiveClient client.Client Logger log.FieldLogger } @@ -150,6 +152,8 @@ func (r *ReconcileCloudCredSecret) Reconcile(ctx context.Context, request reconc return reconcile.Result{}, errors.New(msg) } + r.Logger.Info("verifying clouds.yaml and syncing cacert (if any)") + secret := &corev1.Secret{} err = r.RootCredClient.Get(context.Background(), request.NamespacedName, secret) if err != nil { @@ -168,7 +172,7 @@ func (r *ReconcileCloudCredSecret) Reconcile(ctx context.Context, request reconc // TODO(stephenfin): Remove this syncer in a future release once CCM no longer // relies on the legacy place during bootstrapping. config := &corev1.ConfigMap{} - err = r.RootCredClient.Get(context.Background(), types.NamespacedName{Namespace: "openshift-config", Name: "cloud-provider-config"}, config) + err = r.LiveClient.Get(context.Background(), types.NamespacedName{Namespace: "openshift-config", Name: "cloud-provider-config"}, config) if err != nil { r.Logger.Debugf("cloud provider config not found: %v", err) return reconcile.Result{}, err diff --git a/pkg/operator/secretannotator/openstack/reconciler_test.go b/pkg/operator/secretannotator/openstack/reconciler_test.go index 32aab010c0..6d8f24083c 100644 --- a/pkg/operator/secretannotator/openstack/reconciler_test.go +++ b/pkg/operator/secretannotator/openstack/reconciler_test.go @@ -190,10 +190,12 @@ func TestReconcileCloudCredSecret_Reconcile(t *testing.T) { existing := append(tc.existing, infra, testOperatorConfig(tc.mode)) fakeClient := fake.NewClientBuilder().WithRuntimeObjects(existing...).Build() fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret, ccmConfig).Build() + fakeLiveClient := fake.NewClientBuilder().WithRuntimeObjects(ccmConfig).Build() r := &ReconcileCloudCredSecret{ Client: fakeClient, RootCredClient: fakeRootCredClient, + LiveClient: fakeLiveClient, Logger: log.WithField("controller", "testController"), } _, err := r.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{ @@ -279,11 +281,13 @@ func TestReconcileCloudCredSecret_Reconcile(t *testing.T) { secret := testSecret(tc.cloudsYAML) fakeClient := fake.NewClientBuilder().WithRuntimeObjects(infra, passthrough).Build() fakeRootCredClient := fake.NewClientBuilder().WithRuntimeObjects(secret, ccmConfig).Build() + fakeLiveClient := fake.NewClientBuilder().WithRuntimeObjects(ccmConfig).Build() t.Logf("clouds.yaml: %s", tc.cloudsYAML) r := &ReconcileCloudCredSecret{ Client: fakeClient, RootCredClient: fakeRootCredClient, + LiveClient: fakeLiveClient, Logger: log.WithField("controller", "testController"), }