adding mtls config #1170

laurafitzgerald · 2025-02-20T13:10:54Z

Changes

Verification steps
Run the following on this branch.
make local-setup
kubectl apply -f config/samples/kuadrant_v1beta1_kuadrant.yaml
Follow steps in https://docs.kuadrant.io/dev/kuadrant-operator/doc/user-guides/full-walkthrough/secure-protect-connect/
Update the kuadrant cr to enable mts using
kubectl patch kuadrant kuadrant-sample --type='json' -p='[{"op": "replace", "path": "/spec/mtls/enable", "value": true}]'
Verify that a Peer Authentication exists
kubectl get peerauthentication default -n default -o yaml
which has the following details

apiVersion: security.istio.io/v1
kind: PeerAuthentication
metadata:
  labels:
    kuadrant.io/managed: "true"
  name: default
  namespace: default
spec:
  mtls:
    mode: STRICT

Verify that the two envoyfilters have a CLUSTER patch with a key "transport_socket" in spec.configPatches applyTo CLUSTER.patch.value
kubectl get envoyfilter -A -o yaml

Verify that the limitador and authorino deployments have the expected labels
kubectl get deployment limitador-limitador -o yaml | yq e '.spec.template.metadata.labels' kubectl get deployment authorino -o yaml | yq e '.spec.template.metadata.labels'
Both should output

authorino-resource: authorino
control-plane: controller-manager
sidecar.istio.io/inject: "true

Boomatang

There is a few comments that I have. I didn't try running this locally.

I would like to point out with the state of the world and the topology any time you find your self request data from the cluster you are doing something wrong

controllers/mtls_reconciler.go

controllers/state_of_the_world.go

pkg/limitador/conditions.go

Boomatang · 2025-02-20T16:21:39Z

pkg/reconcilers/deployment.go

+func MergeMapStringString(modified *bool, existing *map[string]string, desired map[string]string) {
+	if *existing == nil {
+		*existing = map[string]string{}
+	}
+
+	// for each desired key value set, e.g. labels
+	// check if it's present in existing. if not add it to existing.
+	// e.g. preserving existing labels while adding those that are in the desired set.
+	for desiredKey, desiredValue := range desired {
+		if existingValue, exists := (*existing)[desiredKey]; !exists || existingValue != desiredValue {
+			(*existing)[desiredKey] = desiredValue
+			*modified = true
+		}
+	}
+}


Could we use labels.Set.Merge from the apimachinery to do this over defining our own.

it's not a map merge operation. It's doing what the comments say.

Boomatang · 2025-02-20T16:23:22Z

controllers/mtls_reconciler.go

+		err = r.ReconcileResource(eventCtx, &v12.Deployment{}, deployment, reconcilers.DeploymentMutator(deploymentMutators...))
+	}
+
+	valueMap := map[string]interface{}{


Everything here being a map[sting]interface seems like a code smell.

I believe this is the standard way that EnvoyFilter Config Object Patches arehandle. You can see an exmaple of this here https://github.com/Kuadrant/kuadrant-controller/blob/22535aa8e4f142bf07a69dc6391c84a59b232255/pkg/istio/envoy_filters.go#L14-L47

Boomatang · 2025-03-04T12:47:29Z

pkg/limitador/conditions.go

+
+func IsLimitadorOperatorInstalled(restMapper meta.RESTMapper, logger logr.Logger) (bool, error) {
+	if ok, err := utils.IsCRDInstalled(restMapper, kuadrantv1beta1.LimitadorGroupKind.Group, kuadrantv1beta1.LimitadorGroupKind.Kind, limitadorv1alpha1.GroupVersion.Version); !ok || err != nil {
+		logger.V(1).Error(err, "Limitador Operator CRD was not installed", "group", kuadrantv1beta1.AuthorinoGroupKind.Group, "kind", kuadrantv1beta1.AuthorinoGroupKind.Kind, "version", authorinooperatorv1beta1.GroupVersion.Version)


Might want to fix the references to the authorino types here.

Boomatang

I think this is now at the stage where we need to be able to compile and run the operator to do a good review. It is getting close.

Boomatang · 2025-03-07T09:32:50Z

api/v1beta1/kuadrant_types.go

+type MTLS struct {
+	Enable bool `json:"enable,omitempty"`
+}
+


At some point can you add a comment to what the mtls struct is for. The comment gets added to the CRD definition as a description. You will also need to run make manifest , bundle and helm at some point.

controllers/mtls_reconciler.go

laurafitzgerald · 2025-03-12T15:10:33Z

controllers/mtls_reconciler.go

+	if kObj == nil && kObj.Spec.MTLS == nil && (!kObj.Spec.MTLS.Enable || anyAttachedAuthorRateLimitPolicy == false) {
+		defer logger.V(1).Info("mtls not enabled or applicable", "status", "completed")
+		peerAuthentications := lo.FilterMap(topology.Objects().Items(), func(item machinery.Object, _ int) (machinery.Object, bool) {
+			if peerAuthentication, ok := item.(*PeerAuthentication); ok {


This isn't working yet. It's not picking up the exising PeerAuthentications.

eguzki

Very good progress.

Few questions:

If the gateway provider is not Istio and a user enables mtls, should we report in kuadrant status?
~~When a user deletes a gateway, the peerAuthentications objects gets deleted? (not very important now and can be tackled in follow up PR's)~~ (I learned that one peerAuthentications is created at the kuadrant namespace)

eguzki · 2025-03-14T14:58:27Z

api/v1beta1/topology.go

+		To:   DeploymentGroupKind,
+		Func: func(child machinery.Object) []machinery.Object {
+			return lo.Filter(authorinos, func(k machinery.Object, _ int) bool {
+				return k.GetNamespace() == child.GetNamespace() && child.GetName() == "authorino"


I am not very familiar with these linking functions. I still need to educate myself.

Question: child's type is Authorino, isn't it? Then, isn't this linking function linking all deployments in Authorino namespace to Authorino? even those that are not authorino deployment, like limitador deployment?

I'm also new to them, the idea here, which may not be implemented correctly yet is that we link the authorino deployment, I'm uisng the GetName to isolate it, although I'd rather not hardcode, that needs to be fixed so that it's only that one that get's picked up and not all in the authorino namespace. AIUI this is to add it as a child of the Authorino cr in the topology.

eguzki · 2025-03-14T15:00:01Z

config/samples/kuadrant_v1beta1_kuadrant.yaml

@@ -3,4 +3,6 @@ apiVersion: kuadrant.io/v1beta1
 kind: Kuadrant
 metadata:
  name: kuadrant-sample
-spec: {}
+spec:


This is the sample being shown in operatorhub web console. Do we want to add (optional) mtls config to it?

If we'd prefer not to i'm fine with that. My reasoning for adding it here is that in the verification steps i'm using kubectl patch kuadrant kuadrant-sample --type='json' -p='[{"op": "replace", "path": "/spec/mtls/enable", "value": false}]' as a quick way to switch it on and off for testing. But if this gets presented in the console where the description will show that it can be used optionally, i would agree it's best not to include it. I'll remove this before we merge.

eguzki · 2025-03-14T15:05:13Z

controllers/auth_workflow_helpers.go

+					"tls_certificate_sds_secret_configs": []interface{}{
+						map[string]interface{}{
+							"name": "default",
+							"sds_config": map[string]interface{}{


Am I correct assuming that the gateway will fetch the client certificate from XDS interface?

I'm actually not familiar with this configuration. I've taken it from https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/install/mtls-configuration.md#envoy-filter making the assumption that it will work as is and doesn't need any changes. @david-martin and @maleck13 are the original authors there so may be answer questions around that.

controllers/istio_auth_cluster_reconciler.go

controllers/mtls_reconciler.go

eguzki · 2025-03-14T15:23:14Z

pkg/istio/utils.go

+		}
+	}
+*/
+func LinkDeploymentToAuthorino(objs controller.Store) machinery.LinkFunc {


Isn't this defined in https://github.com/Kuadrant/kuadrant-operator/pull/1170/files#diff-88007d2400b1fe3a9b5d5d7f78bd367e3721cadcddc40033dd9170fd040dba3fR70 ?

eguzki · 2025-03-14T15:24:43Z

pkg/limitador/conditions.go

+	"k8s.io/apimachinery/pkg/api/meta"
+)
+
+func IsLimitadorOperatorInstalled(restMapper meta.RESTMapper, logger logr.Logger) (bool, error) {


Why is required to check the existence of the operator? Shouldn't it be Limitador itself?

eguzki · 2025-03-14T15:27:22Z

pkg/reconcilers/deployment.go

+func MergeMapStringString(modified *bool, existing *map[string]string, desired map[string]string) {
+	if *existing == nil {
+		*existing = map[string]string{}
+	}
+
+	// for each desired key value set, e.g. labels
+	// check if it's present in existing. if not add it to existing.
+	// e.g. preserving existing labels while adding those that are in the desired set.
+	for desiredKey, desiredValue := range desired {
+		if existingValue, exists := (*existing)[desiredKey]; !exists || existingValue != desiredValue {
+			(*existing)[desiredKey] = desiredValue
+			*modified = true
+		}
+	}
+}


it's not a map merge operation. It's doing what the comments say.

eguzki · 2025-03-17T14:41:15Z

controllers/istio_ratelimit_cluster_reconciler.go

@@ -56,6 +56,10 @@ func (r *IstioRateLimitClusterReconciler) Reconcile(ctx context.Context, _ []con
 	if kuadrant == nil {
 		return nil
 	}
+	mtls := false


Suggested change

mtls := false

mtls := kuadrant != nil && kuadrant.Spec.MTLS != nil && kuadrant.Spec.MTLS.Enable

Actually, it could be mtls := kuadrant.IsMTLSEnabled() according to https://github.com/Kuadrant/kuadrant-operator/pull/1170/files#r1995757686

eguzki · 2025-03-17T14:56:10Z

Trying to run the verification steps: it fails with

{"level":"info","ts":"2025-03-17T14:54:07Z","logger":"kuadrant-operator","msg":"pkg/mod/k8s.io/[email protected]/tools/cache/reflector.go:243: failed to list *v1.PeerAuthentication: peerauthentications.security.istio.io is forbidden: User \"system:serviceaccount:kuadrant-system:kuadrant-operator-controller-manager\" cannot list resource \"peerauthentications\" in API group \"security.istio.io\" at the cluster scope"}

Consider granting required permissions to the operator

eguzki · 2025-03-17T15:02:21Z

controllers/mtls_reconciler.go

+
+		logger.Info("creating peer authentication resource", "status", "processing")
+
+		err = r.CreateResource(eventCtx, peerAuth)


Consider calling ReconcileResource and passing a mutator. Feel free to pick CreateOnlyMutator

eguzki · 2025-03-17T15:07:00Z

controllers/mtls_reconciler.go

+			return nil, false
+		})
+		// add label to authorino deployment {"sidecar.istio.io/inject":"true"}}}}}
+		aDeployment := &v12.Deployment{


It would be nice if the kuadrant operator could add a label to Authorino CR, which is the API exposed from the authorino operator to external users of it. Instead, kuadrant operator needs to speculate and assume that the deployment name will be authorino because it has created a Authorino CR called authorino, which is ugly to the very least and easy to break.

@eguzki i think we may do this in the future. or maybe the progression of this pr will result in us making the final decision around that. I think the con for that was around adding an additional api that we will have to maintain and we wanted to be sure about that first. @maleck13 given the progress of the implementation here does that change our deesire to add taht additional api via the spec of the cr?

eguzki · 2025-03-17T15:15:41Z

controllers/mtls_reconciler.go

+		lDeployment := &v12.Deployment{
+			ObjectMeta: metav1.ObjectMeta{
+				// TODO can't be hardcoded, this is just one example
+				Name:      "limitador-limitador",


Same as https://github.com/Kuadrant/kuadrant-operator/pull/1170/files#r1998972201

The alternative to hardcode the name is to filter by labels we expect to have. Still not perfect.

eguzki · 2025-03-17T15:23:32Z

controllers/mtls_reconciler.go

+			return fmt.Errorf("could not add label to limitador deployment %w", err)
+		}
+
+		peerAuth := &istiosecurity.PeerAuthentication{


Consider adding ownerref to Kuadrant CR

laurafitzgerald · 2025-03-20T11:40:22Z

@eguzki thanks for you reviews. I've been caught up with other work but starting to address those now. Need to rebase first. 🤕 😂

Signed-off-by: Laura Fitzgerald <[email protected]>

…ation Signed-off-by: Laura Fitzgerald <[email protected]>

…ts to limitador and authorino crs Signed-off-by: Laura Fitzgerald <[email protected]>

…e other cleanup and it's runnable now Signed-off-by: Laura Fitzgerald <[email protected]>

Signed-off-by: Laura Fitzgerald <[email protected]>

laurafitzgerald requested a review from a team as a code owner February 20, 2025 13:10

laurafitzgerald requested a review from Boomatang February 20, 2025 13:11

laurafitzgerald marked this pull request as draft February 20, 2025 13:12

Boomatang reviewed Feb 20, 2025

View reviewed changes

laurafitzgerald force-pushed the mtls branch from 7b55e2a to 83a6d54 Compare February 26, 2025 15:16

Boomatang reviewed Mar 4, 2025

View reviewed changes

Boomatang reviewed Mar 7, 2025

View reviewed changes

KevFan reviewed Mar 10, 2025

View reviewed changes

controllers/mtls_reconciler.go Outdated Show resolved Hide resolved

KevFan reviewed Mar 10, 2025

View reviewed changes

controllers/mtls_reconciler.go Outdated Show resolved Hide resolved

laurafitzgerald force-pushed the mtls branch 3 times, most recently from da6de62 to e64e213 Compare March 12, 2025 12:45

laurafitzgerald marked this pull request as ready for review March 12, 2025 12:45

laurafitzgerald force-pushed the mtls branch from e64e213 to 87516f4 Compare March 12, 2025 12:48

laurafitzgerald commented Mar 12, 2025

View reviewed changes

eguzki reviewed Mar 14, 2025

View reviewed changes

eguzki reviewed Mar 17, 2025

View reviewed changes

laurafitzgerald added 7 commits March 20, 2025 11:49

adding mtls config

4e6fb19

Signed-off-by: Laura Fitzgerald <[email protected]>

move istio check to before the reconciler

f1041c0

Signed-off-by: Laura Fitzgerald <[email protected]>

move limitador and authorino installed checks to reconciler initialis…

be62733

…ation Signed-off-by: Laura Fitzgerald <[email protected]>

refactor to add some cleanup logic and adding logic to link deploymen…

2255bfe

…ts to limitador and authorino crs Signed-off-by: Laura Fitzgerald <[email protected]>

moving envoyfilter management to auth and rate limit reconcilers, som…

ad026cf

…e other cleanup and it's runnable now Signed-off-by: Laura Fitzgerald <[email protected]>

cleanup of restmapper and variables

5befbd4

Signed-off-by: Laura Fitzgerald <[email protected]>

rebase and fixup and imports

c6cf2c2

Signed-off-by: Laura Fitzgerald <[email protected]>

laurafitzgerald added 2 commits March 20, 2025 11:52

updating bundle and chart

d5a3b11

Signed-off-by: Laura Fitzgerald <[email protected]>

update sample kuadrant to default to not enable mtls

27bca0a

Signed-off-by: Laura Fitzgerald <[email protected]>

laurafitzgerald force-pushed the mtls branch from 45de713 to 27bca0a Compare March 20, 2025 11:52

fixing up some issues after rebase

7b3986f

Signed-off-by: Laura Fitzgerald <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding mtls config #1170

adding mtls config #1170

laurafitzgerald commented Feb 20, 2025 •

edited

Loading

Boomatang left a comment

Boomatang Feb 20, 2025

eguzki Mar 14, 2025

Boomatang Feb 20, 2025

laurafitzgerald Feb 26, 2025

Boomatang Mar 4, 2025

Boomatang left a comment

Boomatang Mar 7, 2025

laurafitzgerald Mar 12, 2025

eguzki left a comment •

edited

Loading

eguzki Mar 14, 2025

laurafitzgerald Mar 20, 2025

eguzki Mar 14, 2025

laurafitzgerald Mar 20, 2025

eguzki Mar 14, 2025

laurafitzgerald Mar 20, 2025

eguzki Mar 14, 2025

eguzki Mar 14, 2025

eguzki Mar 14, 2025

eguzki Mar 17, 2025

eguzki Mar 17, 2025

eguzki commented Mar 17, 2025

eguzki Mar 17, 2025

eguzki Mar 17, 2025

laurafitzgerald Mar 20, 2025

eguzki Mar 17, 2025

eguzki Mar 17, 2025

laurafitzgerald commented Mar 20, 2025

	mtls := false
	mtls := kuadrant != nil && kuadrant.Spec.MTLS != nil && kuadrant.Spec.MTLS.Enable


		logger.Info("creating peer authentication resource", "status", "processing")

		err = r.CreateResource(eventCtx, peerAuth)

adding mtls config #1170

Are you sure you want to change the base?

adding mtls config #1170

Conversation

laurafitzgerald commented Feb 20, 2025 • edited Loading

Boomatang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Boomatang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eguzki left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eguzki commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurafitzgerald commented Mar 20, 2025

laurafitzgerald commented Feb 20, 2025 •

edited

Loading

eguzki left a comment •

edited

Loading