Skip to content

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

Open
Arsolitt wants to merge 7 commits into
mainfrom
feat/tls-kafka
Open

feat(kafka): add TLS support via Strimzi listener configuration#2681
Arsolitt wants to merge 7 commits into
mainfrom
feat/tls-kafka

Conversation

@Arsolitt
Copy link
Copy Markdown
Contributor

@Arsolitt Arsolitt commented May 19, 2026

What this PR does

Adds TLS support to the Kafka managed service via Strimzi listener configuration.

  • Introduces a tls.enabled field that controls the external listener (port 9094). When unset, defaults to the value of external (auto-on when external access is enabled). Explicit tls.enabled always wins.
  • Strimzi manages the cluster PKI automatically; the operator publishes <release>-cluster-ca-cert and <release>-clients-ca-cert for client trust setup. No cert-manager chain is rendered by this chart — the operator owns the chain end-to-end.
  • The internal TLS listener on 9093 is always on (Strimzi default); this field only governs the external listener on 9094.
  • External LoadBalancer listener is decoupled from tls.enabled: it is gated on external: true only. Previously, an explicit tls.enabled: true with external: false would force the external listener to be rendered.

Verified end-to-end on a sandbox cluster: external listener exposes a TLS endpoint signed by the Strimzi-managed cluster CA, client connections with the published trust bundle succeed.

Note: this chart does not configure listener authentication. With TLS on the external listener, clients can connect without credentials unless KafkaUser resources with tls/scram-sha-512/oauth authentication are configured outside this chart.

Release note

feat(kafka): add TLS support on the external listener via Strimzi (tri-state tls.enabled)

Summary by CodeRabbit

  • New Features

    • Added optional TLS configuration control for the external Kafka listener, enabling independent management of TLS encryption separate from listener creation.
  • Documentation

    • Expanded configuration documentation with TLS parameter details and authentication implications.
  • Tests

    • Added validation tests for TLS schema compliance and external listener configuration behavior.

Review Change Stack

Arsolitt added 4 commits May 19, 2026 14:33
- Add tri-state tls.enabled (*bool) to Kafka API types for strict nil/true/false semantics
- Extend values.yaml and JSON schema with TLS configuration block
- Update Makefile with sed+jq patching for nullable bool schema and test target
- Update cozyrds schema to reflect nullable tls.enabled type
- Update README with Strimzi-managed PKI documentation and authentication notes

Signed-off-by: Arsolitt <arsolitt@gmail.com>
- Configure Kafka CR with TLS listener spec using Strimzi-managed PKI
- Gate external bootstrap RBAC resources on $showExternal predicate
- Use resolved $tlsEnabled variable for consistent tri-state evaluation
- Add Strimzi clients CA secret reference to dashboard resourcemap

Signed-off-by: Arsolitt <arsolitt@gmail.com>
- Add helm-unittest suite covering TLS listener config, tri-state nil/true/false cases
- Cover external=false + tls.enabled=false negative combinations
- Add dashboard resourcemap tests for CA secret inclusion
- Add bats tests for cozyrds schema nullable field and Strimzi CA secret naming

Signed-off-by: Arsolitt <arsolitt@gmail.com>
The external LoadBalancer listener (port 9094) was rendered whenever
tls.enabled was explicitly true, regardless of the external flag. This caused
Strimzi to stall waiting for a LoadBalancer IP when tls.enabled=true but
external=false.

External is now the sole gate for the listener. tls.enabled only controls
TLS on the listener when it is rendered (auto-on when external=true and
tls.enabled is unset, explicit value otherwise). Update tests to match.

Signed-off-by: Arsolitt <arsolitt@gmail.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

📝 Walkthrough

Walkthrough

This PR adds an optional tls.enabled field to Kafka's external listener configuration, decoupling TLS control from the external flag. When unset, TLS inherits from external; when explicitly set, it overrides. Changes span API types, Helm values/schema, template logic with tri-state TLS behavior, RBAC updates, system integration, and comprehensive test coverage.

Changes

Kafka External Listener TLS Control

Layer / File(s) Summary
API Type Contract
api/apps/v1alpha1/kafka/types.go, api/apps/v1alpha1/kafka/zz_generated.deepcopy.go
New TLS struct with nullable Enabled field added to ConfigSpec. Autogenerated deepcopy methods updated to handle Tls field and new TLS type.
Helm Values Schema and Defaults
packages/apps/kafka/values.yaml, packages/apps/kafka/values.schema.json
Helm chart values now include tls.enabled (nullable boolean) with documentation. JSON schema defines field type and interaction with external flag.
Build Process and Schema Alignment
packages/apps/kafka/Makefile
generate target updated to ensure enabled field is nullable in both API type and values schema, with validation checks to fail on mismatch.
External Listener TLS Implementation
packages/apps/kafka/templates/kafka.yaml
Template implements tri-state TLS: external listener created when external: true; TLS on listener driven by explicit tls.enabled or inherited from external.
RBAC and Resource Mapping Updates
packages/apps/kafka/templates/dashboard-resourcemap.yaml
Dashboard resourcemap conditionally grants RBAC for external bootstrap secret based on tri-state logic. Updates secret resourceNames from clients-ca to clients-ca-cert and cluster-ca-cert.
System-Level Resource Definition
packages/system/kafka-rd/cozyrds/kafka.yaml
System Kafka resource now includes tls.enabled schema in openAPISchema. Includes both clients-ca-cert and cluster-ca-cert in secret references and reorders schema keys.
Helm Template Tests
packages/apps/kafka/tests/kafka_test.yaml, packages/apps/kafka/tests/kafka_tls_test.yaml, packages/apps/kafka/tests/dashboard-resourcemap_test.yaml
Updated existing tests for TLS behavior; added comprehensive TLS suite covering all external/tls.enabled combinations. Expanded RBAC tests for CA secret access and TLS-conditional bootstrap behavior.
Validation Tests and Documentation
hack/check-kafka-rd-schema.bats, hack/check-kafka-rd-secrets.bats, packages/apps/kafka/README.md
Bats tests validate schema structure (nullable boolean type) and correct secret naming. README documents tls.enabled parameter behavior and authentication implications.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A listener once bound by external's decree,
Now wears a config cape: TLS runs free!
When tls.enabled whispers true or stays mute,
The RBAC hops closer—no secrets to loot.
With tests aplenty and schema so bright,
This tri-state dance sets encryption right! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding TLS support to Kafka via Strimzi listener configuration, which is the primary focus across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/tls-kafka

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added area/database Issues or PRs related to managed databases (postgres, mariadb, redis, etcd, kafka, clickhouse) kind/feature Categorizes issue or PR as related to a new feature size/L This PR changes 100-499 lines, ignoring generated files labels May 19, 2026
Arsolitt added 3 commits May 19, 2026 14:58
Signed-off-by: Arsolitt <arsolitt@gmail.com>
Replace double-bracket [[ ... ]] with POSIX [ ... ] in
hack/check-kafka-rd-schema.bats so it runs under dash, which is what /bin/sh
resolves to on the Ubuntu CI runners. The test passed locally because
/bin/sh resolves to bash on most developer machines.

Signed-off-by: Arsolitt <arsolitt@gmail.com>
dash's echo interprets backslash escapes by default (unlike bash), which
mangles double-backslash sequences in the embedded JSON pattern strings and
makes jq report an invalid-escape parse error. printf '%s' passes the value
through untouched.

Signed-off-by: Arsolitt <arsolitt@gmail.com>
@Arsolitt Arsolitt marked this pull request as ready for review May 19, 2026 13:03
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with 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.

Inline comments:
In `@hack/check-kafka-rd-secrets.bats`:
- Around line 8-17: Add two tests mirroring the clients-ca checks: create a test
named like 'kafka-rd cozyrds references cluster-ca-cert (Strimzi actual name)'
that greps for "cluster-ca-cert" in the $COZYRDS file, and a test named like
'kafka-rd cozyrds does not reference bare cluster-ca (wrong name)' that fails if
grep -qP "cluster-ca(?!-)" finds a match; follow the same structure and exit
behavior as the existing clients-ca tests so the suite enforces presence of
"cluster-ca-cert" and rejects bare "cluster-ca".

In `@packages/apps/kafka/Makefile`:
- Around line 6-8: The sed patch that replaces "Enabled bool
`json:\"enabled,omitempty\"`" with "Enabled *bool `json:\"enabled,omitempty\"`"
can silently no-op; after running sed (the existing sed command that targets
types.go), add a hard assertion that verifies the Go source now contains the
pointer field (look for the exact token Enabled *bool `json:"enabled,omitempty"`
in types.go) and fail the Makefile target if the check does not match; keep the
existing jq schema changes but ensure the new assertion runs immediately after
the sed step so the build fails if the Go type wasn't actually updated.

In `@packages/apps/kafka/README.md`:
- Line 11: Update the README row for the tls.enabled Helm value to reflect that
its default is unset/null (tri-state inheritance) rather than `false`; change
the default column and the wording so it clearly states the value is
"unset/null" and explains it inherits from `external` when not set, and ensure
`tls.enabled` is referenced exactly as written so the README aligns with the
chart contract.

In `@packages/apps/kafka/templates/dashboard-resourcemap.yaml`:
- Line 9: The RBAC for external-bootstrap is being enabled when tls.enabled is
true because the template sets {{$showExternal := or .Values.external (and
$tlsEnabledExplicit $tlsEnabled)}}, so update the gating logic to require
.Values.external explicitly (remove the TLS-based OR) and apply the same change
to the other occurrences noted (the blocks around lines 21-23) so that
external-bootstrap roles/subjects are only created when .Values.external is
true; search for $showExternal and replace the expression and any conditional
blocks that use it to use .Values.external directly.

In `@packages/apps/kafka/templates/kafka.yaml`:
- Around line 15-21: The template treats a present but null tls.enabled as
explicit, causing tls: null to render; fix by first capturing the raw value
(e.g. introduce $tlsEnabledRaw := index $tlsMap "enabled"), then change the
explicit check to require both key presence and non-nil (use and (hasKey $tlsMap
"enabled") (ne $tlsEnabledRaw nil)), and when that check passes assign
$tlsEnabled = $tlsEnabledRaw, otherwise fall back to the existing
.Values.external | default false; update all uses of
$tlsEnabledExplicit/$tlsEnabled to the new logic so a null value is treated as
unset.

In `@packages/apps/kafka/tests/dashboard-resourcemap_test.yaml`:
- Around line 121-134: The test "grants access to external bootstrap service
when tls.enabled=true and external=false" is asserting over-permissive behavior;
update the case to assert that external bootstrap RBAC is NOT granted when
external: false by changing the spec under "asserts" to verify the resourceNames
does not include "test-kafka-kafka-external-bootstrap" (e.g., replace the
contains check on path rules[0].resourceNames/content
test-kafka-kafka-external-bootstrap with a not-contains or absence check), and
adjust the test description to reflect the corrected expectation.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 07025e25-986f-4953-ba4b-1b179a09393b

📥 Commits

Reviewing files that changed from the base of the PR and between c5e9287 and c806691.

📒 Files selected for processing (14)
  • api/apps/v1alpha1/kafka/types.go
  • api/apps/v1alpha1/kafka/zz_generated.deepcopy.go
  • hack/check-kafka-rd-schema.bats
  • hack/check-kafka-rd-secrets.bats
  • packages/apps/kafka/Makefile
  • packages/apps/kafka/README.md
  • packages/apps/kafka/templates/dashboard-resourcemap.yaml
  • packages/apps/kafka/templates/kafka.yaml
  • packages/apps/kafka/tests/dashboard-resourcemap_test.yaml
  • packages/apps/kafka/tests/kafka_test.yaml
  • packages/apps/kafka/tests/kafka_tls_test.yaml
  • packages/apps/kafka/values.schema.json
  • packages/apps/kafka/values.yaml
  • packages/system/kafka-rd/cozyrds/kafka.yaml

