feat(kafka): add TLS support via Strimzi listener configuration - #2681

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/apps/kafka/Makefile (1)

6-8: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add a hard assertion after the sed patch for TLS.Enabled.

The sed replacement can silently no-op when generated formatting changes, which leaves the Go type as non-nullable while schema patching still succeeds. Fail fast by asserting Enabled *bool immediately after sed.

Suggested fix

-	sed -i 's/Enabled bool `json:"enabled,omitempty"`/Enabled *bool `json:"enabled,omitempty"`/' ../../../api/apps/v1alpha1/kafka/types.go
+	sed -i -E 's/Enabled[[:space:]]+bool[[:space:]]+`json:"enabled,omitempty"`/Enabled *bool `json:"enabled,omitempty"`/' ../../../api/apps/v1alpha1/kafka/types.go
+	grep -Eq 'Enabled[[:space:]]+\*bool[[:space:]]+`json:"enabled,omitempty"`' ../../../api/apps/v1alpha1/kafka/types.go || (echo "ERROR: failed to patch TLS.Enabled to *bool" && exit 1)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kafka/Makefile` around lines 6 - 8, Add a hard assertion
immediately after the sed patch to fail the Makefile if the Go type change
didn't apply: after running the sed that intends to change the Kafka TLS struct
field, grep the generated Go type (look for the symbol `Enabled *bool` in
`types.go`) and exit non-zero with a clear error if that exact string is not
present; ensure the check runs before proceeding to the jq schema edits so the
build fails fast when the `Enabled` field remains non-nullable.

packages/apps/kafka/templates/kafka.yaml (1)

15-21: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle tls.enabled: null as unset before rendering listener TLS.

Current explicitness checks only key presence, so enabled: null is treated as explicit instead of inheriting from external. Use a non-nil explicit check and reuse the raw value.

Suggested fix

 {{- $tlsMap := .Values.tls | default dict -}}
-{{- $tlsEnabledExplicit := hasKey $tlsMap "enabled" -}}
+{{- $tlsEnabledRaw := index $tlsMap "enabled" -}}
+{{- $tlsEnabledExplicit := and (hasKey $tlsMap "enabled") (ne $tlsEnabledRaw nil) -}}
 {{- $tlsEnabled := false -}}
 {{- if $tlsEnabledExplicit -}}
-  {{- $tlsEnabled = index $tlsMap "enabled" -}}
+  {{- $tlsEnabled = $tlsEnabledRaw -}}
 {{- else -}}
   {{- $tlsEnabled = .Values.external | default false -}}
 {{- end -}}

Also applies to: 47-47

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kafka/templates/kafka.yaml` around lines 15 - 21, The template
treats tls.enabled: null as explicitly set; change the explicitness check to
detect non-nil values instead of just key presence: compute the raw value from
$tlsMap (index $tlsMap "enabled") and treat it as explicit only if the key
exists and the value is not nil, then assign $tlsEnabled from that raw value;
otherwise fall back to .Values.external | default false. Update the same logic
used around the later occurrence (line ~47) as well, referencing variables
$tlsMap, $tlsEnabledExplicit and $tlsEnabled.

packages/apps/kafka/templates/dashboard-resourcemap.yaml (1)

1-9: ⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoff

Gate $showExternal solely on .Values.external to prevent over-permissive RBAC.

The logic $showExternal := or .Values.external (and $tlsEnabledExplicit $tlsEnabled) (line 9) still allows RBAC to be granted for the external-bootstrap secret when external=false but tls.enabled=true. According to the PR objectives, external listener rendering is gated solely on external: true, so RBAC should follow the same gate. When external=false, no external listener exists regardless of tls.enabled (as verified by the test in kafka_tls_test.yaml lines 88-98), making this RBAC grant unnecessary and over-permissive.

🔒 Proposed fix to align RBAC with external-only gate

-{{- $tlsMap := .Values.tls | default dict -}}
-{{- $tlsEnabledExplicit := hasKey $tlsMap "enabled" -}}
-{{- $tlsEnabled := false -}}
-{{- if $tlsEnabledExplicit -}}
-  {{- $tlsEnabled = index $tlsMap "enabled" -}}
-{{- else -}}
-  {{- $tlsEnabled = .Values.external | default false -}}
-{{- end -}}
-{{- $showExternal := or .Values.external (and $tlsEnabledExplicit $tlsEnabled) -}}
+{{- $showExternal := .Values.external | default false -}}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kafka/templates/dashboard-resourcemap.yaml` around lines 1 - 9,
The $showExternal gate is computed as or .Values.external (and
$tlsEnabledExplicit $tlsEnabled) which allows RBAC when tls.enabled=true even if
external=false; change the $showExternal assignment to rely solely on
.Values.external (e.g. set $showExternal := .Values.external | default false) so
RBAC and external-bootstrap rendering are only enabled when .Values.external is
true; update the template where $showExternal is defined and remove the
tls-based condition using the existing variables ($tlsEnabled,
$tlsEnabledExplicit) so the gate matches the external-only requirement.

packages/apps/kafka/tests/dashboard-resourcemap_test.yaml (1)

121-134: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Test validates over-permissive RBAC behavior that contradicts PR objectives.

This test asserts that external-bootstrap RBAC is granted when external=false and tls.enabled=true. However, per the PR objectives, the external listener (and its bootstrap secret) only exists when external: true. The comprehensive TLS test suite (kafka_tls_test.yaml lines 88-98) confirms that no external listener is created in this configuration. Therefore, granting RBAC access to a non-existent secret is over-permissive and should not be enforced by tests.

🧪 Proposed test fix to assert correct (restrictive) behavior

-  # External bootstrap service present when tls.enabled=true and external=false ($showExternal is true)
-  - it: grants access to external bootstrap service when tls.enabled=true and external=false
+  # External bootstrap service absent when external=false, regardless of tls.enabled
+  - it: does not grant access to external bootstrap service when tls.enabled=true and external=false
     release:
       name: test-kafka
       namespace: tenant-test
     set:
       external: false
       tls:
         enabled: true
     asserts:
-      - contains:
+      - notContains:
           path: rules[0].resourceNames
           content: test-kafka-kafka-external-bootstrap
         documentIndex: 0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/apps/kafka/tests/dashboard-resourcemap_test.yaml` around lines 121 -
134, The test "grants access to external bootstrap service when tls.enabled=true
and external=false" asserts RBAC for a non-existent external bootstrap secret;
change it to assert restrictive behavior instead: replace the positive contains
assertion on rules[0].resourceNames -> "test-kafka-kafka-external-bootstrap"
with a negative check (ensure the rule does NOT contain that resource name or
assert that no rule references the external bootstrap secret) so when set:
external: false and tls.enabled: true the test verifies absence of
external-bootstrap RBAC (use the existing test case identifier and the same
documentIndex/rules[0].resourceNames references to locate and update the
assertion).

hack/check-kafka-rd-secrets.bats (1)

8-17: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Add cluster CA validation tests to match the clients CA pattern.

The test suite validates clients-ca-cert naming but omits parallel checks for cluster-ca-cert, which is also referenced in the system schema (kafka.yaml line 34). Without these tests, a regression could drop the cluster-ca-cert secret or reintroduce the bare cluster-ca name without detection.

🧪 Proposed test additions for cluster CA

 `@test` "kafka-rd cozyrds references clients-ca-cert (Strimzi actual name)" {
   grep -q "clients-ca-cert" "$COZYRDS"
 }
 
+@test "kafka-rd cozyrds references cluster-ca-cert (Strimzi actual name)" {
+  grep -q "cluster-ca-cert" "$COZYRDS"
+}
+
 `@test` "kafka-rd cozyrds does not reference bare clients-ca (wrong name)" {
   if grep -qP "clients-ca(?!-)" "$COZYRDS"; then
     echo "Found bare 'clients-ca' reference (missing '-cert' suffix)" >&2
     exit 1
   fi
 }
+
+@test "kafka-rd cozyrds does not reference bare cluster-ca (wrong name)" {
+  if grep -qP "cluster-ca(?!-)" "$COZYRDS"; then
+    echo "Found bare 'cluster-ca' reference (missing '-cert' suffix)" >&2
+    exit 1
+  fi
+}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@hack/check-kafka-rd-secrets.bats` around lines 8 - 17, Add two parallel BATS
tests for the cluster CA: mirror the existing tests for clients-ca-cert by
adding a positive test that greps for "cluster-ca-cert" (similar to `@test`
"kafka-rd cozyrds references clients-ca-cert") and a negative test that fails if
a bare "cluster-ca" (without "-cert") is found (parallel to the `@test` that
checks for bare "clients-ca"). Place the new tests adjacent to the existing ones
so they validate presence of "cluster-ca-cert" and reject any occurrences
matching the regex used to detect bare names (the same grep -q and grep -qP
pattern logic as the clients-ca tests).