OCPBUGS-65633: add network-check-target + networking-console-plugin service accounts#2868
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two ServiceAccount manifests and assigns them to existing pod specs: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65633, 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: 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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @bindata/network-diagnostics/001-rbac.yaml:
- Around line 132-138: Change the ServiceAccount manifest to use the correct
core API group by replacing apiVersion: rbac.authorization.k8s.io with
apiVersion: v1 for the ServiceAccount named network-check-target in namespace
openshift-network-diagnostics, and remove the trailing whitespace on the blank
line after namespace (line containing only whitespace) to ensure the YAML is
accepted by Kubernetes.
In @bindata/networking-console-plugin/001-rbac.yaml:
- Around line 1-5: The manifest declares a ServiceAccount with the wrong
apiVersion; change the apiVersion for the ServiceAccount resource from
"rbac.authorization.k8s.io" to "v1" so Kubernetes accepts it; update the
apiVersion field at the top of the ServiceAccount definition (metadata.name:
networking-console-plugin, metadata.namespace: openshift-network-console) to
"v1" and keep the rest of the ServiceAccount spec unchanged.
📜 Review details
Configuration used: Organization 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 (6)
bindata/network-diagnostics/001-rbac.yamlbindata/network-diagnostics/network-check-target.yamlbindata/networking-console-plugin/001-rbac.yamlbindata/networking-console-plugin/002-config-map.yamlbindata/networking-console-plugin/003-deployment.yamlbindata/networking-console-plugin/005-service.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
bindata/networking-console-plugin/001-rbac.yamlbindata/network-diagnostics/001-rbac.yamlbindata/network-diagnostics/network-check-target.yamlbindata/networking-console-plugin/003-deployment.yaml
🔇 Additional comments (2)
bindata/networking-console-plugin/003-deployment.yaml (1)
58-58: LGTM!The serviceAccount assignment correctly references the networking-console-plugin ServiceAccount defined in
001-rbac.yaml. The namespace alignment is correct, and the configuration withautomountServiceAccountToken: falseindicates this is used for pod security context rather than API access.bindata/network-diagnostics/network-check-target.yaml (1)
66-66: LGTM!The serviceAccount assignment correctly references the network-check-target ServiceAccount defined in
bindata/network-diagnostics/001-rbac.yaml. The namespace alignment is correct.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
/retest |
|
@ehearne-redhat: This pull request references Jira Issue OCPBUGS-65633, which is valid. 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. |
|
/test hypershift-e2e-aks |
|
/test e2e-aws-ovn-hypershift-conformance |
|
/retest |
|
@anuragthehatter @arkadeepsen @kyrtapz can we get an override on the The rest of the tests appear to pass - feel free to review when you have the time. :) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@ehearne-redhat Can you please squash the commits into a single one? |
|
@arkadeepsen yeah that's no problem. :) |
b2ea5a2 to
2afeb6e
Compare
|
@arkadeepsen so that's done now. let me know if you need anything else for this PR :) |
Thanks for that. I want to check whether the upgrade jobs are sporadically facing any issues or not. Historically when manifests are updated, sometimes the upgrade jobs face issues due to unpredictable order of component upgrade. So, let's wait for the jobs to pass again. |
|
/retest-required |
|
/test hypershift-e2e-aks This test reports inconsistent errors. No 443 error on latest test run. |
|
/test e2e-aws-ovn-serial-1of2 |
|
I am seeing in other test runs of { fail [github.com/openshift/origin/test/extended/networking/commatrix.go:143]: Unexpected error:
<*errors.errorString | 0xc001c5a470>:
the following ports are used but don't have an endpointslice:
Ingress,TCP,8080,openshift-multus,multi-networkpo,multus-networkpolicy-49tc2,multus-networkpolicy,worker,false
{
s: "the following ports are used but don't have an endpointslice: \n Ingress,TCP,8080,openshift-multus,multi-networkpo,multus-networkpolicy-49tc2,multus-networkpolicy,worker,false\n",
}
occurred}Therefore, since the two pods have their bespoke service accounts running correctly, I will unhold. /unhold |
|
I have also verified that CNO logs look similar in other PRs' runs of /verified by ehearne-redhat |
|
@ehearne-redhat: 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. |
|
/refresh |
|
/lgtm /override ci/prow/e2e-aws-ovn-serial-1of2 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, ehearne-redhat, kyrtapz 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 |
|
@kyrtapz: Overrode contexts on behalf of kyrtapz: ci/prow/e2e-aws-ovn-serial-1of2 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 kubernetes-sigs/prow repository. |
|
/retest |
1 similar comment
|
/retest |
|
New changes are detected. LGTM label has been removed. |
|
/retest There are a few tests here which are undergoing some PR reverts to fix changes. openshift/operator-framework-operator-controller#647 Default service account monitor test failing in Likely due to openshift/origin#30685 being merged. Probably needs a fix. |
460d0c3 to
2ebfbc8
Compare
|
/retest |
|
|
|
Just waiting for openshift/origin#30815 tests to pass and hopefully get review to resolve |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
2 similar comments
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
/test e2e-metal-ipi-ovn-dualstack-bgp-local-gw |
|
@ehearne-redhat: 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. |
|
From e2e-aws-ovn-serial-1of2: ---
apiVersion: v1
kind: Pod
metadata:
...
generateName: networking-console-plugin-56466947-
...
name: networking-console-plugin-56466947-clvfq
namespace: openshift-network-console
...
name: networking-console-plugin
...
serviceAccount: networking-console-plugin
serviceAccountName: networking-console-plugin
...
name: networking-console-plugin
...
phase: Running
...---
apiVersion: v1
kind: Pod
metadata:
...
generateName: network-check-target-
...
labels:
app: network-check-target
...
name: network-check-target-container
...
serviceAccount: network-check-target
serviceAccountName: network-check-target
...
name: network-check-target-container
...
phase: Running
.../verified by ehearne-redhat |
|
@ehearne-redhat: 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. |
|
I believe |
This PR adds a service account to
openshift-network-diagnostics/network-check-target-andopenshift-network-console/networking-console-plugin-pods. It also includes a change to render_test as the number14for number of YAMLs to check for is now15due to added RBAC manifest. The ordering of the manifests have been changed to allow for the correct application of manifests.The reason for this change is that these pods should use their own bespoke service account, not default.