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..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 @@ -230,7 +232,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/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/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.go b/app/kumactl/cmd/uninstall/uninstall_transparent_proxy.go index 33fec929f7dc..076748098a79 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") } - 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 +45,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 +61,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.Info("transparent proxy cleanup completed successfully") + } return nil }, 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", - }), - ) -}) diff --git a/pkg/transparentproxy/config/config.go b/pkg/transparentproxy/config/config.go index f6e7fdb88d80..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 { @@ -171,10 +163,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") } } @@ -244,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{}} @@ -381,14 +361,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 @@ -408,19 +383,10 @@ 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. func (c Comment) Initialize(e InitializedExecutablesIPvX) InitializedComment { return InitializedComment{ Enabled: !c.Disabled && e.Functionality.Modules.Comment, - Prefix: c.Prefix, + Prefix: IptablesRuleCommentPrefix, } } @@ -580,11 +546,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, + Logger: loggerIPv4, LocalhostCIDR: LocalhostCIDRIPv4, InboundPassthroughCIDR: InboundPassthroughSourceAddressCIDRIPv4, enabled: true, @@ -601,7 +570,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, @@ -625,7 +594,7 @@ func (c Config) Initialize(ctx context.Context) (InitializedConfig, error) { if c.IPv6 { initialized.IPv6 = InitializedConfigIPvX{ Config: c, - Logger: l, + Logger: loggerIPv6, Executables: e.IPv6, LoopbackInterfaceName: loopbackInterfaceName, LocalhostCIDR: LocalhostCIDRIPv6, @@ -635,7 +604,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, @@ -688,7 +657,7 @@ func DefaultConfig() Config { ProgramsSourcePath: "/tmp/kuma-ebpf", }, DropInvalidPackets: false, - IPv6: false, + IPv6: true, RuntimeStdout: os.Stdout, RuntimeStderr: os.Stderr, Verbose: false, @@ -708,7 +677,6 @@ func DefaultConfig() Config { Executables: NewExecutablesNftLegacy(), Comment: Comment{ Disabled: false, - Prefix: IptablesRuleCommentPrefix, }, } } @@ -743,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 @@ -842,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, @@ -889,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, "-") { @@ -916,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) @@ -939,8 +862,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 } @@ -952,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 f7baf33d1b04..4a63930b64da 100644 --- a/pkg/transparentproxy/config/config_executables.go +++ b/pkg/transparentproxy/config/config_executables.go @@ -163,107 +163,171 @@ 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() +// 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. +func (c InitializedExecutablesIPvX) restore( + ctx context.Context, + f *os.File, + quiet bool, + args ...string, +) (string, error) { + args = append(args, f.Name()) + + argsAll := slices.DeleteFunc( + slices.Concat(c.IptablesRestore.args, args), + func(s string) bool { return s == "" }, + ) - // Remove the temporary file if an error occurs after creation. - defer func() { - if err != nil { - os.Remove(f.Name()) + for i := 0; i <= c.retry.MaxRetries; i++ { + c.logger.try = i + 1 + + if !quiet { + c.logger.InfoTry(c.IptablesRestore.Path, strings.Join(argsAll, " ")) } - }() - // 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)) + stdout, _, err := c.IptablesRestore.Exec(ctx, args...) + if err == nil { + return stdout.String(), nil + } - // 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(), - ) - } + c.logger.ErrorTry(err, "restoring failed:") - // 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(), - ) + if i < c.retry.MaxRetries { + if !quiet { + c.logger.InfoTry("will try again in", c.retry.SleepBetweenReties) + } + + time.Sleep(c.retry.SleepBetweenReties) + } } - // Return the file path. - return f.Name(), nil + 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. func (c InitializedExecutablesIPvX) Restore( ctx context.Context, rules string, + quiet bool, ) (string, error) { - fileName, err := c.writeRulesToFile(rules) + f, err := createTempFile(c.IptablesRestore.prefix) if err != nil { return "", err } - defer os.Remove(fileName) + defer f.Close() + defer os.Remove(f.Name()) - for i := 0; i <= c.retry.MaxRetries; i++ { - c.logger.try = i + 1 + // Log the file name and the rules to be written. + if !quiet { + c.logger.Info("writing the following rules to file:", f.Name()) + c.logger.InfoWithoutPrefix(strings.TrimSpace(rules)) + } - c.logger.InfoTry( - c.IptablesRestore.Path, - strings.Join(c.IptablesRestore.args, " "), - fileName, - ) + if err := writeToFile(rules, f); err != nil { + return "", err + } - stdout, _, err := c.IptablesRestore.Exec(ctx, fileName) - if err == nil { - return stdout.String(), nil + return c.restore(ctx, f, quiet, 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. +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) + 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, quiet) + if err != nil { + c.logger.Errorf("restoring backup file: %s", backupFile.Name()) + + if _, err := c.restore(ctx, backupFile, quiet); err != nil { + c.logger.Warnf("restoring backup failed: %s", err) } - c.logger.ErrorTry(err, "restoring failed:") + return "", errors.Wrapf(err, "failed to restore rules from file: %s", restoreFile.Name()) + } - if i < c.retry.MaxRetries { - c.logger.InfoTry("will try again in", c.retry.SleepBetweenReties) - time.Sleep(c.retry.SleepBetweenReties) + return output, nil +} + +// 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. +func (c InitializedExecutablesIPvX) RestoreTest( + ctx context.Context, + rules string, +) (string, error) { + 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 { + // 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") } - return "", errors.Errorf("%s failed", c.IptablesRestore.Path) + return stdout.String(), nil } type Executables struct { @@ -294,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, @@ -314,8 +367,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", @@ -323,8 +379,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", @@ -333,8 +392,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, + ) } } @@ -384,9 +445,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: @@ -403,11 +462,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 @@ -417,37 +472,16 @@ 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. -// -// 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. -// -// Returns: -// -// []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"} +// flags based on the provided configuration values. func buildRestoreArgs(cfg Config, mode IptablesMode) []string { - flags := []string{FlagNoFlush} + var flags []string if mode != IptablesModeLegacy { return flags @@ -689,11 +723,71 @@ 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 +} + +// 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. +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. +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. +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. + 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 3e1321f9b5f9..4918bb7e0869 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" ) @@ -12,36 +13,66 @@ 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. -func loglnWithHashPrefix(w io.Writer, a []any) { - logln(w, append([]any{"#"}, a...)) +// loglnWithPrefixes writes a formatted line to the specified writer, +// prefixing it with a hash (#) and additional prefixes for context. +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 != "" { + finalPrefixes = append(finalPrefixes, fmt.Sprintf("[%s]", prefix)) + } + } + + logln(w, slices.Concat(finalPrefixes, args)) } // 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. +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). -func (l Logger) tryPrefix() []any { +// attempts, or an empty string if no retries are needed (maxTry <= 1). +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. 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. +func (l Logger) Infof(format string, a ...any) { + l.Info(fmt.Sprintf(format, a...)) } // InfoWithoutPrefix logs messages to stdout without any prefix. @@ -52,30 +83,34 @@ func (l Logger) InfoWithoutPrefix(a ...any) { // InfoTry logs messages to stdout with retry prefix, showing the attempt number // if retries are configured. 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]:" +// Warn logs warning messages to stderr, prefixed with "[WARNING]:". func (l Logger) Warn(a ...any) { - loglnWithHashPrefix(l.stderr, append([]any{"[WARNING]:"}, a...)) + l.loglnWithPrefixes(l.stderr, a, "WARNING") } -// Warn logs warning messages to stderr, prefixed with "[WARNING]:" +// Warnf logs formatted warning messages to stderr, prefixed with "[WARNING]:". func (l Logger) Warnf(format string, a ...any) { l.Warn(fmt.Sprintf(format, a...)) } // Error logs error messages to stderr, prefixed with a hash. 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. +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) { - 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/builder/builder.go b/pkg/transparentproxy/iptables/builder/builder.go index e7e31b403042..8895fdde4864 100644 --- a/pkg/transparentproxy/iptables/builder/builder.go +++ b/pkg/transparentproxy/iptables/builder/builder.go @@ -50,13 +50,12 @@ 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, BuildIPTablesForRestore(cfg.IPv4), + false, ) if err != nil { return "", errors.Wrap(err, "unable to restore iptables rules") @@ -66,6 +65,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") @@ -74,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/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 diff --git a/pkg/transparentproxy/iptables/setup.go b/pkg/transparentproxy/iptables/setup.go index 173f26fbd435..57f4bde547f4 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,105 @@ 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 andran +// chains are removed based on the presence of iptables comments. If either +// cleanup process fails, an error is returned. +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 and chain name prefixes. It verifies the +// new rules after cleanup and restores them if they are valid. +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 { + 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. + 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") + } + + // 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 { + return strings.HasPrefix(line, "#") || + strings.Contains(line, cfg.Comment.Prefix) || + strings.Contains(line, cfg.Redirect.NamePrefix) + }, + ) + 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.Info("[dry-run]: rules after cleanup:") + cfg.Logger.InfoWithoutPrefix(strings.TrimSpace(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", + ) + } + + cfg.Logger.Info("cleanup of existing transparent proxy rules completed successfully") + + return nil } // dryRun simulates the setup of iptables rules for both IPv4 and IPv6 @@ -43,18 +138,6 @@ func Cleanup(_ config.InitializedConfig) (string, 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( @@ -72,15 +155,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/pkg/transparentproxy/transparentproxy.go b/pkg/transparentproxy/transparentproxy.go index 4a536d757da0..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(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(cfg) + return iptables.Cleanup(ctx, cfg) } diff --git a/test/transparentproxy/transparentproxy_test.go b/test/transparentproxy/transparentproxy_test.go index bd1d2b1587d1..b4dcf0446304 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,26 @@ func EnsureInstallSuccessful( } } +// EnsureUninstallSuccessful runs the `kumactl uninstall transparent-proxy` +// command in the specified container and checks if the uninstallation is +// successful. +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. @@ -155,12 +259,6 @@ func EnsureInstallSuccessful( // 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, @@ -209,15 +307,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, @@ -353,3 +442,77 @@ 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. +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. +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 +}