-
Notifications
You must be signed in to change notification settings - Fork 338
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
Implementation required to enable Forwarding if it is already disabled #4172
Conversation
I have added a function setGlobalForwardingRule() to enable or disable forwarding for all IPv4 and IPv6 interfaces based on config.Gateway.DisableForwarding. |
ack let's talk about this a bit in our next meeting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
main logic looks good, please add tests for this and then I can approve
I won't force you but ... if you have time are you interested in writing up a short documentation on "disable-forwarding" config option under https://ovn-kubernetes.io/developer-guide/configuration/ please? We never had the culture of documenting anything before so I know it might seem unfair to ask you to do this now so feel free to ignore my suggestion but if you do have time.. then all I need is a
subtitle saying "Disable Forwarding Config" => then write details on what this does and add all the sample iptables rules it adds and how one can set it to false to avoid these rules etc.. (PS: Don't add any OCP product info BUT deff mention the use case for this config option and then instead of using OCP/CNO etc use operator or admin or whatever generic terms suits here) (also move configuration.md from developer-guide to getting-started folder)
// delExternalBridgeServiceForwardingRules removes iptables rules which might | ||
// have been added to disable forwarding | ||
func delExternalBridgeServiceForwardingRules(cidrs []*net.IPNet) error { | ||
return deleteIptRules(getGatewayForwardRules(cidrs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need the extra abstraction here? can't we just call nodeipt.DelRules(getGatewayForwardRules(cidrs))
straight from here? its straightforward enough for readability that I don't see the need for abstracting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done this to keep consistency with the initExternalBridgeServiceForwardingRules() and initExternalBridgeDropForwardingRules() function. init functions uses this abstraction mostly due to use of insertIptRules() in one and appendIptRules() in the other init function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but those functions have abstraction since we pass true/false for insert/append. We don't need that here which makes the layer useless right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docs will be done in new PR |
DCO seems confused, not sure why. |
Unit test passed in local environment. Unrelated flake from ovn-kubernetes/go-controller/pkg/ovn/egressip_test.go:1981 /retest-required |
This PR is to remove iptables rules from FORWARD chain if config.Gateway.DisableForwarding is set to false. Following iptables rules gets removed from FORWARD chain given 10.1.0.0/16 is clusterNetwork CIDR and 10.96.0.0/16 is serviceNetwork CIDR. -A FORWARD -s 10.96.0.0/16 -j ACCEPT -A FORWARD -d 10.96.0.0/16 -j ACCEPT -A FORWARD -s 169.254.169.1 -j ACCEPT -A FORWARD -d 169.254.169.1 -j ACCEPT -A FORWARD -d 10.1.0.0/16 -j ACCEPT -A FORWARD -s 10.1.0.0/16 -j ACCEPT -A FORWARD -i breth1 -j DROP -A FORWARD -o breth1 -j DROP Jira: https://issues.redhat.com/browse/OCPBUGS-23758 Signed-off-by: Arnab Ghosh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge after dcoapp/app#211 get's sorted out
CI is green, only the DCO lane is blocked but I have manually verified the signoff is correct. tysm @arghosh93 !! |
This PR is to remove any iptables rules which were created based on config.Gateway.DisableForwarding set to true and later it was changed to false.
This PR also ensures forwarding sysctl parameters are set for all IPv4 and IPv6 interfaces by either enabling or disabling it based on config.Gateway.DisableForwarding
- What this PR does and why is it needed
- Special notes for reviewers
- How to verify it
- Description for the changelog