Repository navigation
feat(odh): wire OIDC_ISSUER_URL from params.env into operator pod - #95
GowthamShanmugam wants to merge 1 commit into
Conversation
- Add OIDC_ISSUER_URL to manager Deployment env (base) - Add param to ODH/RHOAI params.env with empty default - Kustomize replacements from feast-operator-parameters ConfigMap - Document Open Data Hub operator integration Pairs with opendatahub-operator injecting OIDC_ISSUER_URL at reconcile time from GatewayConfig when the cluster uses external OIDC (RHOAIENG-55767). Made-with: Cursor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: GowthamShanmugam 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 |
📝 WalkthroughWalkthroughThe changes establish an Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Security considerationsMissing validation of OIDC issuer URL: The diff establishes a mechanism to pass an OIDC issuer URL to the operator via environment variable, but no input validation or URL sanitization is visible. When the operator code consumes this value (via Undefined empty-value semantics: The documentation states an empty value "signifies" something but does not specify the expected behavior or fallback when the variable is not set. Operator code must explicitly handle the empty case to prevent null pointer or undefined behavior when OIDC integration is not configured. ConfigMap in plaintext: While OIDC issuer URLs are public, storing them in Kubernetes ConfigMaps (unencrypted by default) may conflict with security posture requirements in regulated environments. Consider whether this parameter should use Secrets instead, even if the value itself is non-sensitive. 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 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 `@infra/feast-operator/docs/odh-operator-parameters.md`:
- Around line 13-15: Docs claim operator reads OIDC_ISSUER_URL from env, but
code in repo_config.go (around the OIDC secret parsing at lines ~107-122) only
loads client_id, auth_discovery_url, client_secret, username, password from a
Kubernetes secret and there is no code path reading OIDC_ISSUER_URL; fix by
either (A) implementing env var consumption: add logic in the repo_config.go
OIDC/load function to read os.Getenv("OIDC_ISSUER_URL") (and use it as the
issuer/discovery URL or jwks source, falling back to the secret’s
auth_discovery_url when env var is empty), or (B) update the documentation and
Deployment manifest to remove the dead OIDC_ISSUER_URL guidance and explicitly
document that the operator expects OIDC values in the secret fields (client_id,
auth_discovery_url, client_secret, username, password) and stop injecting the
unused env var into the Deployment. Ensure whichever path you choose is
reflected consistently in repo_config.go, the Deployment manifest, and the docs.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ebc6e979-de83-454b-926a-8ad35310328b
📒 Files selected for processing (6)
infra/feast-operator/config/manager/manager.yamlinfra/feast-operator/config/overlays/odh/kustomization.yamlinfra/feast-operator/config/overlays/odh/params.envinfra/feast-operator/config/overlays/rhoai/kustomization.yamlinfra/feast-operator/config/overlays/rhoai/params.envinfra/feast-operator/docs/odh-operator-parameters.md
| **Set by:** Open Data Hub operator (Feast component reconcile), from `GatewayConfig.spec.oidc.issuerURL` when cluster authentication is OIDC. | ||
|
|
||
| **Consumption:** Operator code should read `os.Getenv("OIDC_ISSUER_URL")` (or equivalent) where JWKS / OIDC discovery is required for managed workloads. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify no code currently reads OIDC_ISSUER_URL environment variable
echo "=== Searching for OIDC_ISSUER_URL usage in Go code ==="
rg -n "OIDC_ISSUER_URL" --type go
echo ""
echo "=== Searching for os.Getenv calls related to OIDC ==="
rg -n -C2 'os\.Getenv.*[Oo][Ii][Dd][Cc]' --type go
echo ""
echo "=== Searching for any issuerURL or issuer_url references ==="
rg -ni "issuer.?url" --type goRepository: opendatahub-io/feast
Length of output: 235
🏁 Script executed:
# First, find and read the documentation file
find . -name "odh-operator-parameters.md" -type fRepository: opendatahub-io/feast
Length of output: 118
🏁 Script executed:
# Read the documentation file to verify the exact content
cat -n "infra/feast-operator/docs/odh-operator-parameters.md" | sed -n '10,20p'Repository: opendatahub-io/feast
Length of output: 476
🏁 Script executed:
# Check repo_config.go for OIDC configuration sourcing
find . -name "repo_config.go" -path "*/feast-operator/*"Repository: opendatahub-io/feast
Length of output: 130
🏁 Script executed:
# Check services_types.go for OidcProperties
find . -name "services_types.go" -path "*/feast-operator/*"Repository: opendatahub-io/feast
Length of output: 133
🏁 Script executed:
# Read repo_config.go around lines 58-85 to see how OIDC is configured
cat -n ./infra/feast-operator/internal/controller/services/repo_config.go | sed -n '50,90p'Repository: opendatahub-io/feast
Length of output: 1619
🏁 Script executed:
# Read services_types.go around lines 88-105 to see OidcProperties structure
cat -n ./infra/feast-operator/internal/controller/services/services_types.go | sed -n '80,115p'Repository: opendatahub-io/feast
Length of output: 1785
🏁 Script executed:
# Search for where auth_discovery_url is populated to confirm it comes from secret
rg -n -A5 "auth_discovery_url" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 784
🏁 Script executed:
# Search for how OIDC secrets are handled and where auth_discovery_url comes from
rg -n -B3 -A8 "OidcAuthDiscoveryUrl|auth_discovery_url" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 13471
🏁 Script executed:
# Search for any OIDC secret extraction patterns
rg -n -B5 -A10 "oidc.*secret\|OIDC.*secret" --type go -i ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check the featurestore_controller file for OIDC auth handling
find ./infra/feast-operator -name "*featurestore_controller*" -type fRepository: opendatahub-io/feast
Length of output: 1145
🏁 Script executed:
# Read the OIDC auth test file to understand how OIDC is configured
cat -n ./infra/feast-operator/internal/controller/featurestore_controller_oidc_auth_test.go | head -100Repository: opendatahub-io/feast
Length of output: 4192
🏁 Script executed:
# Check if there's any environment variable injection for OIDC in the controller
rg -n "OIDC\|issuer" --type go ./infra/feast-operator/internal/controller/featurestore_controller.go | head -30Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Search for where the OIDC_ISSUER_URL environment variable might be set in the deployment
rg -n "OIDC_ISSUER_URL" . --type yaml --type ymlRepository: opendatahub-io/feast
Length of output: 91
🏁 Script executed:
# Search for OIDC_ISSUER_URL in all files (not just code)
rg -n "OIDC_ISSUER_URL" .Repository: opendatahub-io/feast
Length of output: 1591
🏁 Script executed:
# Search for where env vars are injected in YAML manifests
find ./infra/feast-operator -name "*.yaml" -type f -exec grep -l "OIDC\|env:" {} \;Repository: opendatahub-io/feast
Length of output: 783
🏁 Script executed:
# Check the GatewayConfig CRD mentioned in the docs
rg -n "GatewayConfig\|spec.oidc.issuerURL" --type go -A5Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Search for how OIDC secret is extracted and used
rg -n -B5 -A10 "secretExtractionFunc.*oidc\|mockOidcConfigFromSecret" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check the manager.yaml to see how OIDC_ISSUER_URL is injected
cat -n ./infra/feast-operator/config/manager/manager.yaml | sed -n '80,95p'Repository: opendatahub-io/feast
Length of output: 730
🏁 Script executed:
# Check if there's any code that actually reads the OIDC_ISSUER_URL environment variable
rg -n "Getenv\|getenv" --type go ./infra/feast-operator/internal/controller/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Look at how OIDC secret extraction works in repo_config.go
rg -n -B3 -A15 "extractOidcConfig\|getBaseServiceRepoConfig" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Search for any references to OIDC issuer discovery in the actual operator code
rg -n "issuer\|discovery" --type go -i ./infra/feast-operator/internal/controller/ | grep -v testRepository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check if there are any TODOs or FIXMEs related to OIDC_ISSUER_URL
rg -n "TODO\|FIXME" --type go ./infra/feast-operator/ | grep -i oidcRepository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Look at the complete repo_config.go file structure to understand OIDC handling
wc -l ./infra/feast-operator/internal/controller/services/repo_config.goRepository: opendatahub-io/feast
Length of output: 134
🏁 Script executed:
# Search for where secret extraction happens for OIDC
rg -n -A20 "func.*[Oo]idc\|OidcAuthz" --type go ./infra/feast-operator/internal/controller/services/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Get the full repo_config.go and search for all OIDC-related functions
cat -n ./infra/feast-operator/internal/controller/services/repo_config.go | grep -i oidcRepository: opendatahub-io/feast
Length of output: 1243
🏁 Script executed:
# Search the complete services directory for OIDC secret extraction
rg -n "OidcAuthz\|secretRef" --type go ./infra/feast-operator/internal/controller/services/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Look at a specific section where OIDC config might be handled
rg -n -B10 -A20 "authz\|Authz" --type go ./infra/feast-operator/internal/controller/services/repo_config.go | head -80Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check the authz directory to see if there's any OIDC processing
find ./infra/feast-operator/internal/controller/authz -type f -name "*.go"Repository: opendatahub-io/feast
Length of output: 185
🏁 Script executed:
# Look at the full context of where OIDC secret is processed
cat -n ./infra/feast-operator/internal/controller/services/repo_config.go | sed -n '100,125p'Repository: opendatahub-io/feast
Length of output: 1079
🏁 Script executed:
# Search for any deployment container env handling related to OIDC
rg -n "container\|env\|ENV" --type go ./infra/feast-operator/internal/controller/ | grep -i oidcRepository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check the authz.go file to see if it handles OIDC_ISSUER_URL
cat -n ./infra/feast-operator/internal/controller/authz/authz.goRepository: opendatahub-io/feast
Length of output: 15347
🏁 Script executed:
# Check if there are any plans to use OIDC_ISSUER_URL in feature store deployment
rg -n "IssuerURL\|issuerURL" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Look at the GatewayConfig related code in the operator
rg -n "GatewayConfig" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Final verification: Search for all places where OidcProperties are used to confirm nothing reads OIDC_ISSUER_URL
rg -n "OidcProperties\|OidcClientId\|OidcAuthDiscoveryUrl" --type go ./infra/feast-operator/Repository: opendatahub-io/feast
Length of output: 46
🏁 Script executed:
# Check what the documentation says about how the env var should be used
cat -n ./infra/feast-operator/docs/odh-operator-parameters.mdRepository: opendatahub-io/feast
Length of output: 1310
Documentation describes unimplemented functionality.
Line 15 states operator code should read os.Getenv("OIDC_ISSUER_URL"), but the operator's OIDC configuration is sourced exclusively from Kubernetes secrets (repo_config.go:107-122). The secret contains client_id, auth_discovery_url, client_secret, username, and password—no issuerURL field. No code path reads OIDC_ISSUER_URL from the environment. The env var is injected into the Deployment manifest but remains dead configuration.
Align documentation with actual implementation: Either update the code to consume the env var, or document that OIDC config must be provisioned as a secret.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/feast-operator/docs/odh-operator-parameters.md` around lines 13 - 15,
Docs claim operator reads OIDC_ISSUER_URL from env, but code in repo_config.go
(around the OIDC secret parsing at lines ~107-122) only loads client_id,
auth_discovery_url, client_secret, username, password from a Kubernetes secret
and there is no code path reading OIDC_ISSUER_URL; fix by either (A)
implementing env var consumption: add logic in the repo_config.go OIDC/load
function to read os.Getenv("OIDC_ISSUER_URL") (and use it as the
issuer/discovery URL or jwks source, falling back to the secret’s
auth_discovery_url when env var is empty), or (B) update the documentation and
Deployment manifest to remove the dead OIDC_ISSUER_URL guidance and explicitly
document that the operator expects OIDC values in the secret fields (client_id,
auth_discovery_url, client_secret, username, password) and stop injecting the
unused env var into the Deployment. Ensure whichever path you choose is
reflected consistently in repo_config.go, the Deployment manifest, and the docs.
|
@GowthamShanmugam: The following test failed, say
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. |
|
/cc @aniketpalu |
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
Operator PR checks are failing, you may run pre-commit to fit it locally and push it the PR.
The fact that upstream PR feast-dev#6089 contains identical changes for all 6 overlapping files confirms this branch was intentionally carved out as the ODH-specific subset of that larger upstream PR. Once feast-dev#6089 merges upstream and gets synced, these changes become a no-op (already present), and the rest of feast-dev#6089 lands cleanly.
jyejare
left a comment
jyejare
left a comment
There was a problem hiding this comment.
We cn close this PR once upstream is merged.
|
the upstream PR has been merged: opendatahub-io/opendatahub-operator#3364 |
|
@aniketpalu Are we safe to close this ? |
|
Thanks for the PR. I cherry-picked your commit & used this in upstream feast (feast-dev#6089). The changes were good & cleanly merged with upstream. We can close this PR. Thanks again for the support! CC: @jyejare |
|
@GowthamShanmugam Feel free to close it. |
|
PR needs rebase. 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. |
4 participants
Pairs with opendatahub-operator injecting OIDC_ISSUER_URL at reconcile time from GatewayConfig when the cluster uses external OIDC (RHOAIENG-55767).
Made-with: Cursor
What this PR does / why we need it:
Which issue(s) this PR fixes:
Checks
git commit -s)Testing Strategy
Misc
Summary by CodeRabbit
Chores
Documentation