fix(memos): align template standards#671
Open
mberlofa wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:memos
size:L
labels
Contributor
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
📝 WalkthroughWalkthroughThis PR updates chart validation, ingress and network policy rendering, StatefulSet pod labels, and NOTES output, with supporting schema, values, and test changes. ChangesChart validation, networking, and documentation updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 84.84849% |
✅ Security posture acceptable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
charts/memos/templates/networkpolicy.yaml (2)
16-18: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winEgress isolation is only reachable via
extraEgress; no way to enable DNS+HTTPS-only isolation.Per the README, egress isolation (and the
EgresspolicyType) only activates whenextraEgressis non-empty. A user who wants only the baseline DNS/HTTPS egress restriction, without any additional rules, has no supported way to enable it short of passing a placeholder rule. Consider decoupling egress isolation fromextraEgresspresence (e.g., a dedicatednetworkPolicy.egressIsolationboolean) so the baseline-only case is directly supported.Also applies to: 29-49
🤖 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 `@charts/memos/templates/networkpolicy.yaml` around lines 16 - 18, The NetworkPolicy template currently ties enabling the Egress policyType to the presence of $extraEgress, so baseline DNS/HTTPS-only egress isolation cannot be turned on by itself. Update charts/memos/templates/networkpolicy.yaml around the policyTypes and egress rendering logic to decouple isolation from extraEgress, ideally by introducing a dedicated flag such as networkPolicy.egressIsolation in the values consumed by the template. Keep the existing extraEgress handling for additional rules, but make the NetworkPolicy render Egress whenever isolation is explicitly enabled, even if no extra rules are provided.
29-37: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winBaseline DNS egress rule is overly broad.
The DNS allowance uses
namespaceSelector: {}with nopodSelector, which selects all pods in every namespace on port 53. For a rule intended as a restrictive "baseline," this permits egress to essentially any workload in the cluster over UDP/TCP 53, undermining the isolation the feature is meant to provide. Consider scoping this to the DNS provider's namespace/pod labels (e.g.,kube-system+k8s-app: kube-dns/coredns), while acknowledging this may need to be configurable since DNS provider labels vary by cluster/CNI.🔒 Example tightened DNS egress rule
egress: - to: - - namespaceSelector: {} + - namespaceSelector: + matchLabels: + kubernetes.io/metadata.name: kube-system + podSelector: + matchLabels: + k8s-app: kube-dns ports: - protocol: UDP port: 53🤖 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 `@charts/memos/templates/networkpolicy.yaml` around lines 29 - 37, The DNS egress rule in the network policy template is too broad because the egress target uses an empty namespaceSelector without any podSelector, which effectively allows port 53 to every pod in every namespace. Update the NetworkPolicy logic in the template that builds the baseline egress rules to scope DNS traffic to the actual DNS provider namespace/pods instead of all workloads, ideally by using specific namespace and pod labels such as kube-system with k8s-app selectors for kube-dns or coredns. If cluster-specific DNS labels vary, make this selector configurable so the baseline remains restrictive while still working across environments.charts/memos/tests/networkpolicy_test.yaml (1)
17-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a default-case test.
This test only covers
extraEgressbeing set. There's no assertion that withnetworkPolicy.enabled: trueand default (empty)extraEgress, theEgresspolicyType andspec.egressare both absent — the behavior this cohort's design most relies on.🤖 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 `@charts/memos/tests/networkpolicy_test.yaml` around lines 17 - 40, Add a default-case Helm test for the network policy template: in networkPolicy_test.yaml, create a case for networkPolicy.enabled: true with empty or unset extraEgress and assert that spec.policyTypes does not include Egress and spec.egress is absent. Keep the existing extraEgress scenario in place, but add this complementary assertion so the networkPolicy template behavior is covered when no extra egress rules are configured.charts/memos/tests/validation_test.yaml (1)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a companion test for the
app.kubernetes.io/nameoverride case.Only the
instancekey override is tested;_helpers.tplalso fails whenpodLabelssetsapp.kubernetes.io/name, which is currently uncovered.✅ Suggested addition
- it: fails when podLabels override selector labels set: podLabels: app.kubernetes.io/instance: custom asserts: - failedTemplate: errorMessage: "podLabels must not override the selector label app.kubernetes.io/instance" + - it: fails when podLabels override the name selector label + set: + podLabels: + app.kubernetes.io/name: custom + asserts: + - failedTemplate: + errorMessage: "podLabels must not override the selector label app.kubernetes.io/name"🤖 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 `@charts/memos/tests/validation_test.yaml` around lines 34 - 40, Add a companion validation test for the missing selector-label override case in the memos chart. Extend the existing `fails when podLabels override selector labels` coverage in validation_test.yaml to also assert that setting podLabels.app.kubernetes.io/name fails with the corresponding error from the chart helpers. Use the same testing pattern and reference the _helpers.tpl selector-label validation so both app.kubernetes.io/instance and app.kubernetes.io/name override paths are covered.charts/memos/templates/_helpers.tpl (1)
34-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: reduce duplication in selector-key guard.
The two
hasKeyblocks are structurally identical. A loop over a list of protected keys would avoid duplication and stay in sync automatically if the selector label set ever changes.♻️ Optional consolidation
{{- $podLabels := .Values.podLabels | default dict -}} -{{- if hasKey $podLabels "app.kubernetes.io/name" -}} -{{- fail "podLabels must not override the selector label app.kubernetes.io/name" -}} -{{- end -}} -{{- if hasKey $podLabels "app.kubernetes.io/instance" -}} -{{- fail "podLabels must not override the selector label app.kubernetes.io/instance" -}} -{{- end -}} +{{- range list "app.kubernetes.io/name" "app.kubernetes.io/instance" -}} +{{- if hasKey $podLabels . -}} +{{- fail (printf "podLabels must not override the selector label %s" .) -}} +{{- end -}} +{{- end -}}🤖 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 `@charts/memos/templates/_helpers.tpl` around lines 34 - 40, The selector-key guard in the helper template is duplicated for each protected label key. Refactor the logic in the helper that validates podLabels by iterating over a list of reserved selector keys instead of repeating separate hasKey/fail blocks, so the checks stay centralized and are easier to extend if the protected set changes.
🤖 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.
Nitpick comments:
In `@charts/memos/templates/_helpers.tpl`:
- Around line 34-40: The selector-key guard in the helper template is duplicated
for each protected label key. Refactor the logic in the helper that validates
podLabels by iterating over a list of reserved selector keys instead of
repeating separate hasKey/fail blocks, so the checks stay centralized and are
easier to extend if the protected set changes.
In `@charts/memos/templates/networkpolicy.yaml`:
- Around line 16-18: The NetworkPolicy template currently ties enabling the
Egress policyType to the presence of $extraEgress, so baseline DNS/HTTPS-only
egress isolation cannot be turned on by itself. Update
charts/memos/templates/networkpolicy.yaml around the policyTypes and egress
rendering logic to decouple isolation from extraEgress, ideally by introducing a
dedicated flag such as networkPolicy.egressIsolation in the values consumed by
the template. Keep the existing extraEgress handling for additional rules, but
make the NetworkPolicy render Egress whenever isolation is explicitly enabled,
even if no extra rules are provided.
- Around line 29-37: The DNS egress rule in the network policy template is too
broad because the egress target uses an empty namespaceSelector without any
podSelector, which effectively allows port 53 to every pod in every namespace.
Update the NetworkPolicy logic in the template that builds the baseline egress
rules to scope DNS traffic to the actual DNS provider namespace/pods instead of
all workloads, ideally by using specific namespace and pod labels such as
kube-system with k8s-app selectors for kube-dns or coredns. If cluster-specific
DNS labels vary, make this selector configurable so the baseline remains
restrictive while still working across environments.
In `@charts/memos/tests/networkpolicy_test.yaml`:
- Around line 17-40: Add a default-case Helm test for the network policy
template: in networkPolicy_test.yaml, create a case for networkPolicy.enabled:
true with empty or unset extraEgress and assert that spec.policyTypes does not
include Egress and spec.egress is absent. Keep the existing extraEgress scenario
in place, but add this complementary assertion so the networkPolicy template
behavior is covered when no extra egress rules are configured.
In `@charts/memos/tests/validation_test.yaml`:
- Around line 34-40: Add a companion validation test for the missing
selector-label override case in the memos chart. Extend the existing `fails when
podLabels override selector labels` coverage in validation_test.yaml to also
assert that setting podLabels.app.kubernetes.io/name fails with the
corresponding error from the chart helpers. Use the same testing pattern and
reference the _helpers.tpl selector-label validation so both
app.kubernetes.io/instance and app.kubernetes.io/name override paths are
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69e776bb-ec17-492c-a348-e390ec328c8f
📒 Files selected for processing (12)
charts/memos/README.mdcharts/memos/templates/NOTES.txtcharts/memos/templates/_helpers.tplcharts/memos/templates/ingress.yamlcharts/memos/templates/networkpolicy.yamlcharts/memos/templates/statefulset.yamlcharts/memos/templates/validate.yamlcharts/memos/tests/networkpolicy_test.yamlcharts/memos/tests/templates_test.yamlcharts/memos/tests/validation_test.yamlcharts/memos/values.schema.jsoncharts/memos/values.yaml
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
mberlofa
force-pushed
the
fix/memos-template-standards
branch
from
1480ecb to
c9b148f
Compare
Contributor
Author
|
Addressed the CodeRabbit review-body items that still applied. What changed:
Validation:
These CodeRabbit items were posted in the review body/top-level summary. There are currently no review threads on the PR, so there is no thread ID to reply to or resolve. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/memos/values.yaml (1)
165-176: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider making the HTTPS baseline egress destination configurable too.
dnsEgressis user-overridable, but per the referenced template logic the HTTPS baseline egress rule targets0.0.0.0/0/::/0on port 443 unconditionally, allowing egress to any external address onceegressIsolationorextraEgressis enabled. For clusters wanting tighter default HTTPS egress scoping, an equivalent overridablehttpsEgressvalue (mirroringdnsEgress) would be consistent with the pattern already established here.🤖 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 `@charts/memos/values.yaml` around lines 165 - 176, Add a configurable HTTPS baseline egress setting to match the existing dnsEgress pattern, since the current baseline HTTPS rule in the related egress template is hardcoded to allow all destinations. Update the values schema in values.yaml to introduce an overridable httpsEgress default alongside dnsEgress, and then change the egress template logic that builds the baseline HTTPS rule to read from that new value instead of always using 0.0.0.0/0 and ::/0. Keep the existing egressIsolation and extraEgress behavior intact while making the HTTPS destination user-configurable.
🤖 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.
Nitpick comments:
In `@charts/memos/values.yaml`:
- Around line 165-176: Add a configurable HTTPS baseline egress setting to match
the existing dnsEgress pattern, since the current baseline HTTPS rule in the
related egress template is hardcoded to allow all destinations. Update the
values schema in values.yaml to introduce an overridable httpsEgress default
alongside dnsEgress, and then change the egress template logic that builds the
baseline HTTPS rule to read from that new value instead of always using
0.0.0.0/0 and ::/0. Keep the existing egressIsolation and extraEgress behavior
intact while making the HTTPS destination user-configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dc9a3c3-2bed-42c5-bcf8-4763cff35d8f
📒 Files selected for processing (12)
charts/memos/README.mdcharts/memos/templates/NOTES.txtcharts/memos/templates/_helpers.tplcharts/memos/templates/ingress.yamlcharts/memos/templates/networkpolicy.yamlcharts/memos/templates/statefulset.yamlcharts/memos/templates/validate.yamlcharts/memos/tests/networkpolicy_test.yamlcharts/memos/tests/templates_test.yamlcharts/memos/tests/validation_test.yamlcharts/memos/values.schema.jsoncharts/memos/values.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/memos/README.md
- charts/memos/templates/NOTES.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- charts/memos/templates/ingress.yaml
- charts/memos/templates/statefulset.yaml
- charts/memos/tests/validation_test.yaml
- charts/memos/tests/templates_test.yaml
- charts/memos/values.schema.json
- charts/memos/templates/_helpers.tpl
- charts/memos/templates/networkpolicy.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
Issue: #633
Site PR: helmforgedev/site#349
Summary by CodeRabbit
ingressClassNamein rendered Ingress resources when left empty.