feat: Add optional namespace support for secretKeyRef in Analysis argument

[Mujib-Ahasan]

Description

This PR adds support for referencing secrets from explicitly allowed namespaces when resolving secretKeyRef in Analysis arguments.
so currently, secrets referenced by ClusterAnalysisTemplate must be copied into every namespace where the template is used. This PR allows referencing a shared secret stored in an admin-controlled namespace instead. This PR also takes care of backward compatibility.

changes

• CRDs have been generated

• Added an optional namespace field to SecretKeyRe

• Implemented cross-namespace secret resolution in resolveArgs()

• Added controller flags:
  - --enable-cross-namespace-secret-refs
  - --allowed-secret-ref-namespaces

• Added validation to ensure cross-namespace secret access is: disabled by default (backward compatibilty)

• restricted to explicitly allowed namespace only

fixes: #4611

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
• The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
• I've signed my commits with DCO
• My builds are green. Try syncing with master if they are not.
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• I have run all tests locally (including the flaky ones) and they pass on my workstation
• I have used LLM/AI/Agent tools for this PR but I am responsible for all code of this PR
• I understand what the code does and WHY/HOW it works in several scenarios
• I know if my code is just adding new functionality or changing old functionality for existing users
• My organization is added to USERS.md.

[Mujib-Ahasan]

I will write the unit tests shortly.

[github-actions]

Published E2E Test Results

  4 files    4 suites   3h 54m 55s ⏱️
120 tests 105 ✅  7 💤  8 ❌
496 runs  454 ✅ 28 💤 14 ❌

For more details on these failures, see this check.

Results for commit b97218d.

♻️ This comment has been updated with latest results.

[github-actions]

Published Unit Test Results

2 446 tests   2 446 ✅  3m 18s ⏱️
  129 suites      0 💤
    1 files        0 ❌

Results for commit b97218d.

♻️ This comment has been updated with latest results.

[codecov]

Codecov Report

❌ Patch coverage is 91.30435% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.92%. Comparing base (4716b59) to head (b97218d).
⚠️ Report is 12 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/rollouts-controller/main.go	66.66%	4 Missing ⚠️
analysis/controller.go	85.71%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4648      +/-   ##
==========================================
+ Coverage   84.87%   84.92%   +0.04%     
==========================================
  Files         164      164              
  Lines       18907    18990      +83     
==========================================
+ Hits        16048    16127      +79     
- Misses       2001     2006       +5     
+ Partials      858      857       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Mujib-Ahasan]

test cases have been added! Ready for the review.

[zachaller]

I am not sure I love the security implications of this, the usage also makes it easy to say pull secrets from any listed namespace in the allowed list so in say multi team setups if you allow namespace a and b but you want to not let b see a you can't currently in this PR.

I wonder if a middle ground would be to just do something under the secret ref like controllerNamespace: true that then pulls secrets from the namespace the controller is in, this just make it seem more limited to a single namespace?

I do see the issue with ClusterAnalysis needing secrets though, just not sure which way is the best option yet.

[kostis-codefresh]

The PR is also missing any kind of documentation. Not only it needs documentation, but also we should place some big red warnings about the security implications that Zach already highlighted.

Since this is disabled by default and admins need to enable it, I think it is understandable that they choose the risk. But they should at least know about the risk from the docs.

Also the messages "cross-namespace secret references are disabled" and "cross-namespace secret references to namespace %q are not allowed" are not very descriptive. It would be better to explain to the user where the error comes from (the analysis) and what causes it (the namespace property there).

I don't see any e2e tests. Maybe we need some? Or do the unit tests cover everything?

[Mujib-Ahasan]

The PR is also missing any kind of documentation. Not only it needs documentation, but also we should place some big red warnings about the security implications that Zach already highlighted.

Since this is disabled by default and admins need to enable it, I think it is understandable that they choose the risk. But they should at least know about the risk from the docs.

I totally understand the risk. and I am gonna add them properly.

Also the messages "cross-namespace secret references are disabled" and "cross-namespace secret references to namespace %q are not allowed" are not very descriptive. It would be better to explain to the user where the error comes from (the analysis) and what causes it (the namespace property there).

Sure, I will update these shortly.🙂

I don't see any e2e tests. Maybe we need some? Or do the unit tests cover everything?

probably, I need to take a deeper look.

[sonarqubecloud]

Quality Gate failed

Failed conditions
46.0% Duplication on New Code (required ≤ 40%)

See analysis details on SonarQube Cloud

[Mujib-Ahasan]

I have updated the docs and also error messages. also IMO these unit test are sufficient for this changes because the behavior is focused in analysis args resolution than in a broader rollout flowing. Still, let me know if I need to work on it.

cc: @kostis-codefresh, @zachaller

[zachaller]

I still think I want to scope it down to a single namespace not a list because it better aligns the k8s rbac with the behavior vs documenting it, because at that point if admins want someone to be able to see the secrests at the k8s rbac level they have to grant them access to the single namespace and they are not misslead in say team a can put secrets in namespace a and team b can use namespace b, if they just miss or ignore the fact that team b can pull teams a secrete via analysis.

Would a single namespace meet your requimrnets?

[Mujib-Ahasan]

Makes sense, I am ok with either way. I think for single namespace we still have the copy secrets to a single namespace. Although the risk is less here but manual work is there. for multiple namespace, high risk and no manual work. I am confused😅. Even though the risks are documented, admins have to choose their security approaches.
(should we ping the issue creator? ). I need a solid green signal.