Add activation_job_namespace for workload isolation#357
Conversation
📝 WalkthroughWalkthroughIntroduces Changesactivation_job_namespace cross-namespace 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)
Comment |
When set, activation job pods run in a dedicated namespace with their own RBAC, reducing the permissions granted in the EDA control plane namespace. Follows the same pattern as awx-operator PR #2122. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
TheRealHaoLiu
force-pushed
the
worktree-activation-job-namespace
branch
from
fd8356c to
e13ac0c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@config/crd/bases/eda.ansible.com_edas.yaml`:
- Around line 1441-1443: The activation_job_namespace field lacks input
validation, which allows invalid Kubernetes namespace names to be accepted and
cause runtime failures during reconciliation. Add schema validation constraints
to the activation_job_namespace field definition by including a pattern
constraint that enforces DNS-1123 subdomain format (allowing lowercase letters,
numbers, and hyphens), set maxLength to 253 to match Kubernetes namespace naming
limits, and ensure the pattern allows empty strings to preserve the default
behavior when the field is not specified.
In `@config/rbac/role.yaml`:
- Around line 7-13: The namespaces resource is cluster-scoped and cannot be
managed through a namespaced Role. Create a separate ClusterRole (instead of
using the existing namespaced Role) with the apiGroups, resources, and verbs for
the namespaces permission block. Then create a corresponding ClusterRoleBinding
that binds this new ClusterRole to the operator's service account. Remove the
namespaces resource rules from the current namespaced Role definition.
🪄 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: 3d1b4524-1d82-436d-b115-c1e7cc133040
📒 Files selected for processing (5)
config/crd/bases/eda.ansible.com_edas.yamlconfig/rbac/role.yamlroles/common/templates/service_account.yaml.j2roles/eda/defaults/main.ymlroles/eda/templates/eda-activation-worker.deployment.yaml.j2
| activation_job_namespace: | ||
| description: Namespace where activation job pods will run. Creates the namespace and grants required permissions automatically. | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the CRD file to see the field definition and surrounding context
head -n 1450 config/crd/bases/eda.ansible.com_edas.yaml | tail -n 30Repository: ansible/eda-server-operator
Length of output: 2425
🏁 Script executed:
# Also get more context around the field to understand its full definition
sed -n '1430,1460p' config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 2517
🏁 Script executed:
# Find and examine the template file mentioned in the review
find . -name "service_account.yaml.j2" -type fRepository: ansible/eda-server-operator
Length of output: 120
🏁 Script executed:
# Search for how activation_job_namespace is used in the codebase
rg "activation_job_namespace" -B 2 -A 2Repository: ansible/eda-server-operator
Length of output: 3571
🏁 Script executed:
# Check if there are any validation rules defined for this field or similar namespace fields in the CRD
rg "pattern|maxLength|minLength" config/crd/bases/eda.ansible.com_edas.yaml | head -20Repository: ansible/eda-server-operator
Length of output: 2557
🏁 Script executed:
# Get the exact section of the CRD for activation_job_namespace with surrounding context
sed -n '1440,1450p' config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 991
🏁 Script executed:
# Search for the full definition of activation_job_namespace field in the CRD including any validation
rg -A 10 "activation_job_namespace:" config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 638
🏁 Script executed:
# Check if there are other namespace-type fields in the CRD that might have validation patterns we should follow
rg -B 2 -A 5 "metadata.name|namespace.*name|DNS.*label" config/crd/bases/eda.ansible.com_edas.yaml | head -50Repository: ansible/eda-server-operator
Length of output: 53
🏁 Script executed:
# Let's verify Kubernetes namespace naming requirements and check if there's any validation happening elsewhere
# Search for any validationRules or CEL validations in the CRD
rg -i "validationRules|x-kubernetes" config/crd/bases/eda.ansible.com_edas.yaml | head -10Repository: ansible/eda-server-operator
Length of output: 618
🏁 Script executed:
# Check the full context around where activation_job_namespace is defined to be sure there's no validation
sed -n '1435,1455p' config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 1654
🏁 Script executed:
# Get the exact lines around 1441-1443 to see the field definition
sed -n '1440,1450p' config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 991
🏁 Script executed:
# Look for the activation_job_namespace field definition to see if there are any pattern or validation constraints right after type: string
awk '/activation_job_namespace:/{p=1} p{print} /type: string/&&p{if(++count==1) exit}' config/crd/bases/eda.ansible.com_edas.yamlRepository: ansible/eda-server-operator
Length of output: 284
🏁 Script executed:
# Check if there are any webhook validators or admission controllers in the repo
find . -type f \( -name "*webhook*" -o -name "*validation*" -o -name "*admission*" \) ! -path "*/node_modules/*" ! -path "*/.git/*" | head -20Repository: ansible/eda-server-operator
Length of output: 53
🏁 Script executed:
# Look at the template more carefully to understand how activation_job_namespace is used
cat roles/common/templates/service_account.yaml.j2 | grep -A 5 -B 5 "activation_job_namespace"Repository: ansible/eda-server-operator
Length of output: 1452
Add schema validation for activation_job_namespace to enforce Kubernetes namespace naming rules.
This field is rendered directly into Namespace.metadata.name and RBAC namespace fields without validation. Invalid values will cause reconciliation failures at runtime instead of being rejected at admission time. Add a pattern constraint for DNS-1123 subdomain format with maxLength: 253, while allowing empty string for the default behavior.
🤖 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 `@config/crd/bases/eda.ansible.com_edas.yaml` around lines 1441 - 1443, The
activation_job_namespace field lacks input validation, which allows invalid
Kubernetes namespace names to be accepted and cause runtime failures during
reconciliation. Add schema validation constraints to the
activation_job_namespace field definition by including a pattern constraint that
enforces DNS-1123 subdomain format (allowing lowercase letters, numbers, and
hyphens), set maxLength to 253 to match Kubernetes namespace naming limits, and
ensure the pattern allows empty strings to preserve the default behavior when
the field is not specified.
| - apiGroups: | ||
| - "" | ||
| resources: | ||
| - namespaces | ||
| verbs: | ||
| - get | ||
| - create |
There was a problem hiding this comment.
Move namespaces resource rules to a ClusterRole with ClusterRoleBinding.
namespaces is cluster-scoped and cannot be authorized through a namespaced Role. This prevents the activation_job_namespace feature from creating namespaces. Create a separate ClusterRole with the namespace permissions and bind it to the operator's service account.
🤖 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 `@config/rbac/role.yaml` around lines 7 - 13, The namespaces resource is
cluster-scoped and cannot be managed through a namespaced Role. Create a
separate ClusterRole (instead of using the existing namespaced Role) with the
apiGroups, resources, and verbs for the namespaces permission block. Then create
a corresponding ClusterRoleBinding that binds this new ClusterRole to the
operator's service account. Remove the namespaces resource rules from the
current namespaced Role definition.
Remove batch/extensions jobs from EDA namespace Role when activation_job_namespace is set, and restore the ingress rule for the unset case to keep the Role identical to the original. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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/common/templates/service_account.yaml.j2 (1)
21-32:⚠️ Potential issue | 🟠 MajorEmpty
rulesblock produces invalid YAML whenactivation_job_namespaceis set.When
activation_job_namespaceis set, the main Role template outputsrules:followed by no value, which YAML parses asrules: null. Kubernetes RBAC validation expects an array, not null, which will cause API validation failures.Additionally, the job-execution Role (used when
activation_job_namespaceis set) lacks ingress permissions that are present in the main Role. This creates a permissions gap: ingress access is available only whenactivation_job_namespaceis NOT set.Suggested fix: emit empty array instead of null
rules: -{% if not activation_job_namespace %} +{% if activation_job_namespace %} +[] +{% else %} - apiGroups: [""] resources: ["pods", "pods/log", "jobs", "secrets", "services"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - apiGroups: ["batch", "extensions"] resources: ["jobs"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - apiGroups: ["networking.k8s.io"] resources: ["ingresses"] verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] {% endif %}Note: The missing ingress permissions in the job-execution Role also need to be addressed to maintain consistent access across both RBAC configurations.
🤖 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/common/templates/service_account.yaml.j2` around lines 21 - 32, The rules block in the service_account.yaml.j2 template outputs null (invalid YAML) when activation_job_namespace is set because the conditional {% if not activation_job_namespace %} wraps all the rules. To fix this, ensure the rules key always has a value by either providing an empty array [] when the condition is false, or restructuring the template so rules always contains valid content. Additionally, the job-execution Role template (used when activation_job_namespace is set) is missing the networking.k8s.io ingresses permissions that exist in the main Role. Add the ingresses rule block with the same verbs (get, list, watch, create, update, patch, delete) to the job-execution Role to maintain consistent permissions across both RBAC configurations.
🤖 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/common/templates/service_account.yaml.j2`:
- Around line 21-32: The rules block in the service_account.yaml.j2 template
outputs null (invalid YAML) when activation_job_namespace is set because the
conditional {% if not activation_job_namespace %} wraps all the rules. To fix
this, ensure the rules key always has a value by either providing an empty array
[] when the condition is false, or restructuring the template so rules always
contains valid content. Additionally, the job-execution Role template (used when
activation_job_namespace is set) is missing the networking.k8s.io ingresses
permissions that exist in the main Role. Add the ingresses rule block with the
same verbs (get, list, watch, create, update, patch, delete) to the
job-execution Role to maintain consistent permissions across both RBAC
configurations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 4568efd9-4b11-4c9e-889f-9409f477c782
📒 Files selected for processing (1)
roles/common/templates/service_account.yaml.j2
1 participant
Summary
activation_job_namespacefield to the EDA CR that isolates activation job pods in a dedicated namespaceEDA_ACTIVATION_JOB_NAMESPACEto the activation-worker containerFollows the same pattern as awx-operator#2122.
Changes
config/crd/bases/eda.ansible.com_edas.yamlactivation_job_namespacestring field to CRD specroles/eda/defaults/main.ymlactivation_job_namespace: ''defaultroles/common/templates/service_account.yaml.j2config/rbac/role.yamlnamespaces(get, create) permissionroles/eda/templates/eda-activation-worker.deployment.yaml.j2EDA_ACTIVATION_JOB_NAMESPACEenv var to activation-worker when setTest plan
activation_job_namespaceset — verify existing behavior is unchangedactivation_job_namespace: eda-jobs— verify:eda-jobsis creatededa-jobsnamespace with correct permissionsEDA_ACTIVATION_JOB_NAMESPACE=eda-jobsenv vareda-jobsnamespaceSummary by CodeRabbit
New Features
spec.activation_job_namespaceto specify the namespace where activation job pods run.EDA_ACTIVATION_JOB_NAMESPACEwhenactivation_job_namespaceis provided.RBAC & Security
activation_job_namespace.