feat: optional Kubernetes pod metadata for activation jobs#1520
Open
amasolov wants to merge 12 commits intoansible:mainfrom
Open
feat: optional Kubernetes pod metadata for activation jobs#1520amasolov wants to merge 12 commits intoansible:mainfrom
amasolov wants to merge 12 commits intoansible:mainfrom
Conversation
Add k8s_pod_service_account_name, k8s_pod_labels, and k8s_pod_annotations on Activation; apply to rulebook activation Job pod templates on Kubernetes. Validate labels and annotations; reserve EDA selector labels app and job-name. Includes migration, API serializers, analytics export, and tests. Made-with: Cursor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional Kubernetes pod metadata to activations: four fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Serializer
participant Validator
participant Model
participant Engine
participant K8s
participant Settings
Client->>Serializer: POST/PUT activation with k8s_pod_* payload
Serializer->>Validator: _normalize_activation_k8s_pod_fields -> validate k8s fields
Validator->>Settings: check DEPLOYMENT_TYPE / ALLOWED_SERVICE_ACCOUNTS
Validator-->>Serializer: validation result (errors or normalized data)
Serializer->>Model: save Activation with k8s_pod_* fields
Model-->>Serializer: persisted Activation
Serializer-->>Client: response including _activation_k8s_pod_metadata_payload
activate Engine
Engine->>Model: load Activation (contains k8s_pod_* fields)
Engine->>Engine: build ContainerRequest including k8s_pod_* fields
Engine->>Engine: merge labels with defaults (app="eda", job-name=...)
Engine->>Engine: assemble pod metadata & spec (annotations, serviceAccountName, nodeSelector)
Engine->>K8s: create_namespaced_job(pod_template)
K8s-->>Engine: Job created
deactivate Engine
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🧹 Nitpick comments (2)
tests/integration/services/activation/engine/test_kubernetes.py (1)
317-323: Add an assertion for the reservedjob-namepod label.This test already verifies user metadata merge; asserting
job-nametoo will protect selector compatibility regressions.Suggested test addition
labels = created_body.spec.template.metadata.labels assert labels["app"] == "eda" + assert labels["job-name"] == engine.job_name assert labels["cost-centre"] == "cba"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/services/activation/engine/test_kubernetes.py` around lines 317 - 323, Add an assertion that the reserved pod label "job-name" exists and has the expected value on the merged pod template labels: after reading created_body.spec.template.metadata.labels (used above), assert labels["job-name"] == "<expected-job-name>" (replace with the correct job name used in the test setup) to ensure the reserved selector label is preserved during metadata merging.tests/unit/test_k8s_pod_metadata.py (1)
24-46: Validator tests are a good start; add boundary-path coverage.Please add cases for non-object payloads and max-length violations (label value 63+, annotation value 256KiB+) so key validation constraints stay protected.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_k8s_pod_metadata.py` around lines 24 - 46, Add boundary-path unit tests to tests/unit/test_k8s_pod_metadata.py covering non-object payloads and max-length violations: call validate_k8s_pod_labels and validate_k8s_pod_annotations with non-dict inputs (e.g., string, list, None) and assert serializers.ValidationError is raised, and add tests that pass a label value of length >=63 characters to validate_k8s_pod_labels and an annotation value exceeding 256KiB (262144 bytes) to validate_k8s_pod_annotations and assert they raise serializers.ValidationError; reference the existing test functions test_validate_k8s_pod_labels_ok/test_validate_k8s_pod_annotations_ok to mirror style and error-assertion patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aap_eda/api/serializers/activation.py`:
- Around line 610-613: Trim whitespace from service account strings before
persisting: in each serializer validate method that currently does `sa =
data.get("k8s_pod_service_account_name"); if sa is not None and str(sa).strip()
== "": data["k8s_pod_service_account_name"] = None`, instead normalize by
computing trimmed = str(sa).strip() and then set
`data["k8s_pod_service_account_name"] = trimmed if trimmed != "" else None`;
apply the same trimming-and-None logic to the other two analogous validate
methods/locations that currently only map blank strings to None (also update any
similar handling for k8s_pod_labels), so non-empty values are stored trimmed
rather than with surrounding whitespace.
---
Nitpick comments:
In `@tests/integration/services/activation/engine/test_kubernetes.py`:
- Around line 317-323: Add an assertion that the reserved pod label "job-name"
exists and has the expected value on the merged pod template labels: after
reading created_body.spec.template.metadata.labels (used above), assert
labels["job-name"] == "<expected-job-name>" (replace with the correct job name
used in the test setup) to ensure the reserved selector label is preserved
during metadata merging.
In `@tests/unit/test_k8s_pod_metadata.py`:
- Around line 24-46: Add boundary-path unit tests to
tests/unit/test_k8s_pod_metadata.py covering non-object payloads and max-length
violations: call validate_k8s_pod_labels and validate_k8s_pod_annotations with
non-dict inputs (e.g., string, list, None) and assert
serializers.ValidationError is raised, and add tests that pass a label value of
length >=63 characters to validate_k8s_pod_labels and an annotation value
exceeding 256KiB (262144 bytes) to validate_k8s_pod_annotations and assert they
raise serializers.ValidationError; reference the existing test functions
test_validate_k8s_pod_labels_ok/test_validate_k8s_pod_annotations_ok to mirror
style and error-assertion patterns.
🪄 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: Pro
Run ID: 51d0d033-cb9d-46e1-bfa7-e673fffa1ce2
📒 Files selected for processing (11)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/core/migrations/0071_activation_k8s_pod_metadata.pysrc/aap_eda/core/models/activation.pysrc/aap_eda/core/utils/k8s_pod_metadata.pysrc/aap_eda/core/validators.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/activation/engine/kubernetes.pytests/integration/analytics/test_analytics_collectors.pytests/integration/services/activation/engine/test_kubernetes.pytests/unit/test_k8s_pod_metadata.py
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1520 +/- ##
==========================================
+ Coverage 91.92% 92.04% +0.12%
==========================================
Files 239 241 +2
Lines 10796 10973 +177
==========================================
+ Hits 9924 10100 +176
- Misses 872 873 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
- Trim ServiceAccount and label/annotation strings before persist - Stricter qualified-name validation for K8s keys (prefix/name, lengths) - Add integration assert for job-name label; expand unit boundary tests - Add validator docstrings for CodeRabbit docstring coverage Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/aap_eda/api/serializers/activation.py (2)
120-154: Add focused tests for the normalization branches.This helper now owns whitespace trimming,
{}coercion, and empty-key failures for all three write/validate paths, but this file still has uncovered changed lines. A few serializer tests aroundNone, whitespace-only ServiceAccount names, trimmed keys/values, and empty-key rejection would reduce regression risk.Also applies to: 621-636, 812-827, 1312-1342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/api/serializers/activation.py` around lines 120 - 154, Add focused serializer tests that exercise _normalize_activation_k8s_pod_fields behavior: cover k8s_pod_service_account_name being None and whitespace-only (should coerce to None after strip), k8s_pod_labels and k8s_pod_annotations being None (should coerce to {}), labels/annotations with string keys/values containing surrounding whitespace (keys/values should be trimmed) and non-string values preserved, and cases with empty/whitespace-only keys that must raise serializers.ValidationError; create tests that call the serializer/path that triggers _normalize_activation_k8s_pod_fields (the same serializer functions referenced in the diff) and assert normalized outputs or raised errors for each branch.
540-545: Extract a shared pod-metadata payload helper.These same three assignments now live in five places. Centralizing them would reduce drift risk the next time these fields change.
♻️ Possible cleanup
+def _activation_k8s_pod_metadata_payload( + activation: models.Activation, +) -> dict: + return { + "k8s_pod_service_account_name": ( + activation.k8s_pod_service_account_name + ), + "k8s_pod_labels": activation.k8s_pod_labels or {}, + "k8s_pod_annotations": activation.k8s_pod_annotations or {}, + } ... - "k8s_pod_service_account_name": ( - activation.k8s_pod_service_account_name - ), - "k8s_pod_labels": activation.k8s_pod_labels or {}, - "k8s_pod_annotations": activation.k8s_pod_annotations or {}, + **_activation_k8s_pod_metadata_payload(activation),As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 726-730, 978-982, 1263-1267, 1398-1402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/api/serializers/activation.py` around lines 540 - 545, Extract a shared helper that builds the pod-metadata dict from an Activation-like object (readers see fields activation.k8s_pod_service_account_name, activation.k8s_pod_labels, activation.k8s_pod_annotations) and return {"k8s_pod_service_account_name": ..., "k8s_pod_labels": ... or {}, "k8s_pod_annotations": ... or {}}; replace the five inline occurrences (the blocks assigning those three keys) with a call to this helper (e.g., build_pod_metadata(activation) or Activation.to_pod_metadata()) so all serializers reuse the same implementation and avoid duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aap_eda/api/serializers/activation.py`:
- Around line 120-154: Add focused serializer tests that exercise
_normalize_activation_k8s_pod_fields behavior: cover
k8s_pod_service_account_name being None and whitespace-only (should coerce to
None after strip), k8s_pod_labels and k8s_pod_annotations being None (should
coerce to {}), labels/annotations with string keys/values containing surrounding
whitespace (keys/values should be trimmed) and non-string values preserved, and
cases with empty/whitespace-only keys that must raise
serializers.ValidationError; create tests that call the serializer/path that
triggers _normalize_activation_k8s_pod_fields (the same serializer functions
referenced in the diff) and assert normalized outputs or raised errors for each
branch.
- Around line 540-545: Extract a shared helper that builds the pod-metadata dict
from an Activation-like object (readers see fields
activation.k8s_pod_service_account_name, activation.k8s_pod_labels,
activation.k8s_pod_annotations) and return {"k8s_pod_service_account_name": ...,
"k8s_pod_labels": ... or {}, "k8s_pod_annotations": ... or {}}; replace the five
inline occurrences (the blocks assigning those three keys) with a call to this
helper (e.g., build_pod_metadata(activation) or Activation.to_pod_metadata()) so
all serializers reuse the same implementation and avoid duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4eabf054-e86c-4a6e-afc5-c01e87952f09
📒 Files selected for processing (2)
src/aap_eda/api/serializers/activation.pysrc/aap_eda/services/activation/engine/common.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/aap_eda/services/activation/engine/common.py
Made-with: Cursor
- Extract _activation_k8s_pod_metadata_payload helper to replace five identical pod-metadata blocks in serializers (duplication 9% -> ~2%). - Add comprehensive tests for validators.py DEPLOYMENT_TYPE gating, k8s_pod_metadata.py qualified-key edge cases, _normalize helper (trim, None coercion, empty-key rejection), and the payload helper. Made-with: Cursor
Move repeated serializer field declarations for k8s_pod_service_account_name, k8s_pod_labels, and k8s_pod_annotations into _K8sPodMetadataReadFields and _K8sPodMetadataWriteFields mixins. Five serializer classes now inherit them instead of declaring them inline, targeting SonarCloud duplication < 3%. Made-with: Cursor
- _normalize_activation_k8s_pod_fields: extract _trim_string_dict helper to reduce cognitive complexity from 22 to ~8 (S3776, threshold 15). - _validate_qualified_metadata_key: extract _validate_prefixed_key and _is_valid_dns_subdomain helpers to reduce complexity from 18 to ~6. - Replace _DNS_SUBDOMAIN regex with per-label matching via _DNS_LABEL to eliminate backtracking risk flagged as security hotspot (S5852). Made-with: Cursor
Made-with: Cursor
Contributor
|
/run-e2e |
mkanoor
reviewed
Apr 17, 2026
| class _K8sPodMetadataWriteFields: | ||
| """Writable k8s pod-metadata field declarations with validators.""" | ||
|
|
||
| k8s_pod_service_account_name = serializers.CharField( |
Contributor
There was a problem hiding this comment.
@amasolov Can we add Node Selectors to this list too so the user can decide which node will run the Activation. They can enter a dict with all the fields that they would need to identify the node.
When you have a service_account_name do we need credentials to go with it.
Also the UI would have to change to add support for these new fields. The collection would also have to change.
These fields are only needed if the deployment type is k8s. It should get ignored for podman deployment type
Contributor
|
@amasolov The use of SA accounts can lead to privilege escalation e.g With cluster-admin the Rulebook Activation creator can access secrets and anything else that they need. |
Contributor
|
@amasolov We would have to schedule a UI and collection work related to this once the back end has been changed. But overall these are nice things to have in the product. |
- New `k8s_pod_node_selector` JSONField on Activation model (migration 0071) allows users to schedule activation job pods onto specific nodes via Kubernetes nodeSelector. Addresses AAPRFE-2840. - New `ALLOWED_SERVICE_ACCOUNTS` setting (env: EDA_ALLOWED_SERVICE_ACCOUNTS) restricts which ServiceAccount names can be specified on activations, preventing privilege escalation via arbitrary SA names. - Kubernetes engine applies nodeSelector to the pod template spec. - Podman deployments: all k8s_pod_* fields are silently ignored (validators skip when DEPLOYMENT_TYPE != "k8s", podman engine never reads them). - Serializers, analytics export, and tests updated for both features. Made-with: Cursor
Contributor
Author
|
Thanks @mkanoor, all addressed in
|
Tests that patch aap_eda.core.validators.settings use a MagicMock, so ALLOWED_SERVICE_ACCOUNTS resolves to a truthy MagicMock object instead of the default empty list. Explicitly set it to [] in tests that are not exercising the allowlist logic. Signed-off-by: Artem Masolov <amasolov@redhat.com> Made-with: Cursor
Contributor
Contributor
Author
|
Good shout. Happy to tackle that as a follow-up PR to keep this one focused on pod metadata. I'll open an issue for it. |
Contributor
Author
|
Good shout. The k8s engine doesn't set any resource requests/limits today, so I've added |
- Annotation validation: enforce total size (256 KiB) across all keys and values combined instead of per-value limit, matching Kubernetes apimachinery behaviour. - Remove blanket 253-char cap on qualified metadata keys. Prefixed keys (prefix/name) can be up to 253+1+63=317 chars; validation now relies on prefix(<=253) and name(<=63) checks individually. - NodeSelector validation: validate keys as Kubernetes qualified names and values as label values (<=63 chars, name segment regex), not just string type checks. - Update tests accordingly. Signed-off-by: Artem Masolov <amasolov@redhat.com> Made-with: Cursor
A 253-char prefix must be a valid DNS subdomain (labels <=63 chars separated by dots). The previous test used 253 'a' chars without dots, which fails the per-label length check. Signed-off-by: Artem Masolov <amasolov@redhat.com> Made-with: Cursor
|
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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)
src/aap_eda/api/serializers/activation.py (1)
735-776:⚠️ Potential issue | 🟠 MajorValidate copied pod metadata against the current allowlist.
copy()now persists the source activation’sk8s_pod_*values directly throughModelSerializer.create(), bypassing the new write-field validators. IfALLOWED_SERVICE_ACCOUNTSis tightened after an activation was created, copying that activation can still create a new one with a now-disallowed ServiceAccount.Proposed fix
def copy(self) -> dict: activation: models.Activation = self.instance + pod_metadata = _activation_k8s_pod_metadata_payload(activation) + _normalize_activation_k8s_pod_fields(pod_metadata) + validators.check_if_k8s_pod_service_account_name_valid( + pod_metadata["k8s_pod_service_account_name"] + ) + validators.check_if_k8s_pod_labels_valid( + pod_metadata["k8s_pod_labels"] + ) + validators.check_if_k8s_pod_annotations_valid( + pod_metadata["k8s_pod_annotations"] + ) + validators.check_if_k8s_pod_node_selector_valid( + pod_metadata["k8s_pod_node_selector"] + ) copied_data = { "name": self.validated_data["name"], "description": activation.description, @@ "enable_persistence": activation.enable_persistence, "rule_engine_credential_id": activation.rule_engine_credential_id, - **_activation_k8s_pod_metadata_payload(activation), + **pod_metadata, }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aap_eda/api/serializers/activation.py` around lines 735 - 776, The copy() method currently bypasses write-field validators by passing activation k8s pod metadata straight into super().create(copied_data); before calling super().create, run the serializer field validators for any pod-related fields (e.g., "k8s_service_account", "k8s_service_name", and any other k8s_* fields produced by _activation_k8s_pod_metadata_payload) and reject or normalize values that fail the allowlist; implement this by iterating the relevant field names, calling self.fields[field_name].run_validation(copied_data[field_name]) (or raising serializers.ValidationError) and replacing copied_data[field_name] with the validated value, then call super().create(copied_data).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/aap_eda/api/serializers/activation.py`:
- Around line 735-776: The copy() method currently bypasses write-field
validators by passing activation k8s pod metadata straight into
super().create(copied_data); before calling super().create, run the serializer
field validators for any pod-related fields (e.g., "k8s_service_account",
"k8s_service_name", and any other k8s_* fields produced by
_activation_k8s_pod_metadata_payload) and reject or normalize values that fail
the allowlist; implement this by iterating the relevant field names, calling
self.fields[field_name].run_validation(copied_data[field_name]) (or raising
serializers.ValidationError) and replacing copied_data[field_name] with the
validated value, then call super().create(copied_data).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3729a7b9-fcb4-440d-b514-2a796a6f036d
📒 Files selected for processing (12)
src/aap_eda/analytics/analytics_collectors.pysrc/aap_eda/api/serializers/activation.pysrc/aap_eda/core/migrations/0071_activation_k8s_pod_metadata.pysrc/aap_eda/core/models/activation.pysrc/aap_eda/core/utils/k8s_pod_metadata.pysrc/aap_eda/core/validators.pysrc/aap_eda/services/activation/engine/common.pysrc/aap_eda/services/activation/engine/kubernetes.pysrc/aap_eda/settings/defaults.pytests/integration/analytics/test_analytics_collectors.pytests/integration/services/activation/engine/test_kubernetes.pytests/unit/test_k8s_pod_metadata.py
✅ Files skipped from review due to trivial changes (3)
- tests/integration/analytics/test_analytics_collectors.py
- src/aap_eda/services/activation/engine/common.py
- src/aap_eda/core/utils/k8s_pod_metadata.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/aap_eda/analytics/analytics_collectors.py
- src/aap_eda/core/models/activation.py
- tests/integration/services/activation/engine/test_kubernetes.py
- tests/unit/test_k8s_pod_metadata.py
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
4 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
Adds optional Kubernetes pod template settings on Activation so rulebook activation Jobs can use a chosen ServiceAccount and custom labels and annotations (for example workload identity patterns).
What changed
k8s_pod_service_account_name,k8s_pod_labels,k8s_pod_annotations(migration0071).kubernetes.Engineapplies them to the activation Job pod template. Labelsappandjob-nameremain under EDA control; user cannot set those keys.Why
Operators need per-activation pod identity (named ServiceAccount) and provider-specific annotations without post-create mutation.
How to test
Full CI:
black,isort,ruff,flake8, OpenAPI generation (see.github/workflows/ci.yaml).Breaking changes / dependencies
None. Fields are optional; existing activations default to empty / null behaviour unchanged.
Notes
Fixes #1521
Summary by CodeRabbit
New Features
Data / Schema
Validation
Tests