Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions config/forge/workflows/tasks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ spec:
value: "$(params.env)"
- name: FOURNOS_STEP
value: "$(params.job-step)"
- name: FOURNOS_SECRETS
value: /var/run/secrets/fournos
volumeMounts:
- name: kubeconfig
mountPath: /var/run/secrets/fournos-kubeconfig
Expand Down
2 changes: 1 addition & 1 deletion dev/mock-resolve/resolve.sh
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ echo "[mock-resolve] setting secretRefs"
kubectl patch fournosjob "${FOURNOS_JOB_NAME}" \
-n "${FOURNOS_NAMESPACE}" \
--type=merge \
-p '{"spec":{"secretRefs":["vault-placeholder"]}}'
-p '{"spec":{"secretRefs":["placeholder"]}}'

echo "[mock-resolve] done"
88 changes: 77 additions & 11 deletions fournos/core/clusters.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,57 @@ def resolve_kubeconfig_secret(self, cluster_name: str) -> str:
"""Return the Secret name that holds the kubeconfig for *cluster_name*."""
return settings.kubeconfig_secret_pattern.format(cluster=cluster_name)

def copy_kubeconfig_secret(
self, cluster_name: str, fjob_name: str, owner_ref: dict
) -> str:
"""Copy the kubeconfig Secret for *cluster_name* into the operator namespace.

Returns the name of the copied Secret (``<fjob_name>-kubeconfig``).
Idempotent: a 409 (AlreadyExists) is silently ignored.
"""
source_name = self.resolve_kubeconfig_secret(cluster_name)
source = self._k8s.read_namespaced_secret(
source_name, settings.secrets_namespace
)

copied_name = f"{fjob_name}-kubeconfig"

copy_body = client.V1Secret(
metadata=client.V1ObjectMeta(
name=copied_name,
namespace=settings.namespace,
labels={LABEL_MANAGED_BY: "fournos"},
owner_references=[
client.V1OwnerReference(
api_version=owner_ref["apiVersion"],
kind=owner_ref["kind"],
name=owner_ref["name"],
uid=owner_ref["uid"],
controller=False,
block_owner_deletion=True,
)
],
),
type=source.type,
data=source.data,
)

try:
self._k8s.create_namespaced_secret(settings.namespace, copy_body)
logger.info(
"Copied kubeconfig %s from %s as %s",
source_name,
settings.secrets_namespace,
copied_name,
)
except client.exceptions.ApiException as exc:
if exc.status == 409:
logger.debug("Kubeconfig copy %s already exists (409)", copied_name)
else:
raise

return copied_name

def cluster_exists(self, cluster_name: str) -> bool:
"""Return True if the kubeconfig Secret for *cluster_name* exists."""
secret_name = self.resolve_kubeconfig_secret(cluster_name)
Expand All @@ -39,33 +90,43 @@ def cluster_exists(self, cluster_name: str) -> bool:
return False
raise

@staticmethod
def _vault_secret_name(ref: str) -> str:
"""Apply the vault secret naming pattern to a user-supplied ref."""
return settings.vault_secret_pattern.format(entry=ref)

def _resolve_secret_ref(self, ref: str) -> str:
"""Verify that *ref* is a Vault-synced K8s Secret and return its name.

The Secret is read from ``secrets_namespace``. The
``fournos.dev/vault-entry=true`` label is checked to confirm
Users supply refs without the ``vault-`` prefix; the pattern from
``settings.vault_secret_pattern`` is applied to derive the real
Secret name. The Secret is read from ``secrets_namespace``.
The ``fournos.dev/vault-entry=true`` label is checked to confirm
the Secret was actually imported from Vault.

Raises ``KeyError`` if the Secret does not exist or is not
a Vault-synced secret.
"""
secret_name = self._vault_secret_name(ref)
try:
secret = self._k8s.read_namespaced_secret(ref, settings.secrets_namespace)
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 {ref!r} not found in namespace "
f"{settings.secrets_namespace}"
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 {ref!r} exists but is not a Vault-synced secret "
f"Secret {secret_name!r} exists but is not a Vault-synced secret "
f"(missing {LABEL_VAULT_ENTRY}=true label)"
)
logger.debug("Validated secretRef %s", ref)
return ref
logger.debug("Validated secretRef %s -> %s", ref, secret_name)
return secret_name

def resolve_secret_refs(self, refs: list[str]) -> list[str]:
"""Resolve a list of secretRefs to their K8s Secret names."""
Expand All @@ -74,16 +135,20 @@ def resolve_secret_refs(self, refs: list[str]) -> list[str]:
def copy_secret(self, ref: str, fjob_name: str, owner_ref: dict) -> ResolvedSecret:
"""Copy a Vault-synced Secret from the secrets namespace into the pod namespace.

*ref* is the user-supplied name (without ``vault-`` prefix).
The copy is named ``<fjob_name>-<ref>`` and carries an ownerReference
back to the FournosJob so K8s GC cleans it up automatically.
Idempotent: a 409 (AlreadyExists) is silently ignored.
"""
source = self._k8s.read_namespaced_secret(ref, settings.secrets_namespace)
secret_name = self._vault_secret_name(ref)
source = self._k8s.read_namespaced_secret(
secret_name, settings.secrets_namespace
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Suggested change
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).


labels = source.metadata.labels or {}
if labels.get(LABEL_VAULT_ENTRY) != "true":
raise KeyError(
f"Secret {ref!r} in {settings.secrets_namespace} is not a "
f"Secret {secret_name!r} in {settings.secrets_namespace} is not a "
f"Vault-synced secret (missing {LABEL_VAULT_ENTRY}=true label)"
)

Expand Down Expand Up @@ -116,7 +181,8 @@ def copy_secret(self, ref: str, fjob_name: str, owner_ref: dict) -> ResolvedSecr
try:
self._k8s.create_namespaced_secret(settings.namespace, copy_body)
logger.info(
"Copied secret %s from %s as %s",
"Copied secret %s (ref %s) from %s as %s",
secret_name,
ref,
settings.secrets_namespace,
copied_name,
Expand Down
22 changes: 20 additions & 2 deletions fournos/handlers/execution.py
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,25 @@ def reconcile_admitted(spec, name, namespace, status, patch, body):

if pr is None:
cluster = status.get("cluster", "")
secret = ctx.registry.resolve_kubeconfig_secret(cluster)

try:
kubeconfig_secret = ctx.registry.copy_kubeconfig_secret(
cluster, name, owner_ref(body)
)
except client.exceptions.ApiException as exc:
patch.status["phase"] = Phase.FAILED
patch.status["message"] = f"Failed to copy kubeconfig: {exc.reason}"
set_condition(
patch,
conditions,
COND_PIPELINE_RUN_READY,
"False",
"KubeconfigNotFound",
f"Failed to copy kubeconfig: {exc.reason}",
)
ctx.kueue.delete_workload(name)
logger.error("Job %s: kubeconfig copy failed: %s", name, exc)
return

hardware = spec.get("hardware") or {}
gpu_count = hardware.get("gpuCount", 0)
Expand Down Expand Up @@ -183,7 +201,7 @@ def reconcile_admitted(spec, name, namespace, status, patch, body):
forge_project=spec["forge"]["project"],
forge_config=spec["forge"],
env=spec.get("env", {}),
kubeconfig_secret=secret,
kubeconfig_secret=kubeconfig_secret,
gpu_count=gpu_count,
resolved_secrets=resolved_secrets,
cluster=cluster,
Expand Down
11 changes: 6 additions & 5 deletions manifests/crd.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,12 @@ spec:
secretRefs:
type: array
description: >-
Vault-synced K8s Secret names (vault-<entry>) to mount
into the pipeline. Each name must correspond to a
Kubernetes Secret with the fournos.dev/vault-entry=true
label. Populated by Forge during the Resolving phase
when not provided by the user.
Vault entry names (without the vault- prefix) to mount
into the pipeline. The operator prepends vault- to
look up the corresponding K8s Secret (which must carry
the fournos.dev/vault-entry=true label) in the secrets
namespace. Populated by Forge during the Resolving
phase when not provided by the user.
items:
type: string
pattern: "^[a-z0-9]([a-z0-9\\-]{0,61}[a-z0-9])?$"
Expand Down
2 changes: 1 addition & 1 deletion tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ def create_stale_pipelinerun(k8s, name: str) -> None:
"""),
},
{"name": "env", "value": ""},
{"name": "kubeconfig-secret", "value": "kubeconfig-cluster-1"},
{"name": "kubeconfig-secret", "value": "test-stale-kubeconfig"},
{"name": "gpu-count", "value": "0"},
],
},
Expand Down
22 changes: 18 additions & 4 deletions tests/test_scheduling.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from fournos.core.constants import Phase
from tests.conftest import (
NAMESPACE,
create_job,
get_job,
get_k8s_resource,
Expand Down Expand Up @@ -43,8 +44,21 @@ def test_cluster_pinned(k8s):
flavor = get_workload_flavor("test-cluster")
assert flavor == "cluster-2", f"Workload flavor should be cluster-2, got {flavor!r}"
secret = get_pipelinerun_param("test-cluster", "kubeconfig-secret")
assert secret == "kubeconfig-cluster-2", (
f"PipelineRun kubeconfig-secret should be kubeconfig-cluster-2, got {secret!r}"
assert secret == "test-cluster-kubeconfig", (
f"PipelineRun kubeconfig-secret should be test-cluster-kubeconfig, got {secret!r}"
)

kc = get_k8s_resource("secret", "test-cluster-kubeconfig")
assert "kubeconfig" in (kc.get("data") or {}), (
f"Copied kubeconfig secret should have a 'kubeconfig' key, got {list((kc.get('data') or {}).keys())}"
)
kc_owners = kc.get("metadata", {}).get("ownerReferences", [])
assert any(
o.get("kind") == "FournosJob" and o.get("name") == "test-cluster"
for o in kc_owners
), f"Copied kubeconfig should have FournosJob ownerRef, got {kc_owners!r}"
assert kc.get("metadata", {}).get("namespace") == NAMESPACE, (
"Copied kubeconfig should be in the operator namespace"
)

phase = poll_phase(
Expand Down Expand Up @@ -115,8 +129,8 @@ def test_cluster_and_hardware(k8s):
flavor = get_workload_flavor("test-cluster-hw")
assert flavor == "cluster-4", f"Workload flavor should be cluster-4, got {flavor!r}"
secret = get_pipelinerun_param("test-cluster-hw", "kubeconfig-secret")
assert secret == "kubeconfig-cluster-4", (
f"PipelineRun kubeconfig-secret should be kubeconfig-cluster-4, got {secret!r}"
assert secret == "test-cluster-hw-kubeconfig", (
f"PipelineRun kubeconfig-secret should be test-cluster-hw-kubeconfig, got {secret!r}"
)

phase = poll_phase(
Expand Down
8 changes: 4 additions & 4 deletions tests/test_secret_refs.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def test_vault_sync_then_fjob(k8s, core_v1):
)
assert rc == 0, "sync_vault_secrets.sync() returned non-zero"

expected_copy = f"test-e2e-secret-{VAULT_SECRET}"
expected_copy = f"test-e2e-secret-{VAULT_ENTRY}"

try:
secret = core_v1.read_namespaced_secret(VAULT_SECRET, SECRETS_NAMESPACE)
Expand All @@ -134,7 +134,7 @@ def test_vault_sync_then_fjob(k8s, core_v1):
)

poll_resolve_job_complete("test-e2e-secret")
_patch_fjob_secret_refs(k8s, "test-e2e-secret", [VAULT_SECRET])
_patch_fjob_secret_refs(k8s, "test-e2e-secret", [VAULT_ENTRY])

phase = poll_phase(
k8s,
Expand All @@ -147,8 +147,8 @@ def test_vault_sync_then_fjob(k8s, core_v1):
)

refs_param = get_pipelinerun_param("test-e2e-secret", "secret-refs")
assert VAULT_SECRET in refs_param, (
f"PipelineRun secret-refs should contain {VAULT_SECRET!r}, "
assert VAULT_ENTRY in refs_param, (
f"PipelineRun secret-refs should contain {VAULT_ENTRY!r}, "
f"got {refs_param!r}"
)

Expand Down
Loading