Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(transparent-proxy): add option to uninstall transparent proxy #10890

Merged
merged 20 commits into from
Jul 15, 2024

Conversation

bartsmykla
Copy link
Contributor

@bartsmykla bartsmykla commented Jul 14, 2024

Tip

I recommend reviewing each commit individually, as I've organized the work into self-contained, descriptive commits for easier understanding.

Closes #6093

Add logic for Transparent Proxy uninstallation/cleanup

  1. Uninstallation/Cleanup Logic:
    • Added functionality to clean up and uninstall the transparent proxy setup
    • Removed iptables rules and chains related to the transparent proxy, ensuring only relevant rules and chains are removed based on the presence of iptables comments
    • Validated the new rules after cleanup and restored them if they are valid
    • 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
  2. Installation Process Improvement:
    • Added a cleanup step at the beginning of the installation process to remove any existing transparent proxy rules before setting up new rules
  3. Detailed Logging:
    • Improved log messages for better clarity and debugging
    • Logged the successful completion of the transparent proxy cleanup process

Improvements in logging

  • Removed redundant log after restoring iptables: iptables set to divert the traffic to Envoy
  • Consolidated log and error message content into single lines for easier searching in IDEs or code editors
  • Updated Logger instances passed to initialization functions in configs to include appropriate prefixes ([iptables] or [ip6tables])

Add Tests for Transparent Proxy uninstallation

Remove unit tests for uninstalling Transparent Proxy

  • Removed non-functional unit tests for uninstalling the transparent proxy that served only as placeholders due to difficulties in idempotently testing across environments

Remove unused string from cleanup signatures

  • Updated function signatures as Cleanup no longer returns any output

Set IPv6 in DefaultConfig value to true

  • Ensured the default installation of the transparent proxy supports both IPv4 and IPv6, aligning with the --ip-family-mode default value of dualstack

Refactor Executables initialization for better error handling and logging

  1. Method Refactoring:
    • Split the writeRulesToFile method into createTempFile and writeToFile
    • Added createBackupFile for better modularity and reusability
  2. Restore Functionality Enhancements:
    • Refactored restore, Restore, RestoreWithFlush, and RestoreTest methods for better iptables rule restoration and validation, incorporating improved error handling and logging
    • Implemented robust backup and restore mechanisms in RestoreWithFlush
  3. Documentation Improvements:
    • Added detailed comments and descriptions for several functions to enhance code readability and maintainability

Enhance descriptions for Logger methods

  • Improved comments and descriptions for the logger methods within the Logger struct for better readability and maintainability

Add new Logger methods Errorf and Infof

  • Introduced Errorf and Infof methods to the Logger struct for formatted logging of error and information messages

Add RestoreTest method to InitializedExecutablesIPvX

  • Added a method to run iptables-restore with the --test flag to validate iptables rules without applying them

Refactor iptables restore logic and add RestoreWithFlush method

  • Refactored the logic for executing the iptables-restore command, introducing a new internal restore function to handle common restore logic, and added RestoreWithFlush method for scenarios requiring rule flushing

Remove the ability to change the iptables comments prefix

  • Removed the configurability of the iptables comments prefix to simplify the uninstallation process and reduce the risk of errors

Checklist prior to review

  • Link to relevant issue as well as docs and UI issues
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS
    • It won't
  • Tests (Unit test, E2E tests, manual test on universal and k8s)
    • Don't forget ci/ labels to run additional/fewer tests
  • Do you need to update UPGRADE.md?
    • There is no need
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label)
    • There is no need

@bartsmykla bartsmykla added the ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully) label Jul 14, 2024
@bartsmykla bartsmykla force-pushed the feat/add-option-to-uninstall-tproxy branch from 2992885 to 29a05d6 Compare July 15, 2024 04:31
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
…ging

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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Cleanup no longer returns any output, so the function signatures are
updated accordingly.

Signed-off-by: Bart Smykla <[email protected]>
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 <[email protected]>
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 <[email protected]>
- 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 <[email protected]>
- 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 <[email protected]>
@bartsmykla bartsmykla force-pushed the feat/add-option-to-uninstall-tproxy branch from 29a05d6 to f76b233 Compare July 15, 2024 05:31
@bartsmykla bartsmykla marked this pull request as ready for review July 15, 2024 05:33
@bartsmykla bartsmykla requested a review from a team as a code owner July 15, 2024 05:33
@bartsmykla bartsmykla marked this pull request as draft July 15, 2024 08:14
@bartsmykla bartsmykla marked this pull request as ready for review July 15, 2024 09:42
Signed-off-by: Bart Smykla <[email protected]>
pkg/transparentproxy/iptables/setup.go Outdated Show resolved Hide resolved
test/transparentproxy/transparentproxy_test.go Outdated Show resolved Hide resolved
@bartsmykla bartsmykla enabled auto-merge (squash) July 15, 2024 12:19
@bartsmykla bartsmykla merged commit addad99 into master Jul 15, 2024
35 checks passed
@bartsmykla bartsmykla deleted the feat/add-option-to-uninstall-tproxy branch July 15, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/run-full-matrix PR: Runs all possible e2e test combination (expensive use carefully)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce logic to uninstall iptable rules
3 participants