Add container-aware Vault OIDC authentication and browser handling#861
Merged
openshift-merge-bot[bot] merged 7 commits intoopenshift:masterfrom Mar 10, 2026
Merged
Conversation
Enhances osdctl to work properly in containerized environments (e.g., ocm-container) for both Vault authentication and dashboard access. Changes: - Vault OIDC authentication now detects OCM_CONTAINER environment - Uses skip_browser=true and listenaddress=0.0.0.0 in containers - Displays helpful instructions for port 8250 forwarding - Provides detailed error messages when authentication fails - Skips automatic browser launch for dashboards in containers - Adds comprehensive unit tests for container detection logic This allows SREs to authenticate with Vault and access Dynatrace dashboards from within ocm-container by manually opening URLs in their host browser while the OIDC callback works via port forwarding. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci
bot
added
the
approved
WalkthroughAdds container-aware behavior: dashboard browser opening is skipped in container environments and Vault OIDC login switches to container-friendly arguments, output, and error messages when running inside the OCM container. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
cmd/dynatrace/dashboardCmd.go (1)
70-77: Consider informing users when browser launch is skipped.The container detection logic is sound. However, when running in a container, the user sees the URL but no indication that the browser was intentionally skipped. Consider adding a brief message for consistency with the vault.go behavior which prints container-mode notes.
💡 Suggested improvement for user clarity
// Only try to open browser if not in a container environment if os.Getenv("OCM_CONTAINER") == "" { // Open the dashboard in the default browser fmt.Println("\nOpening dashboard in your browser...") if err := openBrowser(dashUrl); err != nil { fmt.Printf("Could not open browser automatically: %s\n", err) } + } else { + fmt.Println("\nRunning in container mode - open the URL above in your host browser.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/dynatrace/dashboardCmd.go` around lines 70 - 77, When the container detection (os.Getenv("OCM_CONTAINER") == "") prevents opening the browser, add a brief informational message to the user notifying them that automatic browser launch was skipped due to container mode; locate the conditional that checks os.Getenv("OCM_CONTAINER") around the openBrowser(dashUrl) call and, in the else branch, print a short message (similar to vault.go's container-mode note) so users see both the URL and an explicit statement that browser launch was intentionally skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/dynatrace/vault_test.go`:
- Around line 121-135: This test is duplicating production logic by
reconstructing loginArgs/loginCmd instead of exercising the real
command-construction code; remove the inline construction and Stdout/Stderr
branching and instead call the actual function used in production to build the
vault login command (the same approach you applied to
TestSetupVaultToken_ContainerEnvironment), set the environment variants
(OCM_CONTAINER present/absent), and assert the returned exec.Cmd (and its
Stdout/Stderr behavior) matches expectations; reference loginArgs/loginCmd in
the assertion to verify the function under test produces the correct arguments
and output redirection.
- Around line 41-45: The test duplicates production logic for building Vault
login args instead of exercising the real behavior; update the code so the
argument-building is testable (e.g., extract into
buildVaultLoginArgs(inContainer bool) used by setupVaultToken and vault_test)
or, if you cannot refactor now, change the test to call setupVaultToken (or the
existing exported function that constructs args) and assert the returned args
include "listenaddress=0.0.0.0" when OCM_CONTAINER is set; ensure tests no
longer inline the logic so they fail if production code is wrong and reference
setupVaultToken and vault.go’s argument construction to locate the change.
- Around line 37-39: The test currently only calls t.Setenv("OCM_CONTAINER",
tt.containerEnvValue) when tt.containerEnvValue != "", which leaves any
pre-existing OCM_CONTAINER in the environment and can make the "empty" case
flaky; change the logic to always set the env var (i.e., call
t.Setenv("OCM_CONTAINER", tt.containerEnvValue) unconditionally) or explicitly
unset it when empty (t.Setenv("OCM_CONTAINER", "")). Update the call site in
cmd/dynatrace/vault_test.go where tt.containerEnvValue and t.Setenv are used so
the empty-case is isolated from the external environment.
In `@cmd/dynatrace/vault.go`:
- Around line 63-74: Update the error remediation text inside the
os.Getenv("OCM_CONTAINER") branch where the fmt.Errorf is returned: replace the
incorrect snippet "ports: [\"8250:8250\"]" with the exact configuration
key/value used in the setup docs (use the key verified in the previous comment)
so the guidance matches the real ocm-container config; this change should be
made in the fmt.Errorf call in the vault login error handling (the block guarded
by if os.Getenv("OCM_CONTAINER") != "" and the associated fmt.Errorf).
- Around line 38-50: The code repeatedly calls os.Getenv("OCM_CONTAINER") when
building loginArgs and conditionally printing container notes; create a local
variable (e.g., containerEnv := os.Getenv("OCM_CONTAINER")) at the start of the
block and replace all subsequent os.Getenv("OCM_CONTAINER") usages with this
variable, then use containerEnv != "" as the condition to set loginArgs (the
slice assigned to loginArgs) and to print the NOTE and port instructions; ensure
you preserve the existing values for the non-container and container branches
(loginArgs = []string{"login","-method=oidc","-no-print"} vs loginArgs =
[]string{"login","-method=oidc","skip_browser=true","listenaddress=0.0.0.0"}).
---
Nitpick comments:
In `@cmd/dynatrace/dashboardCmd.go`:
- Around line 70-77: When the container detection (os.Getenv("OCM_CONTAINER") ==
"") prevents opening the browser, add a brief informational message to the user
notifying them that automatic browser launch was skipped due to container mode;
locate the conditional that checks os.Getenv("OCM_CONTAINER") around the
openBrowser(dashUrl) call and, in the else branch, print a short message
(similar to vault.go's container-mode note) so users see both the URL and an
explicit statement that browser launch was intentionally skipped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb429a47-530c-458f-aaac-8742ba996d28
📒 Files selected for processing (3)
cmd/dynatrace/dashboardCmd.gocmd/dynatrace/vault.gocmd/dynatrace/vault_test.go
joshbranham
reviewed
Mar 9, 2026
Apply gofmt formatting fixes to vault_test.go: - Align struct field spacing in test tables - Remove trailing newline Addresses CI format check failure. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add reusable helper function to pkg/utils: - IsContainerEnvironment() checks OCM_CONTAINER env var - Provides single source of truth for container detection - Add comprehensive unit tests Benefits: - Reusability across commands - Consistency in container detection logic - Independent testability - Easier maintenance Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes to vault.go:
- Use utils.IsContainerEnvironment() instead of repeated os.Getenv()
- Fix configuration guidance: 'ports:' -> 'launch-opts: "-p 8250:8250"'
- Add docstring for setupVaultToken function
Addresses CodeRabbit review comments:
- Eliminates repeated os.Getenv("OCM_CONTAINER") calls (lines 41, 54, 63)
- Corrects ocm-container configuration instructions (lines 44, 69)
- Improves docstring coverage for setupVaultToken (line 16)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes to vault_test.go: - Use utils.IsContainerEnvironment() instead of os.Getenv() - Always call t.Setenv() even for empty case to ensure clean test environment - Add listenaddress=0.0.0.0 assertion in container mode tests - Update test logic to match production code changes Addresses CodeRabbit review comments: - Ensures consistent environment setup in all test cases (line 37) - Verifies listenaddress parameter is included (line 44) - Uses helper function for maintainability (lines 43, 123) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes to dashboardCmd.go: - Use utils.IsContainerEnvironment() instead of os.Getenv() - Add user feedback message when browser launch is skipped in container mode - Add docstring for openBrowser function Addresses CodeRabbit review comments: - Provides user feedback when browser is skipped (lines 70-77) - Improves docstring coverage for openBrowser (line 19) - Uses helper function for consistency Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Replace local utils.IsContainerEnvironment() with the official IsRunningInOcmContainer() function from ocm-container/pkg/utils. Changes: - Import github.com/openshift/ocm-container/pkg/utils - Replace utils.IsContainerEnvironment() with ocmutils.IsRunningInOcmContainer() - Update tests to use IO_OPENSHIFT_MANAGED_NAME="ocm-container" instead of OCM_CONTAINER - Remove local IsContainerEnvironment() implementation from pkg/utils - Remove local IsContainerEnvironment() tests Benefits: - Single source of truth for container detection - Consistent with ocm-container's official implementation - Uses standardized IO_OPENSHIFT_MANAGED_NAME environment variable - Reduces code duplication across Red Hat SRE tools The IsRunningInOcmContainer() function checks for IO_OPENSHIFT_MANAGED_NAME="ocm-container" which is now set in all three ocm-container image variants (micro, minimal, full) as of ocm-container PR openshift#487. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
@clcollins: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Around line 70-75: The go.mod has mixed Kubernetes minor versions: k8s.io/api,
k8s.io/apimachinery, and k8s.io/client-go are bumped to v0.34.3 while
sigs.k8s.io/controller-runtime (v0.20.1), k8s.io/cli-runtime and k8s.io/kubectl
remain at v0.32.1; pick a single tested minor and align all k8s.io/* and
sigs.k8s.io/controller-runtime entries accordingly—either revert k8s.io/api,
k8s.io/apimachinery, k8s.io/client-go back to the v0.32.x series to match
controller-runtime and cli-runtime/kubectl, or upgrade
sigs.k8s.io/controller-runtime, k8s.io/cli-runtime and k8s.io/kubectl to the
v0.34.x-compatible releases—then run go mod tidy and tests to ensure dependency
compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c290642-2a5c-4ba6-bde2-c97bd6a6a4c9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
cmd/dynatrace/dashboardCmd.gocmd/dynatrace/vault.gocmd/dynatrace/vault_test.gogo.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/dynatrace/vault_test.go
Comment on lines
+70
to
+75
| k8s.io/api v0.34.3 | ||
| k8s.io/apimachinery v0.34.3 | ||
| k8s.io/cli-runtime v0.32.1 | ||
| k8s.io/client-go v0.32.6 | ||
| k8s.io/client-go v0.34.3 | ||
| k8s.io/kubectl v0.32.1 | ||
| k8s.io/utils v0.0.0-20241210054802-24370beab758 | ||
| k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import pathlib, re
mods = [
"sigs.k8s.io/controller-runtime",
"k8s.io/api",
"k8s.io/apimachinery",
"k8s.io/client-go",
"k8s.io/cli-runtime",
"k8s.io/kubectl",
]
text = pathlib.Path("go.mod").read_text()
for mod in mods:
m = re.search(rf'^\s*{re.escape(mod)}\s+(\S+)', text, re.M)
print(f"{mod}: {m.group(1) if m else 'MISSING'}")
PYRepository: openshift/osdctl
Length of output: 226
🌐 Web query:
kubernetes-sigs controller-runtime v0.20 compatibility k8s.io client-go
💡 Result:
kubernetes-sigs/controller-runtime v0.20.x is built against Kubernetes 1.32 libraries, i.e. it expects k8s.io/client-go v0.32.x (same minor). Specifically:
controller-runtime v0.20.0requiresk8s.io/client-go v0.32.0(and otherk8s.io/*atv0.32.0). [1]controller-runtime v0.20.4requiresk8s.io/client-go v0.32.1(and otherk8s.io/*atv0.32.1). [2]
The project also notes it generally releases one controller-runtime minor per client-go minor, so you should align v0.20.* with client-go v0.32.*. [3]
Sources: [1] [2] [3]
Keep the Kubernetes dependency set on one tested minor.
controller-runtime v0.20 is tested against client-go and other k8s.io/* modules at v0.32.x. This change bumps k8s.io/api, k8s.io/apimachinery, and k8s.io/client-go to v0.34.3, but sigs.k8s.io/controller-runtime (v0.20.1), k8s.io/cli-runtime, and k8s.io/kubectl remain on v0.32.1, mixing unsupported Kubernetes minors. Either revert to v0.32.x for the entire stack or upgrade all remaining Kubernetes libraries together.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` around lines 70 - 75, The go.mod has mixed Kubernetes minor versions:
k8s.io/api, k8s.io/apimachinery, and k8s.io/client-go are bumped to v0.34.3
while sigs.k8s.io/controller-runtime (v0.20.1), k8s.io/cli-runtime and
k8s.io/kubectl remain at v0.32.1; pick a single tested minor and align all
k8s.io/* and sigs.k8s.io/controller-runtime entries accordingly—either revert
k8s.io/api, k8s.io/apimachinery, k8s.io/client-go back to the v0.32.x series to
match controller-runtime and cli-runtime/kubectl, or upgrade
sigs.k8s.io/controller-runtime, k8s.io/cli-runtime and k8s.io/kubectl to the
v0.34.x-compatible releases—then run go mod tidy and tests to ensure dependency
compatibility.
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: clcollins, joshbranham The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Member
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
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
2 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
Adds container-aware Vault OIDC authentication to properly handle authentication workflows when running inside ocm-container. Integrates with ocm-container's official
IsRunningInOcmContainer()helper function for standardized container detection.Problem
Vault OIDC authentication fails inside ocm-container because:
127.0.0.1, unreachable from the host browserSolution
Container Detection
Uses the official
github.com/openshift/ocm-container/pkg/utils.IsRunningInOcmContainer()helper function to detect when running inside ocm-container. This:IO_OPENSHIFT_MANAGED_NAME="ocm-container"environment variableVault OIDC Behavior
Inside ocm-container:
skip_browser=true)0.0.0.0(reachable from host via port forwarding)Outside ocm-container (normal environment):
127.0.0.1bindingDashboard Command
Also updates
osdctl dynatrace dashboardto skip browser launch in containers and display user-friendly messages.Changes
Phase 1: Fix code formatting
Phase 2-5: Initial Implementation (now replaced)
Created localutils.IsContainerEnvironment()helperAdded local tests for container detectionPhase 6: Use Official ocm-container Helper ⭐ NEW
github.com/openshift/ocm-container/pkg/utilsutils.IsContainerEnvironment()calls withocmutils.IsRunningInOcmContainer()IO_OPENSHIFT_MANAGED_NAME="ocm-container"instead ofOCM_CONTAINERFiles Modified
Container Detection Integration
cmd/dynatrace/vault.go- Usesocmutils.IsRunningInOcmContainer()cmd/dynatrace/dashboardCmd.go- Usesocmutils.IsRunningInOcmContainer()cmd/dynatrace/vault_test.go- Updated tests withIO_OPENSHIFT_MANAGED_NAMEpkg/utils/utils.go- Removed localIsContainerEnvironment()pkg/utils/utils_test.go- Removed local testsgo.mod/go.sum- Added ocm-container dependencyConfiguration Guidance
Both commands now provide clear instructions:
Testing
All tests pass:
$ go test ./cmd/dynatrace/... -v === RUN TestSetupVaultToken_ContainerEnvironment === RUN TestSetupVaultToken_ContainerEnvironment/Container_environment_with_IO_OPENSHIFT_MANAGED_NAME=ocm-container --- PASS: TestSetupVaultToken_ContainerEnvironment (0.00s) === RUN TestSetupVaultToken_OutputRedirection --- PASS: TestSetupVaultToken_OutputRedirection (0.00s) PASSDependencies
IO_OPENSHIFT_MANAGED_NAMEto all container variantsIsRunningInOcmContainer()andGetOcmContainerComponent()helpersBenefits
Related
🤖 Generated with Claude Code