[platform] Mark some secrets as non-user-facing - #1446
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds labels Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant H as Helm Chart (vpn)
participant K as Kubernetes API
participant S as Secret (vpn/tls)
Note over H: Render vpn/templates/tls.yaml
H->>K: Lookup existing Secret (name)
alt Secret exists
K-->>H: Return data (expects ca.crt, tls.crt, tls.key)
H->>S: Render using existing keys
else Secret missing
H->>H: Generate signed cert and key
Note right of H: Changed: produce `ca.crt` (was `cacert`)
H->>S: Create Secret with data: ca.crt, tls.crt, tls.key
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
Summary of ChangesHello Timofei Larkin (@lllamnyp), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the platform's user experience and security by explicitly marking specific Kubernetes Secrets as non-user-facing. By applying a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly adds the apps.cozystack.io/tenantresource: "false" label to several internal secrets to hide them from the UI, which aligns with the stated goal. The changes are mostly correct and consistent.
I have one suggestion for a newly created secret in packages/extra/info/templates/serviceaccount.yaml which is missing this label.
Additionally, you might consider also labeling the secret in packages/apps/vpn/templates/tls.yaml. It appears to be an internal resource, but I couldn't add a specific comment as its metadata section is not part of the diff.
The unrelated change in packages/apps/vpn/templates/tls.yaml to rename cacert to ca.crt is a good fix that improves conformance with the kubernetes.io/tls secret type.
| metadata: | ||
| name: {{ .Release.Namespace }} | ||
| annotations: | ||
| kubernetes.io/service-account.name: {{ .Release.Namespace }} |
There was a problem hiding this comment.
This new secret is a service account token, which is an internal credential. To align with the goal of this pull request to hide non-user-facing secrets, you should add the apps.cozystack.io/tenantresource: "false" label.
metadata:
name: {{ .Release.Namespace }}
labels:
apps.cozystack.io/tenantresource: "false"
annotations:
kubernetes.io/service-account.name: {{ .Release.Namespace }}There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps/vpn/templates/secret.yaml (1)
65-66: Invalid top-level field in Secret (likely apply-time failure)Top-level key foo is not a valid field for a Secret and will be rejected by the API server. Remove it (stringData below already contains the intended content).
Apply this diff to remove the invalid field:
-foo: | - {{ toJson $passwords }}
🧹 Nitpick comments (1)
packages/extra/monitoring/templates/alerta/alerta.yaml (1)
18-20: Consider applying the tenantresource label here as wellThe alerta API key is internal; labeling it will prevent accidental exposure in UI views.
Apply:
metadata: name: alerta labels: app: alerta + apps.cozystack.io/tenantresource: "false"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/apps/clickhouse/templates/backup-script.yaml(1 hunks)packages/apps/mysql/templates/backup-script.yaml(1 hunks)packages/apps/postgres/templates/init-script.yaml(1 hunks)packages/apps/tenant/templates/tenant.yaml(0 hunks)packages/apps/vpn/templates/secret.yaml(1 hunks)packages/apps/vpn/templates/tls.yaml(1 hunks)packages/extra/info/templates/serviceaccount.yaml(1 hunks)packages/extra/monitoring/templates/alerta/alerta.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/apps/tenant/templates/tenant.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (6)
packages/apps/vpn/templates/secret.yaml (1)
25-26: LGTM: label addition aligns with PR intentGood use of apps.cozystack.io/tenantresource: "false" to hide the internal VPN Secret.
Confirm that the {{ .Release.Name }}-urls Secret is intentionally left without this label (likely user‑facing connection info). If it should also be hidden, mirror the label there.
packages/apps/postgres/templates/init-script.yaml (1)
33-34: LGTM: label added to non-user-facing init script SecretMatches the PR goal.
Do we also want to hide the {{ .Release.Name }}-credentials Secret? If yes, add the same label under its metadata:
metadata: name: {{ .Release.Name }}-credentials + labels: + apps.cozystack.io/tenantresource: "false"packages/apps/clickhouse/templates/backup-script.yaml (1)
7-8: LGTM: consistent label on backup script SecretNo concerns.
packages/extra/monitoring/templates/alerta/alerta.yaml (1)
195-196: LGTM: alertmanager Secret now hiddenLabel is appropriate for internal config.
Consider also labeling the earlier alerta API key Secret as non‑user‑facing for consistency:
Suggested change near the first Secret’s metadata.labels:
metadata: name: alerta labels: app: alerta apps.cozystack.io/tenantresource: "false"packages/apps/mysql/templates/backup-script.yaml (1)
7-8: LGTM: consistent label on backup script SecretMatches pattern across apps.
packages/extra/info/templates/serviceaccount.yaml (1)
1-9: ServiceAccount token Secret should be hidden; add labelThis Secret contains credentials — mark it non‑user‑facing to avoid UI exposure.
Apply this diff:
metadata: name: {{ .Release.Namespace }} + labels: + apps.cozystack.io/tenantresource: "false" annotations: kubernetes.io/service-account.name: {{ .Release.Namespace }}I could not find a ServiceAccount resource named exactly "{{ .Release.Namespace }}" in the repo (search found many SAs using helpers/other names). Confirm the ServiceAccount with that exact name exists; otherwise the token will not populate.
Optional: avoid name collisions by making the Secret name more specific:
- name: {{ .Release.Namespace }} + name: {{ .Release.Namespace }}-sa-token
| {{- else }} | ||
| {{- with genSignedCert $cn nil nil 3650 $ca }} | ||
| cacert: {{ b64enc $ca.Cert }} | ||
| ca.crt: {{ b64enc $ca.Cert }} |
There was a problem hiding this comment.
🧩 Analysis chain
CA key renamed to ca.crt — verify downstream consumers
Generation now writes ca.crt; existingSecret path already reads ca.crt. Ensure no remaining consumers expect cacert.
🏁 Script executed:
#!/bin/bash
# Find any references to 'cacert' that may need updating.
rg -n -C2 -i '\bcacert\b|\bca\.crt\b'Length of output: 156852
CA key rename — fix remaining consumer(s) expecting cacert
Generation now emits ca.crt (packages/apps/vpn/templates/tls.yaml); packages/system/kubeovn/charts/kube-ovn/templates/ovn-tls-secret.yaml still uses the cacert key (index $existingSecret.data "cacert" and b64enc $ca.Cert) — update that chart to use ca.crt or emit both keys for backward compatibility.
🤖 Prompt for AI Agents
In packages/apps/vpn/templates/tls.yaml around line 17 and in
packages/system/kubeovn/charts/kube-ovn/templates/ovn-tls-secret.yaml update the
CA key usage to match the new name: either (A) change all references in
ovn-tls-secret.yaml that index $existingSecret.data "cacert" (and any b64enc
$ca.Cert mappings) to use "ca.crt" instead, or (B) make the VPN TLS template
emit both keys by adding a duplicate entry `cacert: {{ b64enc $ca.Cert }}`
alongside `ca.crt: {{ b64enc $ca.Cert }}` so older consumers keep working;
ensure both files remain consistent and adjust any tests or consumers that
expect the old key.
Some k8s secrets created when deploying managed applications are unhelpful to the end user or are outright not meant to be shown, because they contain internal credentials not meant to be presented to the user. This patch adds an `apps.cozystack.io/tenantresource=false` label to such resources which will be later used to filter out such secrets in the web UI. ```release-note [platform] Mark non-user-facing secrets as such to avoid clutter in the dashboard and leaking internal credentials. ``` Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
52ff635 to
7e4883d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/apps/clickhouse/templates/backup-script.yaml(1 hunks)packages/apps/mysql/Chart.yaml(1 hunks)packages/apps/mysql/templates/backup-script.yaml(1 hunks)packages/apps/postgres/Chart.yaml(1 hunks)packages/apps/postgres/templates/init-script.yaml(1 hunks)packages/apps/tenant/Chart.yaml(1 hunks)packages/apps/tenant/templates/tenant.yaml(0 hunks)packages/apps/versions_map(5 hunks)packages/apps/vpn/Chart.yaml(1 hunks)packages/apps/vpn/templates/secret.yaml(1 hunks)packages/apps/vpn/templates/tls.yaml(1 hunks)packages/extra/info/Chart.yaml(1 hunks)packages/extra/info/templates/serviceaccount.yaml(1 hunks)packages/extra/monitoring/Chart.yaml(1 hunks)packages/extra/monitoring/templates/alerta/alerta.yaml(1 hunks)packages/extra/versions_map(2 hunks)
💤 Files with no reviewable changes (1)
- packages/apps/tenant/templates/tenant.yaml
✅ Files skipped from review due to trivial changes (6)
- packages/extra/info/Chart.yaml
- packages/apps/tenant/Chart.yaml
- packages/apps/mysql/Chart.yaml
- packages/extra/monitoring/Chart.yaml
- packages/apps/postgres/Chart.yaml
- packages/apps/versions_map
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/apps/postgres/templates/init-script.yaml
- packages/apps/mysql/templates/backup-script.yaml
- packages/apps/clickhouse/templates/backup-script.yaml
- packages/apps/vpn/templates/secret.yaml
- packages/apps/vpn/templates/tls.yaml
- packages/extra/info/templates/serviceaccount.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (4)
packages/apps/clickhouse/Chart.yaml (1)
19-25: Version/appVersion bump looks good.Semver bump to 0.13.1 and appVersion "24.9.2" are consistent with the PR scope.
Please confirm versions_map and any image tag templating (if referencing .Chart.AppVersion) are aligned with "24.9.2".
packages/apps/vpn/Chart.yaml (1)
19-25: Chart and appVersion bump LGTM.0.8.1 with appVersion "1.8.1" matches the described changes.
Double-check versions_map and release notes reference vpn 0.8.1 and app version "1.8.1".
packages/extra/versions_map (2)
24-25: Pin info 1.2.0 to SHA and add 1.2.1 HEAD — LGTM; please confirm mappingGood move to pin for reproducibility. Please confirm 53fbe7c indeed contains the info chart at version 1.2.0 and that 1.2.1 HEAD is intentional until it’s pinned before release.
61-62: Monitoring mapping updates — pinned SHA verified and mapping lines uniquePinned SHA 53fbe7c exists and the entries "monitoring 1.13.1 53fbe7c" and "monitoring 1.13.2 HEAD" each appear exactly once in packages/extra/versions_map.
| labels: | ||
| apps.cozystack.io/tenantresource: "false" |
There was a problem hiding this comment.
🧩 Analysis chain
Good: non-user-facing Secret labeled. Consider labeling the Alerta API key Secret too.
You’ve marked alertmanager as tenantresource=false. The Secret named "alerta" (holds API key) earlier in this file appears internal as well and likely should be hidden from end users per this PR’s goal.
Add the same label under the first Secret’s metadata.labels:
metadata:
name: alerta
labels:
app: alerta
apps.cozystack.io/tenantresource: "false"To review remaining Secrets lacking the label across templates, run:
🏁 Script executed:
#!/bin/bash
# List Secret blocks and show presence of the tenantresource label nearby for manual review
rg -nP -C4 '(?m)^\s*kind:\s*Secret\s*$' packages | sed 's/^/-- /'
echo
echo "Secrets with tenantresource label:"
rg -nP 'apps\.cozystack\.io/tenantresource:\s*"false"' packages -C2Length of output: 95801
Add tenantresource label to alerta Secret
The Secret "alerta" in packages/extra/monitoring/templates/alerta/alerta.yaml is missing apps.cozystack.io/tenantresource: "false" while the alertmanager Secret in the same file already has it — add the label to alerta's metadata.labels.
Location: packages/extra/monitoring/templates/alerta/alerta.yaml (Secret name: alerta)
Suggested snippet:
metadata:
name: alerta
labels:
app: alerta
apps.cozystack.io/tenantresource: "false"
🤖 Prompt for AI Agents
In packages/extra/monitoring/templates/alerta/alerta.yaml around lines 195-196,
the Secret resource named "alerta" is missing the metadata label
apps.cozystack.io/tenantresource: "false"; add this label under the alerta
Secret's metadata.labels (alongside app: alerta) so the Secret matches the
alertmanager Secret's labels, e.g., add apps.cozystack.io/tenantresource:
"false" to the metadata.labels block for the alerta Secret.
What this PR does
Some k8s secrets created when deploying managed applications are unhelpful to the end user or are outright not meant to be shown, because they contain internal credentials not meant to be presented to the user. This patch adds an
apps.cozystack.io/tenantresource=falselabel to such resources which will be later used to filter out such secrets in the web UI.Release note
Summary by CodeRabbit
New Features
Improvements
Removals
Chores