Console fix up with mutual TLS #1402

RafalKorepta · 2024-06-27T16:36:11Z

This PR is based on #1386 to solve early return problem in go template.

Client certificates does not relay on secret reference, that's why checks are removed.

Add integration test that setup mTLS for all listeners (Kafka, Admin API, HTTP Proxy, Schema Registry, RPC).

Verification in rpk client is not always done using rpk. curl should be replaced with port-forward and simple go based http client Get/Post/Delete implementation.

Fix ClientAuthRequired check as Dig function should start from listeners object.

Expose, in Redpanda values, Console config and chunk of Connectors values.

Mount all client certificates (ca, certificate and key) in Console deployment.

Move Admin API configuration from env vars to ConfigMap.

Reference

https://docs.redpanda.com/current/develop/http-proxy/

https://docs.redpanda.com/current/manage/schema-reg/schema-reg-api/

chrisseto

Not a full review will follow up on the test. Seems like we'll want to solve the subcharting problem soon.

chrisseto · 2024-06-27T18:22:31Z

charts/redpanda/certs.go

@@ -89,7 +89,7 @@ func ClientCerts(dot *helmette.Dot) []certmanagerv1.Certificate {
 		panic(fmt.Sprintf("Certificate %q referenced but not defined", name))
 	}

-	if helmette.Empty(data.SecretRef) || !ClientAuthRequired(dot) {
+	if !ClientAuthRequired(dot) {


I was a bit worried about this. We'll need to ask Azure (cc @c4milo or @ncole ) how they'd like to handle this.
For mTLS to work, the chart needs to be able to get a client certificate for either console or RPK (@RafalKorepta which is it?). This currently works through the cert manager integration but there's not really a fallback. If y'all can mint a cert we can find a field to add a secretRef to or we can talk through relying on the cert-manger integration either partially or fully.

I'm not sure the context on this, but I think you're saying for mTLS we need a client cert. We are already creating a self-signed CA AND a client cert, so we can provide that ref somehow for Console (or RPK, whichever it is @RafalKorepta)?

In fact, in the other providers, if mTLS is enabled, we add a self-signed CA and a cert for kminion to use so we have an existing precedent for this.

For mTLS to work, the chart needs to be able to get a client certificate for either console or RPK (@RafalKorepta which is it?).

In this particular case, client certificate is used for both RPK and console. They will reference and mount the same certificate.

This currently works through the cert manager integration but there's not really a fallback. If y'all can mint a cert we can find a field to add a secretRef to or we can talk through relying on the cert-manger integration either partially or fully.

I agree, that this check needs to accommodate a use case when certificate is provided. I will change that check.

I agree, that this check needs to accommodate a use case when certificate is provided. I will change that check.

Let's handle that in a follow up PR/commit?

chrisseto · 2024-06-27T18:31:20Z

charts/redpanda/client_test.go

+	return event, nil
+}
+
+func (c *Client) GetClusterHealth(ctx context.Context, dot *helmette.Dot) (map[string]any, error) {


I don't love the idea of using *helmette.Dot here. If there's a bug in some of the Values functions that would surface here and potentially obscure issues from our tests, specifically in the case of TLS configurations.

I think it would be a more robust testing strategy to instead have the Client accept the TLS certificates (extracted from the cluster if using the cert-manger integration) and then use those to actually dial into the various listeners of the Pod, piggy backing on kubectl port-forward to actually make the connection. Then we can rely on client libraries instead of having to template out CLI commands.

For an initial implementation thought, let's roll with it!

I will add certificate to the Client.

chrisseto · 2024-06-27T18:40:58Z

charts/redpanda/console.tpl.go

+	}
+
+	if values.Connectors.Enabled {
+		// TODO Do not cal Dig with dot.Values as restPort that is defined in connectors helm chart is not


I can think of ~2 ways we could handle this.

Have a helmette function that runs a "raw" template command (Though it'll be quite difficult to get that to work reliably in go but perhaps that's okay. helmette.Raw("(include connectors.serviceName $var)")

Teach gotohelm how to handle subcharts by resolving the name to the appropriate call but not actually transpile the underlying call.

Given that we'll need to convert the connectors and console chart, I'm leaning towards 2 but 1 would certainly be faster.

charts/redpanda/console.tpl.go

charts/redpanda/helpers.go

chrisseto · 2024-07-01T21:21:11Z

charts/redpanda/testdata/ci/01-default-values.yaml.golden

@@ -654,23 +661,14 @@ spec:
        - name: kafka-default-cert
          secret:
            defaultMode: 272
-            items:


huh. Do you know where these diffs are stemming from? It seems incorrect for these to be removed. Could they just be out of date?

I didn't want transiple the schema, admin and kafka volume definition in the following console deployement:

https://github.com/redpanda-data/helm-charts/pull/1402/files#diff-0b45ecf3a48bcb79531c1cf2372a743fa93c58bc1ee5193a798e9decd52bf7e4L245

https://github.com/redpanda-data/helm-charts/pull/1402/files#diff-0b45ecf3a48bcb79531c1cf2372a743fa93c58bc1ee5193a798e9decd52bf7e4L213

https://github.com/redpanda-data/helm-charts/pull/1402/files#diff-0b45ecf3a48bcb79531c1cf2372a743fa93c58bc1ee5193a798e9decd52bf7e4L185

Regardless of the certification secret reference all will be mounted to console pod.

chrisseto · 2024-07-01T21:21:56Z

charts/redpanda/testdata/ci/01-default-values.yaml.golden

        urls:
        - https://redpanda-0.redpanda.default.svc.cluster.local.:8081
        - https://redpanda-1.redpanda.default.svc.cluster.local.:8081
        - https://redpanda-2.redpanda.default.svc.cluster.local.:8081
      tls:
+        caFilepath: /mnt/cert/kafka/default/ca.crt
+        certFilepath: ""


Are empty strings valid values of certFilepath and keyFilepath?

Console didn't complain and does not use cert nor key if it has empty string in the kafka, schema registry and admin API clinets.

RafalKorepta · 2024-07-09T22:16:20Z

I added new CI execution that using kind cluster is performing new mTLS test cases
https://github.com/redpanda-data/helm-charts/actions/runs/9864729172/job/27240338010?pr=1402#step:8:1111
https://github.com/redpanda-data/helm-charts/actions/runs/9864729172/job/27240338010?pr=1402#step:8:1116

P.S. I don't understand why REDPANDA_LICENSE is not found, so that tiered-storage-secrets is skipped.
https://github.com/redpanda-data/helm-charts/actions/runs/9864729172/job/27240338010?pr=1402#step:8:1039

chrisseto

This is quite a large PR again 😅 Let's try to get the for loop fix merged externally so we can cut down on the diff a bit.

I'm seeing some concerning looking diffs in the golden files. Do you think they're expected?

charts/redpanda/chart_test.go

chrisseto · 2024-07-11T18:37:25Z

charts/redpanda/chart_test.go

+
+	record, err := rpk.RetrieveEventFromTopic(ctx, httpTestTopic, 0)
+	require.NoError(t, err)
+	require.Equal(t, fmt.Sprintf("[{\"topic\":\"%s\",\"key\":null,\"value\":\"Redpanda\",\"partition\":0,\"offset\":0}]", httpTestTopic), record)


nit: maybe use JSONEq here unless the key ordering form the proxy is guaranteed?
nit: Using backticks makes for much more readable JSON.

ordering form the proxy is guaranteed

Yes, it's guaranteed.

Using backticks makes for much more readable JSON.

I will use JSONEq here, so the object will be created as map[string]any

charts/redpanda/chart_test.go

chrisseto · 2024-07-11T20:54:59Z

pkg/kube/ctl.go

@@ -154,3 +158,80 @@ func (c *Ctl) Exec(ctx context.Context, pod *corev1.Pod, opts ExecOptions) error
 		Stdin:  opts.Stdin,
 	})
 }
+
+func (c *Ctl) PortForward(ctx context.Context, pod *corev1.Pod, out, errOut io.Writer) ([]portforward.ForwardedPort, func(), error) {
+	// Apparently, nothing in the k8s SDK, except exec'ing, uses RESTClientFor.


Ugh, I remember hitting this issue way back. Most frequently I've seen people make an instance of the type client set which internally does something like this though it's split across 3 different packages if IIRC.

chrisseto · 2024-07-11T21:02:00Z

pkg/kube/ctl.go

+
+	dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, "POST", req.URL())
+
+	var ports []string


I'm not a huge fan of relying on the "portforwarding" abstraction because it binds to local ports for no reason which adds a bunch of extra goroutines and indirection but it's much easier to get started.

We can rip the guts out of https://github.com/kubernetes/client-go/blob/592d891671b2a09e5f81781b28ebe078d8115e41/tools/portforward/portforward.go#L335 to wrap this up into a Dialer interface that'll place nice with go's networking constructs which is a lot easier to utilize with other client libraries (http.RoundTripper for example).

That's a bigger change for later. Just leaving so notes for ourselves.

pkg/helm/helmtest/helmtest.go

pkg/helm/helm.go

charts/redpanda/testdata/ci/32-statefulset-podspec-novalues.yaml.golden

