Implementation required to enable Forwarding if it is already disabled #4172
Conversation
arghosh93
requested review from
trozet,
dcbw,
girishmg,
jcaamano and
tssurya
as code owners
|
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 |
tssurya
added
kind/bug
There was a problem hiding this comment.
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.
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.
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.
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?
|
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]>
|
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