feat: add configurable namespace for activation job pods#345
Open
amasolov wants to merge 5 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds operator-side support for directing EDA activation job pods into a configurable Kubernetes namespace, extending the CRD, adding cluster- and namespace-scoped RBAC, wiring an env var into the operator ConfigMap, and updating deployment tasks and CI/workflow. ChangesCross-Namespace Activation Job Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/rbac/activation_job_namespace_role.yaml`:
- Around line 1-26: The ClusterRole eda-activation-job-namespace-manager
currently allows create/update/patch/delete on resources roles and rolebindings
across all namespaces, which grants the operator ServiceAccount cluster-wide
RBAC management; update the project documentation and operator
security/threat-model notes to explicitly record that
eda-activation-job-namespace-manager grants these cluster-scoped verbs on
resources roles and rolebindings, why this broad scope is required for dynamic
target namespaces, and list mitigation/alternatives (e.g., dynamic ClusterRole
provisioning via an admission webhook or dedicated RBAC controller) so reviewers
and deployers are aware of the elevated privilege and possible hardening paths.
In `@roles/eda/tasks/deploy_eda.yml`:
- Around line 48-61: The cleanup task never deletes RBAC because it uses
combined_activation_worker.activation_job_namespace (which is empty when
cleared) and _previous_activation_job_namespace is never set; fix by first
capturing the existing activation namespace into
_previous_activation_job_namespace (use a set_fact immediately after loading the
CR/ConfigMap where combined_activation_worker.activation_job_namespace is read)
and then change the k8s task's namespace to use
_previous_activation_job_namespace and adjust the when to check
_previous_activation_job_namespace | default('') | length > 0 and
combined_activation_worker.activation_job_namespace | default('') | length == 0
so the Role/RoleBinding deletion runs against the old namespace.
In `@roles/eda/templates/eda-activation-job-namespace-rbac.yaml.j2`:
- Around line 11-46: The Role's rules for resources "secrets", "pods",
"pods/log", "services", and "jobs" are missing the "update" verb and will cause
403s when the EDA server performs HTTP PUT updates; modify each corresponding
rule in the template (the rules blocks that list apiGroups: "" for
secrets/pods/pods/log and services, and apiGroups: batch for jobs) to include
"update" in the verbs array so the Role permits update operations.
🪄 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: Enterprise
Run ID: daea2dbc-5fd1-4016-94c6-81433770c31f
📒 Files selected for processing (8)
config/crd/bases/eda.ansible.com_edas.yamlconfig/rbac/activation_job_namespace_role.yamlconfig/rbac/activation_job_namespace_role_binding.yamlconfig/rbac/kustomization.yamlroles/eda/defaults/main.ymlroles/eda/tasks/deploy_eda.ymlroles/eda/templates/eda-activation-job-namespace-rbac.yaml.j2roles/eda/templates/eda.configmap.yaml.j2
This was referenced May 4, 2026
Add activation_job_namespace field to the activation_worker section of the EDA CRD. When set, the operator injects EDA_ACTIVATION_JOB_NAMESPACE into the activation worker ConfigMap and creates a Role + RoleBinding in the target namespace so the EDA ServiceAccount can manage Jobs, Pods, Secrets, and Services there. Changes: - CRD: new optional string field activation_worker.activation_job_namespace - Role defaults: activation_job_namespace defaults to empty string - ConfigMap template: conditionally sets EDA_ACTIVATION_JOB_NAMESPACE - New RBAC template for cross-namespace Role and RoleBinding - Deploy task: applies/removes cross-namespace RBAC conditionally - ClusterRole + ClusterRoleBinding for operator to manage RBAC in the target namespace Closes ansible#344 Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amasolov
added a commit
to amasolov/eda-server
that referenced
this pull request
May 4, 2026
Read EDA_ACTIVATION_JOB_NAMESPACE environment variable in _set_namespace() as an override before falling back to the ServiceAccount token namespace file. This allows the eda-server-operator to direct activation job pods into a separate Kubernetes namespace for security isolation, resource quotas, and NetworkPolicy boundaries. Changes: - _set_namespace() checks EDA_ACTIVATION_JOB_NAMESPACE first - Unit tests for override, fallback, whitespace handling, and error case Companion operator PR: ansible/eda-server-operator#345 Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
amasolov
force-pushed
the
feature/activation-job-namespace
branch
from
9f90271 to
1353abe
Compare
Contributor
|
@amasolov I think it would be worthwhile adding a new CR (e.g And then add it to the matrix in pr.yml: We'll also need to add a step to ensure the namespace is create before applying the CR: |
Add a CI test scenario for the new activation_job_namespace parameter. Creates a dedicated CR fixture and pre-creates the target namespace before applying the CR. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
roles/eda/tasks/deploy_eda.yml (1)
22-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevious namespace is captured too late, and cleanup misses namespace changes.
Line 22 applies the env ConfigMap before Lines 41-56 read
_previous_activation_job_namespace, so on unset/change you can lose the old value and skip deletion in the old namespace. Also, Lines 75-76 only clean up when the new value is empty; changingA -> Bleaves RBAC orphaned inA.Suggested fix
+# Move these tasks to run before "Apply ConfigMap resources" - name: Look up existing ConfigMap for previous activation_job_namespace kubernetes.core.k8s_info: api_version: v1 kind: ConfigMap namespace: "{{ ansible_operator_meta.namespace }}" name: "{{ ansible_operator_meta.name }}-{{ deployment_type }}-env-properties" register: _eda_env_cm - name: Record previous activation_job_namespace from ConfigMap ansible.builtin.set_fact: _previous_activation_job_namespace: >- {{ (_eda_env_cm.resources | first).data.EDA_ACTIVATION_JOB_NAMESPACE | default('') }} when: - _eda_env_cm.resources | length > 0 - (_eda_env_cm.resources | first).data is defined - name: Remove cross-namespace RBAC when activation_job_namespace is unset k8s: state: absent api_version: rbac.authorization.k8s.io/v1 kind: "{{ item.kind }}" name: "{{ ansible_operator_meta.name }}-activation-job-manager" namespace: "{{ _previous_activation_job_namespace }}" loop: - { kind: RoleBinding } - { kind: Role } when: - - combined_activation_worker.activation_job_namespace | default('') | length == 0 - _previous_activation_job_namespace | default('') | length > 0 + - combined_activation_worker.activation_job_namespace | default('') != _previous_activation_job_namespaceAlso applies to: 41-77
🤖 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 `@roles/eda/tasks/deploy_eda.yml` around lines 22 - 27, The Apply ConfigMap resources task currently runs before capturing _previous_activation_job_namespace and the RBAC cleanup only triggers when the new namespace is empty, which can leave orphaned RBAC in the old namespace; move or duplicate the lookup/read of _previous_activation_job_namespace so it happens before the "Apply ConfigMap resources" step (ensure the variable is captured prior to any namespace-changing actions), and change the RBAC deletion condition (the logic around the lines that check the new namespace — currently only deleting when empty) to also delete when the previous namespace differs from the new one so RBAC is removed from the old namespace when namespace changes from A to B; ensure you still update/set _previous_activation_job_namespace after successful apply so future runs have the correct previous value.
🤖 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.
Outside diff comments:
In `@roles/eda/tasks/deploy_eda.yml`:
- Around line 22-27: The Apply ConfigMap resources task currently runs before
capturing _previous_activation_job_namespace and the RBAC cleanup only triggers
when the new namespace is empty, which can leave orphaned RBAC in the old
namespace; move or duplicate the lookup/read of
_previous_activation_job_namespace so it happens before the "Apply ConfigMap
resources" step (ensure the variable is captured prior to any namespace-changing
actions), and change the RBAC deletion condition (the logic around the lines
that check the new namespace — currently only deleting when empty) to also
delete when the previous namespace differs from the new one so RBAC is removed
from the old namespace when namespace changes from A to B; ensure you still
update/set _previous_activation_job_namespace after successful apply so future
runs have the correct previous value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 428b68ab-1f1e-4185-aebe-0d8374f73ae9
📒 Files selected for processing (10)
.ci/eda_v1alpha1_eda.activation_job_namespace.ci.yaml.github/workflows/pr.ymlconfig/crd/bases/eda.ansible.com_edas.yamlconfig/rbac/activation_job_namespace_role.yamlconfig/rbac/activation_job_namespace_role_binding.yamlconfig/rbac/kustomization.yamlroles/eda/defaults/main.ymlroles/eda/tasks/deploy_eda.ymlroles/eda/templates/eda-activation-job-namespace-rbac.yaml.j2roles/eda/templates/eda.configmap.yaml.j2
✅ Files skipped from review due to trivial changes (3)
- .ci/eda_v1alpha1_eda.activation_job_namespace.ci.yaml
- config/rbac/activation_job_namespace_role.yaml
- config/rbac/kustomization.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- roles/eda/defaults/main.yml
- config/crd/bases/eda.ansible.com_edas.yaml
- roles/eda/templates/eda.configmap.yaml.j2
- roles/eda/templates/eda-activation-job-namespace-rbac.yaml.j2
…espace changes Move the lookup of _previous_activation_job_namespace to run before the ConfigMap is applied, so the old value is read before it gets overwritten. Also widen the RBAC cleanup condition to trigger whenever the previous namespace differs from the new one (not just when the new value is empty), so changing from namespace A to B correctly removes orphaned Role and RoleBinding from A. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add the escalate verb on the roles resource in the eda-activation-job-namespace-manager ClusterRole. Kubernetes RBAC escalation prevention blocks creating a Role that grants permissions the creator doesn't hold. The escalate verb is the standard mechanism to permit this without granting the operator broad cluster-wide access to pods, secrets, services, and jobs. Also moves the _previous_activation_job_namespace lookup to run before the ConfigMap is applied (so the old value is captured before overwrite) and widens the RBAC cleanup condition to trigger on any namespace change, not just removal. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
The escalate verb fixed Role creation, but creating a RoleBinding that references a Role with elevated permissions also requires the bind verb on the roles resource. Without it, Kubernetes RBAC escalation prevention blocks the RoleBinding creation. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Author
Agreed and done! |
ttuffin
pushed a commit
to ansible/eda-server
that referenced
this pull request
May 19, 2026
- Adds `EDA_ACTIVATION_JOB_NAMESPACE` environment variable support to `_set_namespace()` in the Kubernetes engine, allowing activation job pods to be directed into a separate namespace. Companion PR: ansible/eda-server-operator#345 Related: ansible/eda-server-operator#344 Made with [Cursor](https://cursor.com) Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
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
2 participants
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
activation_job_namespacefield to theactivation_workersection of the EDA CRD, allowing activation job pods to run in a separate Kubernetes namespace for security isolation, resource quota management, and NetworkPolicy boundaries.EDA_ACTIVATION_JOB_NAMESPACEinto the activation worker ConfigMap and creates cross-namespace RBAC so the EDA ServiceAccount can manage Jobs, Pods, Secrets, and Services in the target namespace.What changed
activation_worker.activation_job_namespace.activation_job_namespacedefaults to empty string (feature disabled).EDA_ACTIVATION_JOB_NAMESPACEenv var.eda-activation-job-namespace-rbac.yaml.j2creates a Role and RoleBinding in the target namespace.activation_job_namespaceis set; removes it when unset.Why
Operators running EDA on shared clusters need activation job pods (which run user-supplied rulebooks and decision environments) isolated from the EDA control plane. A dedicated namespace enables independent ResourceQuota, LimitRange, and NetworkPolicy controls.
Server-side companion
The
eda-serverside readsEDA_ACTIVATION_JOB_NAMESPACEas an override in_set_namespace(), falling back to the SA token file. A companion PR will be filed onansible/eda-server.How to test
activation_job_namespace(default behaviour, no change).activation_worker.activation_job_namespace: "eda-jobs"on the CR.eda-jobsnamespace.EDA_ACTIVATION_JOB_NAMESPACEappears in the activation worker ConfigMap.Breaking changes / dependencies
None. Field is optional; existing CRs are unaffected.
Closes #344.
Made with Cursor
Summary by CodeRabbit
New Features
Tests