Console TLS configuration needs to have client certificates for mutual TLS communication. With this commit Console go package is imported to have strongly typed Console configuration. As Console configuration has yaml annotation, not json transpiler is trying to parse yaml annotation as a fallback.

RafalKorepta requested a review from jan-g June 27, 2024 16:36

chrisseto reviewed Jun 27, 2024

View reviewed changes

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch from 6ba9d11 to 4ace5f2 Compare July 1, 2024 11:39

chrisseto reviewed Jul 1, 2024

View reviewed changes

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch 3 times, most recently from 16a8c2d to 38b69a1 Compare July 9, 2024 21:45

RafalKorepta marked this pull request as ready for review July 9, 2024 22:16

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch 10 times, most recently from 942a159 to 9891e67 Compare July 11, 2024 15:20

chrisseto reviewed Jul 11, 2024

View reviewed changes

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch 5 times, most recently from b192503 to 65b0f78 Compare July 12, 2024 12:49

jan-g and others added 2 commits July 12, 2024 18:23

gotohelm: fix return from nested range

4ab1f70

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch from 65b0f78 to a7bfd7b Compare July 12, 2024 16:38

chrisseto approved these changes Jul 12, 2024

View reviewed changes

Add integration tests based on go unit tesst

1af7f16

RafalKorepta force-pushed the rk/K8S-261/mTLS-test-suite branch from a7bfd7b to 1af7f16 Compare July 12, 2024 17:01

RafalKorepta enabled auto-merge (rebase) July 12, 2024 17:31

RafalKorepta merged commit 62f8ec2 into main Jul 12, 2024
42 checks passed

RafalKorepta deleted the rk/K8S-261/mTLS-test-suite branch July 12, 2024 17:43

This was referenced Jul 12, 2024

Fix mutual TLS configuration with Console redpanda-data/redpanda-operator#170

Merged

🫐 When passing Redpanda TLS certs as a secret and enabling mTLS, rpk commands run by the Helm chart are unable to authenticate #1348

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Console fix up with mutual TLS #1402

Console fix up with mutual TLS #1402

RafalKorepta commented Jun 27, 2024

chrisseto left a comment

chrisseto Jun 27, 2024

ncole Jun 29, 2024

RafalKorepta Jul 1, 2024

chrisseto Jul 1, 2024

chrisseto Jun 27, 2024

RafalKorepta Jul 1, 2024

chrisseto Jun 27, 2024

chrisseto Jul 1, 2024

RafalKorepta Jul 2, 2024

chrisseto Jul 1, 2024

RafalKorepta Jul 2, 2024

RafalKorepta commented Jul 9, 2024

chrisseto left a comment

chrisseto Jul 11, 2024

RafalKorepta Jul 12, 2024

chrisseto Jul 11, 2024

chrisseto Jul 11, 2024


		dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, "POST", req.URL())

		var ports []string

Console fix up with mutual TLS #1402

Console fix up with mutual TLS #1402

Conversation

RafalKorepta commented Jun 27, 2024

Reference

chrisseto 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

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

RafalKorepta commented Jul 9, 2024

chrisseto 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