iptables: refactor to avoid global variables #47644
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
howardjohn
added
the
release-notes-none
istio-testing
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
istio-testing
added
the
size/XL
howardjohn
force-pushed
the
iptables/better-flags
branch
from
c79c15b to
062a322
Compare
istio-testing
removed
the
do-not-merge/work-in-progress
Currently, we have a complete mess of configuration for the iptables commands. This is driven a few issues: * It was designed to be *byte for byte* compatible with the original Bash script years ago, so it has a ton of weird bash-isms. * It is designed exclusively as a command line, but now its used beyond that (in CNI, etc) * Uses lots of globals, and each config item ends up in 3-4 places. This PR refactors this to simplify the configuration surface. The actual goal of this is to make it safe to run the iptables setup multiple times in a long running fashion; due to global variable usage this was extremely hard to do so safely previously. As part of this, a new package `pkg/flag` is introduced. This replaces our minimal usage of viper with ~3 lines of code to allow setting a flag's default value from an environment variable. There should be no behavioral differences, except some of the `--help` output is more clear (defaults shown)
howardjohn
force-pushed
the
iptables/better-flags
branch
from
062a322 to
d798c4f
Compare
| DefaultIptablesProbePort = "15002" | ||
| DefaultProbeTimeout = 5 * time.Second | ||
| DefaultIptablesProbePort = "15002" | ||
| DefaultIptablesProbePortUint = 15002 |
There was a problem hiding this comment.
Any reason to not use strconv.ParseUint instead of having 2 constants?
hzxuzhonghu
approved these changes
Nov 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we have a complete mess of configuration for the iptables commands. This is driven a few issues:
This PR refactors this to simplify the configuration surface. The actual goal of this is to make it safe to run the iptables setup multiple times in a long running fashion; due to global variable usage this was extremely hard to do so safely previously.
As part of this, a new package
pkg/flagis introduced. This replaces our minimal usage of viper with ~3 lines of code to allow setting a flag's default value from an environment variable.There should be no behavioral differences, except some of the
--helpoutput is more clear (defaults shown)Please provide a description of this PR: