operator: support TLS admin API on V1 Cluster CRs (K8S-939) - #1817
operator: support TLS admin API on V1 Cluster CRs (K8S-939)#1817sago2k8 wants to merge 2 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
redpandaAdminForV1Cluster refused to build a client whenever
Cluster.AdminAPITLS() returned a listener, with the message "non-TLS
admin API is not supported on V1 CRD". The message is backwards: it
fires when TLS *is* configured. The guard existed because the builder
handed NewNodePoolInternalAdminAPI an empty ClusterCertificates{} as its
TLS provider, which would nil-dereference in getCommonTLS the moment the
internal listener had TLS enabled. Nothing ever resolved the
certificates.
The fallout: every consumer that reaches a vectorized.io/v1alpha1
Cluster over the admin API is dead as soon as that cluster enables admin
TLS. That is the User, Role and ShadowLink CRs, Broker CRs on V1 node
pools, and the ghost-broker decommissioner. Serverless cells run the
admin API over mTLS, so all of them were failing there.
AdminAPITLS() also matches external listeners, so a cluster with a
plaintext internal admin listener and TLS only on the external one was
rejected as well, even though the operator only ever talks to the
internal listener.
Resolve the certificates with v1ClusterCerts, like the V1 Kafka and
Schema Registry builders do, but only when the internal admin listener
has TLS enabled. NewClusterCertificates walks every API's listeners and
reads their Issuers and node secrets, and a plaintext admin client must
not fail on the cert-manager state of the Kafka or Schema Registry
listeners. NewNodePoolInternalAdminAPI now returns an error instead of
dereferencing a nil provider when TLS is on.
Take the FQDN from HeadlessServiceFQDN instead of a hand-built string so
it honours dnsTrailingDotDisabled. Apply the Factory's userAuth like the
V2 admin builder does; the V1 builder silently dropped credentials, so
the acceptance authentication check passed for any password on the
vectorized variant. Close the Kafka client in UsersForCluster when the
admin client fails to build; the new transient failure modes make that
path hot.
Fixes K8S-939.
Drive redpandaAdminForV1Cluster through the stub manager against an httptest server behind a recording dialer, so the test proves the client speaks https with the pinned CA, presents the client certificate under mTLS, and sends Basic auth, instead of only asserting that a struct came back. Missing node or client secrets surface by name, an untrusted CA fails with x509, and a missing Kafka Issuer no longer blocks a plaintext admin client. The external admin listener in the plaintext-internal case could not exist before: port 9645 is outside the nodeport range the webhook enforces, and TLS on an external listener requires a subdomain. Use a shape that validates. Refs K8S-939.
e366983 to
c5c6537
Compare
There was a problem hiding this comment.
Reviewed with an eye on house conventions. The shape is right: TLS keyed off the internal admin listener like the v2 builder, certs resolved through the same machinery as the v1 Kafka/SR clients, userAuth applied consistently, and the wire-level tests with negative cases are a real improvement. Inline comments are nits/observations.
| // test can pin the URL the builder derives without reaching into rpadmin, and | ||
| // a completed request proves the scheme, the trusted CA, and the client | ||
| // certificate all line up. | ||
| type adminServer struct { |
| return pair | ||
| } | ||
|
|
||
| // TestRedpandaAdminForV1Cluster covers the admin API client the Factory builds |
There was a problem hiding this comment.
The httptest harness exercising the real builder over real TLS is a big step up from mock-shaped assertions. Still worth a line in the PR body on whether an acceptance test with a TLS-enabled vectorized cluster is feasible — acceptance coverage has been requested on comparable user-facing fixes (#1347).
| // points its dialer at an httptest server, so the assertions cover what the | ||
| // client does on the wire rather than whether a struct came back. | ||
| func TestRedpandaAdminForV1Cluster(t *testing.T) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
nit: t.Context() over context.Background() in new tests.
| func v1ClusterCerts(ctx context.Context, k8sClient client.Client, cluster *vectorizedv1alpha1.Cluster) (string, *certmanager.ClusterCertificates, error) { | ||
| // v1ClusterFQDN returns the headless service FQDN of a V1 cluster; the | ||
| // internal clients dial its brokers as <pod>.<fqdn>. The cluster domain is | ||
| // hardcoded here while the V1 controller reads it from --cluster-domain, so |
There was a problem hiding this comment.
nit: add the tracking ticket here (TODO + K8S-xxx) — "tracked separately" in the PR body gets lost once this merges.
| var tlsConfig *tls.Config | ||
| if adminInternal.TLS.Enabled { | ||
| if adminTLSProvider == nil { | ||
| return nil, fmt.Errorf("internal admin API of cluster %s/%s has TLS enabled but no TLS provider was given", redpandaCluster.Namespace, redpandaCluster.Name) |
There was a problem hiding this comment.
nit: cockroachdb errors.Newf for new code (stack traces); fmt.Errorf matches the rest of this file though, so fine either way.
| @@ -0,0 +1,4 @@ | |||
| project: operator | |||
| kind: Fixed | |||
| body: Admin API clients for vectorized.io/v1alpha1 Clusters no longer fail with "non-TLS admin API is not supported on V1 CRD" when TLS is enabled on an admin listener. This unblocks every V2 CR and controller that reaches a V1 Cluster over the admin API (User, Role, and ShadowLink CRs, Broker CRs on V1 node pools, and the ghost-broker decommissioner). When the internal admin listener has TLS enabled the client now resolves the cluster's certificates the same way the V1 Kafka and Schema Registry clients do, and a plaintext internal listener keeps working regardless of TLS on the external admin listener or on other APIs' listeners | |||
There was a problem hiding this comment.
nit: this could shrink to a few lines naming the symptom and the affected mode — long single-paragraph entries tend to get rewritten in review.
| fqdn := v1ClusterFQDN(ctx, k8sClient, cluster) | ||
| var certs resourcetypes.AdminTLSConfigProvider | ||
| if internal := cluster.AdminAPIInternal(); internal != nil && internal.TLS.Enabled { | ||
| if _, certs, err = v1ClusterCerts(ctx, k8sClient, cluster); err != nil { |
There was a problem hiding this comment.
Observation, fine as-is: with TLS on, NewClusterCertificates walks every API, so broken cert state on an unrelated listener (e.g. a missing Kafka Issuer) now fails the admin client too. Consistent with the v1 Kafka/SR builders; a follow-up could narrow resolution to the admin graph.
| // with the TLS provider for its listeners. Building the provider resolves the | ||
| // certificate groups of every API, which reads the Issuers and node secrets | ||
| // the listeners reference. | ||
| func v1ClusterCerts(ctx context.Context, k8sClient client.Client, cluster *vectorizedv1alpha1.Cluster) (string, *certmanager.ClusterCertificates, error) { |
There was a problem hiding this comment.
certmanager already exposes GetTLSConfigValues() *ir.TLSConfig, and the ShadowLink path later in this file already routes v1 TLS through ir.TLSConfig. Routing the admin path through IR the same way would let v1 and v2 share one client-construction core instead of parallel builders.
|
@sago2k8 I did a first pass but we probably should get @andrewstucki to take a look as well. |
3 participants
What
Build a working admin API client for
vectorized.io/v1alpha1Clusters that have TLS on the internal admin listener. The v1 path inpkg/clientnow resolves the cluster's certificates throughv1ClusterCerts, the same helper the v1 Kafka and Schema Registry clients already use, but only when the internal admin listener actually has TLS enabled.Why
redpandaAdminForV1Clusterrefused to build a client wheneverCluster.AdminAPITLS()returned a listener and failed with "non-TLS admin API is not supported on V1 CRD". The check is backwards: it fires when TLS is on. The guard existed because the builder handedNewNodePoolInternalAdminAPIan empty certificate stub that would nil-dereference as soon as the internal listener had TLS. Nothing ever resolved the certificates.Every consumer that reaches a v1 Cluster over the admin API goes through this path: User, Role and ShadowLink CRs, Broker CRs on v1 node pools, and the ghost-broker decommissioner. Serverless cells run the admin API over mTLS, so on them:
RedpandaRoleCRs never sync. Cloud RBAC grants do not land as ACLs. This is what fires the "DP - Policy Materializer Roles Not Syncing" sev 2 alert.Two smaller defects in the same lines:
AdminAPITLS()also matches external listeners, so plaintext-internal + TLS-external was rejected too, and the hardcoded FQDN ignoreddnsTrailingDotDisabled.Implementation details
NewClusterCertificateswalks every API's listeners and reads their Issuers and node secrets, so a plaintext admin client must not fail on the cert-manager state of the Kafka or Schema Registry listeners.v1ClusterFQDNis split out so the plaintext path gets the FQDN without building the certificate graph.NewNodePoolInternalAdminAPIreturns an error when TLS is enabled and no provider was given, instead of dereferencing nil.userAuthis applied like the v2 admin builder does. The v1 builder silently dropped credentials, so the acceptance authentication check passed for any password on the vectorized variant.UsersForClustercloses the Kafka client when the admin client fails to build. The new transient failure modes (node cert not issued yet) make that path hot.a.(*rpadmin.AdminAPI)assertion is now checked.operator/pkg/client/v1_admin_tls_test.godrives the real builder through the stub manager against an httptest broker behind a recording dialer: https with the pinned CA on the dotted host, an untrusted CA failing with x509, mTLS presenting<cluster>-admin-api-client, missing node and client secrets surfacing by name, a missing Kafka Issuer not blocking a plaintext client, and the Basic auth header on the wire.v1ClusterCertshardcodescluster.localwhile the v1 controller mints SANs from--cluster-domain. Documented onv1ClusterFQDN, tracked separately.clusterissuersread, so a v1 Cluster withissuerRefkindClusterIssuergets Forbidden. Tracked separately.References