Couple registry server URL and registry auth#4361
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4361 +/- ##
==========================================
- Coverage 69.55% 69.55% -0.01%
==========================================
Files 480 480
Lines 48837 48925 +88
==========================================
+ Hits 33971 34032 +61
- Misses 12249 12267 +18
- Partials 2617 2626 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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 transformationAlternative:
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.
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
There was a problem hiding this comment.
Pull request overview
This PR couples the configured registry source (URL/API/local) with its authentication configuration so that changing the registry also resets/overwrites auth, reducing the risk of sending stale tokens to the wrong registry.
Changes:
- Extend
thv config set-registryto accept OIDC auth flags and clear existing registry auth whenever the command runs; deprecateset-registry-auth/unset-registry-auth. - Update registry login/config behavior to apply auth overrides when flags are provided, and remove
openidfrom default OAuth scopes. - Update the serve-mode Registry API to clear auth when
authis omitted, plus adjust user-facing error messages and CLI docs.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/registry/provider_api.go | Updates 401 guidance to use thv config set-registry ... --issuer/--client-id. |
| pkg/registry/auth_manager.go | Clarifies OIDC wording in AuthManager docs. |
| pkg/registry/auth/oauth_token_source.go | Clarifies OIDC terminology in comments. |
| pkg/registry/auth/login.go | Removes openid from defaults; changes registry/auth override semantics. |
| pkg/registry/auth/login_test.go | Updates tests for new override + “clear auth on registry flag” behavior. |
| pkg/api/v1/registry.go | Changes update semantics to overwrite/clear auth on PUT; updates auth-required messages. |
| cmd/thv/app/registry_login.go | Updates login flag help text to reflect new semantics/default scopes. |
| cmd/thv/app/config.go | Adds auth flags to config set-registry; clears auth on set/unset registry. |
| cmd/thv/app/config_registryauth.go | Deprecates legacy registry-auth subcommands; updates help text/examples. |
| docs/cli/thv_registry_login.md | Updates option descriptions and default scopes. |
| docs/cli/thv_config_set-registry.md | Documents auth-clearing and new auth flags for set-registry. |
| docs/cli/thv_config.md | Removes deprecated registry-auth commands from docs index. |
| docs/cli/thv_config_set-registry-auth.md | Removes doc page for deprecated command. |
| docs/cli/thv_config_unset-registry-auth.md | Removes doc page for deprecated command. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jhrozek
left a comment
There was a problem hiding this comment.
Review from Claude Code.
Overall: The security concept is sound and well-motivated. A few questions and nits on the implementation details.
Summary:
- 1 question on scope defaults (F7)
- 1 ordering concern (F1)
- 1 question on deprecated command removal (F2)
- 1 nit on deprecation message (F5)
- 1 nit on missing context propagation (F6)
There was a problem hiding this comment.
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 transformationAlternative:
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.
PR size has been reduced below the XL threshold. Thank you for splitting this up!
|
✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up! |
Issue #4360
Summary
When the registry URL changes, we should also reset the registry auth configuration.
Changes
thv config set-registrythv config set-registryis calledthv config set-registry-authandthv config unset-registry-auth