STOR-2340: Add e2e tests for storage network policy #30359
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: duanwei33 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@mpatlasov @dobsonj Could you help take a look? |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 92d160d
New tests seen in this PR at sha: 92d160d
|
|
/test verify |
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
1b1417c to
81e1f58
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: 81e1f58
New tests seen in this PR at sha: 81e1f58
|
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
a065245 to
c1d5777
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: c1d5777
|
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
c1d5777 to
4580205
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 4580205
|
| Namespace: CSINamespace, | ||
| Name: "aws-ebs-csi-driver-controller", | ||
| Platform: "aws", | ||
| RequiredLabels: csiControllerRequiredLabels, |
There was a problem hiding this comment.
Should we check for openshift.storage.network-policy.all-egress here as well?
There was a problem hiding this comment.
Thanks for the feedback. You've highlighted a key nuance here.
Ideally, fine-grained policies like allow-egress-to-api-server would be sufficient to enforce the principle of least privilege. However, the broader allow-all-egress policy exists as a practical measure to ensure functionality in certain configurations, acting as a workaround.
And when a pod has both policies applied, the more specific allow-egress-to-api-server becomes redundant, as its permissions are a subset of allow-all-egress(TCP all vs TCP specific port).
Nevertheless, I agree that adding it is the correct approach to accurately reflect the actual implementation.
| g.It("should ensure required NetworkPolicies exist with correct labels", func() { | ||
| for _, c := range networkPolicyChecks { | ||
| _, err := oc.AdminKubeClient().CoreV1().Namespaces().Get(context.TODO(), c.Namespace, metav1.GetOptions{}) | ||
| if err != nil { |
There was a problem hiding this comment.
Should we check if errors.IsNotFound(err) here and g.Fail() otherwise?
There was a problem hiding this comment.
Just to clarify, the openshift-manila-csi-driver namespace is optional; we skip if there is no openshift-manila-csi-driver namespace.
There was a problem hiding this comment.
Yes, I understand. There is another place in the PR where you "Get" a resource:
deployment, err := oc.AdminKubeClient().AppsV1().Deployments(res.Namespace).Get(context.TODO(), res.Name, metav1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
The logic there is to allow err!=nil only if it is NotFound, and g.Fail() otherwise. It's not obvious why getting namespace is special and it's OK to silently skip err!=nil (when !errors.IsNotFound(err)) for optional openshift-manila-csi-driver namespace case.
There was a problem hiding this comment.
Okay, got it. I think they don't have too many differences :)
I've rebased the branch. PTAL and let me know if it's ready for an lgtm
There was a problem hiding this comment.
My suggestion is to make L301-308 look like this:
_, err := oc.AdminKubeClient().CoreV1().Namespaces().Get(context.TODO(), c.Namespace, metav1.GetOptions{})
if err != nil {
if errors.IsNotFound(err) {
if c.Optional {
g.By(fmt.Sprintf("Skipping optional namespace %s (not found)", c.Namespace))
continue
}
o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("namespace %s should exist", c.Namespace))
}
g.Fail(fmt.Sprintf("Error fetching namespace %s: %v", c.Namespace, err))
}
wdyt?
There was a problem hiding this comment.
Oh, got your point, yes it looks clearer.
Updated, thanks for the suggestion.
|
/retest |
|
Risk analysis has seen new tests most likely introduced by this PR. New Test Risks for sha: c514831
New tests seen in this PR at sha: c514831
|
duanwei33
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@duanwei33: This pull request references STOR-2340 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead. 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. |
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
c514831 to
16c7f2c
Compare
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: 16c7f2c
|
|
FYI, go-verify-deps started failing because PR#70376 was merged recently. |
| podTemplateLabels = daemonset.Spec.Template.Labels | ||
|
|
||
| default: | ||
| g.Fail(fmt.Sprintf("Unsupported resource type: %s", res.ResourceType)) |
There was a problem hiding this comment.
I'd suggest to set err explicitly here. Otherwise, if err != nil { continue } below wouldn't work.
There was a problem hiding this comment.
Thank you for the review! I've addressed the issues: I've removed the code if err != nil { continue } and the unused var err error declaration. Here's why:
- Each case in the switch statement uses
:=to declare its own localerrvariable, which shadows the outererr - Both
ResourceTypeDeploymentandResourceTypeDaemonSetcases already handle errors appropriately (eithercontinueorg.Fail) - The
defaultcase callsg.Fail()which terminates the test immediately - Therefore, the outer
errvariable is never set and theif err != nilcheck is unreachable
The current approach is cleaner - each case handles its own errors, and unsupported resource types (which would be a code bug) fail immediately with a clear error message. So I prefer to removing the unless code :)
Let me know if you'd prefer a different approach!
|
Hi @duanwei33 , thank you for reworking test case "should ensure required NetworkPolicies exist with correct labels", it looks great! I have only a couple of nits remaining (see my inline comments). They are minor, I'm ok to lgtm anyway. Let me know if you're going to address them. Otherwise we can go ahead with current code as-is. Btw, how did you verify that |
|
/retest-required |
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
16c7f2c to
55b9a04
Compare
This adds comprehensive e2e tests to verify that storage-related operators and controllers have the required network policy labels and that NetworkPolicy resources exist with correct pod selectors. Changes: - Add namespace constants to helpers.go for reuse across storage tests - Add storage_networkpolicy.go with tests for CSO and CSI operators - Verify required network policy labels on deployments - Validate NetworkPolicy resources in storage namespaces - Skip these tests on MicroShift clusters where they are not applicable - Temporarily disabled ManilaCSINamespace check due to OCPBUGS-61175
duanwei33
force-pushed
the
storage-networkpolicy-e2e
branch
from
55b9a04 to
da3860c
Compare
|
@duanwei33: 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. |
|
Risk analysis has seen new tests most likely introduced by this PR. New tests seen in this PR at sha: da3860c
|
3 participants
This commit adds comprehensive e2e tests to verify that storage-related operators and controllers have the required network policy labels and that NetworkPolicy resources exist with correct pod selectors.
Changes:
Something on-tracking:
Test records: