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

Add features: host netns + route source interface #5

Merged
merged 13 commits into from
May 29, 2023

Conversation

fdomain
Copy link

@fdomain fdomain commented May 19, 2023

  • add option "host_netns" in configuration to move the host veth interface in a specific netns
  • add options "route_source_interface_ipv4/ipv6" to change source IP used in routes configured by the IPAM plugin

fdomain added 11 commits May 19, 2023 16:59
* add option "host_netns" in configuration to move the host veth
  interface in a specific netns
* add options "route_source_interface_ipv4/ipv6" to change source IP
  used in routes configured by the IPAM plugin
* add option "sysctl", taking as input the same format of configuration
  than the "tuning" plugin to configure sysctl parameters in the
  container netns
* result needs to be converted to the right CNI version before passing
  it to another plugin
* when using route_source_interface, we expect to have a single IP
  configured on the interface, however in IPv6, there is always a link
  local address configured in addition to the one provided by the IPAM
  plugin
* getIntfIP function was returning an error as we were seeing 2
  addresses instead of 1, now we filter this link-local address to fix
  it
* default route in IPv4 and IPv6 is equivalent to a Dst = nil in a route
  filter
* we must check that all the new config parameters have been correctly
  set on the host/container netns
@fdomain fdomain marked this pull request as ready for review May 22, 2023 12:13
@Lqp1 Lqp1 requested a review from a team May 22, 2023 13:40
Copy link

@Lqp1 Lqp1 left a comment

Choose a reason for hiding this comment

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

Looks good, but I wonder if a review from someone from your team would be valuable ?

plugins/main/ptp/ptp_test.go Outdated Show resolved Hide resolved
@@ -245,9 +379,49 @@ func cmdAdd(args *skel.CmdArgs) error {
result.DNS = conf.DNS
}

if conf.SysCtl != nil {
Copy link

Choose a reason for hiding this comment

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

Note that on Mesos the sysctls are set by the isolator itself / if we need more tuning for the new setup, the new rules will need to be added in the Mesos isolator too

Copy link
Author

Choose a reason for hiding this comment

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

Ok interesting, so that part might be unnecessary, my need is to disable rp_filter on interfaces created within the container netns. If this is doable with the Mesos isolator, I could drop that part in my changes (would simplify a lot as we won't need to call the tuning plugin).

That being said, we must think of how is it configured for Kubernetes, so it might still be relevant to have this feature ?

Copy link

Choose a reason for hiding this comment

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

For Mesos, you we have some config in the policy if you can apply those configs for all kind of nodes (using CNI or not). If it should be applied only on nodes using the CNI plugin, you'll need to use our isolator path so that all remaining tunings for CNI nodes are at the same place.
For Kube, we only have tunings per clusters in Chef policy; we could put those tunings there if it should be applied on all nodes. We don't have per node logic for now but that could easily be done in chef too. We also don't know yet which CNI we will run to replace Flannel (maybe Cilium), so we can't say if we will use this part of the plugin or not, if the plugin would set those for us, etc....

As it's written already, I'm tempted to keep it, but it might also just never be used and it might be better to not spread this kind of config in several places too...

Maybe we can move this part in a separate PR to be merged later as we need it ? wdyt ?

Copy link
Author

Choose a reason for hiding this comment

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

OK thanks for the explanation, I think these settings could be applied to all nodes (CNI or not).
I'll post that part in another PR then.

@fdomain
Copy link
Author

fdomain commented May 22, 2023

I shared the PR to my team as well

* the sysctl feature might be unnecessary as we could configure sysctl
  parameters on the host itself, or via the mesos isolator
* we'll repropose that feature again if this is needed
@fdomain fdomain changed the title Add features: host netns + route source interface + sysctl Add features: host netns + route source interface May 23, 2023
@Lqp1 Lqp1 merged commit 3ca9d02 into criteo-forks:v1.3.0 May 29, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants