Operator mtls #3

0x5d · 2023-03-14T22:00:13Z

Backports Required

Release Notes

src/go/k8s/pkg/resources/certmanager/type_helpers.go

alenkacz

Juts left some initial comments

alenkacz · 2023-03-16T12:16:56Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

@@ -823,6 +823,7 @@ func hasDifferentNodeSecret(listener1, listener2 KafkaAPITLS) bool {
 		listener1.NodeSecretRef.Name != listener2.NodeSecretRef.Name
 }

+// TODO: might need to change this.


we should validate:

cacrt is provided only when mtls is enabled

the secret exists that's referenced

@0x5d I can help do that. Let me know.

src/go/k8s/pkg/resources/certmanager/type_helpers.go

alenkacz · 2023-03-16T12:19:32Z

src/go/k8s/pkg/resources/certmanager/type_helpers.go

+		tls := l.GetTLS()
+		// Trigger client cert generation only if RequireClientAuth is true and
+		// ClientCACertRef hasn't been set.
+		if tls.RequireClientAuth && tls.ClientCACertRef == nil {


I think it should instead be - if ANY listener has CaCertRef, not generate. Right now it's if any does not have it, generate... maybe that could actually never happen? 🤔

paulzhang97 · 2023-03-17T20:45:58Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_webhook.go

@@ -823,6 +823,7 @@ func hasDifferentNodeSecret(listener1, listener2 KafkaAPITLS) bool {
 		listener1.NodeSecretRef.Name != listener2.NodeSecretRef.Name
 }

+// TODO: might need to change this.


@0x5d I can help do that. Let me know.

paulzhang97 · 2023-03-21T15:20:17Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

@@ -718,14 +718,17 @@ type AdminAPITLS struct {

 // PandaproxyAPITLS configures the TLS of the Pandaproxy API
 //
+// TODO: Don't generate CA if ClientCACertRef isn't nil.


Is the TODO done? Delete comment?

It's not done, but no longer needed since we will need to generate anyway.

paulzhang97 · 2023-03-21T16:39:05Z

src/go/k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/00-redpanda-cluster.yaml

+# Therefore, we need to create a namespace and create every resource in it
+# because the clientCACertRef must point to a specific namespace.


Actually only the secret named ca-cert needs to be in this namespace, and the namespace settings should be removed from all the other resources, so all the other resources will be in the namespace randomly created by kuttl. The test code would be more clean.

Suggested change

# Therefore, we need to create a namespace and create every resource in it

# because the clientCACertRef must point to a specific namespace.

# Therefore, we need to create a namespace for testing the scenario in which the secret

# is copied to the namespace of Cluster CR.

src/go/k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/00-redpanda-cluster.yaml

paulzhang97 · 2023-03-21T16:55:20Z

src/go/k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/00-redpanda-cluster.yaml

+  namespace: pandaproxy-external-tls
+data:
+  client.key: LS0tLS1CRUdJTiBSU0EgUFJJVkFURSBLRVktLS0tLQpNSUlFamdJQkFBS0IvZzdnQXpMZGIzVWFoa1ltZDNkY1pKY0dCaUNEMEdmSjdkVi9yenB2ek1laHdwSHdybmI0Cnd2RDZuMnZmbWdza2hxZ3ZmY0xDVUZ0RmVRR0dWUC9yMGk0L0g5RUJVZHNRVjBZWEVKZ0IxSlV1ZHNSNmQvSEEKbWtSNzNnWmZ6SjRLV2JCTFY1QVdoUHI5UUYyY2dNNXNHdjZpUmxVU21vWnlSNW9vb2tsVWdlQ2tMV3I0SFNpNgo1eis5KzhvcVdBYnNMUCtpdTZEMUVHbGdpaU5XdGtUMU14YkErL1AvTTBBSUtwM0YvZi9maG9HcVBkQXpORzJZCmo2dTM4U29sU1JXMGJHMzBtMUxQZGtDbSt4c2prREdCRDRvMzNrT0JLTk5UejZvb3I1ZklEVUJyeGpCSzg3ZGkKMCtSb21ING9yV21DeGRuMlFwRVc4YVVkV0c2Z1pDaUxBZ01CQUFFQ2dmMDVhRkx2KzZvc1NESHVpb2FHSWc0VQp6U3JZVWQvK3IxTTBwWk9mODlwZW1leFJVSkFDbUEzQitYMUsxYXV0VlVwYnpPMk91RjluNExjaEswN2dZejdIClBOZU55WW9mNlBEcGtCcDZqSlhqS1c4MDJYSVBrVVBQQ2ozV1crTldESndYeHE0cGFML1A2WlIvczRGcEo4OEcKNjBDRkUwZExtY0M5TkJVZTdRWlFGdE5jVVhUckVWck9hN01JVW53V0pPQmpqalJ1bms0WVZlaTJoSVBVY0NscgorcmVmbUtXVnNVK05UMDJybVZkZVp5YzFUN1A4OC81RURTWk91ZWY0UjgrV2RmSGNQaVJEUXQyV3JFQks1a0lCClpvZ3VUOUVSSWNTUkg4eWVNN1RuNVloU25iWHpwbXpPalVoUVZZU2ozRVh0SUZRcmtmVVNHRTZaOW9XQkFuOC8KWFBtVUtuS2F2MFRhTFdyYkF3WTdTTnVKZjBtUUJWOStNS0JtVmNjKzNNeWtYdjlUZVkvVzR5YnhUZ29ISzNoQwpxUWVhNHBRbExBV1hqbEowY1ErRkRqR0lxeENaQ2xiUUk4STk2aFYwaTNJQXBDbkx4VzBIMmdCaEU2K0hnUzFvCmdjejZkaTlWQWNoVmtrN1JiR2NISHFtdDYrTEFDdjRxV0d2aDFUemJBbjg4R1NMWDBUbWVBemdaRFRmcmNPRnYKV0pvajl1Yy9CYXZyclREekt3WkdaMjFRWmlDalhTT3ExQmpmV1Nuemp5QkZ3d0Y2aTQyZTdLazVhWHlFT01BdApXaC9aWjEyaExJOU9RZUZPNDRjRm92UUhhVjNsRUtWR0N2ejdWZ08zcDBDNEF3a2JHdXc4WE4rMU5CVUMwKzV1CitxMVVNUWlKSXBsMlJVV0dUTG9SQW44MXNxR0FRNGprUS93aHVpTzRmNU9rWWxaSzdDaDNlVlk0SnhXSUpHRzYKa0h5TlFFUzVoV2UxQU1SYjgzcmtJSjdHUDJGR2pZWm5DaXVqQ09ZdjhERHEzZUIwcGlSbXpqQk1MRUhOSHJnWQpFS1VJamhjdHJaNTg3Tzh0VmZXSHJKM0MxMTNUVkoxQU9VYUIzb0FWVXZ6dE43c3N2WjlvaU9obEVwSTZ6Ty9KCkFuOGV0cVhrNyt0blFyUG5zYWF6YjRQMm1LeGwxdWdGZ0V1RmJZU0hzYVJLVk04NytJV1RsNlVEeDlOU1Njb3oKekNDdEptVGFFUG4yajNKSWdnMTlzVmNkbG1LU2c4NEk4YkhuUjZueTNEc0QrV3lIWVNUNFRSSjZBbUdadlRLMwpLQVhlUk1iaGtGZk0zWllDa0RSd3RvaXpOTzlpQ0pFWkxKS0xMQW5GQWU5UkFuOEVPSlNORy8vQlUrWHNMNE9MCjJ6b2Nka3ZLd3FHZ0JwblJjNEh0Z2lsanFRR1pxZmdVZlNVOWk1OWg0a3loOVB5dGxKNS9iK04vTXRJcTk1aVYKVlVYNWhUL1RDaWJHWWZPU284RkErQitSSlpnbThJbHJ5QXlTbWJNT3R1UkFzM2VGQ0JDWllaQm5TSFBTZXZwNgpDeVZ0NVI0d1o2d0NpMjNlcHZ3QVVTb2IKLS0tLS1FTkQgUlNBIFBSSVZBVEUgS0VZLS0tLS0K
+  client.crt: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURyVENDQXBVQ0NRQ2RJWm1HZ0VmalpUQU5CZ2txaGtpRzl3MEJBUXNGQURDQm5URUxNQWtHQTFVRUJoTUMKVlZNeEN6QUpCZ05WQkFnTUFrTkJNUll3RkFZRFZRUUhEQTFUWVc0Z1JuSmhibU5wYzJOdk1SY3dGUVlEVlFRSwpEQTVTWldSd1lXNWtZU3dnU1c1akxqRU9NQXdHQTFVRUN3d0ZRMnh2ZFdReEd6QVpCZ05WQkFNTUVtTnNiM1ZrCkxuSmxaSEJoYm1SaExtTnZiVEVqTUNFR0NTcUdTSWIzRFFFSkFSWVVkR1Z6ZEdzNGMwQnlaV1J3WVc1a1lTNWoKYjIwd0hoY05Nak13TXpFM01qSTFOekE0V2hjTk1qWXdNekUyTWpJMU56QTRXakNCbGpFTE1Ba0dBMVVFQmhNQwpWVk14Q3pBSkJnTlZCQWdNQWtOQk1SWXdGQVlEVlFRSERBMVRZVzRnUm5KaGJtTnBjMk52TVJjd0ZRWURWUVFLCkRBNVNaV1J3WVc1a1lTd2dTVzVqTGpFTU1Bb0dBMVVFQ3d3RFN6aHpNUlV3RXdZRFZRUUREQXh5WldSd1lXNWsKWVM1amIyMHhKREFpQmdrcWhraUc5dzBCQ1FFV0ZXczRjeTEwWlhOMFFISmxaSEJoYm1SaExtTnZiVENDQVI0dwpEUVlKS29aSWh2Y05BUUVCQlFBRGdnRUxBRENDQVFZQ2dmNE80QU15M1c5MUdvWkdKbmQzWEdTWEJnWWdnOUJuCnllM1ZmNjg2Yjh6SG9jS1I4SzUyK01MdytwOXIzNW9MSklhb0wzM0N3bEJiUlhrQmhsVC82OUl1UHgvUkFWSGIKRUZkR0Z4Q1lBZFNWTG5iRWVuZnh3SnBFZTk0R1g4eWVDbG13UzFlUUZvVDYvVUJkbklET2JCcitva1pWRXBxRwpja2VhS0tKSlZJSGdwQzFxK0Iwb3V1Yy92ZnZLS2xnRzdDei9vcnVnOVJCcFlJb2pWclpFOVRNV3dQdnovek5BCkNDcWR4ZjMvMzRhQnFqM1FNelJ0bUkrcnQvRXFKVWtWdEd4dDlKdFN6M1pBcHZzYkk1QXhnUStLTjk1RGdTalQKVTgrcUtLK1h5QTFBYThZd1N2TzNZdFBrYUpoK0tLMXBnc1haOWtLUkZ2R2xIVmh1b0dRb2l3SURBUUFCTUEwRwpDU3FHU0liM0RRRUJDd1VBQTRJQkFRQTlFZ0tUNWsrTkdjcmQ1cUN2TEF0dGE4RnkxWTVGYmVnQVR6ZGYwa0ZICmozVVZpaEtWRDgxbWpZTnIzci9tMHk5ZWtrMzBnVHZyMlovYVY5R2F1V2JpRnJ2Y2hoTGdrVlNjMHBiN3RkQy8Ka2xlWlNlVzFRanZlZ0EwN0kvQlpWaCthSHYzaDN1bTFQR2pvMEdTSXlkRFJWRHJJQ3ZBZXhZcWVGTi9aTUZmMwpOVkJDUGRPU3F1TFZYM0Z0Nllva0FrV0ZwaXYwcHFUZ1VIVkdkUmxaQWFBSVZmcFhnSGRmR0dMTzFTcEpNWVNRCnp1SVZ2Vk8zejlwSDJDRCt2QmlTNWdHd2Jqd213N1NreGQ3UkJwYUZWd3RLbk9yQnI3WTd5ZE9lTnRvOTEyZncKUnhma3BJOUtaWkUyWkU1TUlyR1V0R1V6Zkt4d056TjdOeDlJZnptbEI5NlUKLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=


The cert will be expired in 3 years, a little short. Wonder whether we can have cert with the expiration in 10 years, so people would not have to take care of test failures because of expired cert 3 years from now.

Good call. Let's change it and add a comment with the expiry date like you did in the schema reg test.

paulzhang97 · 2023-03-21T16:57:01Z

...k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/04-consume-message-fail.yaml

+      volumes:
+        - name: client-cert
+          secret:
+            secretName: client-cert


Remove since no cert needed in this test step.

paulzhang97 · 2023-03-21T16:57:17Z

...k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/04-consume-message-fail.yaml

+          volumeMounts:
+            - mountPath: "/tmp/client"
+              name: client-cert
+              readOnly: true


Remove since no cert needed in this test step.

paulzhang97 · 2023-03-21T17:00:44Z

src/go/k8s/tests/e2e/pandaproxy-produce-consume-tls-client-external-ca/00-redpanda-cluster.yaml

+        # namespace:
+          # fieldRef:
+          #    fieldPath: metadata.namespace
+          namespace: default


change to pandaproxy-external-tls if agree with my first comment on this file.

paulzhang97 · 2023-03-21T17:03:25Z

src/go/k8s/apis/redpanda/v1alpha1/cluster_types.go

+	// If ClientCACertRef points to a secret in a different namespace, the operator will
+	// duplicate the secret to the same namespace as the cluster CR to be able to
+	// mount it to the nodes.
+	ClientCACertRef *corev1.ObjectReference `json:"clientCACertRef,omitempty"`


In the scenario that the CA cert is updated, will RP pick up the updated CA cert? Is it existing functionality?

Yes! The server watches the configured cert and hot-reloads it. A restart is only needed if the cert path changes, but that shouldn't happen ever.

alenkacz · 2023-03-22T08:03:51Z

src/go/k8s/pkg/resources/certmanager/type_helpers.go

@@ -421,15 +431,109 @@ func (ac *apiCertificates) resources(
 		}
 	}

+	clientCACertSecretRef := ac.externalClientCACertificate
+	if clientCACertSecretRef != nil &&
+		clientCACertSecretRef.Name != "" &&


nit: this should also be validated in the webhook

alenkacz · 2023-03-22T08:05:06Z

src/go/k8s/pkg/resources/certmanager/type_helpers.go

+		return nil
+	}
+	return &types.NamespacedName{
+		Name:      "bundled-" + ac.externalClientCACertificate.Name,


is there any reason for including the externalCA in the name? we're just risking that e.g. the name becomes too long. Can't we just have it as a completely custom name?

You're right. I'll make it shorter.

On second thought, the name needs to depend on something unique (because each cert will have its own secret, so naming this one the same for different APIs would result in a conflict).

@alenkacz please review the latest changes, especifically the ones adding an id field to the struct.

I thought there will always be just one bundle like this per cluster, no?

re:

because each cert will have its own secret

alenkacz · 2023-03-22T08:07:15Z

src/go/k8s/pkg/resources/certmanager/type_helpers.go

+// bundledClientCACertificateName derives the name for the bundled client CA certificate name
+// from the provided externalClientCACertificate.
+func (ac *apiCertificates) bundledClientCACertificateName() *types.NamespacedName {
+	if ac.externalClientCACertificate == nil {


so what happens if external is not provided but we have only the non-generated. Is that still mounted properly? I bet yes, just asking out loud :D

Thanks for asking! In that case, the "bundled" CA will only be one CA cert. I'll make sure we cover that scenario in the kuttl tests.

src/go/k8s/pkg/resources/certmanager/type_helpers.go

The tee_wrapper helper was being used to set the ostream pointer for global logger to be an ostream instance that lived only on the stack, and not resetting it to something global like std::cerr after the test finished. This resulted in the following segfault in other tests that ran after the change but didn't make their own change to ostream pointer: ``` #0 0x7f875b0bbc1f in std::__1::basic_ostream<char, std::__1::char_traits<char> >::sentry::sentry(std::__1::basic_ostream<char, std::__1::char_traits<char> >&) (/vectorized/lib/libc++.so.1+0x65c1f) #1 0x5597c3979afc in std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::__put_character_sequence<char, std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, char const*, unsigned long) /vectorized/llvm/bin/../include/c++/v1/ostream:722:57 #2 0x5597c4079e44 in std::__1::basic_ostream<char, std::__1::char_traits<char> >& std::__1::operator<<<char, std::__1::char_traits<char> >(std::__1::basic_ostream<char, std::__1::char_traits<char> >&, std::__1::basic_string_view<char, std::__1::char_traits<char> >) /vectorized/llvm/bin/../include/c++/v1/ostream:1057:12 #3 0x5597c602c9f3 in seastar::logger::do_log(seastar::log_level, seastar::logger::log_writer&) /v/build/v_deps_build/seastar-prefix/src/seastar/src/util/log.cc:342:15 #4 0x5597c3c4ad20 in void seastar::logger::log<char const*, int, seastar::basic_sstring<char, unsigned int, 15u, true>&>(seastar::log_level, seastar::logger::format_info, char const*&&, int&&, seastar::basic_sstring<char, unsigned int, 15u, true>&) /vectorized/include/seastar/util/log.hh:231:17 redpanda-data#5 0x5597c3f23c51 in void seastar::logger::trace<char const*, int, seastar::basic_sstring<char, unsigned int, 15u, true>&>(seastar::logger::format_info, char const*&&, int&&, seastar::basic_sstring<char, unsigned int, 15u, true>&) /vectorized/include/seastar/util/log.hh:385:9 redpanda-data#6 0x5597c3f14b95 in cloud_roles::signature_v4::sign_header(boost::beast::http::header<true, boost::beast::http::basic_fields<std::__1::allocator<char> > >&, std::__1::basic_string_view<char, std::__1::char_traits<char> >) const /var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-00e717c5e253e2891-1/redpanda/vtools/src/v/cloud_roles/signature.cc:382:5 redpanda-data#7 0x5597c3bf1dfb in cloud_roles::apply_aws_credentials::add_auth(boost::beast::http::header<true, boost::beast::http::basic_fields<std::__1::allocator<char> > >&) const /var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-00e717c5e253e2891-1/redpanda/vtools/src/v/cloud_roles/apply_aws_credentials.cc:83:23 redpanda-data#8 0x5597c3b8f88d in cloud_roles::apply_credentials::add_auth(boost::beast::http::header<true, boost::beast::http::basic_fields<std::__1::allocator<char> > >&) const /var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-00e717c5e253e2891-1/redpanda/vtools/src/v/cloud_roles/apply_credentials.h:47:23 redpanda-data#9 0x5597c3b8c578 in test_aws_headers::test_method() /var/lib/buildkite-agent/builds/buildkite-amd64-builders-i-00e717c5e253e2891-1/redpanda/vtools/src/v/cloud_roles/tests/credential_tests.cc:50:17 ``` This change resets the ostream pointer in the wrapper destructor. Signed-off-by: Noah Watkins <[email protected]>

github-actions bot added the area/k8s label Mar 14, 2023

alenkacz reviewed Mar 15, 2023

View reviewed changes

src/go/k8s/pkg/resources/certmanager/type_helpers.go Outdated Show resolved Hide resolved

0x5d force-pushed the operator-mtls branch from 2cb1720 to c582b08 Compare March 15, 2023 19:53

github-actions bot added area/rpk area/redpanda labels Mar 15, 2023

0x5d force-pushed the operator-mtls branch 3 times, most recently from 27f1107 to f0ad40b Compare March 15, 2023 22:58

github-actions bot removed area/rpk area/redpanda labels Mar 15, 2023

alenkacz reviewed Mar 16, 2023

View reviewed changes

0x5d force-pushed the operator-mtls branch 8 times, most recently from 9bb8bff to 0083d93 Compare March 20, 2023 23:29

github-actions bot added the area/redpanda label Mar 20, 2023

0x5d force-pushed the operator-mtls branch 4 times, most recently from 5c9f8af to 32d3dfd Compare March 21, 2023 01:51

github-actions bot removed the area/redpanda label Mar 21, 2023

0x5d force-pushed the operator-mtls branch from 32d3dfd to 564992c Compare March 21, 2023 01:53

paulzhang97 reviewed Mar 21, 2023

View reviewed changes

0x5d force-pushed the operator-mtls branch 3 times, most recently from 64f5830 to 846590e Compare March 22, 2023 03:56

alenkacz reviewed Mar 22, 2023

View reviewed changes

paulzhang97 reviewed Mar 22, 2023

View reviewed changes

src/go/k8s/pkg/resources/certmanager/type_helpers.go Outdated Show resolved Hide resolved

0x5d force-pushed the operator-mtls branch 2 times, most recently from 6e8ed84 to c0e29a5 Compare March 22, 2023 22:41

0x5d and others added 8 commits March 22, 2023 18:12

k8s/cluster types: fix error messages

c7914af

k8s/type_helpers: simplify per-api certs initialization

692d135

k8s/cluster types: Add ClientCACert

322732c

apis,config: Regenerate code

3e0bd0d

k8s/tests: Add kuttl test for pandaproxy mTLS w/ external CA

2794ef0

k8s/tests: Add kuttl test for schema reg mTLS w/ external CA

77023de

k8s: Support user-provided CA certs

088a21d

wip

1fe920a

0x5d force-pushed the operator-mtls branch from c0e29a5 to 1fe920a Compare March 22, 2023 23:12

0x5d requested a review from alenkacz March 23, 2023 17:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Operator mtls #3

Operator mtls #3

0x5d commented Mar 14, 2023

alenkacz left a comment

alenkacz Mar 16, 2023

paulzhang97 Mar 17, 2023

alenkacz Mar 16, 2023

paulzhang97 Mar 17, 2023

paulzhang97 Mar 21, 2023

0x5d Mar 21, 2023

paulzhang97 Mar 21, 2023

paulzhang97 Mar 21, 2023

0x5d Mar 22, 2023

paulzhang97 Mar 21, 2023

paulzhang97 Mar 21, 2023

0x5d Mar 23, 2023

paulzhang97 Mar 21, 2023

paulzhang97 Mar 21, 2023

0x5d Mar 22, 2023

alenkacz Mar 22, 2023

alenkacz Mar 22, 2023

0x5d Mar 22, 2023

0x5d Mar 22, 2023

0x5d Mar 23, 2023

alenkacz Mar 24, 2023

alenkacz Mar 22, 2023

0x5d Mar 22, 2023 •

edited

Loading

		# Therefore, we need to create a namespace and create every resource in it
		# because the clientCACertRef must point to a specific namespace.

Operator mtls #3

Are you sure you want to change the base?

Operator mtls #3

Conversation

0x5d commented Mar 14, 2023

Backports Required

Release Notes

alenkacz 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

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

0x5d Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

0x5d Mar 22, 2023 •

edited

Loading