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

make iptables timeout configurable #75

Merged
merged 1 commit into from
Jan 5, 2021
Merged

make iptables timeout configurable #75

merged 1 commit into from
Jan 5, 2021

Conversation

aojea
Copy link
Contributor

@aojea aojea commented Jul 8, 2020

if iptables version supports wait, we use -w without any additional
argument, so it keeps waiting forever trying to acquire the lock
at a 1 second interval rate.

This can cause issues on busy environments by software that depends
on this library, because they can be waiting forever.

We can make the timeout configurable, just only for iptables versions
that have the --wait flag.

Signed-off-by: Antonio Ojea [email protected]

@aojea
Copy link
Contributor Author

aojea commented Jul 8, 2020

/cc @dcbw @danwinship

@dcbw
Copy link
Contributor

dcbw commented Jul 8, 2020

@aojea it seems like this should be configurable rather than hardcoded... maybe for now the IPTables object gets a "timeout" member that callers can set if they want to, but by default is 0 meaning "wait forever". That would preserve existing API.

@aojea aojea changed the title don't wait forever for the iptables lock make iptables timeout configurable Jul 8, 2020
@aojea aojea force-pushed the wait branch 2 times, most recently from 958abd0 to 37774f2 Compare July 8, 2020 16:09
iptables/iptables.go Outdated Show resolved Hide resolved

// New creates a new IPTables.
// For backwards compatibility, by default always uses IPv4, i.e. "iptables".
func New(opts ...option) (*IPTables, error) {

Choose a reason for hiding this comment

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

Technically this could still be a breaking change if someone was depending on the type of New to pass it, but that seems unlikely, and it would be relatively easy to fix I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want to support all the options in iptables in the long term, wait interval comes to mind, I think is the more sustainable way.

I didn't like to add a NewWithProtocolTimeout() constructor, but I can change this to NewWithOptions() though

Choose a reason for hiding this comment

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

to be clear this LGTM to me already, but I'm not sure how this repo is maintained

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, I have merge rights, and do releases periodically. Anyways.

Can you update the docblock here to show how options work?

Use functional options to create a new iptables object, so we can
support new options to configure it.

It allows to configure the IP family and the timeout used in the
`-w` flag. Until now, if iptables version supports wait,
we used `-w` without any timeout, so it keeps waiting forever trying
to acquire the lock at a 1 second interval rate.
This can cause issues on busy environments by software that depends
on this library, because they can be waiting forever.

Signed-off-by: Antonio Ojea <[email protected]>
@squeed squeed merged commit abea47c into coreos:master Jan 5, 2021
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.

4 participants