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

Extract iptables binary staging to iptablesutils #5095

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

juanluisvaladas
Copy link
Contributor

Description

CPLB potentially will need to create some iptables rules for its userspace proxy, hence this code should be in iptablesutils rather than pkg/worker/kubelet.
Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

internal/pkg/iptablesutils/iptables.go Outdated Show resolved Hide resolved
// ExtractIPTablesBinaries extracts the iptables binaries from the k0s binary and makes the symlinks
// to the backend detected by DetectHostIPTablesMode.
// ExtractIPTablesBinaries only works on linux, if called in another OS it will return an error.
func ExtractIPTablesBinaries(k0sBinDir string, iptablesMode string) (error, string) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather add it as a separate "IPTablesSetup" (or sth. like that) component, than having it as a free-standing function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean adding it the same way we do with pkg/component/status and pkg/component/prober?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@twz123 friendly bump, I still need clarification on this

@juanluisvaladas
Copy link
Contributor Author

juanluisvaladas commented Nov 18, 2024

Hi Tom, I came back to this and I started doing but I realized kubelet needs both the iptables binaries (which isn't a problem) and an iptables mode (which is either nft or legacy).

If we move the iptables binary staging, to a new component we need to gather this information from the iptables component to the kubelet component. We can set this field in the iptables.Init phase and pass it to kubelet before calling componentManager.Start but that means that we either have to:

1- Add some code between the calls of componentManager.Init and componentManager.Start
2- Other option would be giving the kubelet component a pointer to the iptables but I'm not a big fan of that.
3- Don't make a new component.

Option 1 is probably the cleanest so I implemented it.. Do you have any preference?

@juanluisvaladas juanluisvaladas force-pushed the externalize-ipt branch 2 times, most recently from 9557c99 to 9f2ed63 Compare November 18, 2024 14:56
@juanluisvaladas juanluisvaladas marked this pull request as ready for review November 18, 2024 14:56
@juanluisvaladas juanluisvaladas requested review from a team as code owners November 18, 2024 14:56
@juanluisvaladas juanluisvaladas mentioned this pull request Nov 18, 2024
16 tasks
@twz123
Copy link
Member

twz123 commented Nov 18, 2024

If we move the iptables binary staging, to a new component we need to gather this information from the iptables component to the kubelet component.

After looking at the kubelet component, I think the only reason the component needs to know the iptables mode is because it currently extracts the iptables binaries. So if we put that in its own component, I don't see any reason why the kubelet component would need to know about the iptables mode anymore. I'd do something along those lines:

type IPTablesSetup struct {
	IPTablesMode string
	// ...
}

func (i *IPTablesSetup) Init(context.Context) error  { /* Gather iptables mode, extract binaries */ }
func (i *IPTablesSetup) Start(context.Context) error { /* No-op, probably? */ }
func (i *IPTablesSetup) Stop() error                 { /* No-op, probably? */ }

The IPTablesMode in the kubelet component can then be dropped.

1- Add some code between the calls of componentManager.Init and componentManager.Start
2- Other option would be giving the kubelet component a pointer to the iptables but I'm not a big fan of that.
3- Don't make a new component.

Option 1 is probably the cleanest so I implemented it.. Do you have any preference?

Fortunately, as far as I can see, we won't need any of the above here. However, there's already precedence for the second approach in k0s.

CPLB potentially will need to create some iptables rules for its
userspace proxy, hence this code should be in iptablesutils rather than
pkg/worker/kubelet.

Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <[email protected]>
Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <[email protected]>
@juanluisvaladas
Copy link
Contributor Author

You're right, fixed.

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.

2 participants