feat: optional Kubernetes pod metadata for activation jobs#1520
Open
amasolov wants to merge 8 commits intoansible:mainfrom
Open
feat: optional Kubernetes pod metadata for activation jobs#1520amasolov wants to merge 8 commits intoansible:mainfrom
amasolov wants to merge 8 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: three fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Serializer as Activation Serializer
participant Validator as Validation Layer
participant Model as Activation Model
participant Engine as Kubernetes Engine
participant K8s as Kubernetes API
Client->>Serializer: POST activation with k8s_pod_* fields
Serializer->>Validator: normalize & validate service account, labels, annotations
Validator-->>Serializer: validation result / errors
Serializer->>Model: save Activation with k8s_pod_* fields
Model-->>Serializer: Activation persisted
Serializer-->>Client: response with stored metadata
activate Engine
Engine->>Engine: build ContainerRequest (includes k8s_pod_* from Activation)
Engine->>Engine: merge labels with defaults (app="eda", job-name=...)
Engine->>Engine: assemble pod metadata & spec (apply annotations, serviceAccountName)
Engine->>K8s: create_namespaced_job(pod_template)
K8s-->>Engine: Job created
deactivate Engine
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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% 91.99% +0.07%
==========================================
Files 239 241 +2
Lines 10796 10936 +140
==========================================
+ Hits 9924 10061 +137
- Misses 872 875 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 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
|
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
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