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

Cleanup left-over iptables rules from kubeproxy and cilium #788

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

berkayoz
Copy link
Member

cilium-dbg seems to clean up interfaces, bpf programs, network namespaces properly. Seems like we are leaking some iptables rules so added a cleanup step for left-over iptables rules from kubeproxy and cilium.

@berkayoz berkayoz requested a review from a team as a code owner November 12, 2024 11:00
@eaudetcobello
Copy link
Contributor

eaudetcobello commented Nov 12, 2024

Do you think it's useful to back up the original output in /var/snap before overwriting in case we mess up someone's iptables? Also, how sure are you about the assumption that every rule created by cilium or kubernetes contains the keywords you defined?

Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Great work! Thanks a lot Berkay! Overall LGTM, just some minor comments and questions.
+1 on the backup suggestion by Etienne.

src/k8s/pkg/k8sd/features/cilium/cleanup.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/features/cilium/cleanup.go Show resolved Hide resolved
for i, line := range lines {
for _, word := range []string{"cilium", "kube", "CILIUM", "KUBE"} {
if strings.Contains(line, word) {
lines[i] = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit worried that this might mess with any other rule that might be there in the iptables but we don't want to remove. What if we have a rule with "kubernetes" in it but not related to cilium? Is this scenario even possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

I've adjusted to let kube-proxy cleanup necessary rules. The x-cleanup command is only called on removal, so we would not be nuking things while they are in use. For affecting other rules, this would be the case if there are multiple "k8s"s running on the same node which is unsupported.

As for backing up the initial state, I believe this is not feasible/practical. And we can't save it to any snap location since this command is called on snap remove. I've minimized the risks, if something goes wrong a restart can always fix things as we are not persisting the rules.

Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Thanks a lot @berkayoz! LGTM!

@berkayoz berkayoz merged commit a8e140b into main Nov 15, 2024
16 of 18 checks passed
@berkayoz berkayoz deleted the KU-2029/cilium-cleanup branch November 15, 2024 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants