From cf1926c6d8fd191e34bf363be215d9d134abd886 Mon Sep 17 00:00:00 2001 From: Tenshin Higashi Date: Mon, 28 Aug 2023 16:18:07 -0400 Subject: [PATCH 1/4] Initial Commit Signed-off-by: Tenshin Higashi --- .../getambassador.io/v3alpha1/crd_module.go | 2 +- python/ambassador/envoy/v3/v3listener.py | 44 +++++++++++++++++++ python/ambassador/envoy/v3/v3ready.py | 43 ++++++++++++++++++ python/ambassador/ir/irambassador.py | 13 +++++- 4 files changed, 100 insertions(+), 2 deletions(-) diff --git a/pkg/api/getambassador.io/v3alpha1/crd_module.go b/pkg/api/getambassador.io/v3alpha1/crd_module.go index 5761ca42fa..f033130040 100644 --- a/pkg/api/getambassador.io/v3alpha1/crd_module.go +++ b/pkg/api/getambassador.io/v3alpha1/crd_module.go @@ -141,7 +141,7 @@ type AmbassadorConfigSpec struct { // run a custom lua script on every request. see below for more details. LuaScripts string `json:"lua_scripts,omitempty"` - // +kubebuilder:validation:Enum={"text", "json"} + // +kubebuilder:validation:Enum={"text", "json", "typed_json"} EnvoyLogType string `json:"envoy_log_type,omitempty"` // envoy_log_path defines the path of log envoy will use. By default this is standard output diff --git a/python/ambassador/envoy/v3/v3listener.py b/python/ambassador/envoy/v3/v3listener.py index 0ce2f97133..7b5c67049d 100644 --- a/python/ambassador/envoy/v3/v3listener.py +++ b/python/ambassador/envoy/v3/v3listener.py @@ -430,6 +430,50 @@ def access_log(self) -> List[dict]: }, } ) + # Use sane access log spec in JSON + elif self.config.ir.ambassador_module.envoy_log_type.lower() == "typed_json": + log_format = self.config.ir.ambassador_module.get("envoy_log_format", None) + if log_format is None: + log_format = { + "start_time": "%START_TIME%", + "method": "%REQ(:METHOD)%", + "path": "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%", + "protocol": "%PROTOCOL%", + "response_code": "%RESPONSE_CODE%", + "response_flags": "%RESPONSE_FLAGS%", + "bytes_received": "%BYTES_RECEIVED%", + "bytes_sent": "%BYTES_SENT%", + "duration": "%DURATION%", + "upstream_service_time": "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%", + "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%", + "user_agent": "%REQ(USER-AGENT)%", + "request_id": "%REQ(X-REQUEST-ID)%", + "authority": "%REQ(:AUTHORITY)%", + "upstream_host": "%UPSTREAM_HOST%", + "upstream_cluster": "%UPSTREAM_CLUSTER%", + "upstream_local_address": "%UPSTREAM_LOCAL_ADDRESS%", + "downstream_local_address": "%DOWNSTREAM_LOCAL_ADDRESS%", + "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%", + "requested_server_name": "%REQUESTED_SERVER_NAME%", + "istio_policy_status": "%DYNAMIC_METADATA(istio.mixer:status)%", + "upstream_transport_failure_reason": "%UPSTREAM_TRANSPORT_FAILURE_REASON%", + } + + tracing_config = self.config.ir.tracing + if tracing_config and tracing_config.driver == "envoy.tracers.datadog": + log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" + log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" + + access_log.append( + { + "name": "envoy.access_loggers.file", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog", + "path": self.config.ir.ambassador_module.envoy_log_path, + "typed_json_format": log_format, + }, + } + ) else: # Use a sane access log spec log_format = self.config.ir.ambassador_module.get("envoy_log_format", None) diff --git a/python/ambassador/envoy/v3/v3ready.py b/python/ambassador/envoy/v3/v3ready.py index 22cd8ed7d2..2c205f1234 100644 --- a/python/ambassador/envoy/v3/v3ready.py +++ b/python/ambassador/envoy/v3/v3ready.py @@ -152,6 +152,49 @@ def access_log(cls, config: "V3Config") -> List[dict]: }, } ) + elif config.ir.ambassador_module.envoy_log_type.lower() == "typed_json": + log_format = config.ir.ambassador_module.get("envoy_log_format", None) + if log_format is None: + log_format = { + "start_time": "%START_TIME%", + "method": "%REQ(:METHOD)%", + "path": "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%", + "protocol": "%PROTOCOL%", + "response_code": "%RESPONSE_CODE%", + "response_flags": "%RESPONSE_FLAGS%", + "bytes_received": "%BYTES_RECEIVED%", + "bytes_sent": "%BYTES_SENT%", + "duration": "%DURATION%", + "upstream_service_time": "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%", + "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%", + "user_agent": "%REQ(USER-AGENT)%", + "request_id": "%REQ(X-REQUEST-ID)%", + "authority": "%REQ(:AUTHORITY)%", + "upstream_host": "%UPSTREAM_HOST%", + "upstream_cluster": "%UPSTREAM_CLUSTER%", + "upstream_local_address": "%UPSTREAM_LOCAL_ADDRESS%", + "downstream_local_address": "%DOWNSTREAM_LOCAL_ADDRESS%", + "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%", + "requested_server_name": "%REQUESTED_SERVER_NAME%", + "istio_policy_status": "%DYNAMIC_METADATA(istio.mixer:status)%", + "upstream_transport_failure_reason": "%UPSTREAM_TRANSPORT_FAILURE_REASON%", + } + + tracing_config = config.ir.tracing + if tracing_config and tracing_config.driver == "envoy.tracers.datadog": + log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" + log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" + + access_log.append( + { + "name": "envoy.access_loggers.file", + "typed_config": { + "@type": "type.googleapis.com/envoy.extensions.access_loggers.file.v3.FileAccessLog", + "path": config.ir.ambassador_module.envoy_log_path, + "typed_json_format": log_format, + }, + } + ) else: # Use a sane access log spec log_format = config.ir.ambassador_module.get("envoy_log_format", None) diff --git a/python/ambassador/ir/irambassador.py b/python/ambassador/ir/irambassador.py index b9593d62b4..fd10cd4a7f 100644 --- a/python/ambassador/ir/irambassador.py +++ b/python/ambassador/ir/irambassador.py @@ -382,9 +382,20 @@ def finalize(self, ir: "IR", aconf: Config) -> bool: ) self["envoy_log_format"] = {} return False + elif self.get("envoy_log_type") == "typed_json": + if self.get("envoy_log_format", None) is not None and not isinstance( + self.get("envoy_log_format"), dict + ): + self.post_error( + "envoy_log_type 'typed_json' requires a dictionary in envoy_log_format: {}, invalidating...".format( + self.get("envoy_log_format") + ) + ) + self["envoy_log_format"] = {} + return False else: self.post_error( - "Invalid log_type specified: {}. Supported: json, text".format( + "Invalid log_type specified: {}. Supported: json, text, typed_json".format( self.get("envoy_log_type") ) ) From 2c1293e8995ad43755052064853824ba95ec53c4 Mon Sep 17 00:00:00 2001 From: Tenshin Higashi Date: Wed, 30 Aug 2023 13:17:18 -0400 Subject: [PATCH 2/4] Adding test Signed-off-by: Tenshin Higashi --- python/tests/kat/t_envoy_logs.py | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/python/tests/kat/t_envoy_logs.py b/python/tests/kat/t_envoy_logs.py index 3cfec09b51..0e4ec8c988 100644 --- a/python/tests/kat/t_envoy_logs.py +++ b/python/tests/kat/t_envoy_logs.py @@ -83,3 +83,41 @@ def check(self): assert access_log_entry_regex.match( line ), f"{line} does not match {access_log_entry_regex}" + + +class EnvoyLogTypeJSONTest(AmbassadorTest): + target: ServiceType + log_path: str + + def init(self): + self.target = HTTP() + self.log_path = "/tmp/ambassador/ambassador.log" + + def config(self) -> Generator[Union[str, Tuple[Node, str]], None, None]: + yield self, self.format( + """ +--- +apiVersion: getambassador.io/v3alpha1 +kind: Module +name: ambassador +ambassador_id: [{self.ambassador_id}] +config: + envoy_log_path: {self.log_path} + envoy_log_format: + protocol: "%PROTOCOL%" + duration: "%DURATION%" + envoy_log_type: typed_json +""" + ) + + def check(self): + access_log_entry_regex = re.compile('^({"duration":|{"protocol":)') + + cmd = ShellCommand("tools/bin/kubectl", "exec", self.path.k8s, "cat", self.log_path) + if not cmd.check("check envoy access log"): + pytest.exit("envoy access log does not exist") + + for line in cmd.stdout.splitlines(): + assert access_log_entry_regex.match( + line + ), f"{line} does not match {access_log_entry_regex}" From 299ce55d475e07de188b4d641f54a1e18ed7f9a0 Mon Sep 17 00:00:00 2001 From: Tenshin Higashi Date: Mon, 11 Sep 2023 11:49:19 -0400 Subject: [PATCH 3/4] Addressing Feedback Signed-off-by: Tenshin Higashi --- python/ambassador/envoy/v3/v3listener.py | 103 +++++++++-------------- python/ambassador/envoy/v3/v3ready.py | 2 +- 2 files changed, 39 insertions(+), 66 deletions(-) diff --git a/python/ambassador/envoy/v3/v3listener.py b/python/ambassador/envoy/v3/v3listener.py index 7b5c67049d..9f1d7649c6 100644 --- a/python/ambassador/envoy/v3/v3listener.py +++ b/python/ambassador/envoy/v3/v3listener.py @@ -348,6 +348,41 @@ def add_chain( return chain + def json_helper(self) -> Any: + log_format = self.config.ir.ambassador_module.get("envoy_log_format", None) + if log_format is None: + log_format = { + "start_time": "%START_TIME%", + "method": "%REQ(:METHOD)%", + "path": "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%", + "protocol": "%PROTOCOL%", + "response_code": "%RESPONSE_CODE%", + "response_flags": "%RESPONSE_FLAGS%", + "bytes_received": "%BYTES_RECEIVED%", + "bytes_sent": "%BYTES_SENT%", + "duration": "%DURATION%", + "upstream_service_time": "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%", + "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%", + "user_agent": "%REQ(USER-AGENT)%", + "request_id": "%REQ(X-REQUEST-ID)%", + "authority": "%REQ(:AUTHORITY)%", + "upstream_host": "%UPSTREAM_HOST%", + "upstream_cluster": "%UPSTREAM_CLUSTER%", + "upstream_local_address": "%UPSTREAM_LOCAL_ADDRESS%", + "downstream_local_address": "%DOWNSTREAM_LOCAL_ADDRESS%", + "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%", + "requested_server_name": "%REQUESTED_SERVER_NAME%", + "istio_policy_status": "%DYNAMIC_METADATA(istio.mixer:status)%", + "upstream_transport_failure_reason": "%UPSTREAM_TRANSPORT_FAILURE_REASON%", + } + + tracing_config = self.config.ir.tracing + if tracing_config and tracing_config.driver == "envoy.tracers.datadog": + log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" + log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" + + return log_format + # access_log constructs the access_log configuration for this V3Listener def access_log(self) -> List[dict]: access_log: List[dict] = [] @@ -388,38 +423,7 @@ def access_log(self) -> List[dict]: # Use sane access log spec in JSON if self.config.ir.ambassador_module.envoy_log_type.lower() == "json": - log_format = self.config.ir.ambassador_module.get("envoy_log_format", None) - if log_format is None: - log_format = { - "start_time": "%START_TIME%", - "method": "%REQ(:METHOD)%", - "path": "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%", - "protocol": "%PROTOCOL%", - "response_code": "%RESPONSE_CODE%", - "response_flags": "%RESPONSE_FLAGS%", - "bytes_received": "%BYTES_RECEIVED%", - "bytes_sent": "%BYTES_SENT%", - "duration": "%DURATION%", - "upstream_service_time": "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%", - "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%", - "user_agent": "%REQ(USER-AGENT)%", - "request_id": "%REQ(X-REQUEST-ID)%", - "authority": "%REQ(:AUTHORITY)%", - "upstream_host": "%UPSTREAM_HOST%", - "upstream_cluster": "%UPSTREAM_CLUSTER%", - "upstream_local_address": "%UPSTREAM_LOCAL_ADDRESS%", - "downstream_local_address": "%DOWNSTREAM_LOCAL_ADDRESS%", - "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%", - "requested_server_name": "%REQUESTED_SERVER_NAME%", - "istio_policy_status": "%DYNAMIC_METADATA(istio.mixer:status)%", - "upstream_transport_failure_reason": "%UPSTREAM_TRANSPORT_FAILURE_REASON%", - } - - tracing_config = self.config.ir.tracing - if tracing_config and tracing_config.driver == "envoy.tracers.datadog": - log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" - log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" - + log_format = V3Listener.json_helper(self) access_log.append( { "name": "envoy.access_loggers.file", @@ -430,40 +434,9 @@ def access_log(self) -> List[dict]: }, } ) - # Use sane access log spec in JSON + # Use sane access log spec in Typed JSON elif self.config.ir.ambassador_module.envoy_log_type.lower() == "typed_json": - log_format = self.config.ir.ambassador_module.get("envoy_log_format", None) - if log_format is None: - log_format = { - "start_time": "%START_TIME%", - "method": "%REQ(:METHOD)%", - "path": "%REQ(X-ENVOY-ORIGINAL-PATH?:PATH)%", - "protocol": "%PROTOCOL%", - "response_code": "%RESPONSE_CODE%", - "response_flags": "%RESPONSE_FLAGS%", - "bytes_received": "%BYTES_RECEIVED%", - "bytes_sent": "%BYTES_SENT%", - "duration": "%DURATION%", - "upstream_service_time": "%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%", - "x_forwarded_for": "%REQ(X-FORWARDED-FOR)%", - "user_agent": "%REQ(USER-AGENT)%", - "request_id": "%REQ(X-REQUEST-ID)%", - "authority": "%REQ(:AUTHORITY)%", - "upstream_host": "%UPSTREAM_HOST%", - "upstream_cluster": "%UPSTREAM_CLUSTER%", - "upstream_local_address": "%UPSTREAM_LOCAL_ADDRESS%", - "downstream_local_address": "%DOWNSTREAM_LOCAL_ADDRESS%", - "downstream_remote_address": "%DOWNSTREAM_REMOTE_ADDRESS%", - "requested_server_name": "%REQUESTED_SERVER_NAME%", - "istio_policy_status": "%DYNAMIC_METADATA(istio.mixer:status)%", - "upstream_transport_failure_reason": "%UPSTREAM_TRANSPORT_FAILURE_REASON%", - } - - tracing_config = self.config.ir.tracing - if tracing_config and tracing_config.driver == "envoy.tracers.datadog": - log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" - log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" - + log_format = V3Listener.json_helper(self) access_log.append( { "name": "envoy.access_loggers.file", diff --git a/python/ambassador/envoy/v3/v3ready.py b/python/ambassador/envoy/v3/v3ready.py index 2c205f1234..99092a65cf 100644 --- a/python/ambassador/envoy/v3/v3ready.py +++ b/python/ambassador/envoy/v3/v3ready.py @@ -141,7 +141,6 @@ def access_log(cls, config: "V3Config") -> List[dict]: if tracing_config and tracing_config.driver == "envoy.tracers.datadog": log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" - access_log.append( { "name": "envoy.access_loggers.file", @@ -152,6 +151,7 @@ def access_log(cls, config: "V3Config") -> List[dict]: }, } ) + # Use sane access log spec in Typed JSON elif config.ir.ambassador_module.envoy_log_type.lower() == "typed_json": log_format = config.ir.ambassador_module.get("envoy_log_format", None) if log_format is None: From 38d076c98b7a3582214ad50c69429229a7f72c2a Mon Sep 17 00:00:00 2001 From: Tenshin Higashi Date: Wed, 27 Dec 2023 17:34:20 -0500 Subject: [PATCH 4/4] Make Generate and fix tests Signed-off-by: Tenshin Higashi --- DEPENDENCIES.md | 2 +- python/ambassador/envoy/v3/v3listener.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/DEPENDENCIES.md b/DEPENDENCIES.md index da881253b0..6e14b6d23f 100644 --- a/DEPENDENCIES.md +++ b/DEPENDENCIES.md @@ -162,7 +162,7 @@ libraries: Name Version License(s) ---- ------- ---------- - Cython 0.29.36 Apache License 2.0 + Cython 0.29.37 Apache License 2.0 Flask 3.0.0 3-clause BSD license Jinja2 3.1.2 3-clause BSD license MarkupSafe 2.1.3 3-clause BSD license diff --git a/python/ambassador/envoy/v3/v3listener.py b/python/ambassador/envoy/v3/v3listener.py index 9f1d7649c6..b2eda1f279 100644 --- a/python/ambassador/envoy/v3/v3listener.py +++ b/python/ambassador/envoy/v3/v3listener.py @@ -380,8 +380,7 @@ def json_helper(self) -> Any: if tracing_config and tracing_config.driver == "envoy.tracers.datadog": log_format["dd.trace_id"] = "%REQ(X-DATADOG-TRACE-ID)%" log_format["dd.span_id"] = "%REQ(X-DATADOG-PARENT-ID)%" - - return log_format + return log_format # access_log constructs the access_log configuration for this V3Listener def access_log(self) -> List[dict]: @@ -434,6 +433,7 @@ def access_log(self) -> List[dict]: }, } ) + # Use sane access log spec in Typed JSON elif self.config.ir.ambassador_module.envoy_log_type.lower() == "typed_json": log_format = V3Listener.json_helper(self)