Skip to content

feat: support activation job namespace override via env var#1546

Merged
ttuffin merged 2 commits into
ansible:mainfrom
amasolov:feature/activation-job-namespace-override
May 19, 2026
Merged

feat: support activation job namespace override via env var#1546
ttuffin merged 2 commits into
ansible:mainfrom
amasolov:feature/activation-job-namespace-override

Conversation

@amasolov
Copy link
Copy Markdown
Contributor

@amasolov amasolov commented May 4, 2026

Summary

  • 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.

What changed

  • _set_namespace() reads EDA_ACTIVATION_JOB_NAMESPACE first, falling back to the ServiceAccount token namespace file when the variable is unset or empty.
  • Unit tests covering override, fallback, whitespace stripping, and error cases.

Why

Running activation job pods in a dedicated namespace provides security isolation (user-supplied rulebooks and decision environments are separated from the EDA control plane), independent ResourceQuota/LimitRange policies, and NetworkPolicy boundaries. The companion operator PR (ansible/eda-server-operator#345) injects this env var and manages cross-namespace RBAC.

How to test

pytest tests/unit/test_activation_job_namespace.py -q

Breaking changes / dependencies

None. When the env var is unset (default), behaviour is unchanged.

Companion PR: ansible/eda-server-operator#345
Related: ansible/eda-server-operator#344

Made with Cursor

Summary by CodeRabbit

Release Notes

  • New Features

    • Kubernetes namespace can now be configured via environment variable, providing override capability with automatic fallback to default namespace configuration when not specified.
  • Tests

    • Added comprehensive unit tests for namespace resolution, covering override scenarios, fallback behavior, and error handling.

@amasolov amasolov requested a review from a team as a code owner May 4, 2026 06:40
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 4, 2026

📝 Walkthrough

Walkthrough

The PR adds namespace override support to the Kubernetes engine by reading the EDA_ACTIVATION_JOB_NAMESPACE environment variable in Engine._set_namespace, using its trimmed value when set, and falling back to file-based namespace detection otherwise. Comprehensive unit tests validate the override behavior, whitespace handling, fallback logic, and error cases.

Changes

Namespace Override with Environment Variable

Layer / File(s) Summary
Environment-based namespace override implementation
src/aap_eda/services/activation/engine/kubernetes.py
Engine._set_namespace imports os and adds early-return logic to check EDA_ACTIVATION_JOB_NAMESPACE, trim whitespace, log the override, and skip file-based fallback when the env var is set to a non-whitespace value.
Namespace override behavior tests
tests/unit/test_activation_job_namespace.py
Test module verifies that EDA_ACTIVATION_JOB_NAMESPACE overrides namespace detection with whitespace stripping, falls back to file-based detection when env var is empty/unset/whitespace-only, and raises ContainerEngineInitError when neither source is available.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the main change: adding support for an environment variable to override the activation job namespace in the Kubernetes engine.
Description check ✅ Passed The description covers all key sections: what changed, why it's needed, how to test, breaking changes, and related context. It follows the template structure with clear explanations of purpose and implementation details.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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 amasolov force-pushed the feature/activation-job-namespace-override branch from 385aa84 to 475abf8 Compare May 4, 2026 06:52
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 4, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.02%. Comparing base (daeda95) to head (dcce876).
⚠️ Report is 1 commits behind head on main.

@@           Coverage Diff           @@
##             main    #1546   +/-   ##
=======================================
  Coverage   92.01%   92.02%           
=======================================
  Files         241      241           
  Lines       10972    10978    +6     
=======================================
+ Hits        10096    10102    +6     
  Misses        876      876           
Flag Coverage Δ
unit-int-tests-3.11 92.02% <100.00%> (+<0.01%) ⬆️
unit-int-tests-3.12 92.02% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...c/aap_eda/services/activation/engine/kubernetes.py 81.97% <100.00%> (+0.39%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@AlexSCorey AlexSCorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@mkanoor
Copy link
Copy Markdown
Contributor

mkanoor commented May 6, 2026

@amasolov Should we follow the same pattern as we did with service accounts, where we had an approved list of SA's that the user could use when starting an Activation. Should we have an approved list of Namespaces so we dont just use 1 Namespace from env var. Could there be a mismatch between which SA's would work with which Namespaces?

@amasolov
Copy link
Copy Markdown
Contributor Author

amasolov commented May 7, 2026

@amasolov Should we follow the same pattern as we did with service accounts, where we had an approved list of SA's that the user could use when starting an Activation. Should we have an approved list of Namespaces so we dont just use 1 Namespace from env var. Could there be a mismatch between which SA's would work with which Namespaces?

@mkanoor
Good questions. For this initial PR, the goal is deliberately minimal: let the operator direct all activation job pods out of the control plane namespace into a single dedicated namespace, where cross-namespace RBAC is handled by the companion operator PR (ansible/eda-server-operator#345). The SA that runs the activation jobs is the one mounted into the EDA pod, and the operator grants it the necessary roles in the target namespace.

Extending this to an approved list of namespaces (and mapping which SAs are valid for which namespaces) makes sense as a follow-up when multi-tenant or per-team isolation is needed. My customer wants a bare minimum: a separate namespace and this is what I'm aiming for here. I can open another issue for the full multi-tenant setup. Does that approach work for you?

Copy link
Copy Markdown
Contributor

@ttuffin ttuffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. @mkanoor can you please review Alexey's response to your question. If its all good then lets proceed with merge.

@ttuffin ttuffin enabled auto-merge (squash) May 19, 2026 13:39
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@tests/unit/test_activation_job_namespace.py`:
- Around line 89-96: The test test_set_namespace_no_env_no_file_raises is
nondeterministic because it relies on the real filesystem; wrap the test body
with a mock that forces open() to fail by patching builtins.open to raise
FileNotFoundError (e.g. using mock.patch("builtins.open",
side_effect=FileNotFoundError)) while keeping the existing mock.patch.dict for
os.environ, then assert Engine(...) raises ContainerEngineInitError as before so
the test no longer depends on any existing namespace file.
🪄 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: aab35a14-eb92-4f78-ac56-ebaa57e961f5

📥 Commits

Reviewing files that changed from the base of the PR and between daeda95 and dcce876.

📒 Files selected for processing (2)
  • src/aap_eda/services/activation/engine/kubernetes.py
  • tests/unit/test_activation_job_namespace.py

Comment thread tests/unit/test_activation_job_namespace.py
@ttuffin ttuffin merged commit 1583b3c into ansible:main May 19, 2026
6 checks passed
@sonarqubecloud
Copy link
Copy Markdown

amasolov added a commit to amasolov/eda-server that referenced this pull request May 25, 2026
Merge latest main to resolve conflicts with merged PR ansible#1520
(pod metadata) and PR ansible#1546 (namespace override). Retain all
resource limits additions unique to this branch.

Signed-off-by: Alexey Masolov <amasolov@redhat.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants