[MIMO] Move cluster certificate functionality to ClientHelper #3736
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) | ||
|
There was a problem hiding this comment. As previously mentioned, as we just use the Ensure method of ch, maybe we could define here an interface with just this method to stick to the Interface Segregation Principle. |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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/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 Writer interface { | ||
| client.Writer | ||
| // Ensure applies self-contained objects to a Kubernetes API, merging | ||
|
There was a problem hiding this comment. I know you're not changing this in the PR, so this is not a blocker here, but we should use server-side apply instead of whatever client-side logic exists, as it's guaranteed to be a better implementation :) There was a problem hiding this comment. I would love to use server-side apply, but the fake implementation of controller-runtime doesn't yet support it :( Refs:
There was a problem hiding this comment. No comment, just waving at @hawkowl 👋🏽 There was a problem hiding this comment. Yes, the controller-runtime fakes are horrendous and IMO moving away from them as soon as possible is also a great boon to the project :) SSA is very complex and faking it is likely not going to happen - not even just in some soon time-frame, but perhaps ever. Client fakes have fallen out of favor and everyone has a horror story of working around the mess that is envtest without gaining any confidence in their application functioning in the real world against a real API server. Even past that, the concerns Alvaro has in Troy's PR are a mountain of work that the upstream SIGs are not interested in taking up or sponsoring, from my understanding. This is neither here nor there, but you might want to consider not putting SSA support in the controller-runtime fakes as a precondition to using this (amazingly useful and beneficial) technology in your production deployments. There was a problem hiding this comment. The issue beyond the requisite work are the apply types that are not scaffold with crd/operator-sdk/kubebuilder. That work is a side effect of the client (controller-runtime) not implementing that whereas client-go has that necessary logic. The interface client being used here is the mechanism where that can't be utilized. I have no context around what this is trying to accomplish but if it is indeed using corev1 types where a The faking of the clients are tracked with a "tracker" and trying to track a server side apply in any client would be trying to hold state for things it wouldn't worry about because of the client's responsibility. Having a test suite where that can happen seems outside of the scope of what the client/fakes could do There was a problem hiding this comment. I have worked with the In case we get too far in the weeds - I just want to re-iterate that the comments here are not review comments for this PR, just a general comment. I'm not suggesting any changes are made right now. What I am suggesting, though, is that operator reconciliation loops are written as pure functions - either take some set of cluster state as input, or allow dependency injection to take producers of cluster state as input, and produce state as output. Apply configurations have a wonderful property in that they encode your intent in static data - so, testing your lower-case The top-level func (r *reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
// assemble the data necessary by using listers, etc
input := r.getInput()
// transform input state into intended state
intent, err := r.reconcile(input)
// realize intended state onto the cluster
return r.execute(intent)
}Where In my experience, relying heavily on mocks leads many teams to stumble into deeply frustrating issues, often many years down the line. Examples I've seen:
Teams that spend time writing unit tests for unit-level concerns and do not reach to mocks, in my experience, move faster and get a higher RoI on their tests. End-to-end tests and chaos tests validate production modalities that integration tests attempt to, but cannot. There was a problem hiding this comment. @stevekuznetsov I completely agree with that. 🥇 Even though I am by far the one less experienced in this conversation about this domain, I have seen similar problems in other domains that made me resonate a lot with your comment while reading it. Sometimes we focus too much on trying to reproduce the state of the world where our app will run, and we forget that we should primarily test what our app does, and what our business logic is. It is almost always much simpler and easier to understand to just have our business logic in pure functions that expect input data and produce output data. We then forget about how this data will be gathered and what will be done with the result, and we can simply ensure that domainLogicFn(x) == expectedOutput. This is a common concern that applies to a lot of different domains, but it is pretty common in k8s because most of the time the business logic gets mixed/loosed in the effort to understand/simulate the "state" where all that business logic will happen (due to the stateful nature of k8s controllers). So I just want to reaffirm that avoiding complex mocks when possible with pure functions is a great idea, and not just in this context 🙂 |
||
| // client-side if required. | ||
| Ensure(ctx context.Context, objs ...kruntime.Object) error | ||
| EnsureDeleted(ctx context.Context, gvk schema.GroupVersionKind, key types.NamespacedName) error | ||
| } | ||
|
|
||
| type Reader interface { | ||
| client.Reader | ||
| GetOne(ctx context.Context, key types.NamespacedName, obj kruntime.Object) error | ||
| } | ||
|
|
||
| type Interface interface { | ||
| Reader | ||
| Writer | ||
| } | ||
|
|
||
| type clientHelper struct { | ||
| client.Client | ||
|
|
||
| log *logrus.Entry | ||
| } | ||
|
|
||
| func NewWithClient(log *logrus.Entry, client client.Client) Interface { | ||
| return &clientHelper{ | ||
| log: log, | ||
| 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.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.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.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.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.Update(ctx, candidate) | ||
| }) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super opinionated and optional: could we please move the line
pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw})...
to an aux variable for readability.