From e2b29dd773007dcb71a17e2814ab482cf002c92a Mon Sep 17 00:00:00 2001 From: Michael Schilonka Date: Thu, 25 Aug 2022 19:01:03 +0200 Subject: [PATCH 1/2] fix: fix CLI argument parser, set correct mtu for gefyra network --- client/gefyra/__main__.py | 43 ++++++++++++---- client/gefyra/api/down.py | 5 +- client/gefyra/api/up.py | 1 - client/gefyra/local/networking.py | 20 ++++++++ client/tests/test_up_args.py | 82 +++++++++++++++---------------- 5 files changed, 98 insertions(+), 53 deletions(-) diff --git a/client/gefyra/__main__.py b/client/gefyra/__main__.py index ba2975ae..8d5e603f 100644 --- a/client/gefyra/__main__.py +++ b/client/gefyra/__main__.py @@ -1,6 +1,7 @@ #!/usr/bin/env python3 import argparse import logging +import traceback from gefyra.api import get_containers_and_print, get_bridges_and_print from gefyra.configuration import ClientConfiguration @@ -19,14 +20,25 @@ ) action = parser.add_subparsers(dest="action", help="the action to be performed") parser.add_argument("-d", "--debug", action="store_true", help="add debug output") -parser.add_argument("--kubeconfig", required=False, help="path to kubeconfig file") -parser.add_argument("--context", required=False, help="context name from kubeconfig") +parser.add_argument( + "--kubeconfig", + dest="kube_config_file", + required=False, + help="path to kubeconfig file", +) +parser.add_argument( + "--context", + dest="kube_context", + required=False, + help="context name from kubeconfig", +) up_parser = action.add_parser("up") up_parser.add_argument( "-e", "--endpoint", + dest="cargo_endpoint", help="the Wireguard endpoint in the form : for Gefyra to connect to", required=False, ) @@ -41,35 +53,41 @@ up_parser.add_argument( "-o", "--operator", + dest="operator_image_url", help="Registry url for the operator image.", required=False, ) up_parser.add_argument( "-s", "--stowaway", + dest="stowaway_image_url", help="Registry url for the stowaway image.", required=False, ) up_parser.add_argument( "-c", "--carrier", + dest="carrier_image_url", help="Registry url for the carrier image.", required=False, ) up_parser.add_argument( "-a", "--cargo", + dest="cargo_image_url", help="Registry url for the cargo image.", required=False, ) up_parser.add_argument( "-r", "--registry", + dest="registry_url", help="Base url for registry to pull images from.", required=False, ) up_parser.add_argument( "--wireguard-mtu", + dest="wireguard_mtu", help="The MTU value for the local Wireguard endpoint (default: 1340).", ) run_parser = action.add_parser("run") @@ -234,27 +252,30 @@ def telemetry_command(on, off): def get_client_configuration(args) -> ClientConfiguration: configuration_params = {} - if args.kubeconfig: - configuration_params["kube_config_file"] = args.kubeconfig - if args.context: - configuration_params["kube_context"] = args.context - if args.action == "up": - if args.minikube and bool(args.endpoint): + if args.minikube and bool(args.cargo_endpoint): raise RuntimeError("You cannot use --endpoint together with --minikube.") if args.minikube: configuration_params.update(detect_minikube_config()) else: - if not args.endpoint: + if not args.cargo_endpoint: # #138: Read in the --endpoint parameter from kubeconf endpoint = get_connection_from_kubeconfig() if endpoint: logger.info(f"Setting --endpoint from kubeconfig {endpoint}") else: - endpoint = args.endpoint + endpoint = args.cargo_endpoint configuration_params["cargo_endpoint"] = endpoint + for argument in vars(args): + if argument not in ["action", "debug", "cargo_endpoint", "minikube"]: + configuration_params[argument] = getattr(args, argument) + else: + if args.kube_config_file: + configuration_params["kube_config_file"] = args.kube_config_file + if args.kube_context: + configuration_params["kube_context"] = args.kube_context configuration = ClientConfiguration(**configuration_params) @@ -333,6 +354,8 @@ def main(): else: parser.print_help() except Exception as e: + if args.debug: + traceback.print_exc() logger.fatal(f"There was an error running Gefyra: {e}") exit(1) exit(0) diff --git a/client/gefyra/api/down.py b/client/gefyra/api/down.py index 2ed111ab..27f8ad2b 100644 --- a/client/gefyra/api/down.py +++ b/client/gefyra/api/down.py @@ -19,7 +19,10 @@ def down(config=default_configuration) -> bool: from gefyra.local.bridge import remove_interceptrequest_remainder from gefyra.local.utils import set_gefyra_network_from_cargo - config = set_gefyra_network_from_cargo(config) + try: + config = set_gefyra_network_from_cargo(config) + except RuntimeError: + logger.info("Gefyra client is not running.") try: logger.info("Removing running bridges") remove_interceptrequest_remainder(config) diff --git a/client/gefyra/api/up.py b/client/gefyra/api/up.py index 779c2c7b..bb128102 100644 --- a/client/gefyra/api/up.py +++ b/client/gefyra/api/up.py @@ -27,7 +27,6 @@ def up(config=default_configuration) -> bool: # try: logger.debug("Creating Docker network") - # The 'pool overlap' error was not yet resolved other than retry gefyra_network = create_gefyra_network(config) cargo_connection_details = install_operator( diff --git a/client/gefyra/local/networking.py b/client/gefyra/local/networking.py index 4cc8dc98..551dc8f4 100644 --- a/client/gefyra/local/networking.py +++ b/client/gefyra/local/networking.py @@ -25,6 +25,25 @@ def handle_create_network(config: ClientConfiguration) -> Network: or network.attrs["Labels"][CREATED_BY_LABEL[0]] != "true" ): logger.debug(f"Docker network '{network.name}' is not managed by Gefyra") + if ( + "Options" in network.attrs + and "com.docker.network.driver.mtu" in network.attrs["Options"] + and network.attrs["Options"]["com.docker.network.driver.mtu"] + != config.WIREGUARD_MTU + ) or ( + "Options" in network.attrs + and "com.docker.network.driver.mtu" not in network.attrs["Options"] + ): + _mtu = ( + network.attrs["Options"].get("com.docker.network.driver.mtu") + if "Options" in network.attrs + else "default" + ) + logger.warning( + f"The MTU value of the 'gefyra' network (={_mtu}) is different from the --wireguard-mtu parameter " + f"(={config.WIREGUARD_MTU}) or default. You may experience bad network connections. Consider removing " + f"the network 'gefyra' with 'docker network rm gefyra' before running 'gefyra up'." + ) return network except NotFound: pass @@ -43,6 +62,7 @@ def handle_create_network(config: ClientConfiguration) -> Network: labels={ CREATED_BY_LABEL[0]: CREATED_BY_LABEL[1], }, + options={"com.docker.network.driver.mtu": config.WIREGUARD_MTU}, ) logger.info(f"Created network '{config.NETWORK_NAME}' ({network.short_id})") return network diff --git a/client/tests/test_up_args.py b/client/tests/test_up_args.py index 6ae6e5d2..8dd7b36e 100644 --- a/client/tests/test_up_args.py +++ b/client/tests/test_up_args.py @@ -14,110 +14,110 @@ def test_parse_registry_a(): args = parser.parse_args(["up", "--registry", REGISTRY_URL]) - configuration = ClientConfiguration(registry_url=args.registry) + configuration = ClientConfiguration(registry_url=args.registry_url) assert configuration.REGISTRY_URL == REGISTRY_URL def test_parse_registry_b(): args = parser.parse_args(["up", "--registry", "my-reg.io/gefyra/"]) - configuration = ClientConfiguration(registry_url=args.registry) + configuration = ClientConfiguration(registry_url=args.registry_url) assert configuration.REGISTRY_URL, REGISTRY_URL args = parser.parse_args(["up", "-r", "my-reg.io/gefyra/"]) - configuration = ClientConfiguration(registry_url=args.registry) + configuration = ClientConfiguration(registry_url=args.registry_url) assert configuration.REGISTRY_URL == REGISTRY_URL def test_parse_no_registry(): args = parser.parse_args(["up"]) - configuration = ClientConfiguration(registry_url=args.registry) + configuration = ClientConfiguration(registry_url=args.registry_url) assert configuration.REGISTRY_URL == QUAY_REGISTRY_URL def test_parse_no_stowaway_image(): args = parser.parse_args(["up"]) - configuration = ClientConfiguration(stowaway_image_url=args.stowaway) + configuration = ClientConfiguration(stowaway_image_url=args.stowaway_image_url) assert configuration.STOWAWAY_IMAGE == f"quay.io/gefyra/stowaway:{__VERSION__}" def test_parse_no_carrier_image(): args = parser.parse_args(["up"]) - configuration = ClientConfiguration(carrier_image_url=args.carrier) + configuration = ClientConfiguration(carrier_image_url=args.carrier_image_url) assert configuration.CARRIER_IMAGE == f"quay.io/gefyra/carrier:{__VERSION__}" def test_parse_no_operator_image(): args = parser.parse_args(["up"]) - configuration = ClientConfiguration(operator_image_url=args.operator) + configuration = ClientConfiguration(operator_image_url=args.operator_image_url) assert configuration.OPERATOR_IMAGE == f"quay.io/gefyra/operator:{__VERSION__}" def test_parse_no_cargo_image(): args = parser.parse_args(["up"]) - configuration = ClientConfiguration(cargo_image_url=args.cargo) + configuration = ClientConfiguration(cargo_image_url=args.cargo_image_url) assert configuration.CARGO_IMAGE == f"quay.io/gefyra/cargo:{__VERSION__}" def test_parse_stowaway_image(): args = parser.parse_args(["up", "--stowaway", STOWAWAY_LATEST]) - configuration = ClientConfiguration(stowaway_image_url=args.stowaway) + configuration = ClientConfiguration(stowaway_image_url=args.stowaway_image_url) assert configuration.STOWAWAY_IMAGE == STOWAWAY_LATEST args = parser.parse_args(["up", "-s", STOWAWAY_LATEST]) - configuration = ClientConfiguration(stowaway_image_url=args.stowaway) + configuration = ClientConfiguration(stowaway_image_url=args.stowaway_image_url) assert configuration.STOWAWAY_IMAGE == STOWAWAY_LATEST args = parser.parse_args(["up", "-s", STOWAWAY_LATEST, "-r", QUAY_REGISTRY_URL]) configuration = ClientConfiguration( - registry_url=args.registry, stowaway_image_url=args.stowaway + registry_url=args.registry_url, stowaway_image_url=args.stowaway_image_url ) assert configuration.STOWAWAY_IMAGE == STOWAWAY_LATEST def test_parse_cargo_image(): args = parser.parse_args(["up", "--cargo", CARGO_LATEST]) - configuration = ClientConfiguration(cargo_image_url=args.cargo) + configuration = ClientConfiguration(cargo_image_url=args.cargo_image_url) assert configuration.CARGO_IMAGE == CARGO_LATEST args = parser.parse_args(["up", "-a", CARGO_LATEST]) - configuration = ClientConfiguration(cargo_image_url=args.cargo) + configuration = ClientConfiguration(cargo_image_url=args.cargo_image_url) assert configuration.CARGO_IMAGE == CARGO_LATEST args = parser.parse_args(["up", "-a", CARGO_LATEST, "-r", QUAY_REGISTRY_URL]) configuration = ClientConfiguration( - registry_url=args.registry, cargo_image_url=args.cargo + registry_url=args.registry_url, cargo_image_url=args.cargo_image_url ) assert configuration.CARGO_IMAGE == CARGO_LATEST def test_parse_operator_image(): args = parser.parse_args(["up", "--operator", OPERATOR_LATEST]) - configuration = ClientConfiguration(operator_image_url=args.operator) + configuration = ClientConfiguration(operator_image_url=args.operator_image_url) assert configuration.OPERATOR_IMAGE == OPERATOR_LATEST args = parser.parse_args(["up", "-o", OPERATOR_LATEST]) - configuration = ClientConfiguration(operator_image_url=args.operator) + configuration = ClientConfiguration(operator_image_url=args.operator_image_url) assert configuration.OPERATOR_IMAGE == OPERATOR_LATEST args = parser.parse_args(["up", "-o", OPERATOR_LATEST, "-r", QUAY_REGISTRY_URL]) configuration = ClientConfiguration( - registry_url=args.registry, operator_image_url=args.operator + registry_url=args.registry_url, operator_image_url=args.operator_image_url ) assert configuration.OPERATOR_IMAGE == OPERATOR_LATEST def test_parse_carrier_image(): args = parser.parse_args(["up", "--carrier", CARRIER_LATEST]) - configuration = ClientConfiguration(carrier_image_url=args.carrier) + configuration = ClientConfiguration(carrier_image_url=args.carrier_image_url) assert configuration.CARRIER_IMAGE == CARRIER_LATEST args = parser.parse_args(["up", "-c", CARRIER_LATEST]) - configuration = ClientConfiguration(carrier_image_url=args.carrier) + configuration = ClientConfiguration(carrier_image_url=args.carrier_image_url) assert configuration.CARRIER_IMAGE == CARRIER_LATEST args = parser.parse_args(["up", "-c", CARRIER_LATEST, "-r", QUAY_REGISTRY_URL]) configuration = ClientConfiguration( - registry_url=args.registry, carrier_image_url=args.carrier + registry_url=args.registry_url, carrier_image_url=args.carrier_image_url ) assert configuration.CARRIER_IMAGE == CARRIER_LATEST @@ -125,11 +125,11 @@ def test_parse_carrier_image(): def test_parse_combination_a(): args = parser.parse_args(["up", "-c", CARRIER_LATEST]) configuration = ClientConfiguration( - registry_url=args.registry, - stowaway_image_url=args.stowaway, - operator_image_url=args.operator, - cargo_image_url=args.cargo, - carrier_image_url=args.carrier, + registry_url=args.registry_url, + stowaway_image_url=args.stowaway_image_url, + operator_image_url=args.operator_image_url, + cargo_image_url=args.cargo_image_url, + carrier_image_url=args.carrier_image_url, ) assert configuration.REGISTRY_URL == QUAY_REGISTRY_URL assert configuration.OPERATOR_IMAGE == f"quay.io/gefyra/operator:{__VERSION__}" @@ -139,11 +139,11 @@ def test_parse_combination_a(): def test_parse_combination_b(): args = parser.parse_args(["up", "-r", REGISTRY_URL]) configuration = ClientConfiguration( - registry_url=args.registry, - stowaway_image_url=args.stowaway, - operator_image_url=args.operator, - cargo_image_url=args.cargo, - carrier_image_url=args.carrier, + registry_url=args.registry_url, + stowaway_image_url=args.stowaway_image_url, + operator_image_url=args.operator_image_url, + cargo_image_url=args.cargo_image_url, + carrier_image_url=args.carrier_image_url, ) assert configuration.REGISTRY_URL == REGISTRY_URL assert configuration.OPERATOR_IMAGE == f"my-reg.io/gefyra/operator:{__VERSION__}" @@ -155,11 +155,11 @@ def test_parse_combination_c(): ["up", "-r", REGISTRY_URL, "-c", "quay.io/gefyra/carrier:latest"] ) configuration = ClientConfiguration( - registry_url=args.registry, - stowaway_image_url=args.stowaway, - operator_image_url=args.operator, - cargo_image_url=args.cargo, - carrier_image_url=args.carrier, + registry_url=args.registry_url, + stowaway_image_url=args.stowaway_image_url, + operator_image_url=args.operator_image_url, + cargo_image_url=args.cargo_image_url, + carrier_image_url=args.carrier_image_url, ) assert configuration.REGISTRY_URL == REGISTRY_URL assert configuration.OPERATOR_IMAGE == f"my-reg.io/gefyra/operator:{__VERSION__}" @@ -169,12 +169,12 @@ def test_parse_combination_c(): def test_parse_endpoint(): args = parser.parse_args(["up", "-e", "10.30.34.25:31820"]) configuration = ClientConfiguration( - cargo_endpoint=args.endpoint, - registry_url=args.registry, - stowaway_image_url=args.stowaway, - operator_image_url=args.operator, - cargo_image_url=args.cargo, - carrier_image_url=args.carrier, + cargo_endpoint=args.cargo_endpoint, + registry_url=args.registry_url, + stowaway_image_url=args.stowaway_image_url, + operator_image_url=args.operator_image_url, + cargo_image_url=args.cargo_image_url, + carrier_image_url=args.carrier_image_url, ) assert configuration.CARGO_ENDPOINT == "10.30.34.25:31820" From 5cb83cd18514f17f800a472cc52f731e55eccc52 Mon Sep 17 00:00:00 2001 From: Robert Stein Date: Fri, 26 Aug 2022 15:00:18 +0200 Subject: [PATCH 2/2] refactor: remove code smell through variable --- client/gefyra/local/networking.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/client/gefyra/local/networking.py b/client/gefyra/local/networking.py index 551dc8f4..89d17104 100644 --- a/client/gefyra/local/networking.py +++ b/client/gefyra/local/networking.py @@ -17,6 +17,7 @@ def create_gefyra_network(config: ClientConfiguration) -> Network: def handle_create_network(config: ClientConfiguration) -> Network: + DOCKER_MTU_OPTION = "com.docker.network.driver.mtu" try: network = config.DOCKER.networks.get(config.NETWORK_NAME) logger.info("Gefyra network already exists") @@ -27,15 +28,14 @@ def handle_create_network(config: ClientConfiguration) -> Network: logger.debug(f"Docker network '{network.name}' is not managed by Gefyra") if ( "Options" in network.attrs - and "com.docker.network.driver.mtu" in network.attrs["Options"] - and network.attrs["Options"]["com.docker.network.driver.mtu"] - != config.WIREGUARD_MTU + and DOCKER_MTU_OPTION in network.attrs["Options"] + and network.attrs["Options"][DOCKER_MTU_OPTION] != config.WIREGUARD_MTU ) or ( "Options" in network.attrs - and "com.docker.network.driver.mtu" not in network.attrs["Options"] + and DOCKER_MTU_OPTION not in network.attrs["Options"] ): _mtu = ( - network.attrs["Options"].get("com.docker.network.driver.mtu") + network.attrs["Options"].get(DOCKER_MTU_OPTION) if "Options" in network.attrs else "default" ) @@ -62,7 +62,7 @@ def handle_create_network(config: ClientConfiguration) -> Network: labels={ CREATED_BY_LABEL[0]: CREATED_BY_LABEL[1], }, - options={"com.docker.network.driver.mtu": config.WIREGUARD_MTU}, + options={DOCKER_MTU_OPTION: config.WIREGUARD_MTU}, ) logger.info(f"Created network '{config.NETWORK_NAME}' ({network.short_id})") return network