@awilkins I hesitate with this. I get your reasoning but I don't think we should assume it is bad. We use a docker client, which eventually communicates to the docker daemon. If it was bad or not allowed, I assume docker wouldn't let you do it at all.

What do you mean by: "Stops lamba executors in a user network from working if SAM is executing outside the docker stack"? Are you saying that SAM CLI won't be able to call a container over localhost if a network is provided?

It's been a while since I have dug into the docker networking but I am unsure what side effects this may have. Customers may be using networks and docker.host.internal. We would need to add integration with cases we can think of and validate this is actually safe to be doing.

I think hesitation is entirely correct, because clearly this is a core bit of SAM's APIG support. But I figured opening a PR was a good context to discuss what needs fixing before it would be ready to merge.

If it was bad or not allowed, I assume docker wouldn't let you do it at all.

The CLI client refuses to let you attach containers to both networks :

❯ docker run --net lambda --net bridge -it bash
docker: conflicting options: cannot attach both user-defined and non-user-defined network-modes.
See 'docker run --help'.

The client library doesn't seem to check this though (it literally just posts the request to the daemon, and I assume the daemon doesn't check either).

What do you mean by: "Stops lamba executors in a user network from working if SAM is executing outside the docker stack"

If you've set --docker-network to a value other than host, the patch will make SAM try to invoke the lambdas over their "real" host/port {container_id[:12]}:8080 rather than their exposed host/port localhost:{remapped_port} ; really this should only happen if SAM is running on a container in a network that can reach them (ie, the same user network).

Customers may be using networks and docker.host.internal. We would need to add integration with cases we can think of and validate this is actually safe to be doing.

Agreed. It's working great for my case (which is a docker-compose stack of api-tests --> sam-api --> [invoke_containers] --> localstack using the --exit-code-from api-tests flag to get the test outcome), but I don't want to spoil anyone else's fun.

Guess there's some design docs in my future.

@awilkins

The CLI client refuses to let you attach containers to both networks :

The command you provided isn't what SAM CLI does. We first create the container and then attach it to a network if one is provided. Not sure if that matters but could be a difference that impacts how the docker cli (or daemon) works.

If you've set --docker-network to a value other than host, the patch will make SAM try to invoke the lambdas over their "real" host/port {container_id[:12]}:8080 rather than their exposed host/port localhost:{remapped_port} ; really this should only happen if SAM is running on a container in a network that can reach them (ie, the same user network).

I need to think more here but --docker-network is for putting the local Lambda Container into a specific network. SAM CLI still needs access to call the container from localhost (we have to make sure not to break the non SAM CLI running in a container case).

Guess there's some design docs in my future.

Maybe not :). What I want to make sure is if we do add this we can add tests that verify it works and doesn't regress what we already support. SAM CLI was never really built for running in a container, as it needs Docker. There are ways around it but not something we really design for. Everything that doesn't use docker is fine but Docker in Docker + networking is overhead (and frankly much easier if you don't run in docker but I digress).

I am not going to be able to get back to this until next week. I am going to assign it to myself and come back to this. Again, I don't want to block these docker in docker cases but we have to be very careful because we don't want to break customers using the CLI on their system and not within docker.