Skip to content

Conversation

@dobsonj
Copy link
Member

@dobsonj dobsonj commented Oct 13, 2025

https://issues.redhat.com/browse/STOR-2550

Clean cherry-pick from #530 -- this was reverted in 4.20 due to a limitation in the OLM build pipeline.

/cc @openshift/storage
for code review

/cc @duanwei33 @chao007
for QE approval

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 13, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Oct 13, 2025

@dobsonj: This pull request references STOR-2331 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:

https://issues.redhat.com/browse/STOR-2550

Clean cherry-pick from #530 -- this was reverted in 4.20 due to a limitation in the OLM build pipeline.

/cc @openshift/storage
for code review

/cc @duanwei33 @chao007
for QE approval

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.

@openshift-ci openshift-ci bot requested review from chao007 and duanwei33 October 13, 2025 21:54
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 13, 2025

@dobsonj: GitHub didn't allow me to request PR reviews from the following users: openshift/storage.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

https://issues.redhat.com/browse/STOR-2550

Clean cherry-pick from #530 -- this was reverted in 4.20 due to a limitation in the OLM build pipeline.

/cc @openshift/storage
for code review

/cc @duanwei33 @chao007
for QE approval

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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dobsonj

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 13, 2025
@duanwei33
Copy link
Contributor

@dobsonj Hi, the changes here look very similar to the ones in #530. I just want to double-check a couple of things:

  • Are the contents of these two PRs intended to be identical?
  • If so, my assumption is that this separate PR is now possible because a limitation in the OLM build pipeline no longer exists in version 4.21. Is that correct?
  • And just to be thorough, are there any other checks or validations we should be aware of besides this storage-related one? (I guess we don't need to pay attention on image building.)

@dobsonj
Copy link
Member Author

dobsonj commented Oct 14, 2025

Hi @duanwei33 👋
Yes, you are correct: the contents are intended to be identical to #530, and NetworkPolicies are allowed by OLM starting in 4.21 (reference). IMO, we can focus on validation of storage components (LSO).

@dobsonj
Copy link
Member Author

dobsonj commented Oct 14, 2025

BTW, I included a fix for ci/prow/unit in #554 -- the unit test failure is not related to these changes.

@duanwei33
Copy link
Contributor

/assign @duanwei33

@duanwei33
Copy link
Contributor

Tested with the open PR, the network policy(with pod-select) and the lables all looks good.

$ oc -n openshift-local-storage get pod local-storage-operator-57b9ff6654-ngcn8 -oyaml | yq .metadata.labels | grep allow
openshift.storage.network-policy.api-server: allow
openshift.storage.network-policy.dns: allow
openshift.storage.network-policy.operator-metrics: allow

$ oc -n openshift-local-storage get pod diskmaker-manager-jp8rh -oyaml | yq .metadata.labels
openshift.storage.network-policy.api-server: allow
openshift.storage.network-policy.diskmaker-metrics: allow
openshift.storage.network-policy.dns: allow

$ oc -n openshift-local-storage get pod diskmaker-discovery-lrvlk -oyaml | yq .metadata.labels
openshift.storage.network-policy.api-server: allow
openshift.storage.network-policy.diskmaker-metrics: allow
openshift.storage.network-policy.dns: allow


$ oc -n openshift-local-storage get networkpolicies.networking.k8s.io
NAME                                 POD-SELECTOR                                               AGE
allow-egress-to-api-server           openshift.storage.network-policy.api-server=allow          11m
allow-egress-to-dns                  openshift.storage.network-policy.dns=allow                 11m
allow-ingress-to-diskmaker-metrics   openshift.storage.network-policy.diskmaker-metrics=allow   11m
allow-ingress-to-operator-metrics    openshift.storage.network-policy.operator-metrics=allow    11m
default-deny-all                     <none>                                                     11m

The next step is to verify that all the regression cases pass and perform some negative tests.

@jsafrane
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2025
@jsafrane
Copy link
Contributor

/retest

@Phaow
Copy link
Contributor

Phaow commented Oct 22, 2025

/test e2e-operator-extended

@duanwei33
Copy link
Contributor

Hi @dobsonj, I tried a corner case: we defined two network policies named allow-ingress-to-operator-metrics but with different ports in ns/openshift-cluster-storage-operator and for LSO:
A) in openshift-cluster-storage-operator
spec:
ingress:

  • ports:
    • port: 8443
      protocol: TCP
    • port: 8444
      protocol: TCP
      B) for LSO
  • ports:
    • protocol: TCP
      port: 8080
    • protocol: TCP
      port: 8081

So if I install the LSO in openshift-cluster-storage-operator ns(usually we don't do it but it is allowed), at first the networkpolicy/allow-ingress-to-operator-metrics changes to 8080/8081 and then CSO reconciles it to 8443/8444, which means LSO related pods will use a wrong network policy.

Should we rename allow-ingress-to-operator-metrics for LSO?

@duanwei33
Copy link
Contributor

Thanks @Phaow for helping trigger the regression test, the result looks good.

@dobsonj
Copy link
Member Author

dobsonj commented Oct 22, 2025

if I install the LSO in openshift-cluster-storage-operator ns(usually we don't do it but it is allowed)

Oh! I really hope no one would do that, but you make a good point, and I'll change the name to avoid this :)

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 23, 2025
@dobsonj
Copy link
Member Author

dobsonj commented Oct 23, 2025

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 24, 2025

@dobsonj: all tests passed!

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.

@jsafrane
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2025
@duanwei33
Copy link
Contributor

The following tests are performed withthe latest PR:

  1. NetworkPolicy Creation and Configuration, the NetworkPolicies created with correct pod-selectors and ports and the pod labels match NetworkPolicy selectors
$ oc get networkpolicy
NAME                                     POD-SELECTOR                                                   AGE
lso-allow-egress-to-api-server           openshift.storage.network-policy.lso.api-server=allow          8h
lso-allow-egress-to-dns                  openshift.storage.network-policy.lso.dns=allow                 8h
lso-allow-ingress-to-diskmaker-metrics   openshift.storage.network-policy.lso.diskmaker-metrics=allow   8h
lso-allow-ingress-to-operator-metrics    openshift.storage.network-policy.lso.operator-metrics=allow    8h
lso-default-deny-all                     <none>                                                         8h
$ oc get deployment.apps/local-storage-operator -oyaml | yq .spec.template.metadata.labels
name: local-storage-operator
openshift.storage.network-policy.lso.api-server: allow
openshift.storage.network-policy.lso.dns: allow
openshift.storage.network-policy.lso.operator-metrics: allow

$ oc get daemonset.apps/diskmaker-manager -oyaml | yq .spec.template.metadata.labels
app: diskmaker-manager
openshift.storage.network-policy.lso.api-server: allow
openshift.storage.network-policy.lso.diskmaker-metrics: allow
openshift.storage.network-policy.lso.dns: allow

$ oc get daemonset.apps/diskmaker-discovery -oyaml | yq .spec.template.metadata.labels
app: diskmaker-discovery
openshift.storage.network-policy.lso.api-server: allow
openshift.storage.network-policy.lso.diskmaker-metrics: allow
openshift.storage.network-policy.lso.dns: allow
  1. Traffic Flow Validation

    • Positive tests: Allowed traffic (DNS, API Server, metrics) flows correctly
    • Negative tests: Blocked traffic properly rejected (timeout on non-allowed ports)
    • Default deny-all policy verified
  2. Port Configuration

    • All ports match actual listening ports in pods
  3. Regression Testing

    • Core CRDs functional: LocalVolume, LocalVolumeSet, LocalVolumeDiscovery
    • Metrics collection via Prometheus verified
    • Full e2e test suite passed

@duanwei33
Copy link
Contributor

It looks good to me for merging, adding QE's label.

/verified by @duanwei33

@openshift-ci-robot openshift-ci-robot added the verified Signifies that the PR passed pre-merge verification criteria label Oct 24, 2025
@openshift-ci-robot
Copy link
Contributor

@duanwei33: This PR has been marked as verified by @duanwei33.

In response to this:

It looks good to me for merging, adding QE's label.

/verified by @duanwei33

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
Copy link
Contributor

/unassign @Phaow @chao007 @radeore

@openshift-merge-bot openshift-merge-bot bot merged commit 397dcaa into openshift:main Oct 24, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. verified Signifies that the PR passed pre-merge verification criteria

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants