WIP: poc: deploying upstream component#919
Conversation
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughIn ChangesConditional Image Override for External OIDC
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
| terminationMessagePolicy: FallbackToLogsOnError | ||
| image: ${IMAGE} | ||
| imagePullPolicy: IfNotPresent | ||
| imagePullPolicy: Always |
There was a problem hiding this comment.
Note: this change isn't actually necessary. Just helpful for PoC-ing.
| imageToUse := c.targetImagePullSpec | ||
| klog.Info("UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | ||
| if upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"); len(upstreamImage) > 0 { | ||
| imageToUse = upstreamImage | ||
|
|
||
| klog.Info("using UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | ||
|
|
||
| // configure the API endpoint on the upstream component | ||
| args["authenticate-path"] = []string{"/apis/oauth.openshift.io/v1/tokenreviews"} | ||
| } |
There was a problem hiding this comment.
Note: This isn't actually how we would do this, but this is sufficient for a PoC.
How we would really do this is likely another image pull spec environment variable that is injected by the CVO and used as the ${IMAGE} replacement.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindata/oauth-apiserver/externaloidc-deploy.yaml (1)
49-54: 🛠️ Refactor suggestion | 🟠 MajorUpdate test golden files to match the production manifest.
The production manifest at
bindata/oauth-apiserver/externaloidc-deploy.yaml:49usesexec padlok run(the external upstream OIDC component's entry point), but the test golden files still referenceexec oauth-apiserver external-oidc. SinceTestSyncOAuthAPIServerDeploymentcompares the generated deployment against these files usingcmp.Diff(), the tests for scenarios 4, 5, and 6 will fail. Updatepkg/operator/workload/testdata/sync_ds_scenario_{4,5,6}.yamlline 49 to useexec padlok runinstead ofexec oauth-apiserver external-oidc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindata/oauth-apiserver/externaloidc-deploy.yaml` around lines 49 - 54, The production manifest at bindata/oauth-apiserver/externaloidc-deploy.yaml has been updated to use `exec padlok run` as the entry point command, but the test golden files in pkg/operator/workload/testdata/ for scenarios 4, 5, and 6 still reference the old `exec oauth-apiserver external-oidc` command. Update line 49 in sync_ds_scenario_4.yaml, sync_ds_scenario_5.yaml, and sync_ds_scenario_6.yaml to replace `exec oauth-apiserver external-oidc` with `exec padlok run` to ensure the TestSyncOAuthAPIServerDeployment test comparisons using cmp.Diff() will pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go`:
- Around line 354-359: The code is logging the raw value of the
UPSTREAM_OIDC_COMPONENT_IMAGE environment variable in two places (the klog.Info
calls on lines 354 and 358), which can expose internal registry hostnames and
credentials. Remove the os.Getenv() calls from both klog.Info statements and
replace them with generic log messages that only indicate the variable was set
or is being used, such as "using upstream OIDC component image from environment"
or similar, without exposing the actual image reference value.
---
Outside diff comments:
In `@bindata/oauth-apiserver/externaloidc-deploy.yaml`:
- Around line 49-54: The production manifest at
bindata/oauth-apiserver/externaloidc-deploy.yaml has been updated to use `exec
padlok run` as the entry point command, but the test golden files in
pkg/operator/workload/testdata/ for scenarios 4, 5, and 6 still reference the
old `exec oauth-apiserver external-oidc` command. Update line 49 in
sync_ds_scenario_4.yaml, sync_ds_scenario_5.yaml, and sync_ds_scenario_6.yaml to
replace `exec oauth-apiserver external-oidc` with `exec padlok run` to ensure
the TestSyncOAuthAPIServerDeployment test comparisons using cmp.Diff() will
pass.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de84751e-9acd-4829-8c2f-65fb9d3991e2
📒 Files selected for processing (2)
bindata/oauth-apiserver/externaloidc-deploy.yamlpkg/operator/workload/sync_openshift_oauth_apiserver.go
| klog.Info("UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | ||
| if upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"); len(upstreamImage) > 0 { | ||
| imageToUse = upstreamImage | ||
|
|
||
| klog.Info("using UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | ||
|
|
There was a problem hiding this comment.
Avoid logging raw UPSTREAM_OIDC_COMPONENT_IMAGE values.
Line 354 and Line 358 emit the full env value; this can leak internal registry hostnames (and potentially credential-bearing image refs) into logs.
Suggested fix
- klog.Info("UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"))
- if upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"); len(upstreamImage) > 0 {
+ upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")
+ if len(upstreamImage) > 0 {
imageToUse = upstreamImage
- klog.Info("using UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"))
+ klog.V(2).Info("using UPSTREAM_OIDC_COMPONENT_IMAGE override")
// configure the API endpoint on the upstream component
args["authenticate-path"] = []string{"/apis/oauth.openshift.io/v1/tokenreviews"}
}As per coding guidelines, "Flag logging that may expose ... internal hostnames ..." and "Never log secrets or credentials."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| klog.Info("UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | |
| if upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE"); len(upstreamImage) > 0 { | |
| imageToUse = upstreamImage | |
| klog.Info("using UPSTREAM_OIDC_COMPONENT_IMAGE", os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE")) | |
| upstreamImage := os.Getenv("UPSTREAM_OIDC_COMPONENT_IMAGE") | |
| if len(upstreamImage) > 0 { | |
| imageToUse = upstreamImage | |
| klog.V(2).Info("using UPSTREAM_OIDC_COMPONENT_IMAGE override") | |
| // configure the API endpoint on the upstream component | |
| args["authenticate-path"] = []string{"/apis/oauth.openshift.io/v1/tokenreviews"} | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/workload/sync_openshift_oauth_apiserver.go` around lines 354 -
359, The code is logging the raw value of the UPSTREAM_OIDC_COMPONENT_IMAGE
environment variable in two places (the klog.Info calls on lines 354 and 358),
which can expose internal registry hostnames and credentials. Remove the
os.Getenv() calls from both klog.Info statements and replace them with generic
log messages that only indicate the variable was set or is being used, such as
"using upstream OIDC component image from environment" or similar, without
exposing the actual image reference value.
Source: Coding guidelines
1 participant
Summary by CodeRabbit