Comment on lines +8 to +17
@test "kafka-rd cozyrds references clients-ca-cert (Strimzi actual name)" {
grep -q "clients-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
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add cluster CA naming checks to match the same contract.

This file only guards clients-ca-cert; it should also assert cluster-ca-cert and reject bare cluster-ca to prevent partial regressions.

Suggested test additions
 `@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
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@test "kafka-rd cozyrds references clients-ca-cert (Strimzi actual name)" {
grep -q "clients-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 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 tests
mirroring the clients-ca checks: create a test named like 'kafka-rd cozyrds
references cluster-ca-cert (Strimzi actual name)' that greps for
"cluster-ca-cert" in the $COZYRDS file, and a test named like 'kafka-rd cozyrds
does not reference bare cluster-ca (wrong name)' that fails if grep -qP
"cluster-ca(?!-)" finds a match; follow the same structure and exit behavior as
the existing clients-ca tests so the suite enforces presence of
"cluster-ca-cert" and rejects bare "cluster-ca".

Comment on lines +6 to +8
sed -i 's/Enabled bool `json:"enabled,omitempty"`/Enabled *bool `json:"enabled,omitempty"`/' ../../../api/apps/v1alpha1/kafka/types.go
jq '.properties.tls.properties.enabled.type = ["boolean", "null"]' values.schema.json > values.schema.json.tmp && mv values.schema.json.tmp values.schema.json
jq -e '.properties.tls.properties.enabled.type == ["boolean", "null"]' values.schema.json > /dev/null || (echo "ERROR: jq patch failed" && exit 1)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Guard the Go type patch with a hard assertion.

The sed mutation can silently no-op if cozyvalues-gen output formatting changes, which can reintroduce non-nullable Enabled bool while the schema patch still passes. Add an explicit post-patch check for the pointer type.

Suggested fix
 generate:
 	cozyvalues-gen -m 'kafka' -v values.yaml -s values.schema.json -r README.md -g ../../../api/apps/v1alpha1/kafka/types.go
-	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)
 	jq '.properties.tls.properties.enabled.type = ["boolean", "null"]' values.schema.json > values.schema.json.tmp && mv values.schema.json.tmp values.schema.json
 	jq -e '.properties.tls.properties.enabled.type == ["boolean", "null"]' values.schema.json > /dev/null || (echo "ERROR: jq patch failed" && exit 1)
 	../../../hack/update-crd.sh
🤖 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, The sed patch that replaces
"Enabled bool `json:\"enabled,omitempty\"`" with "Enabled *bool
`json:\"enabled,omitempty\"`" can silently no-op; after running sed (the
existing sed command that targets types.go), add a hard assertion that verifies
the Go source now contains the pointer field (look for the exact token Enabled
*bool `json:"enabled,omitempty"` in types.go) and fail the Makefile target if
the check does not match; keep the existing jq schema changes but ensure the new
assertion runs immediately after the sed step so the build fails if the Go type
wasn't actually updated.

| ------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ------- |
| `external` | Enable external access from outside the cluster. | `bool` | `false` |
| `tls` | TLS configuration. Strimzi manages the cluster PKI automatically (no cert-manager is involved for this chart): the operator auto-creates `<release>-cluster-ca-cert` and `<release>-clients-ca-cert` secrets, both exposed for client trust setup. The internal TLS listener on 9093 is always on; this toggle only controls the external listener on 9094. | `object` | `{}` |
| `tls.enabled` | Enable TLS on the external listener. When unset, inherits the value of `external` (TLS is on when external access is enabled). Warning: setting this to false while external is true exposes Kafka over plaintext on a public IP via LoadBalancer. Strimzi does not provide authentication on this listener unless SCRAM, mTLS, or OAuth is separately configured. Use only in controlled networks. | `bool` | `false` |
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Document tls.enabled default as unset/null, not false.

The row describes tri-state inheritance but lists default false, which is misleading. This should reflect an unset/null default so behavior matches the chart contract.

🤖 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/README.md` at line 11, Update the README row for the
tls.enabled Helm value to reflect that its default is unset/null (tri-state
inheritance) rather than `false`; change the default column and the wording so
it clearly states the value is "unset/null" and explains it inherits from
`external` when not set, and ensure `tls.enabled` is referenced exactly as
written so the README aligns with the chart contract.

{{- else -}}
{{- $tlsEnabled = .Values.external | default false -}}
{{- end -}}
{{- $showExternal := or .Values.external (and $tlsEnabledExplicit $tlsEnabled) -}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate external-bootstrap RBAC only on external.

Line 9 still grants external-bootstrap access when external=false and tls.enabled=true. This reintroduces coupling and violates least-privilege for RBAC.

Proposed fix
-{{- $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 -}}

Also applies to: 21-23

🤖 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` at line 9, The RBAC
for external-bootstrap is being enabled when tls.enabled is true because the
template sets {{$showExternal := or .Values.external (and $tlsEnabledExplicit
$tlsEnabled)}}, so update the gating logic to require .Values.external
explicitly (remove the TLS-based OR) and apply the same change to the other
occurrences noted (the blocks around lines 21-23) so that external-bootstrap
roles/subjects are only created when .Values.external is true; search for
$showExternal and replace the expression and any conditional blocks that use it
to use .Values.external directly.

Comment on lines +15 to +21
{{- $tlsEnabledExplicit := hasKey $tlsMap "enabled" -}}
{{- $tlsEnabled := false -}}
{{- if $tlsEnabledExplicit -}}
{{- $tlsEnabled = index $tlsMap "enabled" -}}
{{- else -}}
{{- $tlsEnabled = .Values.external | default false -}}
{{- end -}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Confirm schema allows null for tls.enabled:"
rg -n '"enabled".*"type":\["boolean","null"\]' packages/system/kafka-rd/cozyrds/kafka.yaml

echo
echo "Confirm template uses key-presence explicitness and renders tls from that value:"
rg -n 'hasKey \$tlsMap "enabled"|index \$tlsMap "enabled"|tls: \{\{ \$tlsEnabled \}\}' packages/apps/kafka/templates/kafka.yaml

Repository: cozystack/cozystack

Length of output: 5898


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

The schema allows enabled: null, but the current logic at line 15 only checks key presence via hasKey, not nil values. When tls.enabled: null is set, line 15 evaluates $tlsEnabledExplicit to true, then line 18 assigns the null value to $tlsEnabled, and line 47 renders tls: null—breaking the tri-state contract and producing an invalid Kafka CR.

The fix distinguishes between "key exists with a boolean" and "key exists but is null" (which should behave as unset). Capture the raw value, update the explicit check to and (hasKey $tlsMap "enabled") (ne $tlsEnabledRaw nil), and reuse it in the conditional.

Proposed 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 a present but null tls.enabled as explicit, causing tls: null to render;
fix by first capturing the raw value (e.g. introduce $tlsEnabledRaw := index
$tlsMap "enabled"), then change the explicit check to require both key presence
and non-nil (use and (hasKey $tlsMap "enabled") (ne $tlsEnabledRaw nil)), and
when that check passes assign $tlsEnabled = $tlsEnabledRaw, otherwise fall back
to the existing .Values.external | default false; update all uses of
$tlsEnabledExplicit/$tlsEnabled to the new logic so a null value is treated as
unset.

Comment on lines +121 to +134
# 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
release:
name: test-kafka
namespace: tenant-test
set:
external: false
tls:
enabled: true
asserts:
- contains:
path: rules[0].resourceNames
content: test-kafka-kafka-external-bootstrap
documentIndex: 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

External bootstrap RBAC should not be granted when external=false.

This test enforces over-permissive behavior (external=false, tls.enabled=true still grants external bootstrap access). It contradicts the external-only gate and can preserve unnecessary RBAC rights.

Suggested test fix
-  # 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
@@
-    asserts:
-      - contains:
+    asserts:
+      - notContains:
           path: rules[0].resourceNames
           content: test-kafka-kafka-external-bootstrap
         documentIndex: 0
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# 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
release:
name: test-kafka
namespace: tenant-test
set:
external: false
tls:
enabled: true
asserts:
- contains:
path: rules[0].resourceNames
content: test-kafka-kafka-external-bootstrap
documentIndex: 0
# 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:
- 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" is asserting over-permissive behavior; update the case to
assert that external bootstrap RBAC is NOT granted when external: false by
changing the spec under "asserts" to verify the resourceNames does not include
"test-kafka-kafka-external-bootstrap" (e.g., replace the contains check on path
rules[0].resourceNames/content test-kafka-kafka-external-bootstrap with a
not-contains or absence check), and adjust the test description to reflect the
corrected expectation.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the summary. You can try again by commenting /gemini summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/database Issues or PRs related to managed databases (postgres, mariadb, redis, etcd, kafka, clickhouse) kind/feature Categorizes issue or PR as related to a new feature size/L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant