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

Install iptables in cloud-api-adaptor daemonset #2016

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

yoheiueda
Copy link
Member

This PR is a prerequisite to fix #2015

To run the iptables command within the cloud-api-adaptor container, we need a wrapper command for iptables, and volumes for accessing a lock file and kernel modules in the host side.

https://github.com/kubernetes-sigs/iptables-wrappers

Network-related projects such as kube-proxy and flannel use this wrapper to manipulate netfilter on worker nodes.

Another PR that actually fixes the issue using iptables will follow after this PR gets merged.

ARG TARGETARCH

WORKDIR /iptables
RUN --mount=type=bind,target=/versions.yaml,source=cloud-api-adaptor/versions.yaml,readonly \
Copy link
Member

Choose a reason for hiding this comment

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

This format is new to me. Do you have any reference on using --mount with RUN?
Is it docker specific or can work with podman also ?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can specify cache mounts and bind mounts to a RUN instruction in a Dockerfile.

https://docs.docker.com/build/guide/mounts/

Podman supports the feature as well.

https://github.com/containers/common/blob/10ced654d57245a78d1b2c3746c5fd99209370bd/docs/Containerfile.5.md?plain=1#L102

Install the iptables command in the cloud-api-adaptor container
image. It will be used to configure netfilter in a worker node for
peer pod networking.  Running the iptables command within a container
requires a wrapper for it.

https://github.com/kubernetes-sigs/iptables-wrappers

Signed-off-by: Yohei Ueda <yohei@jp.ibm.com>
This patch updates the manifest of the cloud-api-adaptor
daemonset to run the iptables command it. The cloud-api-adaptor
daemonset runs in host network mode, and iptables can manage
netfilter of the network namespace of the worker node. The iptables
command needs to access a lock file and kernel modules in the host,
and this patch adds volume mounts for them.

Signed-off-by: Yohei Ueda <yohei@jp.ibm.com>
Copy link
Member

@bpradipt bpradipt left a comment

Choose a reason for hiding this comment

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

/lgtm

@@ -25,6 +25,7 @@ tools:
protoc: 3.15.0
packer: v1.9.4
kcli: 99.0.202407031308
iptables-wrapper: v0.0.0-20240819165702-06cad2ec6cb5
Copy link
Member

Choose a reason for hiding this comment

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

Just to double check by understanding - iptable-wrapper is a tool as it isn't itself installed into the container image, but is just used to install iptables?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

So does that mean it shouldn't go into the tools section, but should be in an external, or dependencies section in version.yaml? Or am I over-thinking this?

Copy link
Member

Choose a reason for hiding this comment

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

Since iptables-wrapper is a tool to manage iptables program, logically it makes sense to be in the tools section. Similar to golang, rust etc..

Copy link
Member

Choose a reason for hiding this comment

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

But I have no preference. If any other section makes more sense that's fine with me as well

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevenhorsman @bpradipt

I have no preference either.

Another possible option is to put it in the git section, and build iptables-wrapper from a cloned git repo. Currently, the builder image does not include the git command, so we need to install it for this option.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Let not overthink it for now and block this work, but I think we go ahead with it in tools and can always update that later if we come up with a different section for this sort of thing. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevenhorsman Thank you. I agree.

I keep it in tools for now, and will update that it if necessary.

BTW, the current code to build the command using go install looks somewhat complicated due to cross compilation.

There is a discussion ongoing golang/go#44469, and we will update the code when a simpler option is available.

Copy link
Contributor

@snir911 snir911 left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@stevenhorsman
Copy link
Member

I think that given we have two approvals and are satisfied with the versions.yaml updates then we can merge this. Thanks @yoheiueda!

@stevenhorsman stevenhorsman merged commit 273199d into confidential-containers:main Aug 29, 2024
31 checks passed
@yoheiueda yoheiueda deleted the iptables branch August 29, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection tracking of VXLAN UDP packets must be disabled
4 participants