Merge pull request #5239 from emissary-ingress/alicewasko/remove-agen…

…t-stats-sink Remove agent stats sink
emissary-ingress · Aug 17, 2023 · 60867c4 · 60867c4
2 parents fb7455d + 74fe4ed
commit 60867c4
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 113 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -105,6 +105,12 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
   be sure to re-apply any v3alpha1 Mappings in order to update the stored v2 Mapping and resolve the
   issue.
 
+- Change: When the Ambassador agent is being used, it will no longer attempt to collect and report
+  Envoy metrics. In previous versions, Emissary-ingress would always create an Envoy stats sink for
+  the agent as long as the AMBASSADOR_GRPC_METRICS_SINK environmet variable was provided. This
+  environment variable was hardcoded on the release manifests and has now been removed and an Envoy
+  stats sink for the agent is no longer created.
+
 [Canary grouping must take labels into account]: https://github.com/emissary-ingress/emissary/issues/4170
 
 ## [3.7.2] July 25, 2023
@@ -113,7 +119,7 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
 ### Emissary-ingress and Ambassador Edge Stack
 
 - Security: This upgrades Emissary-ingress to be built on Envoy v1.26.4 which includes a security
-  fixes for  CVE-2023-35942, CVE-2023-35943,  VE-2023-35944.
+  fixes for CVE-2023-35942, CVE-2023-35943,  VE-2023-35944.
 
 ## [3.7.1] July 13, 2023
 [3.7.1]: https://github.com/emissary-ingress/emissary/compare/v3.7.0...v3.7.1
@@ -152,10 +158,10 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
 - Security: Upgrading to the latest release of Golang as part of our general dependency upgrade
   process. This includes security fixes for CVE-2022-41725, CVE-2022-41723.
 
-- Feature: In Envoy 1.24, experimental support for a native OpenTelemetry tracing driver  was
-  introduced that allows exporting spans in the otlp format. Many  Observability platforms accept
+- Feature: In Envoy 1.24, experimental support for a native OpenTelemetry tracing driver was
+  introduced that allows exporting spans in the otlp format. Many Observability platforms accept
   that format and is the recommend replacement for the LightStep driver. Emissary-ingress now
-  supports setting the  `TracingService.spec.driver=opentelemetry` to export spans in  otlp
+  supports setting the `TracingService.spec.driver=opentelemetry` to export spans in otlp
   format.<br/><br/>
   Thanks to <a href="https://github.com/psalaberria002">Paul</a> for helping us
   get this tested and implemented!
@@ -174,14 +180,14 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
 - Change: Previously, specifying backend ports by name in Ingress was not supported and would result
   in defaulting to port 80. This allows emissary-ingress to now resolve port names for backend
   services. If the port number cannot be resolved by the name (e.g named port in the Service doesn't
-  exist) then it defaults back  to the original behavior. (Thanks to <a
+  exist) then it defaults back to the original behavior. (Thanks to <a
   href="https://github.com/antonu17">Anton Ustyuzhanin</a>!). ([#4809])
 
-- Change: The `emissary-apiext` server is a Kubernetes Conversion Webhook that converts between the 
+- Change: The `emissary-apiext` server is a Kubernetes Conversion Webhook that converts between the
   Emissary-ingress CRD versions. On startup, it ensures that a self-signed cert is available so that
-  K8s API Server can talk to the conversion webhook (*TLS is required by K8s*). We  have introduced
-  a startupProbe to ensure that emissary-apiext server has enough time to configure the webhooks
-  before running liveness and readiness probes. This is to ensure  slow startup doesn't cause K8s to
+  K8s API Server can talk to the conversion webhook (*TLS is required by K8s*). We have introduced a
+  startupProbe to ensure that emissary-apiext server has enough time to configure the webhooks
+  before running liveness and readiness probes. This is to ensure slow startup doesn't cause K8s to
   needlessly restart the pod.
 
 [fix: hostname port issue]: https://github.com/emissary-ingress/emissary/pull/4816
@@ -213,17 +219,17 @@ it will be removed; but as it won't be user-visible this isn't considered a brea
   server to.
   - `AMBASSADOR_HEALTHCHECK_IP_FAMILY`: The IP family to use for the healthcheck
   server.
-  This allows the healthcheck server to be configured to use IPv6-only k8s environments. 
+  This allows the healthcheck server to be configured to use IPv6-only k8s environments.
   (Thanks to <a href="https://github.com/TimonOmsk">Dmitry Golushko</a>!).
 
-- Feature: This upgrades Emissary-ingress to be built on Envoy v1.24.1. One notable change is that 
+- Feature: This upgrades Emissary-ingress to be built on Envoy v1.24.1. One notable change is that
   the team at LightStep and Envoy Maintainers have decided to no longer support the native
-  *LightStep* tracing driver in favor of using the Open Telemetry driver. The code  for LightStep
-  driver has been completely removed from Envoy code base so Emissary-ingress  will no longer
-  support it either.
-  The recommended upgrade path is to leverage a supported Tracing driver such as
-  `Zipkin` and use the [Open Telemetry Collector](https://opentelemetry.io/docs/collector/) to
-  collect and forward Observabity data to LightStep.
+  *LightStep* tracing driver in favor of using the Open Telemetry driver. The code for LightStep
+  driver has been completely removed from Envoy code base so Emissary-ingress will no longer support
+  it either.
+  The recommended upgrade path is to leverage a supported Tracing driver such as `Zipkin`
+  and use the [Open Telemetry Collector](https://opentelemetry.io/docs/collector/) to collect and
+  forward Observabity data to LightStep.
 
 - Feature: /ready endpoint used by emissary is using the admin port (8001 by default). This
   generates a problem during config reloads with large configs as the admin thread is blocking so

diff --git a/charts/emissary-ingress/templates/deployment.yaml b/charts/emissary-ingress/templates/deployment.yaml
@@ -172,12 +172,6 @@ spec:
             - name: admin
               containerPort: {{ .Values.adminService.port }}
           env:
-            - name: AMBASSADOR_GRPC_METRICS_SINK
-              value: {{ include "ambassador.fullname" . }}-agent:80
-            - name: HOST_IP
-              valueFrom:
-                fieldRef:
-                  fieldPath: status.hostIP
             {{- if .Values.prometheusExporter.enabled }}
             - name: STATSD_ENABLED
               value: "true"

diff --git a/docs/releaseNotes.yml b/docs/releaseNotes.yml
@@ -58,6 +58,13 @@ items:
           This issue was only present in v3alpha1 Mappings. For users who may have this issue, please
           be sure to re-apply any v3alpha1 Mappings in order to update the stored v2 Mapping and resolve the
           issue.
+      - title: Ambassador Agent no longer collects Envoy metrics
+        type: change
+        body: >-
+          When the Ambassador agent is being used, it will no longer attempt to collect and report Envoy metrics.
+          In previous versions, $productName$ would always create an Envoy stats sink for the agent as long as the AMBASSADOR_GRPC_METRICS_SINK
+          environmet variable was provided. This environment variable was hardcoded on the release manifests and has now been removed
+          and an Envoy stats sink for the agent is no longer created.
 
   - version: 3.7.2
     prevVersion: 3.7.1
@@ -66,7 +73,7 @@ items:
       - title: Upgrade to Envoy 1.26.4
         type: security
         body: >-
-          This upgrades $productName$ to be built on Envoy v1.26.4 which includes a security fixes for 
+          This upgrades $productName$ to be built on Envoy v1.26.4 which includes a security fixes for
           CVE-2023-35942, CVE-2023-35943,  VE-2023-35944.
 
   - version: 3.7.1
@@ -116,11 +123,11 @@ items:
       - title: TracingService support for native OpenTelemetry driver
         type: feature
         body: >-
-          In Envoy 1.24, experimental support for a native OpenTelemetry tracing driver 
-          was introduced that allows exporting spans in the otlp format. Many 
+          In Envoy 1.24, experimental support for a native OpenTelemetry tracing driver
+          was introduced that allows exporting spans in the otlp format. Many
           Observability platforms accept that format and is the recommend
-          replacement for the LightStep driver. $productName$ now supports setting the 
-          <code>TracingService.spec.driver=opentelemetry</code> to export spans in 
+          replacement for the LightStep driver. $productName$ now supports setting the
+          <code>TracingService.spec.driver=opentelemetry</code> to export spans in
           otlp format.<br/><br/>
 
           Thanks to <a href="https://github.com/psalaberria002">Paul</a> for helping us
@@ -135,7 +142,7 @@ items:
           way emissary was generating Envoy configuration under a single wild-card virtual_host and matching
           on :authority.
 
-          
+
           In v2.Y/v3.Y+, the way emissary generates Envoy configuration changed to address memory pressure and improve
           route lookup speed in Envoy. However, when including a port in the hostname, an incorrect configuration  was
           generated with an sni match including the port. This has been fixed and the correct envoy configuration is
@@ -144,12 +151,12 @@ items:
         - title: "fix: hostname port issue"
           link: https://github.com/emissary-ingress/emissary/pull/4816
 
-      - title: Add support for resolving port names in Ingress resource 
+      - title: Add support for resolving port names in Ingress resource
         type: change
         body: >-
           Previously, specifying backend ports by name in Ingress was not supported and would result in defaulting
           to port 80. This allows emissary-ingress to now resolve port names for backend services. If the port number
-          cannot be resolved by the name (e.g named port in the Service doesn't exist) then it defaults back 
+          cannot be resolved by the name (e.g named port in the Service doesn't exist) then it defaults back
           to the original behavior.
           (Thanks to <a href="https://github.com/antonu17">Anton Ustyuzhanin</a>!).
         github:
@@ -159,13 +166,13 @@ items:
       - title: Add starupProbe to emissary-apiext server
         type: change
         body: >-
-          The <code>emissary-apiext</code> server is a Kubernetes Conversion Webhook that converts between the 
+          The <code>emissary-apiext</code> server is a Kubernetes Conversion Webhook that converts between the
           Emissary-ingress CRD versions. On startup, it ensures that a self-signed cert is available
-          so that K8s API Server can talk to the conversion webhook (*TLS is required by K8s*). We 
+          so that K8s API Server can talk to the conversion webhook (*TLS is required by K8s*). We
           have introduced a startupProbe to ensure that emissary-apiext server has enough time to
-          configure the webhooks before running liveness and readiness probes. This is to ensure 
+          configure the webhooks before running liveness and readiness probes. This is to ensure
           slow startup doesn't cause K8s to needlessly restart the pod.
-      
+
 
   - version: 3.4.0
     prevVersion: 3.3.0
@@ -198,19 +205,19 @@ items:
           - `AMBASSADOR_HEALTHCHECK_BIND_ADDRESS`: The address to bind the healthcheck server to.
 
           - `AMBASSADOR_HEALTHCHECK_BIND_PORT`: The port to bind the healthcheck server to.
-          
+
           - `AMBASSADOR_HEALTHCHECK_IP_FAMILY`: The IP family to use for the healthcheck server.
-          
-          This allows the healthcheck server to be configured to use IPv6-only k8s environments. 
+
+          This allows the healthcheck server to be configured to use IPv6-only k8s environments.
           (Thanks to <a href="https://github.com/TimonOmsk">Dmitry Golushko</a>!).
 
       - title: Upgrade to Envoy 1.24.1
         type: feature
         body: >-
-          This upgrades $productName$ to be built on Envoy v1.24.1. One notable change is that 
+          This upgrades $productName$ to be built on Envoy v1.24.1. One notable change is that
           the team at LightStep and Envoy Maintainers have decided to no longer support the
-          native *LightStep* tracing driver in favor of using the Open Telemetry driver. The code 
-          for LightStep driver has been completely removed from Envoy code base so $productName$ 
+          native *LightStep* tracing driver in favor of using the Open Telemetry driver. The code
+          for LightStep driver has been completely removed from Envoy code base so $productName$
           will no longer support it either.
 
           The recommended upgrade path is to leverage a supported Tracing driver such as `Zipkin`

diff --git a/manifests/emissary/emissary-defaultns.yaml.in b/manifests/emissary/emissary-defaultns.yaml.in
@@ -289,12 +289,6 @@ spec:
             weight: 100
       containers:
       - env:
-        - name: AMBASSADOR_GRPC_METRICS_SINK
-          value: emissary-ingress-agent:80
-        - name: HOST_IP
-          valueFrom:
-            fieldRef:
-              fieldPath: status.hostIP
         - name: AMBASSADOR_NAMESPACE
           valueFrom:
             fieldRef:

diff --git a/manifests/emissary/emissary-emissaryns.yaml.in b/manifests/emissary/emissary-emissaryns.yaml.in
@@ -289,12 +289,6 @@ spec:
             weight: 100
       containers:
       - env:
-        - name: AMBASSADOR_GRPC_METRICS_SINK
-          value: emissary-ingress-agent:80
-        - name: HOST_IP
-          valueFrom:
-            fieldRef:
-              fieldPath: status.hostIP
         - name: AMBASSADOR_NAMESPACE
           valueFrom:
             fieldRef:

diff --git a/python/ambassador/envoy/v3/v3bootstrap.py b/python/ambassador/envoy/v3/v3bootstrap.py
@@ -97,60 +97,6 @@ def __init__(self, config: "V3Config") -> None:
                 clusters.append(V3Cluster(config, typecast(IRCluster, log_service.cluster)))
 
         stats_sinks = []
-
-        grpcSink = os.environ.get("AMBASSADOR_GRPC_METRICS_SINK")
-        if grpcSink:
-            try:
-                host, port = split_host_port(grpcSink)
-                valid = True
-            except ValueError as ex:
-                config.ir.logger.error(
-                    "AMBASSADOR_GRPC_METRICS_SINK value %s is invalid: %s" % (grpcSink, ex)
-                )
-                valid = False
-
-            if valid:
-                stats_sinks.append(
-                    {
-                        "name": "envoy.metrics_service",
-                        "typed_config": {
-                            "@type": "type.googleapis.com/envoy.config.metrics.v3.MetricsServiceConfig",
-                            "transport_api_version": "V3",
-                            "grpc_service": {
-                                "envoy_grpc": {"cluster_name": "envoy_metrics_service"}
-                            },
-                        },
-                    }
-                )
-                clusters.append(
-                    {
-                        "name": "envoy_metrics_service",
-                        "type": "strict_dns",
-                        "connect_timeout": "1s",
-                        "http2_protocol_options": {},
-                        "load_assignment": {
-                            "cluster_name": "envoy_metrics_service",
-                            "endpoints": [
-                                {
-                                    "lb_endpoints": [
-                                        {
-                                            "endpoint": {
-                                                "address": {
-                                                    "socket_address": {
-                                                        "address": host,
-                                                        "port_value": port,
-                                                        "protocol": "TCP",
-                                                    }
-                                                }
-                                            }
-                                        }
-                                    ]
-                                }
-                            ],
-                        },
-                    }
-                )
-
         if config.ir.statsd["enabled"]:
             if config.ir.statsd["dogstatsd"]:
                 name = "envoy.stat_sinks.dog_statsd"

diff --git a/python/tests/integration/manifests/ambassador.yaml b/python/tests/integration/manifests/ambassador.yaml
@@ -91,12 +91,6 @@ metadata:
 spec:
   containers:
   - env:
-    - name: AMBASSADOR_GRPC_METRICS_SINK
-      value: {self.path.k8s}-agent:80
-    - name: HOST_IP
-      valueFrom:
-        fieldRef:
-          fieldPath: status.hostIP
     - name: AMBASSADOR_NAMESPACE
       valueFrom:
         fieldRef: