CNTRLPLANE-947: operator: set oauth-specific relatedObjects dynamically in the operator status #800
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@liouk: This pull request references CNTRLPLANE-947 which is a valid jira issue. In response to this: Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liouk The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
WalkthroughRemoved two static relatedObjects from the ClusterOperator manifest and made relatedObjects conditional in the operator starter: the starter now adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🔇 Additional comments (3)
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 |
openshift-ci
bot
added
the
approved
liouk
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/operator/starter.go (1)
478-478: Clarify the comment - it states the opposite of what the code does.The comment says "oauth-specific relatedObjects must not be defined when OIDC is not available", but the code actually defines them when OIDC is not available (lines 488-491) and omits them when OIDC is available (lines 484-486).
Apply this diff to fix the comment:
- // oauth-specific relatedObjects must not be defined when OIDC is not available + // oauth-specific relatedObjects must be defined only when OIDC is not availableOr alternatively:
- // oauth-specific relatedObjects must not be defined when OIDC is not available + // oauth-specific relatedObjects must not be defined when OIDC is available
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
manifests/08_clusteroperator.yaml(0 hunks)pkg/operator/starter.go(1 hunks)
💤 Files with no reviewable changes (1)
- manifests/08_clusteroperator.yaml
🔇 Additional comments (1)
pkg/operator/starter.go (1)
481-486: Verify that returning no related objects on error is the desired behavior.When
OIDCAvailable()returns an error or when OIDC is available, the code returns(false, nil), which means no related objects are set. For the error case, confirm this is the intended behavior and that it won't cause issues if there's a transient error checking OIDC availability. The current approach is consistent with theoidcAvailablehelper function (line 862) which also returns false on error, but it's worth verifying that the default/fallback behavior is appropriate.
liouk
force-pushed
the
oauth-related-objects
branch
from
314fda7 to
daafb9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
manifests/08_clusteroperator.yaml(0 hunks)pkg/operator/starter.go(1 hunks)
💤 Files with no reviewable changes (1)
- manifests/08_clusteroperator.yaml
🔇 Additional comments (1)
pkg/operator/starter.go (1)
477-494: LGTM! Correct dynamic relatedObjects implementation.The logic correctly sets oauth-openshift Route and Service as related objects only when OIDC is unavailable (Lines 488-491). When OIDC is available, these objects are appropriately omitted (Line 485). The error handling (Lines 481-483) safely returns no objects on failure, which is reasonable for non-critical status information.
| statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer { | ||
| // oauth-specific relatedObjects must not be defined when OIDC is not available | ||
| ss.WithRelatedObjectsFunc(func() (isset bool, objs []configv1.ObjectReference) { | ||
| oidcAvailable, err := authConfigChecker.OIDCAvailable() | ||
| if err != nil { | ||
| klog.Infof("error while checking auth config to determine relatedObjects: %v", err) | ||
| return false, nil | ||
| } else if oidcAvailable { | ||
| return true, nil | ||
| } | ||
|
|
||
| return true, []configv1.ObjectReference{ | ||
| {Group: routev1.GroupName, Resource: "routes", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | ||
| {Resource: "services", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | ||
| } | ||
| }) | ||
| return ss | ||
| }) |
There was a problem hiding this comment.
Fix the misleading comment.
The comment on Line 478 is incorrect and contradicts the implementation. It states "oauth-specific relatedObjects must not be defined when OIDC is not available", but the code does the opposite—it defines the oauth-specific objects when OIDC is not available (Lines 488-491), which is the correct behavior.
The logic is sound: when OIDC is unavailable, OAuth resources are in use and should be listed as related objects. When OIDC is available, OAuth resources are not needed and should not be listed.
Apply this diff to clarify the comment:
- statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer {
- // oauth-specific relatedObjects must not be defined when OIDC is not available
+ statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer {
+ // oauth-specific relatedObjects should only be defined when OIDC is not available (i.e., when OAuth is in use)
ss.WithRelatedObjectsFunc(func() (isset bool, objs []configv1.ObjectReference) {Additionally, consider using klog.Warningf instead of klog.Infof on Line 482, since encountering an error while determining related objects is an abnormal condition worth highlighting.
oidcAvailable, err := authConfigChecker.OIDCAvailable()
if err != nil {
- klog.Infof("error while checking auth config to determine relatedObjects: %v", err)
+ klog.Warningf("error while checking auth config to determine relatedObjects: %v", err)
return false, nil📝 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.
| statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer { | |
| // oauth-specific relatedObjects must not be defined when OIDC is not available | |
| ss.WithRelatedObjectsFunc(func() (isset bool, objs []configv1.ObjectReference) { | |
| oidcAvailable, err := authConfigChecker.OIDCAvailable() | |
| if err != nil { | |
| klog.Infof("error while checking auth config to determine relatedObjects: %v", err) | |
| return false, nil | |
| } else if oidcAvailable { | |
| return true, nil | |
| } | |
| return true, []configv1.ObjectReference{ | |
| {Group: routev1.GroupName, Resource: "routes", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | |
| {Resource: "services", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | |
| } | |
| }) | |
| return ss | |
| }) | |
| statusControllerOptions = append(statusControllerOptions, func(ss *status.StatusSyncer) *status.StatusSyncer { | |
| // oauth-specific relatedObjects should only be defined when OIDC is not available (i.e., when OAuth is in use) | |
| ss.WithRelatedObjectsFunc(func() (isset bool, objs []configv1.ObjectReference) { | |
| oidcAvailable, err := authConfigChecker.OIDCAvailable() | |
| if err != nil { | |
| klog.Warningf("error while checking auth config to determine relatedObjects: %v", err) | |
| return false, nil | |
| } else if oidcAvailable { | |
| return true, nil | |
| } | |
| return true, []configv1.ObjectReference{ | |
| {Group: routev1.GroupName, Resource: "routes", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | |
| {Resource: "services", Name: "oauth-openshift", Namespace: "openshift-authentication"}, | |
| } | |
| }) | |
| return ss | |
| }) |
🤖 Prompt for AI Agents
In pkg/operator/starter.go around lines 477 to 494, the inline comment is
misleading (it says oauth-specific relatedObjects must not be defined when OIDC
is not available) but the code correctly defines oauth-related objects when OIDC
is not available; update the comment to accurately state that we list OAuth
resources as relatedObjects when OIDC is unavailable, and change the klog.Infof
call on the error path to klog.Warningf to better surface an abnormal condition
when checking OIDC availability.
8d37f95 to
667ec70
Compare
openshift-merge-robot
added
the
needs-rebase
liouk
force-pushed
the
oauth-related-objects
branch
from
667ec70 to
be1d638
Compare
openshift-merge-robot
removed
the
needs-rebase
liouk
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@liouk: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
| s.WithRelatedObjectsFunc(func() (isset bool, objs []configv1.ObjectReference) { | ||
| oidcAvailable, err := authConfigChecker.OIDCAvailable() | ||
| if err != nil { | ||
| klog.Infof("error while checking auth config to determine relatedObjects: %v", err) |
|
Pre-merge tested this and PR After configuring external oidc auth, the OAuth route and service are not shown anymore: Also checked rolling back to IDP: after rolling back, above OAuth route and service are shown again. /verified by @xingxingxia |
openshift-ci-robot
added
the
verified
|
@xingxingxia: This PR has been marked as verified by In response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
5 participants
No description provided.