OCPBUGS-65476: Add openshift-ingress-operator ClusterRole to ClusterO…#1313
OCPBUGS-65476: Add openshift-ingress-operator ClusterRole to ClusterO…#1313davidesalerno wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
@davidesalerno: This pull request references Jira Issue OCPBUGS-65476, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
14bb378 to
21827f4
Compare
|
/retest-required |
21827f4 to
103253e
Compare
|
/assign |
103253e to
35b0724
Compare
|
[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 |
| - group: rbac.authorization.k8s.io | ||
| name: ingress-operator | ||
| resource: clusterrolebindings |
There was a problem hiding this comment.
The name here should match the name in manifests/01-cluster-role-binding.yaml:
| - group: rbac.authorization.k8s.io | |
| name: ingress-operator | |
| resource: clusterrolebindings | |
| - group: rbac.authorization.k8s.io | |
| name: openshift-ingress-operator | |
| resource: clusterrolebindings |
There was a problem hiding this comment.
Maybe you were looking at the rolebinding's name? We should probably include these too:
cluster-ingress-operator/manifests/01-role-binding.yaml
Lines 2 to 6 in 35b0724
cluster-ingress-operator/manifests/01-role-binding.yaml
Lines 23 to 27 in 35b0724
There was a problem hiding this comment.
...and then we might as well include the roles as well:
cluster-ingress-operator/manifests/01-role.yaml
Lines 2 to 6 in 35b0724
cluster-ingress-operator/manifests/01-role.yaml
Lines 43 to 47 in 35b0724
There was a problem hiding this comment.
@Miciah I updated the code accordingly and in particular:
- fixed ClusterRole name
- added the Roles and RolesBinding
There was a problem hiding this comment.
As we discussed, it would also make sense to get the cluster ingress config and copy entries from Ingress.status.componentRoutes.relatedObjects to ClusterOperator.status.relatedObjects in Reconcile.
There was a problem hiding this comment.
I updated the controller with the requested change.
d00d433 to
1c93ae3
Compare
1c93ae3 to
56d32cc
Compare
|
/test all |
56d32cc to
4303bbd
Compare
|
/retest-required |
grzpiotrowski
left a comment
There was a problem hiding this comment.
/lgtm
@Miciah would you like to have another look since the last comments?
|
/retest-required |
1 similar comment
|
/retest-required |
|
/jira refresh |
|
@lihongan: This pull request references Jira Issue OCPBUGS-65476, which is invalid:
Comment DetailsIn 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. |
|
/jira refresh |
|
@lihongan: This pull request references Jira Issue OCPBUGS-65476, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn 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. |
|
/retest-required |
|
One e2e test is failing |
Hence marking as verified |
|
@melvinjoseph86: This PR has been marked as verified by DetailsIn 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. |
|
/retest |
|
/retest-required |
|
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. |
…perator relatedObjects Signed-off-by: Davide Salerno <dsalerno@redhat.com>
daf3330 to
2ba8ac3
Compare
|
New changes are detected. LGTM label has been removed. |
📝 WalkthroughWalkthroughThis change extends the ingress cluster operator's status reporting to include RBAC-related objects in the ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 `@pkg/operator/controller/status/controller.go`:
- Around line 490-497: Reconcile reads configv1.Ingress.status.componentRoutes
but New() doesn't watch Ingress, causing stale relatedObjects; update the
controller setup in New() to add a watch for configv1.Ingress so ingress status
changes trigger reconciles. Concretely, import configv1 and call
builder.Watches(&source.Kind{Type: &configv1.Ingress{}},
handler.EnqueueRequestsFromMapFunc(<map func that maps the Ingress event to the
primary resource this controller manages>)) or an appropriate handler (e.g.,
EnqueueRequestsForOwner/EnqueueRequestsForObject) so updates to
Ingress.status.ComponentRoutes cause the controller (the one constructed in
New()) to reconcile and refresh state.IngressComponentRoutesStatus.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7a057001-6a81-473b-8cdd-3ee1ba7ddd44
📒 Files selected for processing (3)
manifests/03-cluster-operator.yamlpkg/operator/controller/status/controller.gotest/e2e/operator_test.go
| ingressConfig := &configv1.Ingress{} | ||
| if err := r.client.Get(ctx, operatorcontroller.IngressClusterConfigName(), ingressConfig); err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return state, fmt.Errorf("failed to get default ingress: %v", err) | ||
| } | ||
| } else { | ||
| state.IngressComponentRoutesStatus = ingressConfig.Status.ComponentRoutes | ||
| } |
There was a problem hiding this comment.
Add a watch for configv1.Ingress to avoid stale relatedObjects
Line [490] makes reconcile depend on Ingress.status.componentRoutes, but New() does not watch configv1.Ingress. Updates to component routes may not propagate until an unrelated event triggers reconcile.
Proposed fix
@@
if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.ClusterOperator{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.NewPredicateFuncs(isIngressClusterOperator))); err != nil {
return nil, err
}
+
+ isClusterIngressConfig := predicate.NewPredicateFuncs(func(o client.Object) bool {
+ return o.GetName() == operatorcontroller.IngressClusterConfigName().Name
+ })
+ if err := c.Watch(source.Kind[client.Object](
+ operatorCache,
+ &configv1.Ingress{},
+ handler.EnqueueRequestsFromMapFunc(toDefaultIngressController),
+ isClusterIngressConfig,
+ )); err != nil {
+ return nil, err
+ }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| ingressConfig := &configv1.Ingress{} | |
| if err := r.client.Get(ctx, operatorcontroller.IngressClusterConfigName(), ingressConfig); err != nil { | |
| if !errors.IsNotFound(err) { | |
| return state, fmt.Errorf("failed to get default ingress: %v", err) | |
| } | |
| } else { | |
| state.IngressComponentRoutesStatus = ingressConfig.Status.ComponentRoutes | |
| } | |
| if err := c.Watch(source.Kind[client.Object](operatorCache, &configv1.ClusterOperator{}, handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), predicate.NewPredicateFuncs(isIngressClusterOperator))); err != nil { | |
| return nil, err | |
| } | |
| isClusterIngressConfig := predicate.NewPredicateFuncs(func(o client.Object) bool { | |
| return o.GetName() == operatorcontroller.IngressClusterConfigName().Name | |
| }) | |
| if err := c.Watch(source.Kind[client.Object]( | |
| operatorCache, | |
| &configv1.Ingress{}, | |
| handler.EnqueueRequestsFromMapFunc(toDefaultIngressController), | |
| isClusterIngressConfig, | |
| )); err != nil { | |
| return nil, err | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/status/controller.go` around lines 490 - 497,
Reconcile reads configv1.Ingress.status.componentRoutes but New() doesn't watch
Ingress, causing stale relatedObjects; update the controller setup in New() to
add a watch for configv1.Ingress so ingress status changes trigger reconciles.
Concretely, import configv1 and call builder.Watches(&source.Kind{Type:
&configv1.Ingress{}}, handler.EnqueueRequestsFromMapFunc(<map func that maps the
Ingress event to the primary resource this controller manages>)) or an
appropriate handler (e.g., EnqueueRequestsForOwner/EnqueueRequestsForObject) so
updates to Ingress.status.ComponentRoutes cause the controller (the one
constructed in New()) to reconcile and refresh
state.IngressComponentRoutesStatus.
|
@davidesalerno: The following tests 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. |
…perator relatedObjects
This change adds the
openshift-ingress-operatorClusterRole to the ClusterOperatorrelatedObjects