fix(qdrant): align template standards#673
Merged
Merged
Conversation
github-actions
Bot
added
area:docs
area:schema
area:templates
area:tests
area:values
chart:qdrant
size:L
labels
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughThis PR centralizes Qdrant chart validation, adds NetworkPolicy egress support, makes ingress class rendering conditional, aligns StatefulSet pod labels with selector labels, and rewrites install notes into a structured runbook. ChangesChart hardening and operator guidance
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
Standards Check (GR-079) — PASSEvery changed chart fully passes standards-check. |
Contributor
🟢 Security Scan:
|
| Framework | Score |
|---|---|
| MITRE + NSA + SOC2 | 75.757576% |
✅ Security posture acceptable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/qdrant/templates/networkpolicy.yaml (1)
39-55: 🔒 Security & Privacy | 🔵 TrivialUnrestricted HTTPS egress (0.0.0.0/0:443) significantly limits the isolation guarantee.
The baseline egress rule permits TCP/443 to any IPv4/IPv6 destination. Since most outbound C2/exfiltration traffic also rides over 443, this rule effectively neutralizes egress isolation for anything using standard TLS, leaving
extraEgressmainly restricting non-HTTPS traffic. This appears to be an intentional, documented tradeoff (README states "preserves DNS and HTTPS egress"), so flagging as an advisory rather than a blocking issue — consider whether narrowing this to known egress endpoints (e.g., via an allow-list value) would better match the "isolation" naming for users who need stricter guarantees.🤖 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/qdrant/templates/networkpolicy.yaml` around lines 39 - 55, The extra egress rule in the network policy currently allows TCP/443 to all IPv4 and IPv6 destinations, which weakens the isolation guarantee. Review the egress section in the networkPolicy template and adjust the handling of `extraEgress` so HTTPS access is scoped to configurable allow-listed endpoints instead of `0.0.0.0/0` and `::/0`, while keeping the DNS rule intact. If broad HTTPS egress is intentionally retained, make that tradeoff explicit in the template/value naming or comments tied to the `networkPolicy.extraEgress` logic.
🤖 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 `@charts/qdrant/templates/NOTES.txt`:
- Line 38: The recovery command in NOTES.txt uses dot notation in the jsonpath
expression for the secret data key, which fails for default and any API key
names containing special characters like hyphens or dots. Update the kubectl
command in the NOTES template to use bracket notation for the
.Values.auth.apiKeyKey lookup so it works regardless of the configured secret
key; locate this in the NOTES.txt template around the kubectl get secret
command.
---
Nitpick comments:
In `@charts/qdrant/templates/networkpolicy.yaml`:
- Around line 39-55: The extra egress rule in the network policy currently
allows TCP/443 to all IPv4 and IPv6 destinations, which weakens the isolation
guarantee. Review the egress section in the networkPolicy template and adjust
the handling of `extraEgress` so HTTPS access is scoped to configurable
allow-listed endpoints instead of `0.0.0.0/0` and `::/0`, while keeping the DNS
rule intact. If broad HTTPS egress is intentionally retained, make that tradeoff
explicit in the template/value naming or comments tied to the
`networkPolicy.extraEgress` logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 950ebd65-36e3-4c72-bf56-c6cf3b9ac651
📒 Files selected for processing (14)
charts/qdrant/README.mdcharts/qdrant/templates/NOTES.txtcharts/qdrant/templates/_helpers.tplcharts/qdrant/templates/ingress.yamlcharts/qdrant/templates/networkpolicy.yamlcharts/qdrant/templates/statefulset.yamlcharts/qdrant/templates/validate.yamlcharts/qdrant/tests/networkpolicy-extra-egress-values.yamlcharts/qdrant/tests/networkpolicy_test.yamlcharts/qdrant/tests/podlabels-selector-instance-values.yamlcharts/qdrant/tests/podlabels-selector-name-values.yamlcharts/qdrant/tests/validation_test.yamlcharts/qdrant/values.schema.jsoncharts/qdrant/values.yaml
mberlofa
force-pushed
the
fix/qdrant-template-standards
branch
from
ab19ab3 to
59481fa
Compare
Contributor
Author
|
Addressed the remaining CodeRabbit advisory from the review body. Decision:
Validation:
The remaining NetworkPolicy advisory was posted in the CodeRabbit review body/top-level summary, not as an unresolved review thread, so there is no thread ID to reply to or resolve. |
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
podLabels.ingress.ingressClassNamerendering.qdrant.validatehelper and add validations for ingress hosts plus selector-label podLabels overrides.networkPolicy.extraEgresssupport with DNS, HTTPS, and distributed p2p baseline egress rules before user rules.NOTES.txtwith numbered HelmForge operational sections.Validation
helm unittest charts/qdrant: 28 tests passedhelm lint --strict charts/qdrant: passedmake template-standards-check CHART=qdrant: passedmake standards-check CHART=qdrant: passedmake standards-guard CHART=qdrant: passedmake site-sync-check CHART=qdrant: passednpm run lint: passed in site reponpm run format:check: passed in site reponpm run build: passed in site reponode scripts/charts/validate-chart.js --chart qdrant --no-k3d: all local layers passed except the known local remote schema download failure fromraw.githubusercontent.comci/*.yamlpassed withSkipped: 0k3d-helmforge-tests-wsl: default plus everyci/*.yamlpassed sequentiallymake release-check REPO=charts: passed with expected post-merge release warningmake attribution-check REPO=charts: passedSite Sync
Site PR: helmforgedev/site#351
Summary by CodeRabbit
New Features
extraEgress(with example).networkPolicy.extraEgressin chart configuration and schema.Bug Fixes / Improvements
ingressClassNameonly when configured.Tests