Skip to content

Conversation

@amirejaz
Copy link
Contributor

Summary

Enhances MCPExternalAuthConfig controller to include secret values in hash calculation and adds secret watching to automatically trigger reconciliation when referenced secrets change.

Depends on: PR #3487

Changes

  • Secret-aware hash calculation: Includes bearer token and token exchange client secret values in configHash to detect secret rotations
  • Secret watching: Automatically reconciles MCPExternalAuthConfig resources when their referenced secrets change
  • Comprehensive test coverage: Added tests for secret-aware hashing and secret watching functionality

Benefits

  • Automatic reconciliation on secret rotation
  • Accurate change detection (hash includes secret values)
  • Consistent with existing watch patterns in the codebase

Testing

All tests pass, including new tests for secret-aware hashing and secret watching.

@amirejaz amirejaz marked this pull request as draft January 29, 2026 00:27
@github-actions github-actions bot added the size/L Large PR: 600-999 lines changed label Jan 29, 2026
@github-actions github-actions bot added size/XL Extra large PR: 1000+ lines changed and removed size/L Large PR: 600-999 lines changed labels Jan 29, 2026
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.


This review will be automatically dismissed once you add the justification section.

@codecov
Copy link

codecov bot commented Jan 29, 2026

Codecov Report

❌ Patch coverage is 71.17647% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.26%. Comparing base (fc01a6c) to head (fc79413).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...or/controllers/mcpexternalauthconfig_controller.go 59.77% 29 Missing and 6 partials ⚠️
...d/thv-operator/pkg/controllerutil/tokenexchange.go 79.59% 5 Missing and 5 partials ⚠️
...-operator/controllers/mcpremoteproxy_deployment.go 66.66% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3497      +/-   ##
==========================================
- Coverage   65.27%   65.26%   -0.02%     
==========================================
  Files         399      400       +1     
  Lines       38955    39275     +320     
==========================================
+ Hits        25429    25633     +204     
- Misses      11560    11641      +81     
- Partials     1966     2001      +35     

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

@jhrozek
Copy link
Contributor

jhrozek commented Jan 29, 2026

I don't love that this adds the ability to read secrets, but if we want to support updates when secrets change as a feature then we don't have a lot of options.

One thing that should be improved is that the PR seems to watch for /all/ secrets? Wouldn't it be better to implement predicates and filter by secrets based on e.g. labels or some naming conventions?

@jhrozek
Copy link
Contributor

jhrozek commented Jan 29, 2026

Would it be acceptable to poll for secrets to be able to limit the RBAC permissions to get instead?

Ideally we'd split the secret watching into a separate process that only does that and it not exposed to the outside unlike mcpremote proxy

@amirejaz
Copy link
Contributor Author

@jhrozek Good points. This is more of a nice-to-have feature. The core functionality without Secret watching is implemented in a separate, currently open PR (#3499), and I agree that the security concerns outweigh the benefit with the current implementation, especially when the operator runs cluster-scoped.

I like the idea of splitting Secret watching into a separate internal process that has the required privileges but is not externally exposed.

As an interim step, would it make sense to avoid watching Secrets for now and rely on get-only access with periodic reconciliation to detect changes? This would let us keep least-privilege defaults while we design the separate secret-watcher component.

Happy to either defer this feature until then, or adjust the current PR in that direction if that’s preferable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants