Skip to content

OCPCLOUD-2980: add ingress/egress network policy #1387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

huali9
Copy link
Contributor

@huali9 huali9 commented Jul 3, 2025

No description provided.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 3, 2025

@huali9: This pull request references OCPCLOUD-2951 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.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 3, 2025
@openshift-ci openshift-ci bot requested review from JoelSpeed and racheljpg July 3, 2025 10:27
Copy link
Contributor

openshift-ci bot commented Jul 3, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign damdo for approval. For more information see the Code Review Process.

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

@huali9
Copy link
Contributor Author

huali9 commented Jul 4, 2025

/retest

Comment on lines +16 to +22
- ports:
- protocol: TCP
port: 1
endPort: 65535
- ports:
- protocol: UDP
port: 5353
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to just allow all traffic to egress? What does the recommendations document suggest?

ingress:
- ports:
- protocol: TCP
port: 8443
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is present in multiple policies, what's it for?

- ports:
- protocol: TCP
port: 8440
endPort: 8444
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also includes 8443, third time now, I assume it's not an issue including a port multiple times, just curious why it's showing up so much

@@ -0,0 +1,16 @@
# Default deny all ingress and egress
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks different to @miyadav's one for CAPI, can we co-ordinate what is and isn't needed between the two PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked @miyadav's pr openshift/cluster-capi-operator#325, this file install/0000_90_machine-api-operator_05_networkpolicy-default-deny.yaml looks similar with his manifests/0000_30_cluster-api_17_deny-all.yaml, but he added many comments, do you mean it's different here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right, they are the same apart from naming and comments.

Can we call these both default-deny? @miyadav would you be happy to update yours so the name is consistent with Huali's one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes sure , thanks

@huali9
Copy link
Contributor Author

huali9 commented Jul 15, 2025

Thank you @JoelSpeed for your review. I addressed the comments on the jira, because table can be added on that.

@huali9 huali9 changed the title OCPCLOUD-2951: add ingress/egress network policy OCPCLOUD-2980: add ingress/egress network policy Jul 15, 2025
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jul 15, 2025

@huali9: This pull request references OCPCLOUD-2980 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 story to target the "4.20.0" version, but no target version was set.

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.

@huali9
Copy link
Contributor Author

huali9 commented Jul 16, 2025

/retest

@JoelSpeed
Copy link
Contributor

This change looks like it's impacting our spot instance e2e tests, can you PTAL?

@huali9
Copy link
Contributor Author

huali9 commented Jul 17, 2025

This change looks like it's impacting our spot instance e2e tests, can you PTAL?

Yeah, thank you Joel, I'm checking that.

@huali9
Copy link
Contributor Author

huali9 commented Jul 17, 2025

I found out the root cause of the case Machine Suite: [It] Running on Spot should handle the spot instances [mapi, disruptive] failed. This case will deploy a new deployment and it listens on a new port which is 8082, I tried adding this new port (8082) in the allow ingress network policy, then this case passed. But I shouldn't seem to be doing this, because this port is not a must for openshift-machine-api namespace, if I add that, seems it breaks the intention of adding network policy. What do you think? @JoelSpeed

@JoelSpeed
Copy link
Contributor

@huali9 Could the test suite be updated to create a network policy to allow this traffic? And then remove it once the test is complete?

@huali9
Copy link
Contributor Author

huali9 commented Jul 17, 2025

@huali9 Could the test suite be updated to create a network policy to allow this traffic? And then remove it once the test is complete?

Cool, good idea! Let me update that case.

@huali9
Copy link
Contributor Author

huali9 commented Jul 18, 2025

/retest

Copy link
Contributor

openshift-ci bot commented Jul 18, 2025

@huali9: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6 d353379 link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-vsphere-ovn-techpreview-serial d353379 link false /test e2e-vsphere-ovn-techpreview-serial
ci/prow/e2e-metal-ipi-virtualmedia d353379 link false /test e2e-metal-ipi-virtualmedia
ci/prow/e2e-vsphere-static-ovn d353379 link false /test e2e-vsphere-static-ovn
ci/prow/e2e-metal-ipi d353379 link false /test e2e-metal-ipi
ci/prow/e2e-metal-ipi-ovn-dualstack d353379 link false /test e2e-metal-ipi-ovn-dualstack

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants