RHCLOUD-51702: prefer V2 dependency endpoint for RBAC - #2351
Open
platex-rehor-bot wants to merge 2 commits into
Open
platex-rehor-bot wants to merge 2 commits into
platex-rehor-bot wants to merge 2 commits into
Conversation
The Clowder V2 endpoint API provides a complete URI including scheme and
port, avoiding the manual URL construction from V1 flat endpoint lists.
This change tries V2 first via GetV2DependencyEndpoint("rbac", "service")
and falls back to V1 iteration when V2 is not available. The RBAC_ADDRESS
env var override is preserved as the final layer.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewer's GuideMigrates RBAC service discovery to prefer the complete URI from Clowder V2 dependency endpoints, falls back to the existing V1 endpoint construction when V2 is unavailable, and preserves the final File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="base/utils/config.go" line_range="289" />
<code_context>
func initServicesFromClowder() {
+ // Try V2 dependency endpoint for RBAC (preferred over V1 flat list).
+ if addr := resolveRbacV2Address(); addr != "" {
+ CoreCfg.RbacAddress = addr
+ }
+
</code_context>
<issue_to_address>
**nitpick:** When V2 returns an HTTPS URI, `printServicesParams()` formats it as `RBAC_ADDRESS=http://https://...`, because that diagnostic function always prepends `http://` to `CoreCfg.RbacAddress`. The printed configuration is therefore invalid and misleading.
**Triggers:** When service parameters are printed after RBAC is resolved from a V2 HTTPS endpoint.
**Suggested fix:** Print `CoreCfg.RbacAddress` directly without prepending a scheme.
</issue_to_address>
### Comment 2
<location path="base/utils/config_clowder_v2_test.go" line_range="79-80" />
<code_context>
+ CoreCfg.RbacAddress = "https://rbac-service.svc:8443"
+
+ // Env var override takes precedence (initServicesFromEnv behavior)
+ os.Setenv("RBAC_ADDRESS", "http://localhost:8080")
+ defer os.Unsetenv("RBAC_ADDRESS")
+ initServicesFromEnv()
+ assert.Equal(t, "http://localhost:8080", CoreCfg.RbacAddress)
</code_context>
<issue_to_address>
**nitpick (testing):** The test overwrites any pre-existing `RBAC_ADDRESS` value and unconditionally unsets it during cleanup, so a test process or subsequent test that relies on the original environment value observes a changed environment.
**Triggers:** When the test suite is run with `RBAC_ADDRESS` already set or alongside tests that depend on that variable.
**Suggested fix:** Use `t.Setenv("RBAC_ADDRESS", "http://localhost:8080")`, which restores the original value automatically.
```suggestion
t.Setenv("RBAC_ADDRESS", "http://localhost:8080")
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. The change redirects RBAC requests to a V2-provided URI, so a wrong or unintended endpoint could make authorization fail or accept decisions from the wrong service. Reverting stops future requests through that endpoint, but any access granted or denied while it was active cannot be undone.
| func initServicesFromClowder() { | ||
| // Try V2 dependency endpoint for RBAC (preferred over V1 flat list). | ||
| if addr := resolveRbacV2Address(); addr != "" { | ||
| CoreCfg.RbacAddress = addr |
There was a problem hiding this comment.
nitpick: When V2 returns an HTTPS URI, printServicesParams() formats it as RBAC_ADDRESS=http://https://..., because that diagnostic function always prepends http:// to CoreCfg.RbacAddress. The printed configuration is therefore invalid and misleading.
Triggers: When service parameters are printed after RBAC is resolved from a V2 HTTPS endpoint.
Suggested fix: Print CoreCfg.RbacAddress directly without prepending a scheme.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2351 +/- ##
==========================================
- Coverage 58.95% 58.95% -0.01%
==========================================
Files 150 150
Lines 9605 9612 +7
==========================================
+ Hits 5663 5667 +4
- Misses 3348 3349 +1
- Partials 594 596 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
MichaelMraka
requested changes
Sep 30, 2026
| func initServicesFromClowder() { | ||
| // Try V2 dependency endpoint for RBAC (preferred over V1 flat list). | ||
| if addr := resolveRbacV2Address(); addr != "" { | ||
| CoreCfg.RbacAddress = addr |
Collaborator
There was a problem hiding this comment.
move setting to rbac section; this will try to set it even if there's no rbac
Author
There was a problem hiding this comment.
Good catch — moved the V2 resolution into the case "rbac": block so it only runs when RBAC endpoints are present. V2 is tried first; if unavailable, falls back to the V1 flat endpoint.
RHCLOUD-51702 Address review feedback: V2 endpoint resolution now scoped to the rbac case block so it only runs when rbac endpoints exist. Tests use t.Setenv for automatic cleanup, preventing env var side effects across test runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
3 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
Migrates RBAC service discovery from the Clowder V1 flat endpoint list to V2 dependency endpoints. The V2 endpoint API (
GetV2DependencyEndpoint("rbac", "service")) provides a complete URI including scheme and port, preferred over manual URL construction from V1 fields. V1 iteration is preserved as a fallback when V2 is not available. TheRBAC_ADDRESSenv var override remains as the final layer.Jira: RHCLOUD-51702
Implemented
resolveRbacV2Address()helper that queries V2 dependency endpoints for RBAC (app=rbac,deployment=service)initServicesFromClowder()to try V2 first, falling back to V1 flat endpoint iteration only when V2 is unavailableAssumptions
GetV2DependencyEndpoint— verified from module cacheCoreCfg.RbacURLset fromrbac-serviceendpoint, never read) is intentionally left as-is per reviewer directionHuman Verification Required
.CaCertificateis not consumed by this change. The existing RBAC HTTP client uses system trust (http.Client{}). If custom CA handling is needed in the future, it can be added as a follow-up..Authenticatedis not consumed. RBAC calls use x-rh-identity forwarding (in-cluster,authenticated: false). No auth mechanism change.rpm/rpmlib.h(gorpm) — will validate via CI.Considered Follow-ups
CoreCfg.RbacURL/RBAC_URLenv var (deferred per reviewer)Validation
gofmt— cleanCGO_ENABLED=0 go build ./base/utils/— passes/clowder-v2-assess --phase after— PASS, no mechanical errorsSecure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Update RBAC service discovery to prefer Clowder V2 dependency endpoints while preserving existing fallback and override behavior.
Bug Fixes:
Tests: