From 95198dba7a0a30e684e3dfbfc1b32221b6cd8ab9 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Fri, 12 Jul 2024 10:07:29 +0200 Subject: [PATCH 01/20] Remove the ability to change the iptables comments prefix The iptables comments prefix will be used during the uninstallation of the transparent proxy. Allowing this prefix to be configurable adds unnecessary complexity, as it would require an additional flag during the uninstallation process. This could potentially lead to issues if the prefix is changed during installation but not specified during uninstallation. Removing this configurability simplifies the process and reduces the risk of errors. Signed-off-by: Bart Smykla --- app/kumactl/cmd/completion/testdata/bash.golden | 4 ---- .../cmd/install/install_transparent_proxy.go | 1 - pkg/transparentproxy/config/config.go | 14 ++++---------- 3 files changed, 4 insertions(+), 15 deletions(-) diff --git a/app/kumactl/cmd/completion/testdata/bash.golden b/app/kumactl/cmd/completion/testdata/bash.golden index 2fbe96fe57ce..e077851a1b08 100644 --- a/app/kumactl/cmd/completion/testdata/bash.golden +++ b/app/kumactl/cmd/completion/testdata/bash.golden @@ -5802,10 +5802,6 @@ _kumactl_install_transparent-proxy() flags_with_completion=() flags_completion=() - flags+=("--comments-prefix=") - two_word_flags+=("--comments-prefix") - local_nonpersistent_flags+=("--comments-prefix") - local_nonpersistent_flags+=("--comments-prefix=") flags+=("--disable-comments") local_nonpersistent_flags+=("--disable-comments") flags+=("--drop-invalid-packets") diff --git a/app/kumactl/cmd/install/install_transparent_proxy.go b/app/kumactl/cmd/install/install_transparent_proxy.go index 5928869c74f3..ed51ab32932b 100644 --- a/app/kumactl/cmd/install/install_transparent_proxy.go +++ b/app/kumactl/cmd/install/install_transparent_proxy.go @@ -230,7 +230,6 @@ runuser -u kuma-dp -- \ cmd.Flags().BoolVar(&cfg.Log.Enabled, "iptables-logs", cfg.Log.Enabled, "enable logs for iptables rules using the LOG chain. This option activates kernel logging for packets matching the rules, where details about the IP/IPv6 headers are logged. This information can be accessed via dmesg(1) or syslog.") cmd.Flags().BoolVar(&cfg.Comment.Disabled, "disable-comments", cfg.Comment.Disabled, "Disable the addition of comments to iptables rules") - cmd.Flags().StringVar(&cfg.Comment.Prefix, "comments-prefix", cfg.Comment.Prefix, "Prefix for comments added to iptables rules") cmd.Flags().StringArrayVar(&cfg.Redirect.Inbound.ExcludePortsForIPs, "exclude-inbound-ips", []string{}, "specify IP addresses (IPv4 or IPv6, with or without CIDR notation) to be excluded from transparent proxy inbound redirection. Examples: '10.0.0.1', '192.168.0.0/24', 'fe80::1', 'fd00::/8'. This flag can be specified multiple times or with multiple addresses separated by commas to exclude multiple IP addresses or ranges.") cmd.Flags().StringArrayVar(&cfg.Redirect.Outbound.ExcludePortsForIPs, "exclude-outbound-ips", []string{}, "specify IP addresses (IPv4 or IPv6, with or without CIDR notation) to be excluded from transparent proxy outbound redirection. Examples: '10.0.0.1', '192.168.0.0/24', 'fe80::1', 'fd00::/8'. This flag can be specified multiple times or with multiple addresses separated by commas to exclude multiple IP addresses or ranges.") diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index f6e7fdb88d80..ec8b7bc3722f 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -381,14 +381,9 @@ type RetryConfig struct { } // Comment struct contains the configuration for iptables rule comments. -// It includes options to enable or disable comments and a prefix to use -// for comment text. +// It includes an option to enable or disable comments. type Comment struct { Disabled bool - // Prefix defines the prefix to be used for comments on iptables rules, - // aiding in identifying and organizing rules created by the transparent - // proxy. - Prefix string } // InitializedComment struct contains the processed configuration for iptables @@ -415,12 +410,12 @@ type InitializedComment struct { // // Returns: // - InitializedComment: The struct containing the processed comment -// configuration, indicating whether comments are enabled and the prefix to -// use for comments. +// configuration, indicating whether comments are enabled and the prefix +// to use for comments ("kuma/mesh/transparent/proxy"). func (c Comment) Initialize(e InitializedExecutablesIPvX) InitializedComment { return InitializedComment{ Enabled: !c.Disabled && e.Functionality.Modules.Comment, - Prefix: c.Prefix, + Prefix: IptablesRuleCommentPrefix, } } @@ -708,7 +703,6 @@ func DefaultConfig() Config { Executables: NewExecutablesNftLegacy(), Comment: Comment{ Disabled: false, - Prefix: IptablesRuleCommentPrefix, }, } } From a362a05ad586c83392ca46ae4511d55e0ec54721 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Fri, 12 Jul 2024 10:49:55 +0200 Subject: [PATCH 02/20] Refactor iptables-restore logic and add `RestoreWithFlush` method This commit refactors the logic for executing the iptables-restore command, introducing a new internal `restore` function to handle common restore logic. This change simplifies the external interface and adds flexibility for different restore scenarios. Key changes: - Refactored restore logic: Moved the common logic for executing the iptables-restore command into a new `restore` function. This function handles writing rules to a temporary file, executing the restore command, and retrying on failure. - New `Restore` method: The `Restore` method now wraps the `restore` function with the `--noflush` flag, ensuring that the current rules are not flushed before restoring. - New `RestoreWithFlush` method: Introduced the `RestoreWithFlush` method to allow restoring rules while flushing the current rules. This method is intended for use in the transparent proxy uninstallation process, ensuring a clean state before removal. - Improved argument handling: The `restore` function now accepts additional arguments, enabling more flexible command execution. These changes improve the maintainability and readability of the code, making it easier to manage different iptables-restore scenarios and enhancing the robustness of the command execution logic. Signed-off-by: Bart Smykla --- .../config/config_executables.go | 106 +++++++++++++----- 1 file changed, 80 insertions(+), 26 deletions(-) diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index f7baf33d1b04..f943d057c379 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -231,9 +231,24 @@ func (c InitializedExecutablesIPvX) writeRulesToFile(rules string) (string, erro return f.Name(), nil } -func (c InitializedExecutablesIPvX) Restore( +// restore executes the iptables-restore command with the given rules and +// additional arguments. It writes the rules to a temporary file and tries +// to restore the iptables rules from this file. If the command fails, it +// retries the specified number of times. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - rules (string): The iptables rules to restore. +// - args (...string): Additional arguments for the iptables-restore command. +// +// Returns: +// - string: The standard output from the iptables-restore command if +// successful. +// - error: An error if the iptables-restore command fails after all retries. +func (c InitializedExecutablesIPvX) restore( ctx context.Context, rules string, + args ...string, ) (string, error) { fileName, err := c.writeRulesToFile(rules) if err != nil { @@ -241,16 +256,19 @@ func (c InitializedExecutablesIPvX) Restore( } defer os.Remove(fileName) + args = append(args, fileName) + + argsAll := slices.DeleteFunc( + slices.Concat(c.IptablesRestore.args, args), + func(s string) bool { return s == "" }, + ) + for i := 0; i <= c.retry.MaxRetries; i++ { c.logger.try = i + 1 - c.logger.InfoTry( - c.IptablesRestore.Path, - strings.Join(c.IptablesRestore.args, " "), - fileName, - ) + c.logger.InfoTry(c.IptablesRestore.Path, strings.Join(argsAll, " ")) - stdout, _, err := c.IptablesRestore.Exec(ctx, fileName) + stdout, _, err := c.IptablesRestore.Exec(ctx, args...) if err == nil { return stdout.String(), nil } @@ -266,6 +284,45 @@ func (c InitializedExecutablesIPvX) Restore( return "", errors.Errorf("%s failed", c.IptablesRestore.Path) } +// Restore executes the iptables-restore command with the given rules and the +// --noflush flag to ensure that the current rules are not flushed before +// restoring. This function is a wrapper around the restore function with the +// --noflush flag. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - rules (string): The iptables rules to restore. +// +// Returns: +// - string: The standard output from the iptables-restore command if +// successful. +// - error: An error if the iptables-restore command fails after all retries. +func (c InitializedExecutablesIPvX) Restore( + ctx context.Context, + rules string, +) (string, error) { + return c.restore(ctx, rules, FlagNoFlush) +} + +// RestoreWithFlush executes the iptables-restore command with the given rules, +// allowing the current rules to be flushed before restoring. This function is +// a wrapper around the restore function without the --noflush flag. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - rules (string): The iptables rules to restore. +// +// Returns: +// - string: The standard output from the iptables-restore command if +// successful. +// - error: An error if the iptables-restore command fails after all retries. +func (c InitializedExecutablesIPvX) RestoreWithFlush( + ctx context.Context, + rules string, +) (string, error) { + return c.restore(ctx, rules) +} + type Executables struct { IPv4 ExecutablesIPvX IPv6 ExecutablesIPvX @@ -417,37 +474,34 @@ func (c ExecutablesNftLegacy) Initialize( } // buildRestoreArgs constructs a slice of flags for restoring iptables rules -// based on the provided wait time, wait interval, and iptables mode. +// based on the provided configuration and iptables mode. // // This function generates a list of command-line flags to be used with // iptables-restore, tailored to the given parameters: -// - By default, it includes the `--noflush` flag to prevent flushing of -// existing rules. -// - If the iptables mode is not legacy, it returns only the `--noflush` flag. +// - For non-legacy iptables mode, it returns an empty slice, as no additional +// flags are required. // - For legacy mode, it conditionally adds the `--wait` and `--wait-interval` -// flags based on the provided wait time and waitInterval. +// flags based on the provided configuration values. // // Args: -// -// wait (uint): The wait time in seconds for iptables-restore to wait for the -// xtables lock before aborting. -// interval (uint): The wait interval in seconds between attempts to acquire -// the xtables lock. -// mode (IptablesMode): The mode of iptables in use, determining the -// applicability of the wait flags. +// - cfg (Config): The configuration struct containing wait times for +// iptables-restore. +// - mode (consts.IptablesMode): The mode of iptables in use, determining the +// applicability of the wait flags. // // Returns: -// -// []string: A slice of strings representing the constructed flags for -// iptables-restore. +// - []string: A slice of strings representing the constructed flags for +// iptables-restore. // // Example: // -// buildRestoreArgs(5, 2, IptablesModeNft) # Returns: []string{"--noflush"} -// buildRestoreArgs(5, 2, IptablesModeLegacy) -// # Returns: []string{"--noflush", "--wait=5", "--wait-interval=2"} +// buildRestoreArgs(Config{Wait: 5, WaitInterval: 2}, IptablesModeNft) +// # Returns: []string{} +// +// buildRestoreArgs(Config{Wait: 5, WaitInterval: 2}, IptablesModeLegacy) +// # Returns: []string{"--wait=5", "--wait-interval=2"} func buildRestoreArgs(cfg Config, mode IptablesMode) []string { - flags := []string{FlagNoFlush} + var flags []string if mode != IptablesModeLegacy { return flags From 6e3300eea301faf50a16e228aa25c8d7f9686c40 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Fri, 12 Jul 2024 11:04:07 +0200 Subject: [PATCH 03/20] Add `RestoreTest` method to `InitializedExecutablesIPvX` This method runs iptables-restore with the `--test` flag to validate iptables rules without applying them. It helps in ensuring that the rules are correct and can be applied without errors. Signed-off-by: Bart Smykla --- .../config/config_executables.go | 24 +++++++++++++++++++ .../iptables/consts/consts.go | 1 + 2 files changed, 25 insertions(+) diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index f943d057c379..97f6aa402cca 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -323,6 +323,30 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( return c.restore(ctx, rules) } +// RestoreTest runs iptables-restore with the --test flag to validate the +// iptables rules without applying them. +// +// This function calls the internal `restore` method with the --test flag to +// ensure that the iptables rules specified in the `rules` string are valid. If +// the rules are valid, it returns the output from iptables-restore. If there is +// an error, it returns the error message. +// +// Args: +// - ctx (context.Context): The context for controlling the execution of the +// iptables-restore command. +// - rules (string): The iptables rules to be tested. +// +// Returns: +// - string: The output from iptables-restore if the rules are valid. +// - error: An error message if the rules are invalid or if there is an issue +// running iptables-restore. +func (c InitializedExecutablesIPvX) RestoreTest( + ctx context.Context, + rules string, +) (string, error) { + return c.restore(ctx, rules, FlagTest) +} + type Executables struct { IPv4 ExecutablesIPvX IPv6 ExecutablesIPvX diff --git a/pkg/transparentproxy/iptables/consts/consts.go b/pkg/transparentproxy/iptables/consts/consts.go index 0467e4d83d3a..54a67a95d16d 100644 --- a/pkg/transparentproxy/iptables/consts/consts.go +++ b/pkg/transparentproxy/iptables/consts/consts.go @@ -113,6 +113,7 @@ const ( FlagWait = "--wait" FlagWaitInterval = "--wait-interval" FlagNoFlush = "--noflush" + FlagTest = "--test" ) // FlagVariationsMap maps a flag name (e.g., "-t") to a map containing its long From 8d937caff625c738bc051f3af4de296115df8ac5 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 07:42:14 +0200 Subject: [PATCH 04/20] Add new logger methods `Errorf` and `Infof` This commit introduces two new methods, `Errorf` and `Infof`, to the `Logger` struct. These methods allow for formatted logging of error and information messages, respectively, improving flexibility and readability in log output. Additionally, all relevant comments and descriptions have been updated to reflect these changes. Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config_logger.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/transparentproxy/config/config_logger.go b/pkg/transparentproxy/config/config_logger.go index 3e1321f9b5f9..b07ebee184b2 100644 --- a/pkg/transparentproxy/config/config_logger.go +++ b/pkg/transparentproxy/config/config_logger.go @@ -44,6 +44,15 @@ func (l Logger) Info(a ...any) { loglnWithHashPrefix(l.stdout, a) } +// Infof logs formatted messages to stdout, prefixed with a hash. +// +// Args: +// - format (string): The format string for the log message. +// - a (...any): The arguments to format and log to stdout. +func (l Logger) Infof(format string, a ...any) { + l.Info(fmt.Sprintf(format, a...)) +} + // InfoWithoutPrefix logs messages to stdout without any prefix. func (l Logger) InfoWithoutPrefix(a ...any) { logln(l.stdout, a) @@ -70,6 +79,15 @@ func (l Logger) Error(a ...any) { loglnWithHashPrefix(l.stderr, a) } +// Errorf logs formatted error messages to stderr, prefixed with a hash. +// +// Args: +// - format (string): The format string for the error message. +// - a (...any): The arguments to format and log as an error to stderr. +func (l Logger) Errorf(format string, a ...any) { + l.Error(fmt.Sprintf(format, a...)) +} + // ErrorTry logs error messages with retry context to stderr, transforming // multi-line error messages into a single line. func (l Logger) ErrorTry(err error, a ...any) { From 073777b1cb034b7b315c10777789ac2c4bcf8736 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 07:43:48 +0200 Subject: [PATCH 05/20] Enhance Descriptions for Logger Methods This commit improves the comments and descriptions for the logger methods within the `Logger` struct. The updated documentation provides clearer explanations of each method's purpose and functionality, enhancing readability and maintainability of the codebase. Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config_logger.go | 39 +++++++++++++++++++- 1 file changed, 37 insertions(+), 2 deletions(-) diff --git a/pkg/transparentproxy/config/config_logger.go b/pkg/transparentproxy/config/config_logger.go index b07ebee184b2..68bc0186221b 100644 --- a/pkg/transparentproxy/config/config_logger.go +++ b/pkg/transparentproxy/config/config_logger.go @@ -8,12 +8,20 @@ import ( // logln writes a line of output to the specified writer. It directly passes the // provided arguments to the writer without any formatting or prefix. +// +// Args: +// - w (io.Writer): The writer where the line will be written. +// - a ([]any): The arguments to write to the writer. func logln(w io.Writer, a []any) { fmt.Fprintln(w, a...) } // loglnWithHashPrefix writes a formatted line to the specified writer, // prefixing it with a hash (#) to denote the line as a comment. +// +// Args: +// - w (io.Writer): The writer where the line will be written. +// - a ([]any): The arguments to write to the writer, prefixed with a hash. func loglnWithHashPrefix(w io.Writer, a []any) { logln(w, append([]any{"#"}, a...)) } @@ -31,6 +39,10 @@ type Logger struct { // tryPrefix generates a logging prefix based on the current retry attempt. // It returns a formatted string showing the current attempt and the total // attempts, or nil if no retries are needed (maxTry <= 1). +// +// Returns: +// - []any: The formatted prefix for the current retry attempt, or nil if no +// retries. func (l Logger) tryPrefix() []any { if l.maxTry <= 1 { return nil @@ -40,6 +52,9 @@ func (l Logger) tryPrefix() []any { } // Info logs standard messages to stdout, prefixed with a hash. +// +// Args: +// - a (...any): The arguments to log to stdout. func (l Logger) Info(a ...any) { loglnWithHashPrefix(l.stdout, a) } @@ -54,27 +69,43 @@ func (l Logger) Infof(format string, a ...any) { } // InfoWithoutPrefix logs messages to stdout without any prefix. +// +// Args: +// - a (...any): The arguments to log to stdout. func (l Logger) InfoWithoutPrefix(a ...any) { logln(l.stdout, a) } // InfoTry logs messages to stdout with retry prefix, showing the attempt number // if retries are configured. +// +// Args: +// - a (...any): The arguments to log to stdout with retry context. func (l Logger) InfoTry(a ...any) { l.Info(append(l.tryPrefix(), a...)...) } -// Warn logs warning messages to stderr, prefixed with "[WARNING]:" +// Warn logs warning messages to stderr, prefixed with "[WARNING]:". +// +// Args: +// - a (...any): The arguments to log as a warning to stderr. func (l Logger) Warn(a ...any) { loglnWithHashPrefix(l.stderr, append([]any{"[WARNING]:"}, a...)) } -// Warn logs warning messages to stderr, prefixed with "[WARNING]:" +// Warnf logs formatted warning messages to stderr, prefixed with "[WARNING]:". +// +// Args: +// - format (string): The format string for the warning message. +// - a (...any): The arguments to format and log as a warning to stderr. func (l Logger) Warnf(format string, a ...any) { l.Warn(fmt.Sprintf(format, a...)) } // Error logs error messages to stderr, prefixed with a hash. +// +// Args: +// - a (...any): The arguments to log as an error to stderr. func (l Logger) Error(a ...any) { loglnWithHashPrefix(l.stderr, a) } @@ -90,6 +121,10 @@ func (l Logger) Errorf(format string, a ...any) { // ErrorTry logs error messages with retry context to stderr, transforming // multi-line error messages into a single line. +// +// Args: +// - err (error): The error to log. +// - a (...any): Additional context to log with the error message. func (l Logger) ErrorTry(err error, a ...any) { result := append(l.tryPrefix(), a...) // Convert multi-line errors to a single line for clarity in logs. From 595c3b71ee8f872447dfe3e41bf51115711396bc Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 07:47:36 +0200 Subject: [PATCH 06/20] Add `IPv6` Field to `InitializedConfigIPvX` struct This commit introduces a new field, `IPv6`, to the `InitializedConfigIPvX` struct. This boolean field is used to indicate whether the current configuration refers to IPv6. It enhances the configuration structure by providing a clear and straightforward way to handle IPv6-specific settings and logic. Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index ec8b7bc3722f..1290922d3c91 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -533,6 +533,10 @@ type InitializedConfigIPvX struct { // text. This helps in identifying and organizing iptables rules created by // the transparent proxy, making them easier to manage and debug. Comment InitializedComment + // IPv6 marks whether the current configuration refers to an IPv6 setup. + // This boolean field helps determine the appropriate actions and settings + // needed for handling IPv6-specific iptables rules and configurations. + IPv6 bool enabled bool } @@ -627,6 +631,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { InboundPassthroughCIDR: InboundPassthroughSourceAddressCIDRIPv6, Comment: c.Comment.Initialize(e.IPv6), DropInvalidPackets: c.DropInvalidPackets && e.IPv6.Functionality.Tables.Mangle, + IPv6: true, enabled: true, } From 022e3a6b21e7a5c659d354f6066fa403f3010cd9 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 07:48:45 +0200 Subject: [PATCH 07/20] Refactor Executables Initialization for Better Error Handling and Logging This commit introduces significant refactoring to the `config` package, focusing on improving the management of iptables executables and enhancing error handling and logging. The key improvements and changes include: 1. Method Refactoring: - Split the `writeRulesToFile` method into separate functions, `createTempFile` and `writeToFile`. - Added a new function, `createBackupFile`, for better modularity and reusability. 2. Restore Functionality Enhancements: - Refactored the `restore`, `Restore`, `RestoreWithFlush`, and `RestoreTest` methods to handle iptables rule restoration and validation more effectively, incorporating improved error handling and logging. - Implemented robust backup and restore mechanisms in `RestoreWithFlush` to ensure that current iptables rules are safely backed up before applying new rules. 3. Documentation Improvements: - Added detailed comments and descriptions for several functions to enhance code readability and maintainability, making it easier for future developers to understand and extend the functionality. Signed-off-by: Bart Smykla --- .../config/config_executables.go | 276 +++++++++++++----- 1 file changed, 197 insertions(+), 79 deletions(-) diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index 97f6aa402cca..e5820eceb871 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -163,74 +163,6 @@ type InitializedExecutablesIPvX struct { logger Logger } -// writeRulesToFile writes the provided iptables rules to a temporary file and -// returns the file path. -// -// This method performs the following steps: -// 1. Creates a temporary file with a name based on the -// `IptablesRestore.prefix` field of the `InitializedExecutablesIPvX` -// struct. -// 2. Logs the contents that will be written to the file. -// 3. Writes the rules to the file using a buffered writer for efficiency. -// 4. Flushes the buffer to ensure all data is written to the file. -// 5. Returns the file path if successful, or an error if any step fails. -// -// Args: -// -// rules (string): The iptables rules to be written to the temporary file. -// -// Returns: -// -// string: The path to the temporary file containing the iptables rules. -// error: An error if the file creation, writing, or flushing fails. -func (c InitializedExecutablesIPvX) writeRulesToFile(rules string) (string, error) { - // Create a temporary file with a name template based on the IptablesRestore - // prefix. - nameTemplate := fmt.Sprintf("%s-rules.*.txt", c.IptablesRestore.prefix) - f, err := os.CreateTemp("", nameTemplate) - if err != nil { - return "", errors.Wrapf( - err, - "failed to create temporary file: %s", - nameTemplate, - ) - } - defer f.Close() - - // Remove the temporary file if an error occurs after creation. - defer func() { - if err != nil { - os.Remove(f.Name()) - } - }() - - // Log the file name and the rules to be written. - c.logger.Info("writing the following rules to file:", f.Name()) - c.logger.InfoWithoutPrefix(strings.TrimSpace(rules)) - - // Write the rules to the file using a buffered writer. - writer := bufio.NewWriter(f) - if _, err = writer.WriteString(rules); err != nil { - return "", errors.Wrapf( - err, - "failed to write rules to the temporary file: %s", - f.Name(), - ) - } - - // Flush the buffer to ensure all data is written. - if err = writer.Flush(); err != nil { - return "", errors.Wrapf( - err, - "failed to flush the buffered writer for file: %s", - f.Name(), - ) - } - - // Return the file path. - return f.Name(), nil -} - // restore executes the iptables-restore command with the given rules and // additional arguments. It writes the rules to a temporary file and tries // to restore the iptables rules from this file. If the command fails, it @@ -247,16 +179,10 @@ func (c InitializedExecutablesIPvX) writeRulesToFile(rules string) (string, erro // - error: An error if the iptables-restore command fails after all retries. func (c InitializedExecutablesIPvX) restore( ctx context.Context, - rules string, + f *os.File, args ...string, ) (string, error) { - fileName, err := c.writeRulesToFile(rules) - if err != nil { - return "", err - } - defer os.Remove(fileName) - - args = append(args, fileName) + args = append(args, f.Name()) argsAll := slices.DeleteFunc( slices.Concat(c.IptablesRestore.args, args), @@ -301,7 +227,22 @@ func (c InitializedExecutablesIPvX) Restore( ctx context.Context, rules string, ) (string, error) { - return c.restore(ctx, rules, FlagNoFlush) + f, err := createTempFile(c.IptablesRestore.prefix) + if err != nil { + return "", err + } + defer f.Close() + defer os.Remove(f.Name()) + + // Log the file name and the rules to be written. + c.logger.Info("writing the following rules to file:", f.Name()) + c.logger.InfoWithoutPrefix(strings.TrimSpace(rules)) + + if err := writeToFile(rules, f); err != nil { + return "", err + } + + return c.restore(ctx, f, FlagNoFlush) } // RestoreWithFlush executes the iptables-restore command with the given rules, @@ -320,7 +261,68 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( ctx context.Context, rules string, ) (string, error) { - return c.restore(ctx, rules) + // Create a backup file for existing iptables rules. + backupFile, err := createBackupFile(c.IptablesRestore.prefix) + if err != nil { + return "", errors.Wrap( + err, + "failed to create backup file for iptables rules", + ) + } + defer backupFile.Close() + + // Save the current iptables rules to the backup file. + stdout, _, err := c.IptablesSave.Exec(ctx) + if err != nil { + return "", errors.Wrap( + err, + "failed to execute iptables-save command", + ) + } + + if err := writeToFile(stdout.String(), backupFile); err != nil { + return "", errors.Wrap( + err, + "failed to write current iptables rules to backup file", + ) + } + + // Create a temporary file for the new iptables rules. + restoreFile, err := createTempFile(c.IptablesRestore.prefix) + if err != nil { + return "", errors.Wrap( + err, + "failed to create temporary file for new iptables rules", + ) + } + defer restoreFile.Close() + defer os.Remove(restoreFile.Name()) + + // Write the new iptables rules to the temporary file. + if err := writeToFile(rules, restoreFile); err != nil { + return "", errors.Wrap( + err, + "failed to write new iptables rules to temporary file", + ) + } + + // Attempt to restore the new iptables rules from the temporary file. + output, err := c.restore(ctx, restoreFile) + if err != nil { + c.logger.Errorf("restoring backup file: %s", backupFile.Name()) + + if _, err := c.restore(ctx, backupFile); err != nil { + c.logger.Warnf("restoring backup failed: %s", err) + } + + return "", errors.Wrapf( + err, + "failed to restore rules from file: %s", + restoreFile.Name(), + ) + } + + return output, nil } // RestoreTest runs iptables-restore with the --test flag to validate the @@ -344,7 +346,23 @@ func (c InitializedExecutablesIPvX) RestoreTest( ctx context.Context, rules string, ) (string, error) { - return c.restore(ctx, rules, FlagTest) + f, err := createTempFile(c.IptablesRestore.prefix) + if err != nil { + return "", err + } + defer f.Close() + defer os.Remove(f.Name()) + + if err := writeToFile(rules, f); err != nil { + return "", err + } + + stdout, _, err := c.IptablesRestore.Exec(ctx, FlagTest, f.Name()) + if err != nil { + return "", errors.Wrap(err, "rules are invalid") + } + + return stdout.String(), nil } type Executables struct { @@ -776,3 +794,103 @@ func configureIPv6OutboundAddress() error { return nil } + +// createBackupFile generates a backup file with a specified prefix and a +// timestamp suffix. The file is created in the system's temporary directory and +// is used to store iptables rules for backup purposes. +// +// This function performs the following steps: +// 1. Generates a timestamp suffix in the format "YYYY-MM-DD-HHMMSS". +// 2. Constructs the backup file name using the provided prefix and the +// timestamp suffix. +// 3. Creates the backup file in the system's temporary directory. +// +// Args: +// - prefix (string): The prefix to be used in the backup file name. +// +// Returns: +// - *os.File: A pointer to the created backup file. +// - error: An error if the file creation fails. +func createBackupFile(prefix string) (*os.File, error) { + // Generate a timestamp suffix for the backup file name. + dateSuffix := time.Now().Format("2006-01-02-150405") + + // Construct the backup file name using the provided prefix and the + // timestamp suffix. + fileName := fmt.Sprintf("%s-rules.%s.txt.backup", prefix, dateSuffix) + filePath := filepath.Join(os.TempDir(), fileName) + + // Create the backup file in the system's temporary directory. + f, err := os.Create(filePath) + if err != nil { + return nil, errors.Wrapf( + err, + "failed to create backup file: %s", + filePath, + ) + } + + return f, nil +} + +// createTempFile generates a temporary file with a specified prefix. +// The file is created in the system's default temporary directory and is used +// for storing iptables rules temporarily. +// +// This function performs the following steps: +// 1. Constructs a template for the temporary file name using the provided +// prefix. +// 2. Creates the temporary file in the system's default temporary directory. +// +// Args: +// - prefix (string): The prefix to be used in the temporary file name. +// +// Returns: +// - *os.File: A pointer to the created temporary file. +// - error: An error if the file creation fails. +func createTempFile(prefix string) (*os.File, error) { + // Construct a template for the temporary file name using the provided prefix. + nameTemplate := fmt.Sprintf("%s-rules.*.txt", prefix) + + // Create the temporary file in the system's default temporary directory. + f, err := os.CreateTemp(os.TempDir(), nameTemplate) + if err != nil { + return nil, errors.Wrapf( + err, + "failed to create temporary file with template: %s", + nameTemplate, + ) + } + + return f, nil +} + +// writeToFile writes the provided content to the specified file using a +// buffered writer. It ensures that all data is written to the file by flushing +// the buffer. +// +// This function performs the following steps: +// 1. Writes the content to the file using a buffered writer for efficiency. +// 2. Flushes the buffer to ensure all data is written to the file. +// +// Args: +// - content (string): The content to be written to the file. +// - f (*os.File): The file to which the content should be written. +// +// Returns: +// - error: An error if writing or flushing the buffer fails, with detailed +// context. +func writeToFile(content string, f *os.File) error { + // Write the content to the file using a buffered writer. + writer := bufio.NewWriter(f) + if _, err := writer.WriteString(content); err != nil { + return errors.Wrapf(err, "failed to write to file: %s", f.Name()) + } + + // Flush the buffer to ensure all data is written. + return errors.Wrapf( + writer.Flush(), + "failed to flush the buffered writer for file: %s", + f.Name(), + ) +} From f0410f0b4783f4f7c520b8ce97cacdac65d09be0 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 08:16:08 +0200 Subject: [PATCH 08/20] Improve Log Message for Transparent Proxy Setup Completion Signed-off-by: Bart Smykla --- app/kumactl/cmd/install/install_transparent_proxy.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/kumactl/cmd/install/install_transparent_proxy.go b/app/kumactl/cmd/install/install_transparent_proxy.go index ed51ab32932b..da8de6603222 100644 --- a/app/kumactl/cmd/install/install_transparent_proxy.go +++ b/app/kumactl/cmd/install/install_transparent_proxy.go @@ -184,7 +184,9 @@ runuser -u kuma-dp -- \ } if !initializedConfig.DryRun { - initializedConfig.Logger.Info("Tansparent proxy set up successfully, you can now run kuma-dp using transparent-proxy.") + initializedConfig.Logger.Info( + "transparent proxy setup completed successfully. You can now run kuma-dp with the transparent-proxy feature enabled", + ) } return nil From c94365a3065e97be5e33f5a3176ee304b5bfe2be Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 08:25:29 +0200 Subject: [PATCH 09/20] Add quiet mode to restore methods for suppressing verbose logging This commit enhances the `restore`, `Restore`, and `RestoreWithFlush` methods by adding a `quiet` parameter to control verbose logging during the iptables restore process. This allows for optional suppression of detailed log messages, improving flexibility in logging behavior. Signed-off-by: Bart Smykla --- .../config/config_executables.go | 29 ++++++++++++++----- .../iptables/builder/builder.go | 2 ++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index e5820eceb871..cdd5d4c91360 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -170,7 +170,8 @@ type InitializedExecutablesIPvX struct { // // Args: // - ctx (context.Context): The context for command execution. -// - rules (string): The iptables rules to restore. +// - f (*os.File): The file containing the iptables rules to restore. +// - quiet (bool): If true, suppresses verbose logging of the restore process. // - args (...string): Additional arguments for the iptables-restore command. // // Returns: @@ -180,6 +181,7 @@ type InitializedExecutablesIPvX struct { func (c InitializedExecutablesIPvX) restore( ctx context.Context, f *os.File, + quiet bool, args ...string, ) (string, error) { args = append(args, f.Name()) @@ -192,7 +194,9 @@ func (c InitializedExecutablesIPvX) restore( for i := 0; i <= c.retry.MaxRetries; i++ { c.logger.try = i + 1 - c.logger.InfoTry(c.IptablesRestore.Path, strings.Join(argsAll, " ")) + if !quiet { + c.logger.InfoTry(c.IptablesRestore.Path, strings.Join(argsAll, " ")) + } stdout, _, err := c.IptablesRestore.Exec(ctx, args...) if err == nil { @@ -202,7 +206,10 @@ func (c InitializedExecutablesIPvX) restore( c.logger.ErrorTry(err, "restoring failed:") if i < c.retry.MaxRetries { - c.logger.InfoTry("will try again in", c.retry.SleepBetweenReties) + if !quiet { + c.logger.InfoTry("will try again in", c.retry.SleepBetweenReties) + } + time.Sleep(c.retry.SleepBetweenReties) } } @@ -218,6 +225,7 @@ func (c InitializedExecutablesIPvX) restore( // Args: // - ctx (context.Context): The context for command execution. // - rules (string): The iptables rules to restore. +// - quiet (bool): If true, suppresses verbose logging of the restore process. // // Returns: // - string: The standard output from the iptables-restore command if @@ -226,6 +234,7 @@ func (c InitializedExecutablesIPvX) restore( func (c InitializedExecutablesIPvX) Restore( ctx context.Context, rules string, + quiet bool, ) (string, error) { f, err := createTempFile(c.IptablesRestore.prefix) if err != nil { @@ -235,14 +244,16 @@ func (c InitializedExecutablesIPvX) Restore( defer os.Remove(f.Name()) // Log the file name and the rules to be written. - c.logger.Info("writing the following rules to file:", f.Name()) - c.logger.InfoWithoutPrefix(strings.TrimSpace(rules)) + if !quiet { + c.logger.Info("writing the following rules to file:", f.Name()) + c.logger.InfoWithoutPrefix(strings.TrimSpace(rules)) + } if err := writeToFile(rules, f); err != nil { return "", err } - return c.restore(ctx, f, FlagNoFlush) + return c.restore(ctx, f, quiet, FlagNoFlush) } // RestoreWithFlush executes the iptables-restore command with the given rules, @@ -252,6 +263,7 @@ func (c InitializedExecutablesIPvX) Restore( // Args: // - ctx (context.Context): The context for command execution. // - rules (string): The iptables rules to restore. +// - quiet (bool): If true, suppresses verbose logging of the restore process. // // Returns: // - string: The standard output from the iptables-restore command if @@ -260,6 +272,7 @@ func (c InitializedExecutablesIPvX) Restore( func (c InitializedExecutablesIPvX) RestoreWithFlush( ctx context.Context, rules string, + quiet bool, ) (string, error) { // Create a backup file for existing iptables rules. backupFile, err := createBackupFile(c.IptablesRestore.prefix) @@ -307,11 +320,11 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( } // Attempt to restore the new iptables rules from the temporary file. - output, err := c.restore(ctx, restoreFile) + output, err := c.restore(ctx, restoreFile, quiet) if err != nil { c.logger.Errorf("restoring backup file: %s", backupFile.Name()) - if _, err := c.restore(ctx, backupFile); err != nil { + if _, err := c.restore(ctx, backupFile, quiet); err != nil { c.logger.Warnf("restoring backup failed: %s", err) } diff --git a/pkg/transparentproxy/iptables/builder/builder.go b/pkg/transparentproxy/iptables/builder/builder.go index e7e31b403042..8d694de97686 100644 --- a/pkg/transparentproxy/iptables/builder/builder.go +++ b/pkg/transparentproxy/iptables/builder/builder.go @@ -57,6 +57,7 @@ func RestoreIPTables(ctx context.Context, cfg config.InitializedConfig) (string, output, err := cfg.IPv4.Executables.Restore( ctx, BuildIPTablesForRestore(cfg.IPv4), + false, ) if err != nil { return "", errors.Wrap(err, "unable to restore iptables rules") @@ -66,6 +67,7 @@ func RestoreIPTables(ctx context.Context, cfg config.InitializedConfig) (string, ipv6Output, err := cfg.IPv6.Executables.Restore( ctx, BuildIPTablesForRestore(cfg.IPv6), + false, ) if err != nil { return "", errors.Wrap(err, "unable to restore ip6tables rules") From b65601592ebf5dce439fe5313a4c56a1d8dd0246 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 09:32:03 +0200 Subject: [PATCH 10/20] Add Logic for Transparent Proxy Uninstallation/Cleanup This commit introduces the following enhancements and features: 1. Uninstallation/Cleanup Logic: - Added functionality to clean up and uninstall the transparent proxy setup. - The cleanup process removes iptables rules and chains related to the transparent proxy. - Ensures that only relevant rules and chains are removed based on the presence of iptables comments. - Validates the new rules after cleanup and restores them if they are valid. 2. Installation Process Improvement: - The installation process for the transparent proxy now includes a cleanup step at the beginning. - This ensures that any existing transparent proxy rules are removed before setting up the new rules, preventing conflicts and ensuring a clean installation. 3. Detailed Logging: - Improved log messages for better clarity and debugging. - Logs the successful completion of the transparent proxy cleanup process. Signed-off-by: Bart Smykla --- .../uninstall/uninstall_transparent_proxy.go | 12 +- pkg/transparentproxy/iptables/setup.go | 124 +++++++++++++++++- pkg/transparentproxy/transparentproxy.go | 4 +- 3 files changed, 128 insertions(+), 12 deletions(-) diff --git a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go index 33fec929f7dc..6c753eea9a88 100644 --- a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go +++ b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go @@ -37,8 +37,8 @@ func newUninstallTransparentProxy() *cobra.Command { return errors.Wrap(err, "failed to initialize config") } - output, err := transparentproxy.Cleanup(initializedConfig) - if err != nil { + + if _, err := transparentproxy.Cleanup(cmd.Context(), initializedConfig); err != nil { return errors.Wrap(err, "transparent proxy cleanup failed") } @@ -46,10 +46,6 @@ func newUninstallTransparentProxy() *cobra.Command { return nil } - if cfg.DryRun { - fmt.Fprintln(cfg.RuntimeStdout, output) - } - if _, err := os.Stat("/etc/resolv.conf.kuma-backup"); !os.IsNotExist(err) { content, err := os.ReadFile("/etc/resolv.conf.kuma-backup") if err != nil { @@ -66,7 +62,9 @@ func newUninstallTransparentProxy() *cobra.Command { fmt.Fprintln(cfg.RuntimeStdout, string(content)) } - fmt.Fprintln(cfg.RuntimeStdout, "Transparent proxy cleaned up successfully") + if !initializedConfig.DryRun { + initializedConfig.Logger.InfoWithoutPrefix("transparent proxy cleanup completed successfully") + } return nil }, diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index 173f26fbd435..08097efb4ad4 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -2,11 +2,12 @@ package iptables import ( "context" - "errors" "fmt" "slices" "strings" + "github.com/pkg/errors" + "github.com/kumahq/kuma/pkg/transparentproxy/config" "github.com/kumahq/kuma/pkg/transparentproxy/iptables/builder" "github.com/kumahq/kuma/pkg/transparentproxy/iptables/consts" @@ -17,11 +18,128 @@ func Setup(ctx context.Context, cfg config.InitializedConfig) (string, error) { return dryRun(cfg), nil } + cfg.Logger.Info( + "cleaning up any existing transparent proxy iptables rules", + ) + + if err := Cleanup(ctx, cfg); err != nil { + return "", errors.Wrap(err, "cleanup failed during setup") + } + return builder.RestoreIPTables(ctx, cfg) } -func Cleanup(_ config.InitializedConfig) (string, error) { - return "", errors.New("cleanup is not supported") +// Cleanup removes iptables rules and chains related to the transparent proxy +// for both IPv4 and IPv6 configurations. It calls the internal cleanupIPvX +// function for each IP version, ensuring that only the relevant rules and +// chains are removed based on the presence of iptables comments. If either +// cleanup process fails, an error is returned. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - cfg (config.InitializedConfig): The configuration containing the +// iptables settings for both IPv4 and IPv6, including comments and redirect +// information. +// +// Returns: +// - error: An error if the cleanup process for either IPv4 or IPv6 fails, +// including specific context about which IP version encountered the error. +func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { + if err := cleanupIPvX(ctx, cfg.IPv4); err != nil { + return errors.Wrap(err, "failed to cleanup IPv4 rules") + } + + if err := cleanupIPvX(ctx, cfg.IPv6); err != nil { + return errors.Wrap(err, "failed to cleanup IPv6 rules") + } + + return nil +} + +// cleanupIPvX removes iptables rules and chains related to the transparent +// proxy, ensuring that only the relevant rules and chains are removed based on +// the presence of iptables comments. It verifies the new rules after cleanup +// and restores them if they are valid. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - cfg (config.InitializedConfigIPvX): The configuration containing the +// iptables settings, including comments and redirect information. +// +// Returns: +// - error: An error if the cleanup process or verification fails. +func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { + // Execute iptables-save to retrieve current rules. + stdout, _, err := cfg.Executables.IptablesSave.Exec(ctx) + if err != nil { + return errors.Wrap(err, "failed to execute iptables-save command") + } + + output := stdout.String() + containsTProxyRules := strings.Contains(output, cfg.Redirect.NamePrefix) + containsTProxyComments := strings.Contains(output, cfg.Comment.Prefix) + + switch { + case !containsTProxyRules && !containsTProxyComments: + // If there are no transparent proxy rules or chains, there is + // nothing to do. + if cfg.Verbose { + cfg.Logger.Infof( + "no transparent proxy %s rules detected. No cleanup necessary", + consts.IptablesCommandByFamily[cfg.IPv6], + ) + } + return nil + case containsTProxyRules && !containsTProxyComments: + return errors.New( + "transparent proxy rules detected, but expected comments are missing. Cleanup cannot proceed safely without comments to identify rules. Please remove the transparent proxy iptables rules manually", + ) + } + + // Split the output into lines and remove lines related to transparent + // proxy rules and chains. + lines := strings.Split(output, "\n") + linesCleaned := slices.DeleteFunc( + lines, + func(line string) bool { + isComment := strings.HasPrefix(line, "#") + isTProxyRule := strings.Contains(line, cfg.Comment.Prefix) + isTProxyChain := strings.HasPrefix( + line, + fmt.Sprintf(":%s_", cfg.Redirect.NamePrefix), + ) + + return isComment || isTProxyRule || isTProxyChain + }, + ) + newRules := strings.Join(linesCleaned, "\n") + + // Verify if the new rules after cleanup are correct. + if _, err := cfg.Executables.RestoreTest(ctx, newRules); err != nil { + return errors.Wrap( + err, + "verification of new rules after cleanup failed", + ) + } + + if cfg.DryRun { + cfg.Logger.Infof( + "dry run mode: %s rules after cleanup:", + consts.IptablesCommandByFamily[cfg.IPv6], + ) + cfg.Logger.InfoWithoutPrefix(newRules) + return nil + } + + // Restore the new rules with flushing. + if _, err := cfg.Executables.RestoreWithFlush(ctx, newRules, true); err != nil { + return errors.Wrap( + err, + "failed to restore rules with flush after cleanup", + ) + } + + return nil } // dryRun simulates the setup of iptables rules for both IPv4 and IPv6 diff --git a/pkg/transparentproxy/transparentproxy.go b/pkg/transparentproxy/transparentproxy.go index 4a536d757da0..083481c4be0a 100644 --- a/pkg/transparentproxy/transparentproxy.go +++ b/pkg/transparentproxy/transparentproxy.go @@ -89,10 +89,10 @@ func Setup(ctx context.Context, cfg config.InitializedConfig) (string, error) { return iptables.Setup(ctx, cfg) } -func Cleanup(cfg config.InitializedConfig) (string, error) { +func Cleanup(ctx context.Context, cfg config.InitializedConfig) (string, error) { if cfg.IPv4.Ebpf.Enabled { return ebpf.Cleanup(cfg.IPv4) } - return iptables.Cleanup(cfg) + return "", iptables.Cleanup(ctx, cfg) } From 03ff6e3969193792ef43d4e062c3c4f759569940 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 09:38:26 +0200 Subject: [PATCH 11/20] Set `IPv6` DefaultConfig Value to True This change ensures the default installation of the transparent proxy supports both IPv4 and IPv6, aligning with the `--ip-family-mode` default value of `dualstack`. This is crucial for cleanup operations, which are not modified via CLI flags, and it is safe to run even if IPv6 is unavailable. Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index 1290922d3c91..3a301afc9cf4 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -688,7 +688,7 @@ func DefaultConfig() Config { ProgramsSourcePath: "/tmp/kuma-ebpf", }, DropInvalidPackets: false, - IPv6: false, + IPv6: true, RuntimeStdout: os.Stdout, RuntimeStderr: os.Stderr, Verbose: false, From b8ade1a42dce55441c81dc19556300e8cc650590 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 10:55:42 +0200 Subject: [PATCH 12/20] Remove unused string from Cleanup signatures Cleanup no longer returns any output, so the function signatures are updated accordingly. Signed-off-by: Bart Smykla --- app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go | 3 +-- pkg/transparentproxy/transparentproxy.go | 7 ++++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go index 6c753eea9a88..899455ca4498 100644 --- a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go +++ b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go @@ -37,8 +37,7 @@ func newUninstallTransparentProxy() *cobra.Command { return errors.Wrap(err, "failed to initialize config") } - - if _, err := transparentproxy.Cleanup(cmd.Context(), initializedConfig); err != nil { + if err := transparentproxy.Cleanup(cmd.Context(), initializedConfig); err != nil { return errors.Wrap(err, "transparent proxy cleanup failed") } diff --git a/pkg/transparentproxy/transparentproxy.go b/pkg/transparentproxy/transparentproxy.go index 083481c4be0a..f7a2bb53245f 100644 --- a/pkg/transparentproxy/transparentproxy.go +++ b/pkg/transparentproxy/transparentproxy.go @@ -89,10 +89,11 @@ func Setup(ctx context.Context, cfg config.InitializedConfig) (string, error) { return iptables.Setup(ctx, cfg) } -func Cleanup(ctx context.Context, cfg config.InitializedConfig) (string, error) { +func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { if cfg.IPv4.Ebpf.Enabled { - return ebpf.Cleanup(cfg.IPv4) + _, err := ebpf.Cleanup(cfg.IPv4) + return err } - return "", iptables.Cleanup(ctx, cfg) + return iptables.Cleanup(ctx, cfg) } From f478d8c1493d5c54abf951250cdc1c42dc29f39b Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 11:03:17 +0200 Subject: [PATCH 13/20] Remove unit tests for uninstalling transparent proxy The existing unit tests for uninstalling the transparent proxy were not functional and served only as placeholders. Given the difficulty of idempotently testing this command across all environments, these tests are being removed. A following commit will introduce functional tests for transparent proxy uninstallation, ensuring proper coverage. Signed-off-by: Bart Smykla --- .../cmd/uninstall/uninstall_suite_test.go | 11 ----- .../uninstall_transparent_proxy_test.go | 43 ------------------- 2 files changed, 54 deletions(-) delete mode 100644 app/kumactl/cmd/uninstall/uninstall_suite_test.go delete mode 100644 app/kumactl/cmd/uninstall/uninstall_transparent_proxy_test.go diff --git a/app/kumactl/cmd/uninstall/uninstall_suite_test.go b/app/kumactl/cmd/uninstall/uninstall_suite_test.go deleted file mode 100644 index 810c9090b7f2..000000000000 --- a/app/kumactl/cmd/uninstall/uninstall_suite_test.go +++ /dev/null @@ -1,11 +0,0 @@ -package uninstall_test - -import ( - "testing" - - "github.com/kumahq/kuma/pkg/test" -) - -func TestInstallCmd(t *testing.T) { - test.RunSpecs(t, "Uninstall Cmd Suite") -} diff --git a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy_test.go b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy_test.go deleted file mode 100644 index 072b0e4f8f07..000000000000 --- a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy_test.go +++ /dev/null @@ -1,43 +0,0 @@ -package uninstall_test - -import ( - "strings" - - . "github.com/onsi/ginkgo/v2" - . "github.com/onsi/gomega" - - test_kumactl "github.com/kumahq/kuma/app/kumactl/pkg/test" -) - -var _ = Describe("kumactl uninstall transparent-proxy", func() { - type testCase struct { - extraArgs []string - goldenFile string - } - - DescribeTable("should uninstall transparent proxy", - func(given testCase) { - // given - args := append([]string{"uninstall", "transparent-proxy", "--dry-run"}, given.extraArgs...) - _, stderr, rootCmd := test_kumactl.DefaultTestingRootCmd(args...) - - // when - err := rootCmd.Execute() - // then - Expect(err).To(HaveOccurred()) - // and - Expect(stderr.String()).To(WithTransform(func(in string) string { - return strings.ReplaceAll( - in, - "# [WARNING]: dry-run mode: No valid iptables executables found. The generated iptables rules may differ from those generated in an environment with valid iptables executables\n", "", - ) - }, Equal("Error: transparent proxy cleanup failed: cleanup is not supported\n"))) - - // TODO once delete works again to something similar to what we do for `install_transparent_proxy_test.go` with Transform. - }, - Entry("should generate defaults with username", testCase{ - extraArgs: nil, - goldenFile: "uninstall-transparent-proxy.defaults.golden.txt", - }), - ) -}) From aa5e8af1b0cee82ab7ae04141173e671d35bc836 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sat, 13 Jul 2024 12:03:01 +0200 Subject: [PATCH 14/20] Replace `IPv6` property in `InitializedConfigIPvX` with prefixed Logger Removed the `IPv6` property from `InitializedConfigIPvX` and replaced it with a `Logger` that includes `iptables` or `ip6tables` prefixes. This change required enhancing the Logger abstraction to support dynamic prefixing. Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config.go | 9 +-- pkg/transparentproxy/config/config_logger.go | 72 ++++++++++++++------ pkg/transparentproxy/iptables/setup.go | 10 +-- 3 files changed, 57 insertions(+), 34 deletions(-) diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index 3a301afc9cf4..6bc26b3b8bac 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -533,10 +533,6 @@ type InitializedConfigIPvX struct { // text. This helps in identifying and organizing iptables rules created by // the transparent proxy, making them easier to manage and debug. Comment InitializedComment - // IPv6 marks whether the current configuration refers to an IPv6 setup. - // This boolean field helps determine the appropriate actions and settings - // needed for handling IPv6-specific iptables rules and configurations. - IPv6 bool enabled bool } @@ -583,7 +579,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { Logger: l, IPv4: InitializedConfigIPvX{ Config: c, - Logger: l, + Logger: l.WithPrefix(IptablesCommandByFamily[false]), LocalhostCIDR: LocalhostCIDRIPv4, InboundPassthroughCIDR: InboundPassthroughSourceAddressCIDRIPv4, enabled: true, @@ -624,14 +620,13 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { if c.IPv6 { initialized.IPv6 = InitializedConfigIPvX{ Config: c, - Logger: l, + Logger: l.WithPrefix(IptablesCommandByFamily[true]), Executables: e.IPv6, LoopbackInterfaceName: loopbackInterfaceName, LocalhostCIDR: LocalhostCIDRIPv6, InboundPassthroughCIDR: InboundPassthroughSourceAddressCIDRIPv6, Comment: c.Comment.Initialize(e.IPv6), DropInvalidPackets: c.DropInvalidPackets && e.IPv6.Functionality.Tables.Mangle, - IPv6: true, enabled: true, } diff --git a/pkg/transparentproxy/config/config_logger.go b/pkg/transparentproxy/config/config_logger.go index 68bc0186221b..82d8f9449cdf 100644 --- a/pkg/transparentproxy/config/config_logger.go +++ b/pkg/transparentproxy/config/config_logger.go @@ -3,6 +3,7 @@ package config import ( "fmt" "io" + "slices" "strings" ) @@ -16,39 +17,71 @@ func logln(w io.Writer, a []any) { fmt.Fprintln(w, a...) } -// loglnWithHashPrefix writes a formatted line to the specified writer, -// prefixing it with a hash (#) to denote the line as a comment. +// loglnWithPrefixes writes a formatted line to the specified writer, +// prefixing it with a hash (#) and additional prefixes for context. // // Args: // - w (io.Writer): The writer where the line will be written. // - a ([]any): The arguments to write to the writer, prefixed with a hash. -func loglnWithHashPrefix(w io.Writer, a []any) { - logln(w, append([]any{"#"}, a...)) +// - p (...string): Additional prefixes to add for context. +func (l Logger) loglnWithPrefixes(w io.Writer, a []any, p ...string) { + prefixes := []any{"#"} + for _, prefix := range slices.Concat(p, l.defaultPrefixes) { + if prefix != "" { + prefixes = append(prefixes, fmt.Sprintf("[%s]", prefix)) + } + } + + logln(w, slices.Concat(prefixes, a)) } // Logger provides simple logging capabilities directing output to specified // stdout and stderr writers. It supports retry-aware logging, tracking the // current and maximum retry counts to adjust log messages accordingly. type Logger struct { - stdout io.Writer // Target writer where standard output logs are written. - stderr io.Writer // Target writer where error output logs are written. - maxTry int // The maximum number of tries for retry operations. - try int // The current try number within the retry operation ctx. + // Target writer where standard output logs are written. + stdout io.Writer + // Target writer where error output logs are written. + stderr io.Writer + // The maximum number of tries for retry operations. + maxTry int + // The current try number within the retry operation context. + try int + // Default prefixes to be used in log messages. + defaultPrefixes []string +} + +// WithPrefix returns a new Logger instance with the specified prefix added to +// the default prefixes. +// +// Args: +// - prefix (string): The prefix to add to the logger. +// +// Returns: +// - Logger: A new Logger instance with the added prefix. +func (l Logger) WithPrefix(prefix string) Logger { + return Logger{ + stdout: l.stdout, + stderr: l.stderr, + maxTry: l.maxTry, + try: l.try, + defaultPrefixes: append(l.defaultPrefixes, prefix), + } } // tryPrefix generates a logging prefix based on the current retry attempt. // It returns a formatted string showing the current attempt and the total -// attempts, or nil if no retries are needed (maxTry <= 1). +// attempts, or an empty string if no retries are needed (maxTry <= 1). // // Returns: -// - []any: The formatted prefix for the current retry attempt, or nil if no -// retries. -func (l Logger) tryPrefix() []any { +// - string: The formatted prefix for the current retry attempt, or an empty +// string if no retries. +func (l Logger) tryPrefix() string { if l.maxTry <= 1 { - return nil + return "" } - return []any{fmt.Sprintf("[%d/%d]", l.try, l.maxTry)} + return fmt.Sprintf("%d/%d", l.try, l.maxTry) } // Info logs standard messages to stdout, prefixed with a hash. @@ -56,7 +89,7 @@ func (l Logger) tryPrefix() []any { // Args: // - a (...any): The arguments to log to stdout. func (l Logger) Info(a ...any) { - loglnWithHashPrefix(l.stdout, a) + l.loglnWithPrefixes(l.stdout, a) } // Infof logs formatted messages to stdout, prefixed with a hash. @@ -82,7 +115,7 @@ func (l Logger) InfoWithoutPrefix(a ...any) { // Args: // - a (...any): The arguments to log to stdout with retry context. func (l Logger) InfoTry(a ...any) { - l.Info(append(l.tryPrefix(), a...)...) + l.loglnWithPrefixes(l.stdout, a, l.tryPrefix()) } // Warn logs warning messages to stderr, prefixed with "[WARNING]:". @@ -90,7 +123,7 @@ func (l Logger) InfoTry(a ...any) { // Args: // - a (...any): The arguments to log as a warning to stderr. func (l Logger) Warn(a ...any) { - loglnWithHashPrefix(l.stderr, append([]any{"[WARNING]:"}, a...)) + l.loglnWithPrefixes(l.stderr, a, "WARNING") } // Warnf logs formatted warning messages to stderr, prefixed with "[WARNING]:". @@ -107,7 +140,7 @@ func (l Logger) Warnf(format string, a ...any) { // Args: // - a (...any): The arguments to log as an error to stderr. func (l Logger) Error(a ...any) { - loglnWithHashPrefix(l.stderr, a) + l.loglnWithPrefixes(l.stderr, a) } // Errorf logs formatted error messages to stderr, prefixed with a hash. @@ -126,9 +159,8 @@ func (l Logger) Errorf(format string, a ...any) { // - err (error): The error to log. // - a (...any): Additional context to log with the error message. func (l Logger) ErrorTry(err error, a ...any) { - result := append(l.tryPrefix(), a...) // Convert multi-line errors to a single line for clarity in logs. errInOneLine := strings.ReplaceAll(err.Error(), "\n", "") - l.Error(append(result, errInOneLine)...) + l.loglnWithPrefixes(l.stderr, append(a, errInOneLine), l.tryPrefix()) } diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index 08097efb4ad4..dd9d0ee9b3c4 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -84,9 +84,8 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { // If there are no transparent proxy rules or chains, there is // nothing to do. if cfg.Verbose { - cfg.Logger.Infof( - "no transparent proxy %s rules detected. No cleanup necessary", - consts.IptablesCommandByFamily[cfg.IPv6], + cfg.Logger.Info( + "no transparent proxy rules detected. No cleanup necessary", ) } return nil @@ -123,10 +122,7 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { } if cfg.DryRun { - cfg.Logger.Infof( - "dry run mode: %s rules after cleanup:", - consts.IptablesCommandByFamily[cfg.IPv6], - ) + cfg.Logger.Info("[dry-run]: rules after cleanup:") cfg.Logger.InfoWithoutPrefix(newRules) return nil } From 3e30fdd06a90d9ed9a7e6216001fe0da98b832ff Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sun, 14 Jul 2024 12:50:04 +0200 Subject: [PATCH 15/20] Add tests for transparent proxy uninstallation Signed-off-by: Bart Smykla --- .../transparentproxy/transparentproxy_test.go | 213 +++++++++++++++++- 1 file changed, 211 insertions(+), 2 deletions(-) diff --git a/test/transparentproxy/transparentproxy_test.go b/test/transparentproxy/transparentproxy_test.go index bd1d2b1587d1..b067e1afecf3 100644 --- a/test/transparentproxy/transparentproxy_test.go +++ b/test/transparentproxy/transparentproxy_test.go @@ -2,6 +2,7 @@ package transparentproxy import ( "context" + std_errors "errors" "fmt" "io" "slices" @@ -9,6 +10,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/pkg/errors" "github.com/testcontainers/testcontainers-go" "github.com/testcontainers/testcontainers-go/exec" @@ -36,11 +38,44 @@ var ( apkAddIptables = []string{"apk", "add", "iptables"} ) +// customIptablesRules defines a set of custom iptables rules that are used to +// ensure our cleanup process does not remove non-transparent-proxy related +// rules. These rules create custom chains and add rules to various tables +// (nat, raw, mangle) to confirm that the cleanup process will only remove +// rules related to the transparent proxy, leaving other custom rules intact. +// +// The rules include: +// - Creating custom chains in the nat, raw, and mangle tables. +// - Adding rules to the OUTPUT and PREROUTING chains that direct traffic to +// the custom chains. +var ( + customIptablesRules = [][]string{ + {"-t", "nat", "-N", "CUSTOM_CHAIN_NAT"}, + {"-t", "raw", "-N", "CUSTOM_CHAIN_RAW"}, + {"-t", "mangle", "-N", "CUSTOM_CHAIN_MANGLE"}, + {"-t", "nat", "-A", "OUTPUT", "-p", "tcp", "-j", "CUSTOM_CHAIN_NAT"}, + {"-t", "nat", "-A", "PREROUTING", "-p", "udp", "-j", "CUSTOM_CHAIN_NAT"}, + {"-t", "raw", "-A", "OUTPUT", "-p", "tcp", "--dport", "53", "-j", "CUSTOM_CHAIN_RAW"}, + {"-t", "raw", "-A", "PREROUTING", "-j", "CUSTOM_CHAIN_RAW"}, + {"-t", "mangle", "-A", "OUTPUT", "-p", "udp", "--sport", "53", "-j", "CUSTOM_CHAIN_MANGLE"}, + {"-t", "mangle", "-A", "PREROUTING", "-j", "CUSTOM_CHAIN_MANGLE"}, + } +) + // The following variables are used in tests to manage and verify iptables rules // across different iptables implementations and versions. These lists include -// commands for saving the rules to check against expected outcomes -// (ipv4SaveCmds, ipv6SaveCmds). +// commands for installing custom rules (iptablesCmds) and for saving the rules +// to check against expected outcomes (ipv4SaveCmds, ipv6SaveCmds, +// iptablesSaveCmds). var ( + iptablesCmds = []string{ + "iptables", + "iptables-nft", + "iptables-legacy", + "ip6tables", + "ip6tables-nft", + "ip6tables-legacy", + } ipv4SaveCmds = []string{ "iptables-save", "iptables-nft-save", @@ -51,6 +86,7 @@ var ( "ip6tables-nft-save", "ip6tables-legacy-save", } + iptablesSaveCmds = slices.Concat(slices.Clone(ipv4SaveCmds), ipv6SaveCmds) ) type testCase struct { @@ -93,6 +129,54 @@ var _ = Describe("Transparent Proxy", func() { // Generate entries for each Docker image to test genEntriesForImages(Config.DockerImagesToTest, Entry, FlakeAttempts(3)), ) + + DescribeTable( + "uninstall in container", + func(tc testCase) { + ctx := context.Background() + + // Given the kumactl binary path is not empty + Expect(Config.KumactlLinuxBin).NotTo(BeEmpty()) + + // Given a container setup with specified image and settings + c, err := test_container.NewContainerSetup(). + WithImage(tc.image). + WithKumactlBinary(Config.KumactlLinuxBin). + WithPostStart(tc.postStart). + WithPrivileged(true). + Start(ctx) + Expect(err).ToNot(HaveOccurred()) + + // Clean up the container after the test + DeferCleanup(func() { + Expect(c.Terminate(ctx)).To(Succeed()) + }) + + // Given custom iptables rules are added to the container + Expect(addCustomIptablesRules(ctx, c)).To(Succeed()) + + // Given the iptables-save output before installing the proxy + before := getIptablesSaveOutput(ctx, c) + + // When the transparent proxy is installed successfully + EnsureInstallSuccessful(ctx, c, tc.additionalFlags) + + // When the transparent proxy is uninstalled successfully + EnsureUninstallSuccessful(ctx, c) + + // Then the iptables-save output after uninstall should match the + // output before install + after := getIptablesSaveOutput(ctx, c) + + Expect(before).To(HaveLen(len(after))) + + for cmd, output := range before { + Expect(after[cmd]).To(Equal(output)) + } + }, + // Generate entries for each Docker image to test + genEntriesForImages(Config.DockerImagesToTest, Entry, FlakeAttempts(3)), + ) }) // EnsureInstallSuccessful installs the transparent proxy in a test container @@ -140,6 +224,39 @@ func EnsureInstallSuccessful( } } +// EnsureUninstallSuccessful runs the `kumactl uninstall transparent-proxy` +// command in the specified container and checks if the uninstallation is +// successful. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - c (testcontainers.Container): The container where the command will run. +// +// This function performs the following steps: +// 1. Executes the `kumactl uninstall transparent-proxy` command inside the +// specified container using the provided context. +// 2. Checks if the command execution returns an error. If an error occurs, the +// function fails the test. +// 3. If the command exit code is not zero, it reads the command output, copies +// it to a buffer, and fails the test with a message containing the exit +// code and the command output. +func EnsureUninstallSuccessful(ctx context.Context, c testcontainers.Container) { + GinkgoHelper() + + exitCode, reader, err := c.Exec( + ctx, + []string{"kumactl", "uninstall", "transparent-proxy"}, + exec.Multiplexed(), + ) + Expect(err).NotTo(HaveOccurred()) + + if exitCode != 0 { + buf := new(strings.Builder) + Expect(io.Copy(buf, reader)).Error().NotTo(HaveOccurred()) + Fail(fmt.Sprintf("uninstall ended with code %d: %s", exitCode, buf)) + } +} + // EnsureGoldenFiles validates the current iptables rules in a test container // against predefined golden files, ensuring the rules match the expected // configuration. @@ -353,3 +470,95 @@ func genEntriesForImage( return entries } + +// getIptablesSaveOutput retrieves the output of iptables-save commands from a +// given container. +// +// This function executes all the defined iptables-save commands inside the +// provided container and collects their outputs. The outputs are cleaned and +// returned in a map where the keys are the command names and the values are +// their respective outputs. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - container (testcontainers.Container): The container from which to +// retrieve the iptables-save output. +// +// Returns: +// - map[string]string: A map containing the iptables-save command outputs, +// keyed by command name. +func getIptablesSaveOutput( + ctx context.Context, + container testcontainers.Container, +) map[string]string { + GinkgoHelper() + + output := map[string]string{} + + for _, cmd := range iptablesSaveCmds { + if exitCode, reader, err := container.Exec( + ctx, + []string{cmd}, + exec.Multiplexed(), + ); exitCode == 0 && err == nil { + buf := new(strings.Builder) + Expect(io.Copy(buf, reader)).Error().NotTo(HaveOccurred()) + output[cmd] = utils.CleanIptablesSaveOutput(buf.String()) + } + } + + return output +} + +// addCustomIptablesRules adds a set of custom iptables rules to a given +// container. +// +// This function iterates over the predefined iptables commands and custom +// iptables rules, attempting to add each rule using each iptables command. +// If all attempts to add the rules using all iptables commands fail, it +// returns an error detailing the failures. +// +// Args: +// - ctx (context.Context): The context for command execution. +// - c (testcontainers.Container): The container to which the custom +// iptables rules will be added. +// +// Returns: +// - error: An error if all attempts to add the custom iptables rules +// fail, nil otherwise. +func addCustomIptablesRules( + ctx context.Context, + c testcontainers.Container, +) error { + var errs []error + + for _, iptables := range iptablesCmds { + var cmdErrs []error + + for _, rule := range customIptablesRules { + if exitCode, _, err := c.Exec( + ctx, + slices.Concat([]string{iptables}, rule), + exec.Multiplexed(), + ); err != nil || exitCode != 0 { + cmdErrs = append( + cmdErrs, + errors.Wrapf(err, "exit code %d", exitCode), + ) + } + } + + if len(cmdErrs) > 0 { + errs = append(errs, std_errors.Join(cmdErrs...)) + } + } + + if len(errs) == len(iptablesCmds) { + return errors.Wrap( + std_errors.Join(errs...), + "all iptables commands used to add custom iptables rules failed", + ) + } + + return nil +} From 163ff47f05fdc5a5713c8597244efb59cf48001b Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sun, 14 Jul 2024 12:58:09 +0200 Subject: [PATCH 16/20] Improve transparent-proxy cleanup logic - Updated cleanup logic to handle custom iptables rules more effectively by removing all rules and chains containing comments with our prefix or names starting with the prefix (KUMA_MESH). - Enhanced log and error messages for better clarity and troubleshooting. - Addressed a known bug on Ubuntu 20.04 related to `iptables-nft-restore --test` failing with more than two tables, adding appropriate handling for this issue. Signed-off-by: Bart Smykla --- .../config/config_executables.go | 12 ++++++ pkg/transparentproxy/iptables/setup.go | 38 ++++++++++++------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index cdd5d4c91360..68c91ab918be 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -372,6 +372,18 @@ func (c InitializedExecutablesIPvX) RestoreTest( stdout, _, err := c.IptablesRestore.Exec(ctx, FlagTest, f.Name()) if err != nil { + // There is an existing bug which occurs on Ubuntu 20.04 + // ref. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=960003 + if strings.Contains(strings.ToLower(err.Error()), "segmentation fault") { + c.logger.Warnf( + `cannot confirm rules are valid because "%s %s" is returning unexpected error: %q. See https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=960003 for more details`, + c.IptablesRestore.Path, + FlagTest, + err, + ) + return "", nil + } + return "", errors.Wrap(err, "rules are invalid") } diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index dd9d0ee9b3c4..51b3731e154e 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -18,9 +18,7 @@ func Setup(ctx context.Context, cfg config.InitializedConfig) (string, error) { return dryRun(cfg), nil } - cfg.Logger.Info( - "cleaning up any existing transparent proxy iptables rules", - ) + cfg.Logger.Info("cleaning up any existing transparent proxy iptables rules") if err := Cleanup(ctx, cfg); err != nil { return "", errors.Wrap(err, "cleanup failed during setup") @@ -58,8 +56,8 @@ func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { // cleanupIPvX removes iptables rules and chains related to the transparent // proxy, ensuring that only the relevant rules and chains are removed based on -// the presence of iptables comments. It verifies the new rules after cleanup -// and restores them if they are valid. +// the presence of iptables comments and chain name prefixes. It verifies the +// new rules after cleanup and restores them if they are valid. // // Args: // - ctx (context.Context): The context for command execution. @@ -69,6 +67,16 @@ func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { // Returns: // - error: An error if the cleanup process or verification fails. func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { + if !cfg.Enabled() { + return nil + } + + cfg.Logger.Infof( + "starting cleanup of existing transparent proxy rules. Any rule found in chains with names starting with %q or containing comments starting with %q will be deleted", + cfg.Redirect.NamePrefix, + cfg.Comment.Prefix, + ) + // Execute iptables-save to retrieve current rules. stdout, _, err := cfg.Executables.IptablesSave.Exec(ctx) if err != nil { @@ -98,17 +106,17 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { // Split the output into lines and remove lines related to transparent // proxy rules and chains. lines := strings.Split(output, "\n") + // Remove lines related to transparent proxy rules and chains. + // This includes: + // 1. Lines starting with "#" (comments). + // 2. Lines containing the comment prefix specified in the config. + // 3. Lines containing the redirect name prefix specified in the config. linesCleaned := slices.DeleteFunc( lines, func(line string) bool { - isComment := strings.HasPrefix(line, "#") - isTProxyRule := strings.Contains(line, cfg.Comment.Prefix) - isTProxyChain := strings.HasPrefix( - line, - fmt.Sprintf(":%s_", cfg.Redirect.NamePrefix), - ) - - return isComment || isTProxyRule || isTProxyChain + return strings.HasPrefix(line, "#") || + strings.Contains(line, cfg.Comment.Prefix) || + strings.Contains(line, cfg.Redirect.NamePrefix) }, ) newRules := strings.Join(linesCleaned, "\n") @@ -123,7 +131,7 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { if cfg.DryRun { cfg.Logger.Info("[dry-run]: rules after cleanup:") - cfg.Logger.InfoWithoutPrefix(newRules) + cfg.Logger.InfoWithoutPrefix(strings.TrimSpace(newRules)) return nil } @@ -135,6 +143,8 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { ) } + cfg.Logger.Info("cleanup of existing transparent proxy rules completed successfully") + return nil } From f76b233c0f318380ba87a54027312240209199d1 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Sun, 14 Jul 2024 13:09:10 +0200 Subject: [PATCH 17/20] Improvements in logging - Changed the order of prefixes in logger for clearer logs: instead of `# [WARNING] [iptables] ...` it is now `# [iptables] [WARNING] ...` - Removed unnecessary log after restoring iptables: "iptables set to divert the traffic to Envoy" as earlier logs are more informative. - Consolidated log and error message content into single lines instead of breaking them into multiple lines, making phrase searches in IDEs or code editors easier. - Updated `Logger` instances passed to initialization functions in configs to include appropriate prefixes (`[iptables ]` or `[ip6tables]`). Signed-off-by: Bart Smykla --- .../install/install_transparent_proxy_test.go | 39 ++++++++++++------- pkg/transparentproxy/config/config.go | 19 +++++---- .../config/config_executables.go | 30 +++++++------- pkg/transparentproxy/config/config_logger.go | 2 +- .../iptables/builder/builder.go | 6 +-- pkg/transparentproxy/iptables/setup.go | 10 +---- 6 files changed, 55 insertions(+), 51 deletions(-) diff --git a/app/kumactl/cmd/install/install_transparent_proxy_test.go b/app/kumactl/cmd/install/install_transparent_proxy_test.go index 65e71d6894e9..a41fd788b062 100644 --- a/app/kumactl/cmd/install/install_transparent_proxy_test.go +++ b/app/kumactl/cmd/install/install_transparent_proxy_test.go @@ -1,7 +1,9 @@ package install_test import ( + "bytes" "regexp" + "slices" "strings" . "github.com/onsi/ginkgo/v2" @@ -20,6 +22,21 @@ var _ = Context("kumactl install transparent proxy", func() { errorMatcher gomega_types.GomegaMatcher } + cleanStderr := func(stderr *bytes.Buffer) string { + return strings.Join( + slices.DeleteFunc( + strings.Split(stderr.String(), "\n"), + func(line string) bool { + return strings.Contains( + line, + "no valid iptables executables found", + ) + }, + ), + "\n", + ) + } + DescribeTable("should install transparent proxy", func(given testCase) { // given @@ -29,9 +46,7 @@ var _ = Context("kumactl install transparent proxy", func() { // when err := rootCmd.Execute() stdout := stdoutBuf.String() - stderr := strings.NewReplacer( - "# [WARNING]: dry-run mode: No valid iptables executables found. The generated iptables rules may differ from those generated in an environment with valid iptables executables\n", "", - ).Replace(stderrBuf.String()) + stderr := cleanStderr(stderrBuf) if given.skip != nil && given.skip(stdout, stderr) { Skip("test skipped") @@ -46,7 +61,7 @@ var _ = Context("kumactl install transparent proxy", func() { Expect(stderr).To(given.errorMatcher) } - Expect(stdoutBuf.String()).To(WithTransform(func(in string) string { + Expect(stdout).To(WithTransform(func(in string) string { // Replace some stuff that are environment dependent with placeholders out := regexp.MustCompile(`-o ([^ ]+)`).ReplaceAllString(in, "-o ifPlaceholder") out = regexp.MustCompile(`-([sd]) ([^ ]+)`).ReplaceAllString(out, "-$1 subnetPlaceholder/mask") @@ -77,12 +92,12 @@ var _ = Context("kumactl install transparent proxy", func() { "--redirect-dns-port", "12345", }, skip: func(stdout, stderr string) bool { - return !strings.HasPrefix( + return !strings.Contains( stderr, - "# [WARNING]: conntrack zone splitting for IPv4 is disabled. Functionality requires the 'conntrack' iptables module", + "conntrack zone splitting is disabled. Functionality requires the 'conntrack' iptables module", ) }, - errorMatcher: HavePrefix("# [WARNING]: conntrack zone splitting for IPv4 is disabled. Functionality requires the 'conntrack' iptables module"), + errorMatcher: ContainSubstring("conntrack zone splitting is disabled. Functionality requires the 'conntrack' iptables module"), goldenFile: "install-transparent-proxy.dns.no-conntrack.golden.txt", }), Entry("should generate defaults with user id and DNS redirected", testCase{ @@ -92,9 +107,9 @@ var _ = Context("kumactl install transparent proxy", func() { "--redirect-dns-port", "12345", }, skip: func(stdout, stderr string) bool { - return strings.HasPrefix( + return strings.Contains( stderr, - "# [WARNING]: conntrack zone splitting for IPv4 is disabled. Functionality requires the 'conntrack' iptables module", + "conntrack zone splitting is disabled. Functionality requires the 'conntrack' iptables module", ) }, goldenFile: "install-transparent-proxy.dns.golden.txt", @@ -181,11 +196,9 @@ var _ = Context("kumactl install transparent proxy", func() { // when err := rootCmd.Execute() - stderr := strings.NewReplacer( - "# [WARNING]: dry-run mode: No valid iptables executables found. The generated iptables rules may differ from those generated in an environment with valid iptables executables\n", "", - ).Replace(stderrBuf.String()) - // then + stderr := cleanStderr(stderrBuf) // then Expect(err).To(HaveOccurred()) + // and Expect(stderr).To(given.errorMatcher) }, diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index 6bc26b3b8bac..67357a4881ab 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -171,10 +171,7 @@ func (c DNS) Initialize( if c.ConntrackZoneSplit { initialized.ConntrackZoneSplit = executables.Functionality.ConntrackZoneSplit() if !initialized.ConntrackZoneSplit { - l.Warnf( - "conntrack zone splitting for %s is disabled. Functionality requires the 'conntrack' iptables module", - IPTypeMap[ipv6], - ) + l.Warn("conntrack zone splitting is disabled. Functionality requires the 'conntrack' iptables module") } } @@ -575,11 +572,14 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { maxTry: c.Retry.MaxRetries + 1, } + loggerIPv4 := l.WithPrefix(IptablesCommandByFamily[false]) + loggerIPv6 := l.WithPrefix(IptablesCommandByFamily[true]) + initialized := InitializedConfig{ Logger: l, IPv4: InitializedConfigIPvX{ Config: c, - Logger: l.WithPrefix(IptablesCommandByFamily[false]), + Logger: loggerIPv4, LocalhostCIDR: LocalhostCIDRIPv4, InboundPassthroughCIDR: InboundPassthroughSourceAddressCIDRIPv4, enabled: true, @@ -596,7 +596,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { } initialized.IPv4.Executables = e.IPv4 - ipv4Redirect, err := c.Redirect.Initialize(l, e.IPv4, false) + ipv4Redirect, err := c.Redirect.Initialize(loggerIPv4, e.IPv4, false) if err != nil { return initialized, errors.Wrap( err, @@ -620,7 +620,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { if c.IPv6 { initialized.IPv6 = InitializedConfigIPvX{ Config: c, - Logger: l.WithPrefix(IptablesCommandByFamily[true]), + Logger: loggerIPv6, Executables: e.IPv6, LoopbackInterfaceName: loopbackInterfaceName, LocalhostCIDR: LocalhostCIDRIPv6, @@ -630,7 +630,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { enabled: true, } - ipv6Redirect, err := c.Redirect.Initialize(l, e.IPv6, true) + ipv6Redirect, err := c.Redirect.Initialize(loggerIPv6, e.IPv6, true) if err != nil { return initialized, errors.Wrap( err, @@ -933,8 +933,7 @@ func validateIP(address string, ipv6 bool) (error, bool) { // message. if ip == nil { return errors.Errorf( - "invalid IP address: '%s'. Expected format: or / "+ - "(e.g., 10.0.0.1, 172.16.0.0/16, fe80::1, fe80::/10)", + "invalid IP address: '%s'. Expected format: or / (e.g., 10.0.0.1, 172.16.0.0/16, fe80::1, fe80::/10)", address, ), false } diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index 68c91ab918be..3f2de07937a0 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -438,8 +438,11 @@ func (c Executables) Initialize( initialized := InitializedExecutables{Executables: c} - initialized.IPv4, err = c.IPv4.Initialize(ctx, l, cfg) - if err != nil { + if initialized.IPv4, err = c.IPv4.Initialize( + ctx, + l.WithPrefix(IptablesCommandByFamily[false]), + cfg, + ); err != nil { return InitializedExecutables{}, errors.Wrap( err, "failed to initialize IPv4 executables", @@ -447,8 +450,11 @@ func (c Executables) Initialize( } if cfg.IPv6 { - initialized.IPv6, err = c.IPv6.Initialize(ctx, l, cfg) - if err != nil { + if initialized.IPv6, err = c.IPv6.Initialize( + ctx, + l.WithPrefix(IptablesCommandByFamily[true]), + cfg, + ); err != nil { return InitializedExecutables{}, errors.Wrap( err, "failed to initialize IPv6 executables", @@ -457,8 +463,10 @@ func (c Executables) Initialize( if err := configureIPv6OutboundAddress(); err != nil { initialized.IPv6 = InitializedExecutablesIPvX{} - l.Warn("failed to configure IPv6 outbound address. IPv6 rules "+ - "will be skipped:", err) + l.Warn( + "failed to configure IPv6 outbound address. IPv6 rules will be skipped:", + err, + ) } } @@ -508,9 +516,7 @@ func (c ExecutablesNftLegacy) Initialize( switch { // Dry-run mode when no valid iptables executables are found. case len(errs) == 2 && cfg.DryRun: - l.Warn("dry-run mode: No valid iptables executables found. The " + - "generated iptables rules may differ from those generated in an " + - "environment with valid iptables executables") + l.Warn("[dry-run]: no valid iptables executables found. The generated iptables rules may differ from those generated in an environment with valid iptables executables") return InitializedExecutables{}, nil // Regular mode when no vaild iptables executables are found case len(errs) == 2: @@ -527,11 +533,7 @@ func (c ExecutablesNftLegacy) Initialize( // Both types of executables contain custom DOCKER_OUTPUT chain in nat // table. We are prioritizing nft case nft.hasDockerOutputChain() && legacy.hasDockerOutputChain(): - l.Warn("conflicting iptables modes detected. Two iptables " + - "versions (iptables-nft and iptables-legacy) were found. " + - "Both contain a nat table with a chain named 'DOCKER_OUTPUT'. " + - "To avoid potential conflicts, iptables-legacy will be ignored " + - "and iptables-nft will be used") + l.Warn("conflicting iptables modes detected. Two iptables versions (iptables-nft and iptables-legacy) were found. Both contain a nat table with a chain named 'DOCKER_OUTPUT'. To avoid potential conflicts, iptables-legacy will be ignored and iptables-nft will be used") return nft, nil case legacy.hasDockerOutputChain(): return legacy, nil diff --git a/pkg/transparentproxy/config/config_logger.go b/pkg/transparentproxy/config/config_logger.go index 82d8f9449cdf..9b8580798b55 100644 --- a/pkg/transparentproxy/config/config_logger.go +++ b/pkg/transparentproxy/config/config_logger.go @@ -26,7 +26,7 @@ func logln(w io.Writer, a []any) { // - p (...string): Additional prefixes to add for context. func (l Logger) loglnWithPrefixes(w io.Writer, a []any, p ...string) { prefixes := []any{"#"} - for _, prefix := range slices.Concat(p, l.defaultPrefixes) { + for _, prefix := range slices.Concat(slices.Clone(l.defaultPrefixes), p) { if prefix != "" { prefixes = append(prefixes, fmt.Sprintf("[%s]", prefix)) } diff --git a/pkg/transparentproxy/iptables/builder/builder.go b/pkg/transparentproxy/iptables/builder/builder.go index 8d694de97686..8895fdde4864 100644 --- a/pkg/transparentproxy/iptables/builder/builder.go +++ b/pkg/transparentproxy/iptables/builder/builder.go @@ -50,9 +50,7 @@ func BuildIPTablesForRestore(cfg config.InitializedConfigIPvX) string { } func RestoreIPTables(ctx context.Context, cfg config.InitializedConfig) (string, error) { - cfg.Logger.Info("kumactl is about to apply the iptables rules that " + - "will enable transparent proxying on the machine. The SSH connection " + - "may drop. If that happens, just reconnect again.") + cfg.Logger.Info("kumactl is about to apply the iptables rules that will enable transparent proxying on the machine. The SSH connection may drop. If that happens, just reconnect again") output, err := cfg.IPv4.Executables.Restore( ctx, @@ -76,7 +74,5 @@ func RestoreIPTables(ctx context.Context, cfg config.InitializedConfig) (string, output += ipv6Output } - cfg.Logger.Info("iptables set to divert the traffic to Envoy") - return output, nil } diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index 51b3731e154e..6a4abff4d862 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -91,16 +91,10 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { case !containsTProxyRules && !containsTProxyComments: // If there are no transparent proxy rules or chains, there is // nothing to do. - if cfg.Verbose { - cfg.Logger.Info( - "no transparent proxy rules detected. No cleanup necessary", - ) - } + cfg.Logger.Info("no transparent proxy rules detected. No cleanup necessary") return nil case containsTProxyRules && !containsTProxyComments: - return errors.New( - "transparent proxy rules detected, but expected comments are missing. Cleanup cannot proceed safely without comments to identify rules. Please remove the transparent proxy iptables rules manually", - ) + return errors.New("transparent proxy rules detected, but expected comments are missing. Cleanup cannot proceed safely without comments to identify rules. Please remove the transparent proxy iptables rules manually") } // Split the output into lines and remove lines related to transparent From 30c7139c11b434ff13125f27d7c6d6dfed44d759 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Mon, 15 Jul 2024 13:11:00 +0200 Subject: [PATCH 18/20] Address review remarks Signed-off-by: Bart Smykla --- .../uninstall/uninstall_transparent_proxy.go | 2 +- .../config/config_executables.go | 101 +++--------------- pkg/transparentproxy/config/config_logger.go | 60 +---------- 3 files changed, 23 insertions(+), 140 deletions(-) diff --git a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go index 899455ca4498..076748098a79 100644 --- a/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go +++ b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go @@ -62,7 +62,7 @@ func newUninstallTransparentProxy() *cobra.Command { } if !initializedConfig.DryRun { - initializedConfig.Logger.InfoWithoutPrefix("transparent proxy cleanup completed successfully") + initializedConfig.Logger.Info("transparent proxy cleanup completed successfully") } return nil diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index 3f2de07937a0..9beaefa8a3e3 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -277,46 +277,31 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( // Create a backup file for existing iptables rules. backupFile, err := createBackupFile(c.IptablesRestore.prefix) if err != nil { - return "", errors.Wrap( - err, - "failed to create backup file for iptables rules", - ) + return "", errors.Wrap(err, "failed to create backup file for iptables rules") } defer backupFile.Close() // Save the current iptables rules to the backup file. stdout, _, err := c.IptablesSave.Exec(ctx) if err != nil { - return "", errors.Wrap( - err, - "failed to execute iptables-save command", - ) + return "", errors.Wrap(err, "failed to execute iptables-save command") } if err := writeToFile(stdout.String(), backupFile); err != nil { - return "", errors.Wrap( - err, - "failed to write current iptables rules to backup file", - ) + return "", errors.Wrap(err, "failed to write current iptables rules to backup file") } // Create a temporary file for the new iptables rules. restoreFile, err := createTempFile(c.IptablesRestore.prefix) if err != nil { - return "", errors.Wrap( - err, - "failed to create temporary file for new iptables rules", - ) + return "", errors.Wrap(err, "failed to create temporary file for new iptables rules") } defer restoreFile.Close() defer os.Remove(restoreFile.Name()) // Write the new iptables rules to the temporary file. if err := writeToFile(rules, restoreFile); err != nil { - return "", errors.Wrap( - err, - "failed to write new iptables rules to temporary file", - ) + return "", errors.Wrap(err, "failed to write new iptables rules to temporary file") } // Attempt to restore the new iptables rules from the temporary file. @@ -328,11 +313,7 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( c.logger.Warnf("restoring backup failed: %s", err) } - return "", errors.Wrapf( - err, - "failed to restore rules from file: %s", - restoreFile.Name(), - ) + return "", errors.Wrapf(err, "failed to restore rules from file: %s", restoreFile.Name()) } return output, nil @@ -561,14 +542,6 @@ func (c ExecutablesNftLegacy) Initialize( // Returns: // - []string: A slice of strings representing the constructed flags for // iptables-restore. -// -// Example: -// -// buildRestoreArgs(Config{Wait: 5, WaitInterval: 2}, IptablesModeNft) -// # Returns: []string{} -// -// buildRestoreArgs(Config{Wait: 5, WaitInterval: 2}, IptablesModeLegacy) -// # Returns: []string{"--wait=5", "--wait-interval=2"} func buildRestoreArgs(cfg Config, mode IptablesMode) []string { var flags []string @@ -812,11 +785,7 @@ func configureIPv6OutboundAddress() error { return nil } - return errors.Wrapf( - err, - "failed to add IPv6 address %s to loopback interface", - addr.IPNet, - ) + return errors.Wrapf(err, "failed to add IPv6 address %s to loopback interface", addr.IPNet) } return nil @@ -825,19 +794,6 @@ func configureIPv6OutboundAddress() error { // createBackupFile generates a backup file with a specified prefix and a // timestamp suffix. The file is created in the system's temporary directory and // is used to store iptables rules for backup purposes. -// -// This function performs the following steps: -// 1. Generates a timestamp suffix in the format "YYYY-MM-DD-HHMMSS". -// 2. Constructs the backup file name using the provided prefix and the -// timestamp suffix. -// 3. Creates the backup file in the system's temporary directory. -// -// Args: -// - prefix (string): The prefix to be used in the backup file name. -// -// Returns: -// - *os.File: A pointer to the created backup file. -// - error: An error if the file creation fails. func createBackupFile(prefix string) (*os.File, error) { // Generate a timestamp suffix for the backup file name. dateSuffix := time.Now().Format("2006-01-02-150405") @@ -850,31 +806,20 @@ func createBackupFile(prefix string) (*os.File, error) { // Create the backup file in the system's temporary directory. f, err := os.Create(filePath) if err != nil { - return nil, errors.Wrapf( - err, - "failed to create backup file: %s", - filePath, - ) + return nil, errors.Wrapf(err, "failed to create backup file: %s", filePath) } return f, nil } -// createTempFile generates a temporary file with a specified prefix. -// The file is created in the system's default temporary directory and is used -// for storing iptables rules temporarily. +// createTempFile generates a temporary file with a specified prefix. The file +// is created in the system's default temporary directory and is used for +// storing iptables rules temporarily. // // This function performs the following steps: // 1. Constructs a template for the temporary file name using the provided // prefix. // 2. Creates the temporary file in the system's default temporary directory. -// -// Args: -// - prefix (string): The prefix to be used in the temporary file name. -// -// Returns: -// - *os.File: A pointer to the created temporary file. -// - error: An error if the file creation fails. func createTempFile(prefix string) (*os.File, error) { // Construct a template for the temporary file name using the provided prefix. nameTemplate := fmt.Sprintf("%s-rules.*.txt", prefix) @@ -882,11 +827,7 @@ func createTempFile(prefix string) (*os.File, error) { // Create the temporary file in the system's default temporary directory. f, err := os.CreateTemp(os.TempDir(), nameTemplate) if err != nil { - return nil, errors.Wrapf( - err, - "failed to create temporary file with template: %s", - nameTemplate, - ) + return nil, errors.Wrapf(err, "failed to create temporary file with template: %s", nameTemplate) } return f, nil @@ -899,14 +840,6 @@ func createTempFile(prefix string) (*os.File, error) { // This function performs the following steps: // 1. Writes the content to the file using a buffered writer for efficiency. // 2. Flushes the buffer to ensure all data is written to the file. -// -// Args: -// - content (string): The content to be written to the file. -// - f (*os.File): The file to which the content should be written. -// -// Returns: -// - error: An error if writing or flushing the buffer fails, with detailed -// context. func writeToFile(content string, f *os.File) error { // Write the content to the file using a buffered writer. writer := bufio.NewWriter(f) @@ -915,9 +848,9 @@ func writeToFile(content string, f *os.File) error { } // Flush the buffer to ensure all data is written. - return errors.Wrapf( - writer.Flush(), - "failed to flush the buffered writer for file: %s", - f.Name(), - ) + if err := writer.Flush(); err != nil { + return errors.Wrapf(err, "failed to flush the buffered writer for file: %s", f.Name()) + } + + return nil } diff --git a/pkg/transparentproxy/config/config_logger.go b/pkg/transparentproxy/config/config_logger.go index 9b8580798b55..4918bb7e0869 100644 --- a/pkg/transparentproxy/config/config_logger.go +++ b/pkg/transparentproxy/config/config_logger.go @@ -9,30 +9,21 @@ import ( // logln writes a line of output to the specified writer. It directly passes the // provided arguments to the writer without any formatting or prefix. -// -// Args: -// - w (io.Writer): The writer where the line will be written. -// - a ([]any): The arguments to write to the writer. func logln(w io.Writer, a []any) { fmt.Fprintln(w, a...) } // loglnWithPrefixes writes a formatted line to the specified writer, // prefixing it with a hash (#) and additional prefixes for context. -// -// Args: -// - w (io.Writer): The writer where the line will be written. -// - a ([]any): The arguments to write to the writer, prefixed with a hash. -// - p (...string): Additional prefixes to add for context. -func (l Logger) loglnWithPrefixes(w io.Writer, a []any, p ...string) { - prefixes := []any{"#"} - for _, prefix := range slices.Concat(slices.Clone(l.defaultPrefixes), p) { +func (l Logger) loglnWithPrefixes(w io.Writer, args []any, prefixes ...string) { + finalPrefixes := []any{"#"} + for _, prefix := range slices.Concat(slices.Clone(l.defaultPrefixes), prefixes) { if prefix != "" { - prefixes = append(prefixes, fmt.Sprintf("[%s]", prefix)) + finalPrefixes = append(finalPrefixes, fmt.Sprintf("[%s]", prefix)) } } - logln(w, slices.Concat(prefixes, a)) + logln(w, slices.Concat(finalPrefixes, args)) } // Logger provides simple logging capabilities directing output to specified @@ -53,12 +44,6 @@ type Logger struct { // WithPrefix returns a new Logger instance with the specified prefix added to // the default prefixes. -// -// Args: -// - prefix (string): The prefix to add to the logger. -// -// Returns: -// - Logger: A new Logger instance with the added prefix. func (l Logger) WithPrefix(prefix string) Logger { return Logger{ stdout: l.stdout, @@ -72,10 +57,6 @@ func (l Logger) WithPrefix(prefix string) Logger { // tryPrefix generates a logging prefix based on the current retry attempt. // It returns a formatted string showing the current attempt and the total // attempts, or an empty string if no retries are needed (maxTry <= 1). -// -// Returns: -// - string: The formatted prefix for the current retry attempt, or an empty -// string if no retries. func (l Logger) tryPrefix() string { if l.maxTry <= 1 { return "" @@ -85,79 +66,48 @@ func (l Logger) tryPrefix() string { } // Info logs standard messages to stdout, prefixed with a hash. -// -// Args: -// - a (...any): The arguments to log to stdout. func (l Logger) Info(a ...any) { l.loglnWithPrefixes(l.stdout, a) } // Infof logs formatted messages to stdout, prefixed with a hash. -// -// Args: -// - format (string): The format string for the log message. -// - a (...any): The arguments to format and log to stdout. func (l Logger) Infof(format string, a ...any) { l.Info(fmt.Sprintf(format, a...)) } // InfoWithoutPrefix logs messages to stdout without any prefix. -// -// Args: -// - a (...any): The arguments to log to stdout. func (l Logger) InfoWithoutPrefix(a ...any) { logln(l.stdout, a) } // InfoTry logs messages to stdout with retry prefix, showing the attempt number // if retries are configured. -// -// Args: -// - a (...any): The arguments to log to stdout with retry context. func (l Logger) InfoTry(a ...any) { l.loglnWithPrefixes(l.stdout, a, l.tryPrefix()) } // Warn logs warning messages to stderr, prefixed with "[WARNING]:". -// -// Args: -// - a (...any): The arguments to log as a warning to stderr. func (l Logger) Warn(a ...any) { l.loglnWithPrefixes(l.stderr, a, "WARNING") } // Warnf logs formatted warning messages to stderr, prefixed with "[WARNING]:". -// -// Args: -// - format (string): The format string for the warning message. -// - a (...any): The arguments to format and log as a warning to stderr. func (l Logger) Warnf(format string, a ...any) { l.Warn(fmt.Sprintf(format, a...)) } // Error logs error messages to stderr, prefixed with a hash. -// -// Args: -// - a (...any): The arguments to log as an error to stderr. func (l Logger) Error(a ...any) { l.loglnWithPrefixes(l.stderr, a) } // Errorf logs formatted error messages to stderr, prefixed with a hash. -// -// Args: -// - format (string): The format string for the error message. -// - a (...any): The arguments to format and log as an error to stderr. func (l Logger) Errorf(format string, a ...any) { l.Error(fmt.Sprintf(format, a...)) } // ErrorTry logs error messages with retry context to stderr, transforming // multi-line error messages into a single line. -// -// Args: -// - err (error): The error to log. -// - a (...any): Additional context to log with the error message. func (l Logger) ErrorTry(err error, a ...any) { // Convert multi-line errors to a single line for clarity in logs. errInOneLine := strings.ReplaceAll(err.Error(), "\n", "") From a514b14ba0411b17266970266a8605437e8ce550 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Mon, 15 Jul 2024 13:32:09 +0200 Subject: [PATCH 19/20] Remove unnecessary parts of comments Signed-off-by: Bart Smykla --- pkg/transparentproxy/config/config.go | 78 ------------------- .../config/config_executables.go | 62 --------------- pkg/transparentproxy/iptables/setup.go | 42 +--------- .../transparentproxy/transparentproxy_test.go | 33 -------- 4 files changed, 2 insertions(+), 213 deletions(-) diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index 67357a4881ab..f4e5fc4499d5 100644 --- a/pkg/transparentproxy/config/config.go +++ b/pkg/transparentproxy/config/config.go @@ -33,14 +33,6 @@ type ValueOrRangeList string // converts it to a ValueOrRangeList, which is a comma-separated string // representation of the values. // -// Args: -// - v (T): The input value which can be a slice of uint16, a single uint16, -// or a string. -// -// Returns: -// - ValueOrRangeList: A comma-separated string representation of the input -// values. -// // The function panics if an unsupported type is provided, although the type // constraints should prevent this from occurring. func NewValueOrRangeList[T ~[]uint16 | ~uint16 | ~string](v T) ValueOrRangeList { @@ -241,15 +233,6 @@ type VNet struct { // it is added to the InterfaceCIDRs map. // 5. Constructs and returns an InitializedVNet struct containing the // populated InterfaceCIDRs map. -// -// Args: -// - ipv6 (bool): Indicates whether to process IPv6 addresses. If false, -// only IPv4 addresses are processed. -// -// Returns: -// - InitializedVNet: Struct containing the parsed interface names and -// corresponding CIDRs for the specified IP version. -// - error: Error indicating any issues encountered during initialization. func (c VNet) Initialize(ipv6 bool) (InitializedVNet, error) { initialized := InitializedVNet{InterfaceCIDRs: map[string]string{}} @@ -400,15 +383,6 @@ type InitializedComment struct { // iptables rule comments should be enabled. It checks the system's // functionality to see if the comment module is available and returns // an InitializedComment struct with the result. -// -// Args: -// - e (InitializedExecutablesIPvX): The initialized executables containing -// system functionality details, including available modules. -// -// Returns: -// - InitializedComment: The struct containing the processed comment -// configuration, indicating whether comments are enabled and the prefix -// to use for comments ("kuma/mesh/transparent/proxy"). func (c Comment) Initialize(e InitializedExecutablesIPvX) InitializedComment { return InitializedComment{ Enabled: !c.Disabled && e.Functionality.Modules.Comment, @@ -737,15 +711,6 @@ func getLoopbackInterfaceName() (string, error) { // - "udp:53:1001" (UDP protocol, port 53, UID 1001) // - "80:1002" (Any protocol, port 80, UID 1002) // - "1003" (Any protocol, any port, UID 1003) -// -// Args: -// - exclusionRules ([]string): A slice of strings specifying port exclusion -// rules based on UIDs. -// -// Returns: -// - []Exclusion: A slice of Exclusion structs representing the parsed port -// exclusion rules. -// - error: An error if the input format is invalid or if validation fails. func parseExcludePortsForUIDs(exclusionRules []string) ([]Exclusion, error) { var result []Exclusion @@ -836,22 +801,6 @@ func parseExcludePortsForUIDs(exclusionRules []string) ([]Exclusion, error) { // This function currently allows each exclusion rule to be a valid IPv4 or IPv6 // address, with or without a CIDR suffix. It is designed to potentially support // more complex exclusion rules in the future. -// -// Examples: -// - "10.0.0.1" -// - "10.0.0.0/8" -// - "fe80::1" -// - "fe80::/10" -// -// Args: -// - exclusionRules ([]string): A slice of strings specifying port exclusion -// rules based on IP addresses. -// - ipv6 (bool): A boolean flag indicating whether the rules are for IPv6. -// -// Returns: -// - []IPToPorts: A slice of IPToPorts structs representing the parsed port -// exclusion rules. -// - error: An error if the input format is invalid or if validation fails. func parseExcludePortsForIPs( exclusionRules []string, ipv6 bool, @@ -883,15 +832,6 @@ func parseExcludePortsForIPs( // validateUintValueOrRange validates whether a given string represents a valid // single uint16 value or a range of uint16 values. The input string can contain // multiple comma-separated values or ranges (e.g., "80,1000-2000"). -// -// Args: -// - valueOrRange (string): The input string to validate, which can be a -// single value, a range of values, or a comma-separated list of -// values/ranges. -// -// Returns: -// - error: An error if any value or range in the input string is not a valid -// uint16 value. func validateUintValueOrRange(valueOrRange string) error { for _, element := range strings.Split(valueOrRange, ",") { for _, port := range strings.Split(element, "-") { @@ -910,17 +850,6 @@ func validateUintValueOrRange(valueOrRange string) error { // validateIP validates an IP address or CIDR and checks if it matches the // expected IP version (IPv4 or IPv6). -// -// Args: -// - address (string): The IP address or CIDR to validate. -// - ipv6 (bool): A boolean flag indicating whether the expected IP version is -// IPv6. -// -// Returns: -// - error: An error if the IP address is invalid, with a message explaining -// the expected format. -// - bool: A boolean indicating whether the IP address matches the expected IP -// version (true for a match, false otherwise). func validateIP(address string, ipv6 bool) (error, bool) { // Attempt to parse the address as a CIDR. ip, _, err := net.ParseCIDR(address) @@ -945,13 +874,6 @@ func validateIP(address string, ipv6 bool) (error, bool) { // parseUint16 parses a string representing a uint16 value and returns its // uint16 representation. -// -// Args: -// - port (string): The input string to parse. -// -// Returns: -// - uint16: The parsed uint16 value. -// - error: An error if the input string is not a valid uint16 value. func parseUint16(port string) (uint16, error) { parsedPort, err := strconv.ParseUint(port, 10, 16) if err != nil { diff --git a/pkg/transparentproxy/config/config_executables.go b/pkg/transparentproxy/config/config_executables.go index 9beaefa8a3e3..4a63930b64da 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -167,17 +167,6 @@ type InitializedExecutablesIPvX struct { // additional arguments. It writes the rules to a temporary file and tries // to restore the iptables rules from this file. If the command fails, it // retries the specified number of times. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - f (*os.File): The file containing the iptables rules to restore. -// - quiet (bool): If true, suppresses verbose logging of the restore process. -// - args (...string): Additional arguments for the iptables-restore command. -// -// Returns: -// - string: The standard output from the iptables-restore command if -// successful. -// - error: An error if the iptables-restore command fails after all retries. func (c InitializedExecutablesIPvX) restore( ctx context.Context, f *os.File, @@ -221,16 +210,6 @@ func (c InitializedExecutablesIPvX) restore( // --noflush flag to ensure that the current rules are not flushed before // restoring. This function is a wrapper around the restore function with the // --noflush flag. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - rules (string): The iptables rules to restore. -// - quiet (bool): If true, suppresses verbose logging of the restore process. -// -// Returns: -// - string: The standard output from the iptables-restore command if -// successful. -// - error: An error if the iptables-restore command fails after all retries. func (c InitializedExecutablesIPvX) Restore( ctx context.Context, rules string, @@ -259,16 +238,6 @@ func (c InitializedExecutablesIPvX) Restore( // RestoreWithFlush executes the iptables-restore command with the given rules, // allowing the current rules to be flushed before restoring. This function is // a wrapper around the restore function without the --noflush flag. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - rules (string): The iptables rules to restore. -// - quiet (bool): If true, suppresses verbose logging of the restore process. -// -// Returns: -// - string: The standard output from the iptables-restore command if -// successful. -// - error: An error if the iptables-restore command fails after all retries. func (c InitializedExecutablesIPvX) RestoreWithFlush( ctx context.Context, rules string, @@ -326,16 +295,6 @@ func (c InitializedExecutablesIPvX) RestoreWithFlush( // ensure that the iptables rules specified in the `rules` string are valid. If // the rules are valid, it returns the output from iptables-restore. If there is // an error, it returns the error message. -// -// Args: -// - ctx (context.Context): The context for controlling the execution of the -// iptables-restore command. -// - rules (string): The iptables rules to be tested. -// -// Returns: -// - string: The output from iptables-restore if the rules are valid. -// - error: An error message if the rules are invalid or if there is an issue -// running iptables-restore. func (c InitializedExecutablesIPvX) RestoreTest( ctx context.Context, rules string, @@ -399,17 +358,6 @@ func NewExecutables(mode IptablesMode) Executables { // 3. If IPv6 initialization is successful, it attempts to configure the IPv6 // outbound address. If this configuration fails, a warning is logged, and // IPv6 rules will be skipped. -// -// Args: -// - ctx (context.Context): The context for managing request lifetime. -// - cfg (Config): Configuration settings for initializing the executables. -// - l (Logger): Logger for logging initialization steps and errors. -// -// Returns: -// - InitializedExecutables: Struct containing the initialized executables -// for both IPv4 and IPv6. -// - error: Error indicating the failure of either IPv4 or IPv6 -// initialization. func (c Executables) Initialize( ctx context.Context, l Logger, @@ -532,16 +480,6 @@ func (c ExecutablesNftLegacy) Initialize( // flags are required. // - For legacy mode, it conditionally adds the `--wait` and `--wait-interval` // flags based on the provided configuration values. -// -// Args: -// - cfg (Config): The configuration struct containing wait times for -// iptables-restore. -// - mode (consts.IptablesMode): The mode of iptables in use, determining the -// applicability of the wait flags. -// -// Returns: -// - []string: A slice of strings representing the constructed flags for -// iptables-restore. func buildRestoreArgs(cfg Config, mode IptablesMode) []string { var flags []string diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index 6a4abff4d862..d91db6c5588d 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -29,19 +29,9 @@ func Setup(ctx context.Context, cfg config.InitializedConfig) (string, error) { // Cleanup removes iptables rules and chains related to the transparent proxy // for both IPv4 and IPv6 configurations. It calls the internal cleanupIPvX -// function for each IP version, ensuring that only the relevant rules and +// function for each IP version, ensuring that only the relevant rules andran // chains are removed based on the presence of iptables comments. If either // cleanup process fails, an error is returned. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - cfg (config.InitializedConfig): The configuration containing the -// iptables settings for both IPv4 and IPv6, including comments and redirect -// information. -// -// Returns: -// - error: An error if the cleanup process for either IPv4 or IPv6 fails, -// including specific context about which IP version encountered the error. func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { if err := cleanupIPvX(ctx, cfg.IPv4); err != nil { return errors.Wrap(err, "failed to cleanup IPv4 rules") @@ -58,14 +48,6 @@ func Cleanup(ctx context.Context, cfg config.InitializedConfig) error { // proxy, ensuring that only the relevant rules and chains are removed based on // the presence of iptables comments and chain name prefixes. It verifies the // new rules after cleanup and restores them if they are valid. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - cfg (config.InitializedConfigIPvX): The configuration containing the -// iptables settings, including comments and redirect information. -// -// Returns: -// - error: An error if the cleanup process or verification fails. func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { if !cfg.Enabled() { return nil @@ -161,18 +143,6 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { // 4. Logs the final combined output using the configured logger without // prefixing, to ensure that the output is clear and unmodified, suitable // for review or documentation purposes. -// -// Args: -// -// - cfg (config.InitializedConfig): Configuration settings that include flags -// for dry run, logging, and IP version preferences. -// -// Returns: -// -// - string: A combined string of formatted iptables commands for both IPv4 -// and IPv6. -// - error: An error if there is a failure in generating the iptables commands -// for any version. func dryRun(cfg config.InitializedConfig) string { output := strings.Join( slices.Concat( @@ -190,15 +160,7 @@ func dryRun(cfg config.InitializedConfig) string { // dryRunIPvX generates iptables rules for either IPv4 or IPv6 based on the // provided configuration. It returns a slice with a header indicating the // IP version and the generated rules as a single string. -// -// Args: -// - cfg (config.InitializedConfigIPvX): Configuration settings for IPv4 or -// IPv6. -// - ipv6 (bool): Indicates if the configuration is for IPv6. -// -// Returns: -// - []string: A slice containing the header and the iptables rules for the -// specified IP version. + func dryRunIPvX(cfg config.InitializedConfigIPvX, ipv6 bool) []string { if !cfg.Enabled() { return nil diff --git a/test/transparentproxy/transparentproxy_test.go b/test/transparentproxy/transparentproxy_test.go index b067e1afecf3..5cf301d40359 100644 --- a/test/transparentproxy/transparentproxy_test.go +++ b/test/transparentproxy/transparentproxy_test.go @@ -272,12 +272,6 @@ func EnsureUninstallSuccessful(ctx context.Context, c testcontainers.Container) // file. // - If the command exits unsuccessfully, it checks if the error is due to the // executable not being found and handles it accordingly. -// -// Args: -// - ctx (context.Context): The context for controlling the command execution. -// - c (testcontainers.Container): The test container where the commands will -// be executed. -// - tc (testCase): The test case containing the name and golden file suffix. func EnsureGoldenFiles( ctx context.Context, c testcontainers.Container, @@ -326,15 +320,6 @@ func EnsureGoldenFiles( // genEntriesForImages generates Ginkgo test entries for Transparent Proxy // (tproxy) installation scenarios across a set of Docker images. -// -// Args: -// - images (map[string]string): A map of Docker image names for testing. -// - entry (func(description interface{}, args ...interface{}) TableEntry): -// A function to create Ginkgo TableEntry objects. -// - decorators (...interface{}): Optional decorators for each TableEntry. -// -// Returns: -// - []TableEntry: A slice of Ginkgo test entries for each Docker image. func genEntriesForImages( images map[string]string, entry func(description interface{}, args ...interface{}) TableEntry, @@ -478,15 +463,6 @@ func genEntriesForImage( // provided container and collects their outputs. The outputs are cleaned and // returned in a map where the keys are the command names and the values are // their respective outputs. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - container (testcontainers.Container): The container from which to -// retrieve the iptables-save output. -// -// Returns: -// - map[string]string: A map containing the iptables-save command outputs, -// keyed by command name. func getIptablesSaveOutput( ctx context.Context, container testcontainers.Container, @@ -517,15 +493,6 @@ func getIptablesSaveOutput( // iptables rules, attempting to add each rule using each iptables command. // If all attempts to add the rules using all iptables commands fail, it // returns an error detailing the failures. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - c (testcontainers.Container): The container to which the custom -// iptables rules will be added. -// -// Returns: -// - error: An error if all attempts to add the custom iptables rules -// fail, nil otherwise. func addCustomIptablesRules( ctx context.Context, c testcontainers.Container, From d16e9548d195bbf8dec95ed54dcedf4467a952d1 Mon Sep 17 00:00:00 2001 From: Bart Smykla Date: Mon, 15 Jul 2024 13:57:21 +0200 Subject: [PATCH 20/20] Chore another comment cleanup Signed-off-by: Bart Smykla --- pkg/transparentproxy/iptables/setup.go | 5 ----- test/transparentproxy/transparentproxy_test.go | 13 ------------- 2 files changed, 18 deletions(-) diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index d91db6c5588d..57f4bde547f4 100644 --- a/pkg/transparentproxy/iptables/setup.go +++ b/pkg/transparentproxy/iptables/setup.go @@ -82,11 +82,6 @@ func cleanupIPvX(ctx context.Context, cfg config.InitializedConfigIPvX) error { // Split the output into lines and remove lines related to transparent // proxy rules and chains. lines := strings.Split(output, "\n") - // Remove lines related to transparent proxy rules and chains. - // This includes: - // 1. Lines starting with "#" (comments). - // 2. Lines containing the comment prefix specified in the config. - // 3. Lines containing the redirect name prefix specified in the config. linesCleaned := slices.DeleteFunc( lines, func(line string) bool { diff --git a/test/transparentproxy/transparentproxy_test.go b/test/transparentproxy/transparentproxy_test.go index 5cf301d40359..b4dcf0446304 100644 --- a/test/transparentproxy/transparentproxy_test.go +++ b/test/transparentproxy/transparentproxy_test.go @@ -227,19 +227,6 @@ func EnsureInstallSuccessful( // EnsureUninstallSuccessful runs the `kumactl uninstall transparent-proxy` // command in the specified container and checks if the uninstallation is // successful. -// -// Args: -// - ctx (context.Context): The context for command execution. -// - c (testcontainers.Container): The container where the command will run. -// -// This function performs the following steps: -// 1. Executes the `kumactl uninstall transparent-proxy` command inside the -// specified container using the provided context. -// 2. Checks if the command execution returns an error. If an error occurs, the -// function fails the test. -// 3. If the command exit code is not zero, it reads the command output, copies -// it to a buffer, and fails the test with a message containing the exit -// code and the command output. func EnsureUninstallSuccessful(ctx context.Context, c testcontainers.Container) { GinkgoHelper()