Enable MITM proxy for requestor probes #535
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
azawlocki
force-pushed
the
az/enable-proxy-for-requestor
branch
from
8f4c6e0 to
92f41c4
Compare
kmazurek
suggested changes
Aug 18, 2021
| @@ -13,7 +13,7 @@ services: | |||
| # harness on the host machine | |||
| image: proxy-nginx | |||
| ports: | |||
| - "6000:6000" | |||
| - "16000-16100:6000-6100" | |||
There was a problem hiding this comment.
Perhaps it would be a good idea to add a short comment about why this port mapping is necessary here.
goth/runner/__init__.py
Outdated
| nginx_containers = [ | ||
| info.address | ||
| for name, info in self._container_info.items() | ||
| if "nginx" in name |
There was a problem hiding this comment.
I'd change this to use the exact name as defined in the default compose file, that is: proxy-nginx. In theory, the compose file may be modified by the user (it's part of the default assets) and it could define another nginx* container.
| @@ -106,7 +119,12 @@ async def start_network(self, log_dir: Path, force_build: bool = False) -> None: | |||
|
|
|||
| self._start_log_monitors(log_dir) | |||
| await self._wait_for_containers() | |||
| self._log_running_containers() | |||
There was a problem hiding this comment.
We can safely remove this function.
| async def start_network(self, log_dir: Path, force_build: bool = False) -> None: | ||
| async def start_network( | ||
| self, log_dir: Path, force_build: bool = False | ||
| ) -> Dict[str, ContainerInfo]: | ||
| """Start the compose network based on this manager's compose file. | ||
|
|
||
| This step may include (re)building the network's docker images. |
There was a problem hiding this comment.
Let's mention the newly-added return value here.
goth/runner/probe/__init__.py
Outdated
| @@ -225,6 +238,22 @@ async def _start_container(self) -> None: | |||
| ) | |||
| self._logger.info("IP address: %s", self.ip_address) | |||
There was a problem hiding this comment.
With the below log lines added I think we can remove this one.
Apply suggestions from code review Co-authored-by: Kuba Mazurek <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #429
This was meant to be a small PR that adds
use_proxy = Trueto the specification of the requestor node in the default goth config file and checks that the communication between the requestor agent (running on Docker host) and the requestor daemon passes through the MITM proxy.Changes:
Ports 6000-6100 on
proxy-nginxare mapped to ports 16000-16100 on the host. This is to make ports 6000-6100 onproxy-nginxreachable via localhost (127.0.0.1) and is important for some non-Linux systems, where the Docker compose network is not reachable directly. On such a system, an agent running on Docker host may reach the daemon by sending a request to an address such as127.0.0.1:16001. Such request is then forwarded toproxy-nginx:6001in the Docker network (according to port mapping defined indocker-compose.yml) and then to the MITM proxy (bynginx).ComposeNetworkManager.start_network()andrun_compose_network()context manager return a dictionary with information on the containers started by Docker compose. This information is stored by the Runner and is used to determine the address of theproxy-nginxcontainer. This is not really needed, since thanks to port mapping defined in 1., ports onproxy-nginxassigned to specific daemons are mapped to ports on the Docker host.Using proxy is turned on for Market API. For some reason, it was turned off, even if calls to Activity API and to Payment API were made through proxy.