[MIMO] Move cluster certificate functionality to ClientHelper (#3736)

* move over TLS applying, as well as some clienthelper work
Azure · Sep 5, 2024 · 44bc3cc · 44bc3cc
1 parent c554e98
commit 44bc3cc
Show file tree

Hide file tree

Showing 9 changed files with 105 additions and 101 deletions.
diff --git a/pkg/cluster/apply.go b/pkg/cluster/apply.go
@@ -0,0 +1,56 @@
+package cluster
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"crypto/x509"
+	"encoding/pem"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+
+	"github.com/Azure/ARO-RP/pkg/util/clienthelper"
+	"github.com/Azure/ARO-RP/pkg/util/keyvault"
+	utilpem "github.com/Azure/ARO-RP/pkg/util/pem"
+)
+
+func EnsureTLSSecretFromKeyvault(ctx context.Context, kv keyvault.Manager, ch clienthelper.Writer, target types.NamespacedName, certificateName string) error {
+	bundle, err := kv.GetSecret(ctx, certificateName)
+	if err != nil {
+		return err
+	}
+
+	key, certs, err := utilpem.Parse([]byte(*bundle.Value))
+	if err != nil {
+		return err
+	}
+
+	b, err := x509.MarshalPKCS8PrivateKey(key)
+	if err != nil {
+		return err
+	}
+
+	var cb []byte
+	for _, cert := range certs {
+		cb = append(cb, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})...)
+	}
+
+	privateKey := pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: b})
+
+	secret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      target.Name,
+			Namespace: target.Namespace,
+		},
+		Data: map[string][]byte{
+			corev1.TLSCertKey:       cb,
+			corev1.TLSPrivateKeyKey: privateKey,
+		},
+		Type: corev1.SecretTypeTLS,
+	}
+
+	return ch.Ensure(ctx, secret)
+}
diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go
@@ -23,7 +23,6 @@ import (
 	extensionsclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/cluster/graph"
@@ -43,6 +42,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network"
 	"github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/privatedns"
 	"github.com/Azure/ARO-RP/pkg/util/billing"
+	"github.com/Azure/ARO-RP/pkg/util/clienthelper"
 	"github.com/Azure/ARO-RP/pkg/util/dns"
 	"github.com/Azure/ARO-RP/pkg/util/encryption"
 	utilgraph "github.com/Azure/ARO-RP/pkg/util/graph"
@@ -105,7 +105,7 @@ type manager struct {
 	graph   graph.Manager
 	rpBlob  azblob.Manager
 
-	client           client.Client
+	ch               clienthelper.Interface
 	kubernetescli    kubernetes.Interface
 	dynamiccli       dynamic.Interface
 	extensionscli    extensionsclient.Interface

diff --git a/pkg/cluster/install.go b/pkg/cluster/install.go
@@ -27,6 +27,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/database"
 	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
 	"github.com/Azure/ARO-RP/pkg/operator/deploy"
+	"github.com/Azure/ARO-RP/pkg/util/clienthelper"
 	utilgenerics "github.com/Azure/ARO-RP/pkg/util/generics"
 	"github.com/Azure/ARO-RP/pkg/util/restconfig"
 	"github.com/Azure/ARO-RP/pkg/util/steps"
@@ -534,16 +535,18 @@ func (m *manager) initializeKubernetesClients(ctx context.Context) error {
 		return err
 	}
 
-	m.client, err = client.New(restConfig, client.Options{
+	client, err := client.New(restConfig, client.Options{
 		Mapper: mapper,
 	})
+
+	m.ch = clienthelper.NewWithClient(m.log, client)
 	return err
 }
 
 // initializeKubernetesClients initializes clients which are used
 // once the cluster is up later on in the install process.
 func (m *manager) initializeOperatorDeployer(ctx context.Context) (err error) {
-	m.aroOperatorDeployer, err = deploy.New(m.log, m.env, m.doc.OpenShiftCluster, m.arocli, m.client, m.extensionscli, m.kubernetescli, m.operatorcli, m.subscriptionDoc)
+	m.aroOperatorDeployer, err = deploy.New(m.log, m.env, m.doc.OpenShiftCluster, m.subscriptionDoc, m.arocli, m.ch, m.extensionscli, m.kubernetescli, m.operatorcli)
 	return
 }
 

diff --git a/pkg/cluster/tls.go b/pkg/cluster/tls.go
@@ -5,20 +5,16 @@ package cluster
 
 import (
 	"context"
-	"crypto/x509"
-	"encoding/pem"
 
 	configv1 "github.com/openshift/api/config/v1"
 	corev1 "k8s.io/api/core/v1"
-	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/retry"
 
 	"github.com/Azure/ARO-RP/pkg/env"
 	"github.com/Azure/ARO-RP/pkg/util/dns"
 	"github.com/Azure/ARO-RP/pkg/util/keyvault"
-	utilpem "github.com/Azure/ARO-RP/pkg/util/pem"
 )
 
 func (m *manager) createCertificates(ctx context.Context) error {
@@ -69,57 +65,6 @@ func (m *manager) createCertificates(ctx context.Context) error {
 	return nil
 }
 
-func (m *manager) ensureSecret(ctx context.Context, secrets corev1client.SecretInterface, certificateName string) error {
-	bundle, err := m.env.ClusterKeyvault().GetSecret(ctx, certificateName)
-	if err != nil {
-		return err
-	}
-
-	key, certs, err := utilpem.Parse([]byte(*bundle.Value))
-	if err != nil {
-		return err
-	}
-
-	b, err := x509.MarshalPKCS8PrivateKey(key)
-	if err != nil {
-		return err
-	}
-
-	var cb []byte
-	for _, cert := range certs {
-		cb = append(cb, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})...)
-	}
-
-	_, err = secrets.Create(ctx, &corev1.Secret{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: certificateName,
-		},
-		Data: map[string][]byte{
-			corev1.TLSCertKey:       cb,
-			corev1.TLSPrivateKeyKey: pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: b}),
-		},
-		Type: corev1.SecretTypeTLS,
-	}, metav1.CreateOptions{})
-	if kerrors.IsAlreadyExists(err) {
-		err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
-			s, err := secrets.Get(ctx, certificateName, metav1.GetOptions{})
-			if err != nil {
-				return err
-			}
-
-			s.Data = map[string][]byte{
-				corev1.TLSCertKey:       cb,
-				corev1.TLSPrivateKeyKey: pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: b}),
-			}
-			s.Type = corev1.SecretTypeTLS
-
-			_, err = secrets.Update(ctx, s, metav1.UpdateOptions{})
-			return err
-		})
-	}
-	return err
-}
-
 func (m *manager) configureAPIServerCertificate(ctx context.Context) error {
 	if m.env.FeatureIsSet(env.FeatureDisableSignedCertificates) {
 		return nil
@@ -135,7 +80,7 @@ func (m *manager) configureAPIServerCertificate(ctx context.Context) error {
 	}
 
 	for _, namespace := range []string{"openshift-config", "openshift-azure-operator"} {
-		err = m.ensureSecret(ctx, m.kubernetescli.CoreV1().Secrets(namespace), m.doc.ID+"-apiserver")
+		err = EnsureTLSSecretFromKeyvault(ctx, m.env.ClusterKeyvault(), m.ch, types.NamespacedName{Name: m.doc.ID + "-apiserver", Namespace: namespace}, m.doc.ID+"-apiserver")
 		if err != nil {
 			return err
 		}
@@ -178,7 +123,7 @@ func (m *manager) configureIngressCertificate(ctx context.Context) error {
 	}
 
 	for _, namespace := range []string{"openshift-ingress", "openshift-azure-operator"} {
-		err = m.ensureSecret(ctx, m.kubernetescli.CoreV1().Secrets(namespace), m.doc.ID+"-ingress")
+		err = EnsureTLSSecretFromKeyvault(ctx, m.env.ClusterKeyvault(), m.ch, types.NamespacedName{Namespace: namespace, Name: m.doc.ID + "-ingress"}, m.doc.ID+"-ingress")
 		if err != nil {
 			return err
 		}

diff --git a/pkg/operator/deploy/deploy.go b/pkg/operator/deploy/deploy.go
@@ -29,7 +29,6 @@ import (
 	appsv1client "k8s.io/client-go/kubernetes/typed/apps/v1"
 	corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
 	"k8s.io/client-go/util/retry"
-	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/Azure/ARO-RP/pkg/api"
@@ -39,6 +38,7 @@ import (
 	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging"
+	"github.com/Azure/ARO-RP/pkg/util/clienthelper"
 	"github.com/Azure/ARO-RP/pkg/util/dynamichelper"
 	utilkubernetes "github.com/Azure/ARO-RP/pkg/util/kubernetes"
 	utilpem "github.com/Azure/ARO-RP/pkg/util/pem"
@@ -63,20 +63,20 @@ type Operator interface {
 }
 
 type operator struct {
-	log *logrus.Entry
-	env env.Interface
-	oc  *api.OpenShiftCluster
-
-	arocli          aroclient.Interface
-	client          client.Client
-	extensionscli   extensionsclient.Interface
-	kubernetescli   kubernetes.Interface
-	operatorcli     operatorclient.Interface
-	dh              dynamichelper.Interface
+	log             *logrus.Entry
+	env             env.Interface
+	oc              *api.OpenShiftCluster
 	subscriptiondoc *api.SubscriptionDocument
+
+	arocli        aroclient.Interface
+	client        clienthelper.Interface
+	extensionscli extensionsclient.Interface
+	kubernetescli kubernetes.Interface
+	operatorcli   operatorclient.Interface
+	dh            dynamichelper.Interface
 }
 
-func New(log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, arocli aroclient.Interface, client client.Client, extensionscli extensionsclient.Interface, kubernetescli kubernetes.Interface, operatorcli operatorclient.Interface, subscriptionDoc *api.SubscriptionDocument) (Operator, error) {
+func New(log *logrus.Entry, env env.Interface, oc *api.OpenShiftCluster, subscriptionDoc *api.SubscriptionDocument, arocli aroclient.Interface, client clienthelper.Interface, extensionscli extensionsclient.Interface, kubernetescli kubernetes.Interface, operatorcli operatorclient.Interface) (Operator, error) {
 	restConfig, err := restconfig.RestConfig(env, oc)
 	if err != nil {
 		return nil, err

diff --git a/pkg/operator/deploy/deploy_test.go b/pkg/operator/deploy/deploy_test.go
@@ -12,6 +12,7 @@ import (
 	configv1 "github.com/openshift/api/config/v1"
 	operatorv1 "github.com/openshift/api/operator/v1"
 	operatorfake "github.com/openshift/client-go/operator/clientset/versioned/fake"
+	"github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -20,6 +21,7 @@ import (
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	pkgoperator "github.com/Azure/ARO-RP/pkg/operator"
+	"github.com/Azure/ARO-RP/pkg/util/clienthelper"
 	"github.com/Azure/ARO-RP/pkg/util/cmp"
 	mock_env "github.com/Azure/ARO-RP/pkg/util/mocks/env"
 	utilerror "github.com/Azure/ARO-RP/test/util/error"
@@ -233,7 +235,7 @@ func TestCreateDeploymentData(t *testing.T) {
 			o := operator{
 				oc:     oc,
 				env:    env,
-				client: ctrlfake.NewClientBuilder().WithObjects(cv).Build(),
+				client: clienthelper.NewWithClient(logrus.NewEntry(logrus.StandardLogger()), ctrlfake.NewClientBuilder().WithObjects(cv).Build()),
 			}
 
 			deploymentData, err := o.createDeploymentData(ctx)
@@ -306,7 +308,7 @@ func TestOperatorVersion(t *testing.T) {
 			o := &operator{
 				oc:     &api.OpenShiftCluster{Properties: *oc},
 				env:    _env,
-				client: ctrlfake.NewClientBuilder().WithObjects(cv).Build(),
+				client: clienthelper.NewWithClient(logrus.NewEntry(logrus.StandardLogger()), ctrlfake.NewClientBuilder().WithObjects(cv).Build()),
 			}
 
 			staticResources, err := o.createObjects(ctx)

diff --git a/pkg/operator/helpers.go b/pkg/operator/helpers.go
@@ -23,7 +23,7 @@ func GatewayEnabled(cluster *arov1alpha1.Cluster) bool {
 // ShouldUsePodSecurityStandard is an admissions controller
 // for pods which replaces pod security policies, enabled on
 // OpenShift 4.11 and up
-func ShouldUsePodSecurityStandard(ctx context.Context, client client.Client) (bool, error) {
+func ShouldUsePodSecurityStandard(ctx context.Context, client client.Reader) (bool, error) {
 	cv := &configv1.ClusterVersion{}
 
 	err := client.Get(ctx, types.NamespacedName{Name: "version"}, cv)

diff --git a/pkg/util/clienthelper/clienthelper.go b/pkg/util/clienthelper/clienthelper.go
@@ -25,7 +25,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/retry"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
@@ -34,35 +33,34 @@ import (
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
-type Interface interface {
-	EnsureDeleted(ctx context.Context, gvk schema.GroupVersionKind, key types.NamespacedName) error
+type Writer interface {
+	client.Writer
+	// Ensure applies self-contained objects to a Kubernetes API, merging
+	// client-side if required.
 	Ensure(ctx context.Context, objs ...kruntime.Object) error
-	GetOne(ctx context.Context, key types.NamespacedName, obj kruntime.Object) error
+	EnsureDeleted(ctx context.Context, gvk schema.GroupVersionKind, key types.NamespacedName) error
 }
 
-type clientHelper struct {
-	log *logrus.Entry
+type Reader interface {
+	client.Reader
+	GetOne(ctx context.Context, key types.NamespacedName, obj kruntime.Object) error
+}
 
-	client client.Client
+type Interface interface {
+	Reader
+	Writer
 }
 
-func New(log *logrus.Entry, restconfig *rest.Config) (Interface, error) {
-	mapper, err := apiutil.NewDynamicRESTMapper(restconfig, apiutil.WithLazyDiscovery)
-	if err != nil {
-		return nil, err
-	}
+type clientHelper struct {
+	client.Client
 
-	client, err := client.New(restconfig, client.Options{Mapper: mapper})
-	if err != nil {
-		return nil, err
-	}
-	return NewWithClient(log, client), nil
+	log *logrus.Entry
 }
 
 func NewWithClient(log *logrus.Entry, client client.Client) Interface {
 	return &clientHelper{
 		log:    log,
-		client: client,
+		Client: client,
 	}
 }
 
@@ -74,7 +72,7 @@ func (ch *clientHelper) EnsureDeleted(ctx context.Context, gvk schema.GroupVersi
 	a.SetGroupVersionKind(gvk)
 
 	ch.log.Infof("Delete kind %s ns %s name %s", gvk.Kind, key.Namespace, key.Name)
-	err := ch.client.Delete(ctx, a)
+	err := ch.Delete(ctx, a)
 	if kerrors.IsNotFound(err) {
 		return nil
 	}
@@ -87,7 +85,7 @@ func (ch *clientHelper) GetOne(ctx context.Context, key types.NamespacedName, ob
 		return errors.New("can't convert object")
 	}
 
-	return ch.client.Get(ctx, key, newObj)
+	return ch.Get(ctx, key, newObj)
 }
 
 // Ensure that one or more objects match their desired state.  Only update
@@ -125,10 +123,10 @@ func (ch *clientHelper) ensureOne(ctx context.Context, new kruntime.Object) erro
 			return fmt.Errorf("object of kind %s can't be made a client.Object", gvk.String())
 		}
 
-		err = ch.client.Get(ctx, client.ObjectKey{Namespace: newObj.GetNamespace(), Name: newObj.GetName()}, oldObj)
+		err = ch.Get(ctx, client.ObjectKey{Namespace: newObj.GetNamespace(), Name: newObj.GetName()}, oldObj)
 		if kerrors.IsNotFound(err) {
 			ch.log.Infof("Create %s", keyFunc(gvk.GroupKind(), newObj.GetNamespace(), newObj.GetName()))
-			return ch.client.Create(ctx, newObj)
+			return ch.Create(ctx, newObj)
 		}
 		if err != nil {
 			return err
@@ -138,7 +136,7 @@ func (ch *clientHelper) ensureOne(ctx context.Context, new kruntime.Object) erro
 			return err
 		}
 		ch.log.Infof("Update %s: %s", keyFunc(gvk.GroupKind(), candidate.GetNamespace(), candidate.GetName()), diff)
-		return ch.client.Update(ctx, candidate)
+		return ch.Update(ctx, candidate)
 	})
 }