Skip to content
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

Change the default ip forward policy to DROP #2409

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

Conversation

karampok
Copy link

When an OCP node has many interfaces, this node can act as router because the current default forward is ACCEPT. The current solution which is blocking ip forwarding only in sysctl level net.ipv4.ip_forward=0 that is inherited to all interfaces is not working because it breaks the external LB (metallb) case and therefore forwarding on those interface must be on.

The IP_FORWARD_MODE to Global is an opt-out option if for any reason there is a use case that accidentally works due to previous ACCEPT policy.

https://issues.redhat.com/browse/OCPBUGS-3176

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2024
Copy link
Contributor

openshift-ci bot commented Jun 11, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Jun 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: karampok
Once this PR has been reviewed and has the lgtm label, please assign kyrtapz for approval. For more information see the Kubernetes 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

@karampok
Copy link
Author

/test all

1 similar comment
@karampok
Copy link
Author

/test all

@karampok karampok force-pushed the iptables-drop-forward-policy branch from 3eb16d8 to b5d6be9 Compare June 11, 2024 12:47
@karampok
Copy link
Author

/retest

When an OCP node has many interfaces, this node can act as router
because the current default forward is ACCEPT. The current solution
which is blocking ip forwarding only in sysctl level
`net.ipv4.ip_forward=0` that is inherited to all interfaces is not
working because it breaks the external LB (metallb) case and therefore
forwarding on those interface must be on.

The IP_FORWARD_MODE to Global is an opt-out option if for any reason
there is a use case that accidentally works due to previous ACCEPT
policy.

Signed-off-by: karampok <karampok@gmail.com>
@karampok karampok force-pushed the iptables-drop-forward-policy branch from b5d6be9 to e3cb8d5 Compare June 13, 2024 07:20
@karampok karampok marked this pull request as ready for review June 13, 2024 08:27
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2024
@karampok
Copy link
Author

@trozet this PR should be the minimum changes, CI seems to fails for different reason. Thanks for the feedback

Copy link
Contributor

openshift-ci bot commented Jun 13, 2024

@karampok: 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-openstack-ovn e3cb8d5 link false /test e2e-openstack-ovn
ci/prow/security e3cb8d5 link false /test security
ci/prow/e2e-azure-ovn-upgrade e3cb8d5 link true /test e2e-azure-ovn-upgrade
ci/prow/e2e-metal-ipi-ovn-ipv6-ipsec e3cb8d5 link false /test e2e-metal-ipi-ovn-ipv6-ipsec
ci/prow/e2e-aws-ovn-local-to-shared-gateway-mode-migration e3cb8d5 link false /test e2e-aws-ovn-local-to-shared-gateway-mode-migration
ci/prow/e2e-vsphere-ovn-dualstack e3cb8d5 link false /test e2e-vsphere-ovn-dualstack
ci/prow/e2e-ovn-step-registry e3cb8d5 link false /test e2e-ovn-step-registry
ci/prow/e2e-aws-ovn-shared-to-local-gateway-mode-migration e3cb8d5 link false /test e2e-aws-ovn-shared-to-local-gateway-mode-migration
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6 e3cb8d5 link false /test e2e-vsphere-ovn-dualstack-primaryv6
ci/prow/e2e-aws-ovn-upgrade e3cb8d5 link true /test e2e-aws-ovn-upgrade
ci/prow/e2e-aws-ovn-hypershift-conformance e3cb8d5 link true /test e2e-aws-ovn-hypershift-conformance

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.

@karampok
Copy link
Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 13, 2024
@trozet
Copy link
Contributor

trozet commented Jun 13, 2024

why the hold @karampok ?

@karampok
Copy link
Author

karampok commented Jun 17, 2024

There is the PR that actually does the same ovn-org/ovn-kubernetes#4376, if this PR goes in do we still want to add this?

@trozet the on-hold is to avoid just merge without a CI run, and not sure if we need to followup something as part of documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants