Deploy Fournos in a dedicated namespace#79
Deploy Fournos in a dedicated namespace#79avasilevskii wants to merge 3 commits intoopenshift-psap:mainfrom
Conversation
Signed-off-by: avasilev <avasilev@redhat.com>
|
[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 |
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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. 📝 WalkthroughWalkthroughThis PR introduces a dual-namespace deployment model for Fournos, separating the operator controller (FOURNOS_CONTROLLER_NAMESPACE) from the execution workload namespace (FOURNOS_NAMESPACE). Changes span Makefile, manifests, scripts, orchestration logic, and documentation to propagate namespace variables, update RBAC bindings, and coordinate multi-namespace deployment, moving away from the previous single psap-automation namespace. ChangesMulti-Namespace Deployment Model
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/rbac/rolebinding_fournos.yaml (1)
13-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: second
ServiceAccountsubject is missing the requirednamespace.For
subjects[*]ofkind: ServiceAccount, Kubernetes API validation requires thenamespacefield. As written, applying this RoleBinding will fail with a validation error:subjects[1].namespace: Required value. Per the official Kubernetes documentation, RBAC subjects of kind ServiceAccount must specify the namespace they live in.The second entry has the same
name: fournosas the first subject. The Makefile appliessa_fournos.yamlto both$(FOURNOS_CONTROLLER_NAMESPACE)and$(FOURNOS_NAMESPACE), then applies this RoleBinding with-n $(FOURNOS_NAMESPACE). The apparent intent is to bind the fournos ServiceAccount from the execution namespace (different from the first subject). This requires an explicit namespace specification—likely a new${NAMESPACE}template variable to match the pattern already used elsewhere in the Makefile.🔧 Suggested fix (introduce a NAMESPACE token for the execution namespace)
subjects: - kind: ServiceAccount name: fournos namespace: ${CONTROLLER_NAMESPACE} - kind: ServiceAccount name: fournos + namespace: ${NAMESPACE}…and update the corresponding
make deploystep to passNAMESPACE:- cat $$rbac_file | CONTROLLER_NAMESPACE=$(FOURNOS_CONTROLLER_NAMESPACE) envsubst '$$CONTROLLER_NAMESPACE' | kubectl apply -f- -n $(FOURNOS_NAMESPACE); \ + cat $$rbac_file | CONTROLLER_NAMESPACE=$(FOURNOS_CONTROLLER_NAMESPACE) NAMESPACE=$(FOURNOS_NAMESPACE) envsubst '$$CONTROLLER_NAMESPACE $$NAMESPACE' | kubectl apply -f- -n $(FOURNOS_NAMESPACE); \If the second subject is unintentional, drop it instead.
🤖 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 `@manifests/rbac/rolebinding_fournos.yaml` around lines 13 - 15, The RoleBinding subjects list has a second subject with kind: ServiceAccount and name: fournos but is missing the required namespace field, causing Kubernetes validation errors; either remove the duplicate subject if unintended or add an explicit namespace to that subject (e.g., the execution namespace token used elsewhere) and update the related Makefile deployment to pass the new NAMESPACE variable so the RoleBinding references the correct ServiceAccount namespace; locate the subjects block in rolebinding_fournos.yaml (the entries with kind: ServiceAccount and name: fournos) and align it with the sa_fournos.yaml / FOURNOS_CONTROLLER_NAMESPACE and FOURNOS_NAMESPACE usage.
🧹 Nitpick comments (3)
hacks/generate-ocpci-kubeconfig.sh (1)
9-9: ⚡ Quick winHardcoded namespace won't match dev/CI defaults.
The Makefile defines
FOURNOS_CONTROLLER_NAMESPACEdefaults offournos-controller-local(dev) andfournos-controller-ci-test(CI), but this script hardcodesfournos-controller. Running this helper against a dev/CI cluster will silently loop on Line 19 waiting for a secret that lives in a different namespace. Consider making this overridable via an env var.♻️ Proposed change
-NAMESPACE="fournos-controller" +NAMESPACE="${NAMESPACE:-fournos-controller}"🤖 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 `@hacks/generate-ocpci-kubeconfig.sh` at line 9, The script hacks/generate-ocpci-kubeconfig.sh hardcodes NAMESPACE="fournos-controller" which mismatches Makefile defaults and causes waiting for a secret in the wrong namespace; make NAMESPACE configurable by reading an environment variable (e.g. FOURNOS_CONTROLLER_NAMESPACE) with a sensible default fallback, update any references in the script that use NAMESPACE (search for the NAMESPACE symbol and any lines that wait for the secret) so the script uses the env-overridable value, and document the env var usage in the script header or help text.Makefile (1)
35-55: 💤 Low valueDeploy target rework looks correct; one consistency note.
The multi-namespace flow is well structured: namespaces created idempotently via dry-run, RBAC scoped per namespace, envsubst correctly limits substitution to
$$CONTROLLER_NAMESPACE/$$SECRETS_NAMESPACE/$$NAMESPACE. A few small notes:
- Line 41 applies
sa_fournos.yamlinto the execution namespace as well as the controller namespace. This is meaningful only if the second subject ofmanifests/rbac/rolebinding_fournos.yamlis fixed (see comment on that file). With its current shape, the SA in the execution namespace would be unbound by the local RoleBinding.- Line 99 / Line 106 still default
FOURNOS_NAMESPACEtopsap-automation-ci-testfor CI. That's the execution namespace and may be intentional, but if the goal is to fully detach Fournos from thepsap-automation-*naming, consider renaming to e.g.fournos-ci-testfor symmetry with the controller default.🤖 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 `@Makefile` around lines 35 - 55, The deploy target applies the same ServiceAccount manifest (sa_fournos.yaml) into both FOURNOS_CONTROLLER_NAMESPACE and FOURNOS_NAMESPACE, but the RoleBinding (rolebinding_fournos.yaml) currently does not bind the execution-namespace SA so the second apply is ineffective; either remove the second kubectl apply for sa_fournos.yaml in FOURNOS_NAMESPACE or update rolebinding_fournos.yaml so its subjects include the ServiceAccount in FOURNOS_NAMESPACE (or make the RoleBinding reference a ClusterRole/ClusterRoleBinding if cross-namespace binding is intended). Also, normalize the CI default names for clarity by updating the FOURNOS_NAMESPACE default (currently set elsewhere) from the psap-automation-* name to a Fournos-specific name like fournos-ci-test if you want symmetric naming with the controller.tests/forge/deploy/orchestration/deploy.py (1)
367-403: 💤 Low valueDeploy ordering is correct: SA first in controller ns, then bindings in execution ns.
controller_rbac(SA) is deployed beforerbac(Role/RoleBinding whose subjects reference the controller-ns SA), matching what the bindings need at apply time. The empty-list guard at line 379 is nice; you may want the sameif rbac_files:guard on line 392 for symmetry, sincemanifests_config["rbac"]is currently a hardKeyErrorif the key is ever omitted in a future config variant — minor.♻️ Optional symmetry tweak
- # Deploy execution RBAC + CRD to execution namespace - rbac_files = manifests_config["rbac"] - crd_files = manifests_config["crd"] + # Deploy execution RBAC + CRD to execution namespace + rbac_files = manifests_config.get("rbac", []) + crd_files = manifests_config.get("crd", []) manifest_files = rbac_files + crd_files🤖 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 `@tests/forge/deploy/orchestration/deploy.py` around lines 367 - 403, The code assumes manifests_config contains "rbac" and "crd" and will raise KeyError if those keys are omitted; change the block that sets rbac_files/crd_files and calls _deploy_manifest_list to first fetch them safely (e.g., using manifests_config.get("rbac", []) and manifests_config.get("crd", [])) and add an if rbac_files: guard (mirroring the controller_rbac_files guard) before logging and calling _deploy_manifest_list for the RBAC+CRD deployment so missing keys result in no-op instead of an exception.
🤖 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 `@dev/setup.sh`:
- Around line 93-99: The Role in manifests/rbac/role_fournos.yaml is missing
permissions for KopfPeering which Kopf needs for leader election when the
operator runs in namespace-scoped mode; update role_fournos.yaml to add a rule
granting apiGroups: "kopf.dev" for resources "kopfpeerings" with verbs "list",
"watch", "patch", and "get" so the controller (running with --namespace
$FOURNOS_NAMESPACE) can access KopfPeering objects; alternatively document or
change the operator startup to use --standalone if peering is not required.
In `@manifests/deployment.yaml`:
- Around line 24-27: The deployment injects FOURNOS_CONTROLLER_NAMESPACE which
is unused; either remove that env var from the manifest or add a consuming
Settings field and usage: extend fourn os.settings.Settings
(env_prefix="FOURNOS_") with a controller_namespace: str | None field (or
similar) and wire any operator logic that needs the controller namespace to read
settings.controller_namespace (e.g., RBAC/secret resolution/self-reference code
paths such as resolve-job or secret read functions); if you choose to remove the
var, delete the FOURNOS_CONTROLLER_NAMESPACE env entry from the manifest to
avoid dead config.
- Around line 22-23: The deployment manifest sets FOURNOS_NAMESPACE to the
literal "${NAMESPACE}", which forces templating; change it to the same
in-cluster lookup used by FOURNOS_CONTROLLER_NAMESPACE (i.e., use an env var
with valueFrom.fieldRef.fieldPath: metadata.namespace) so the pod receives the
actual namespace without requiring envsubst, and if you prefer keeping
templating instead update Fournos_Design_Document.md to explicitly document the
Makefile/envsubst workflow and the requirement to replace ${NAMESPACE}.
---
Outside diff comments:
In `@manifests/rbac/rolebinding_fournos.yaml`:
- Around line 13-15: The RoleBinding subjects list has a second subject with
kind: ServiceAccount and name: fournos but is missing the required namespace
field, causing Kubernetes validation errors; either remove the duplicate subject
if unintended or add an explicit namespace to that subject (e.g., the execution
namespace token used elsewhere) and update the related Makefile deployment to
pass the new NAMESPACE variable so the RoleBinding references the correct
ServiceAccount namespace; locate the subjects block in rolebinding_fournos.yaml
(the entries with kind: ServiceAccount and name: fournos) and align it with the
sa_fournos.yaml / FOURNOS_CONTROLLER_NAMESPACE and FOURNOS_NAMESPACE usage.
---
Nitpick comments:
In `@hacks/generate-ocpci-kubeconfig.sh`:
- Line 9: The script hacks/generate-ocpci-kubeconfig.sh hardcodes
NAMESPACE="fournos-controller" which mismatches Makefile defaults and causes
waiting for a secret in the wrong namespace; make NAMESPACE configurable by
reading an environment variable (e.g. FOURNOS_CONTROLLER_NAMESPACE) with a
sensible default fallback, update any references in the script that use
NAMESPACE (search for the NAMESPACE symbol and any lines that wait for the
secret) so the script uses the env-overridable value, and document the env var
usage in the script header or help text.
In `@Makefile`:
- Around line 35-55: The deploy target applies the same ServiceAccount manifest
(sa_fournos.yaml) into both FOURNOS_CONTROLLER_NAMESPACE and FOURNOS_NAMESPACE,
but the RoleBinding (rolebinding_fournos.yaml) currently does not bind the
execution-namespace SA so the second apply is ineffective; either remove the
second kubectl apply for sa_fournos.yaml in FOURNOS_NAMESPACE or update
rolebinding_fournos.yaml so its subjects include the ServiceAccount in
FOURNOS_NAMESPACE (or make the RoleBinding reference a
ClusterRole/ClusterRoleBinding if cross-namespace binding is intended). Also,
normalize the CI default names for clarity by updating the FOURNOS_NAMESPACE
default (currently set elsewhere) from the psap-automation-* name to a
Fournos-specific name like fournos-ci-test if you want symmetric naming with the
controller.
In `@tests/forge/deploy/orchestration/deploy.py`:
- Around line 367-403: The code assumes manifests_config contains "rbac" and
"crd" and will raise KeyError if those keys are omitted; change the block that
sets rbac_files/crd_files and calls _deploy_manifest_list to first fetch them
safely (e.g., using manifests_config.get("rbac", []) and
manifests_config.get("crd", [])) and add an if rbac_files: guard (mirroring the
controller_rbac_files guard) before logging and calling _deploy_manifest_list
for the RBAC+CRD deployment so missing keys result in no-op instead of an
exception.
🪄 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: e568e9f8-b6a0-41fa-abd7-016ccfe14a4a
📒 Files selected for processing (14)
MakefileREADME.mddev/setup.shhacks/generate-ocpci-kubeconfig.shmanifests/deployment.yamlmanifests/ocpci-sa/rbac-wip-ns.yamlmanifests/ocpci-sa/rolebinding.yamlmanifests/ocpci-sa/sa.yamlmanifests/ocpci-sa/secret.yamlmanifests/rbac/clusterrolebinding_fournos.yamlmanifests/rbac/rolebinding_fournos.yamlmanifests/secrets-ns-rbac.yamltests/forge/deploy/orchestration/config.yamltests/forge/deploy/orchestration/deploy.py
Signed-off-by: avasilev <avasilev@redhat.com>
Signed-off-by: avasilev <avasilev@redhat.com>
| - name: FOURNOS_NAMESPACE | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.namespace | ||
| value: "${NAMESPACE}" |
There was a problem hiding this comment.
Maybe we can have a more meaningful name than FOURNOS_NAMESPACE 🤔
- FOURNOS_WORKLOAD_NAMESPACE?
- FOURNOS_TARGET_NAMESPACE?
not sure
2 participants
Closes #36.
Summary by CodeRabbit
New Features
Documentation
Chores