Fix Secrets Mounting - #69
Conversation
…dd FOURNOS_SECRETS envvar Signed-off-by: avasilev <avasilev@redhat.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughOperator now resolves Vault-entry refs (given without the Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Operator as Operator
participant K8sAPI as Kubernetes API
participant Tekton as Tekton PipelineRun
User->>Operator: Create FournosJob(spec.secretRefs = "my-entry")
Operator->>K8sAPI: Resolve Secret `vault-my-entry` (get Secret)
K8sAPI-->>Operator: Return Secret (or 404)
Operator->>K8sAPI: Create copy in operator namespace with ownerReference
K8sAPI-->>Operator: 201 Created (or 409 if exists)
Operator->>Tekton: Create PipelineRun with param kubeconfig-secret=<job>-kubeconfig
Tekton->>K8sAPI: Pod mounts operator namespace secret at /var/run/secrets/fournos
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
looks good at first glance, let's get it tested ! |
|
/lgtm |
|
/test fournos-deploy-wip |
|
/test fournos |
|
/test deploy-fournos-wip |
![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 (2)
tests/test_secret_refs.py (1)
150-152: Tightensecret-refsassertion to avoid false positives.Line 150 uses containment; exact comparison is safer for catching malformed values.
💡 Proposed tightening
- assert VAULT_ENTRY in refs_param, ( - f"PipelineRun secret-refs should contain {VAULT_ENTRY!r}, " + assert refs_param == [VAULT_ENTRY], ( + f"PipelineRun secret-refs should equal {[VAULT_ENTRY]!r}, " f"got {refs_param!r}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_secret_refs.py` around lines 150 - 152, The current assertion in tests/test_secret_refs.py uses containment (assert VAULT_ENTRY in refs_param) which can give false positives; update the assertion to require exact equality of the secret-refs value by asserting refs_param == VAULT_ENTRY (and keep a clear error message), locating and replacing the assertion that references VAULT_ENTRY and refs_param so the test fails on malformed or extra characters rather than only missing substrings.tests/conftest.py (1)
467-467: Derive stale kubeconfig secret from the helper input name.Line 467 hardcodes a specific value inside a generic helper. Prefer
f"{name}-kubeconfig"to keep fixture behavior consistent across callers.♻️ Proposed fix
- {"name": "kubeconfig-secret", "value": "test-stale-kubeconfig"}, + {"name": "kubeconfig-secret", "value": f"{name}-kubeconfig"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conftest.py` at line 467, Replace the hardcoded "test-stale-kubeconfig" value for the "kubeconfig-secret" entry with a value derived from the helper's input parameter `name` (e.g., use f"{name}-kubeconfig") so the fixture generates a kubeconfig secret name based on the caller-provided `name`; update the dict entry that currently contains "kubeconfig-secret" to set its "value" using that derived string in the helper in tests/conftest.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fournos/core/clusters.py`:
- Around line 143-146: The copy_secret implementation currently calls
self._k8s.read_namespaced_secret(secret_name, settings.secrets_namespace) and
lets ApiException(404) escape, losing the richer ref+namespace context used
elsewhere; update copy_secret to catch k8s.client.exceptions.ApiException (or
ApiException) around the read_namespaced_secret call, translate 404 responses
into a KeyError that includes the original ref and settings.secrets_namespace
(or simply call/reuse self._resolve_secret_ref(ref) to get the resolved name and
raise a KeyError with that info), and re-raise other ApiExceptions unchanged so
diagnostics remain consistent with other code paths (mentioning copy_secret,
_vault_secret_name, _resolve_secret_ref, read_namespaced_secret, ApiException,
KeyError).
---
Nitpick comments:
In `@tests/conftest.py`:
- Line 467: Replace the hardcoded "test-stale-kubeconfig" value for the
"kubeconfig-secret" entry with a value derived from the helper's input parameter
`name` (e.g., use f"{name}-kubeconfig") so the fixture generates a kubeconfig
secret name based on the caller-provided `name`; update the dict entry that
currently contains "kubeconfig-secret" to set its "value" using that derived
string in the helper in tests/conftest.py.
In `@tests/test_secret_refs.py`:
- Around line 150-152: The current assertion in tests/test_secret_refs.py uses
containment (assert VAULT_ENTRY in refs_param) which can give false positives;
update the assertion to require exact equality of the secret-refs value by
asserting refs_param == VAULT_ENTRY (and keep a clear error message), locating
and replacing the assertion that references VAULT_ENTRY and refs_param so the
test fails on malformed or extra characters rather than only missing substrings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 877a26aa-e757-45f1-b524-9818dc8f8ec1
📒 Files selected for processing (8)
config/forge/workflows/tasks.yamldev/mock-resolve/resolve.shfournos/core/clusters.pyfournos/handlers/execution.pymanifests/crd.yamltests/conftest.pytests/test_scheduling.pytests/test_secret_refs.py
| secret_name = self._vault_secret_name(ref) | ||
| source = self._k8s.read_namespaced_secret( | ||
| secret_name, settings.secrets_namespace | ||
| ) |
There was a problem hiding this comment.
Preserve detailed missing-secret errors in copy_secret.
Line 143-146 now raises raw ApiException on 404, which drops the richer ref + namespace context provided elsewhere. Convert 404 to KeyError here (or reuse _resolve_secret_ref) to keep diagnostics consistent.
🐛 Proposed fix
secret_name = self._vault_secret_name(ref)
- source = self._k8s.read_namespaced_secret(
- secret_name, settings.secrets_namespace
- )
+ try:
+ source = self._k8s.read_namespaced_secret(
+ secret_name, settings.secrets_namespace
+ )
+ except client.exceptions.ApiException as exc:
+ if exc.status == 404:
+ raise KeyError(
+ f"Secret {secret_name!r} (ref {ref!r}) not found in "
+ f"namespace {settings.secrets_namespace}"
+ ) from exc
+ raise📝 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.
| secret_name = self._vault_secret_name(ref) | |
| source = self._k8s.read_namespaced_secret( | |
| secret_name, settings.secrets_namespace | |
| ) | |
| secret_name = self._vault_secret_name(ref) | |
| try: | |
| source = self._k8s.read_namespaced_secret( | |
| secret_name, settings.secrets_namespace | |
| ) | |
| except client.exceptions.ApiException as exc: | |
| if exc.status == 404: | |
| raise KeyError( | |
| f"Secret {secret_name!r} (ref {ref!r}) not found in " | |
| f"namespace {settings.secrets_namespace}" | |
| ) from exc | |
| raise |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fournos/core/clusters.py` around lines 143 - 146, The copy_secret
implementation currently calls self._k8s.read_namespaced_secret(secret_name,
settings.secrets_namespace) and lets ApiException(404) escape, losing the richer
ref+namespace context used elsewhere; update copy_secret to catch
k8s.client.exceptions.ApiException (or ApiException) around the
read_namespaced_secret call, translate 404 responses into a KeyError that
includes the original ref and settings.secrets_namespace (or simply call/reuse
self._resolve_secret_ref(ref) to get the resolved name and raise a KeyError with
that info), and re-raise other ApiExceptions unchanged so diagnostics remain
consistent with other code paths (mentioning copy_secret, _vault_secret_name,
_resolve_secret_ref, read_namespaced_secret, ApiException, KeyError).
|
🟢 Test of 'fournos_deploy --project-source' succeeded after 00 hours 10 minutes 23 seconds 🟢 • Link to the test results. • No reports index generated... Test configuration: |
Signed-off-by: avasilev <avasilev@redhat.com>
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: avasilev <avasilev@redhat.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fournos/core/clusters.py (1)
143-146:⚠️ Potential issue | 🟠 MajorAvoid double-read TOCTOU after secret validation.
Line 143 validates via
_resolve_secret_ref(ref), then Lines 144-146 read the same Secret again. If the Secret changes/deletes between these calls,copy_secretcan fail with a rawApiExceptionand lose the contextualKeyErrorpath.🐛 Proposed fix (single-read helper)
+ def _resolve_secret(self, ref: str) -> tuple[str, client.V1Secret]: + secret_name = self._vault_secret_name(ref) + try: + secret = self._k8s.read_namespaced_secret( + secret_name, settings.secrets_namespace + ) + except client.exceptions.ApiException as exc: + if exc.status == 404: + raise KeyError( + f"Secret {secret_name!r} (ref {ref!r}) not found in " + f"namespace {settings.secrets_namespace}" + ) from exc + raise + labels = secret.metadata.labels or {} + if labels.get(LABEL_VAULT_ENTRY) != "true": + raise KeyError( + f"Secret {secret_name!r} exists but is not a Vault-synced secret " + f"(missing {LABEL_VAULT_ENTRY}=true label)" + ) + return secret_name, secret + def _resolve_secret_ref(self, ref: str) -> str: - secret_name = self._vault_secret_name(ref) - try: - secret = self._k8s.read_namespaced_secret( - secret_name, settings.secrets_namespace - ) - except client.exceptions.ApiException as exc: - if exc.status == 404: - raise KeyError( - f"Secret {secret_name!r} (ref {ref!r}) not found in " - f"namespace {settings.secrets_namespace}" - ) from exc - raise - labels = secret.metadata.labels or {} - if labels.get(LABEL_VAULT_ENTRY) != "true": - raise KeyError( - f"Secret {secret_name!r} exists but is not a Vault-synced secret " - f"(missing {LABEL_VAULT_ENTRY}=true label)" - ) + secret_name, _ = self._resolve_secret(ref) logger.debug("Validated secretRef %s -> %s", ref, secret_name) return secret_name def copy_secret(self, ref: str, fjob_name: str, owner_ref: dict) -> ResolvedSecret: - secret_name = self._resolve_secret_ref(ref) - source = self._k8s.read_namespaced_secret( - secret_name, settings.secrets_namespace - ) + secret_name, source = self._resolve_secret(ref)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fournos/core/clusters.py` around lines 143 - 146, The code currently calls _resolve_secret_ref(ref) to validate the secret and then calls read_namespaced_secret again, risking a TOCTOU race; change to perform a single read that both validates and returns the secret object (e.g. implement a helper like _get_validated_secret(ref) or modify _resolve_secret_ref to return the Secret instead of only the name) and use that single Secret in copy_secret; ensure the helper catches ApiException and converts a not-found condition into the existing KeyError path so callers (e.g. copy_secret) preserve the original contextual error handling instead of exposing raw ApiException.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fournos/core/clusters.py`:
- Around line 143-146: The code currently calls _resolve_secret_ref(ref) to
validate the secret and then calls read_namespaced_secret again, risking a
TOCTOU race; change to perform a single read that both validates and returns the
secret object (e.g. implement a helper like _get_validated_secret(ref) or modify
_resolve_secret_ref to return the Secret instead of only the name) and use that
single Secret in copy_secret; ensure the helper catches ApiException and
converts a not-found condition into the existing KeyError path so callers (e.g.
copy_secret) preserve the original contextual error handling instead of exposing
raw ApiException.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b56c74f7-3749-40be-8c9e-418d302173db
📒 Files selected for processing (5)
dev/mock-secrets.yamlfournos/core/clusters.pytests/conftest.pytests/test_scheduling.pytests/test_secret_refs.py
✅ Files skipped from review due to trivial changes (1)
- dev/mock-secrets.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/conftest.py
- tests/test_scheduling.py
- tests/test_secret_refs.py
Signed-off-by: avasilev <avasilev@redhat.com>
2 participants
Fix naming pattern for vault secrets, copy kubeconfig to target ns and add FOURNOS_SECRETS envvar
Summary by CodeRabbit
New Features
Improvements
Tests