-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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: add option --container-host to commands local start-api, local start-lambda and local invoke #2700
feat: add option --container-host to commands local start-api, local start-lambda and local invoke #2700
Changes from 10 commits
3b302a2
8b862e4
2364efe
5238725
48b3cf9
f8d9217
28778bd
b98bf73
21b3b6b
33e0aa1
3814f39
f039f2d
a236b09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,6 +43,7 @@ def __init__( | |
aws_region: Optional[str] = None, | ||
env_vars_values: Optional[Dict[Any, Any]] = None, | ||
debug_context: Optional[DebugContext] = None, | ||
container_host: Optional[str] = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since there is a default value for the parameter, would this param be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This param won't be
|
||
) -> None: | ||
""" | ||
Initializes the class | ||
|
@@ -55,6 +56,7 @@ def __init__( | |
:param string aws_region: Optional. AWS Region to use. | ||
:param dict env_vars_values: Optional. Dictionary containing values of environment variables. | ||
:param DebugContext debug_context: Optional. Debug context for the function (includes port, args, and path). | ||
:param string container_host: Optional. Host of locally emulated Lambda container | ||
""" | ||
|
||
self.local_runtime = local_runtime | ||
|
@@ -66,6 +68,7 @@ def __init__( | |
self.debug_context = debug_context | ||
self._boto3_session_creds: Optional[Dict[str, str]] = None | ||
self._boto3_region: Optional[str] = None | ||
self.container_host = container_host | ||
|
||
def invoke( | ||
self, | ||
|
@@ -121,7 +124,14 @@ def invoke( | |
|
||
# Invoke the function | ||
try: | ||
self.local_runtime.invoke(config, event, debug_context=self.debug_context, stdout=stdout, stderr=stderr) | ||
self.local_runtime.invoke( | ||
config, | ||
event, | ||
debug_context=self.debug_context, | ||
stdout=stdout, | ||
stderr=stderr, | ||
container_host=self.container_host, | ||
) | ||
except ContainerResponseException: | ||
# NOTE(sriram-mv): This should still result in a exit code zero to avoid regressions. | ||
LOG.info("No response from invoke container for %s", function.name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,7 +38,7 @@ class Container: | |
_STDOUT_FRAME_TYPE = 1 | ||
_STDERR_FRAME_TYPE = 2 | ||
RAPID_PORT_CONTAINER = "8080" | ||
URL = "http://localhost:{port}/2015-03-31/functions/{function_name}/invocations" | ||
URL = "http://{host}:{port}/2015-03-31/functions/{function_name}/invocations" | ||
# Set connection timeout to 1 sec to support the large input. | ||
RAPID_CONNECTION_TIMEOUT = 1 | ||
|
||
|
@@ -55,6 +55,7 @@ def __init__( | |
docker_client=None, | ||
container_opts=None, | ||
additional_volumes=None, | ||
container_host="localhost", | ||
): | ||
""" | ||
Initializes the class with given configuration. This does not automatically create or run the container. | ||
|
@@ -71,6 +72,7 @@ def __init__( | |
:param docker_client: Optional, a docker client to replace the default one loaded from env | ||
:param container_opts: Optional, a dictionary containing the container options | ||
:param additional_volumes: Optional list of additional volumes | ||
:param string container_host: Optional. Host of locally emulated Lambda container | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this cannot be optional anymore, because if customers do not specify a value, click will use the default value There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason as my another comment below. |
||
""" | ||
|
||
self._image = image | ||
|
@@ -96,6 +98,9 @@ def __init__( | |
# selecting the first free port in a range that's not ephemeral. | ||
self._start_port_range = 5000 | ||
self._end_port_range = 9000 | ||
|
||
self._container_host = container_host | ||
|
||
try: | ||
self.rapid_port_host = find_free_port(start=self._start_port_range, end=self._end_port_range) | ||
except NoFreePortsError as ex: | ||
|
@@ -266,8 +271,9 @@ def wait_for_http_response(self, name, event, stdout): | |
# TODO(sriram-mv): `aws-lambda-rie` is in a mode where the function_name is always "function" | ||
# NOTE(sriram-mv): There is a connection timeout set on the http call to `aws-lambda-rie`, however there is not | ||
# a read time out for the response received from the server. | ||
|
||
resp = requests.post( | ||
self.URL.format(port=self.rapid_port_host, function_name="function"), | ||
self.URL.format(host=self._container_host, port=self.rapid_port_host, function_name="function"), | ||
data=event.encode("utf-8"), | ||
timeout=(self.RAPID_CONNECTION_TIMEOUT, None), | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For any security reasons, do we need to validate if this option valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. Let me check on this and get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For security reasons, there is nothing we can validate. There is a use case that docker daemon is running on a different host and the customer wants to use that host. We won't be able to understand if it's a good host provided by customer or just a bad actor.