Skip to content

Commit 00320fb

Browse files
QuantumEnigmaaQuantumEnigmaa
authored
feat(xds): add xdsNACKTotal metric (#9205)
* feat(xds): add xdsNACKTotal metric Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io> * add release-note Signed-off-by: QuantumEnigmaa <thibaud@giantswarm.io>
1 parent 332081c commit 00320fb

6 files changed

Lines changed: 78 additions & 12 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ require (
5858
golang.org/x/exp v0.0.0-20260410095643-746e56fc9e2f
5959
golang.org/x/sync v0.20.0
6060
gomodules.xyz/jsonpatch/v2 v2.5.0
61+
google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa
6162
google.golang.org/grpc v1.81.1
6263
google.golang.org/grpc/security/advancedtls v1.0.0
6364
google.golang.org/protobuf v1.36.12-0.20260120151049-f2248ac996af
@@ -274,7 +275,6 @@ require (
274275
golang.org/x/time v0.15.0 // indirect
275276
golang.org/x/tools v0.45.0 // indirect
276277
google.golang.org/genproto/googleapis/api v0.0.0-20260526163538-3dc84a4a5aaa // indirect
277-
google.golang.org/genproto/googleapis/rpc v0.0.0-20260526163538-3dc84a4a5aaa // indirect
278278
gopkg.in/evanphx/json-patch.v4 v4.13.0 // indirect
279279
gopkg.in/inf.v0 v0.9.1 // indirect
280280
gopkg.in/yaml.v2 v2.4.0 // indirect

‎internal/xds/cache/metrics.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,13 @@ var (
2424
[]float64{0.1, 10, 50, 100, 1000, 10000},
2525
)
2626

27+
xdsNACKTotal = metrics.NewCounter(
28+
"xds_nack_total",
29+
"Total number of xds updates rejected (NACKed) by Envoy, by node id and resource type.",
30+
)
31+
2732
nodeIDLabel = metrics.NewLabel("nodeID")
2833
streamIDLabel = metrics.NewLabel("streamID")
2934
isDeltaStreamLabel = metrics.NewLabel("isDeltaStream")
35+
typeURLLabel = metrics.NewLabel("typeURL")
3036
)

‎internal/xds/cache/snapshotcache.go‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -254,10 +254,13 @@ func (s *snapshotCache) OnStreamRequest(streamID int64, req *discoveryv3.Discove
254254
s.log.Debugf("Got a new request, version_info %s, response_nonce %s, nodeID %s, node_version %s", req.VersionInfo, req.ResponseNonce, nodeID, nodeVersion)
255255

256256
if status := req.ErrorDetail; status != nil {
257-
// if Envoy rejected the last update log the details here.
258-
// TODO(youngnick): Handle NACK properly
257+
// Envoy rejected (NACKed) the last update. It keeps serving its last known
258+
// good config, so this is silent from the user's point of view, but any
259+
// proxy that (re)starts will fail to load the rejected config. Surface it
260+
// as a metric so it can be alerted on, in addition to logging the details.
259261
errorCode = status.Code
260262
errorMessage = status.Message
263+
xdsNACKTotal.With(nodeIDLabel.Value(nodeID), typeURLLabel.Value(req.GetTypeUrl())).Increment()
261264
}
262265

263266
s.log.Debugf("handling v3 xDS resource request, version_info %s, response_nonce %s, nodeID %s, node_version %s, resource_names %v, type_url %s, errorCode %d, errorMessage %s",
@@ -266,7 +269,8 @@ func (s *snapshotCache) OnStreamRequest(streamID int64, req *discoveryv3.Discove
266269
errorCode, errorMessage)
267270

268271
if errorCode != 0 {
269-
s.log.Errorf("Envoy rejected the last update with code %d and message %s", errorCode, errorMessage)
272+
s.log.Errorf("Envoy rejected the last update for type %s on node %s with code %d and message %s; the proxy is still serving its last known good config and a restarting proxy will fail to load this config",
273+
req.GetTypeUrl(), nodeID, errorCode, errorMessage)
270274
}
271275

272276
return nil
@@ -373,10 +377,13 @@ func (s *snapshotCache) OnStreamDeltaRequest(streamID int64, req *discoveryv3.De
373377
s.log.Debugf("Got a new request, response_nonce %s, nodeID %s, node_version %s",
374378
req.ResponseNonce, nodeID, nodeVersion)
375379
if status := req.ErrorDetail; status != nil {
376-
// if Envoy rejected the last update log the details here.
377-
// TODO(youngnick): Handle NACK properly
380+
// Envoy rejected (NACKed) the last update. It keeps serving its last known
381+
// good config, so this is silent from the user's point of view, but any
382+
// proxy that (re)starts will fail to load the rejected config. Surface it
383+
// as a metric so it can be alerted on, in addition to logging the details.
378384
errorCode = status.Code
379385
errorMessage = status.Message
386+
xdsNACKTotal.With(nodeIDLabel.Value(nodeID), typeURLLabel.Value(req.GetTypeUrl())).Increment()
380387
}
381388
s.log.Debugf("handling v3 xDS resource request, response_nonce %s, nodeID %s, node_version %s, resource_names_subscribe %v, resource_names_unsubscribe %v, type_url %s, errorCode %d, errorMessage %s",
382389
req.ResponseNonce,
@@ -386,7 +393,8 @@ func (s *snapshotCache) OnStreamDeltaRequest(streamID int64, req *discoveryv3.De
386393
errorCode, errorMessage)
387394

388395
if errorCode != 0 {
389-
s.log.Errorf("Envoy rejected the last update with code %d and message %s", errorCode, errorMessage)
396+
s.log.Errorf("Envoy rejected the last update for type %s on node %s with code %d and message %s; the proxy is still serving its last known good config and a restarting proxy will fail to load this config",
397+
req.GetTypeUrl(), nodeID, errorCode, errorMessage)
390398
}
391399

392400
return nil

‎internal/xds/cache/snapshotcache_test.go‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ import (
1111
"sync"
1212
"testing"
1313

14+
corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
15+
discoveryv3 "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
16+
cachev3 "github.com/envoyproxy/go-control-plane/pkg/cache/v3"
17+
resourcev3 "github.com/envoyproxy/go-control-plane/pkg/resource/v3"
1418
"github.com/stretchr/testify/require"
19+
statusv3 "google.golang.org/genproto/googleapis/rpc/status"
1520

1621
egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
1722
"github.com/envoyproxy/gateway/internal/logging"
@@ -50,6 +55,51 @@ func TestOnStreamResponseConcurrentAccess(t *testing.T) {
5055
wg.Wait()
5156
}
5257

58+
// TestOnStreamRequestNACK verifies that a NACK from Envoy is handled
59+
// gracefully on both the SotW and delta request paths.
60+
func TestOnStreamRequestNACK(t *testing.T) {
61+
const (
62+
nodeID = "test-node"
63+
cluster = "test-cluster"
64+
)
65+
66+
newPrimedCache := func(t *testing.T) *snapshotCache {
67+
t.Helper()
68+
sc := newTestSnapshotCache(t)
69+
// A non-nil last snapshot for the node's cluster is required, otherwise the
70+
// request handlers return early before reaching the NACK handling.
71+
snap, err := cachev3.NewSnapshot("1", nil)
72+
require.NoError(t, err)
73+
sc.lastSnapshot[cluster] = snap
74+
return sc
75+
}
76+
77+
node := &corev3.Node{Id: nodeID, Cluster: cluster}
78+
errorDetail := &statusv3.Status{Code: 13, Message: "invalid access log format"}
79+
80+
t.Run("sotw", func(t *testing.T) {
81+
sc := newPrimedCache(t)
82+
require.NoError(t, sc.OnStreamOpen(context.Background(), 1, ""))
83+
err := sc.OnStreamRequest(1, &discoveryv3.DiscoveryRequest{
84+
Node: node,
85+
TypeUrl: resourcev3.ListenerType,
86+
ErrorDetail: errorDetail,
87+
})
88+
require.NoError(t, err)
89+
})
90+
91+
t.Run("delta", func(t *testing.T) {
92+
sc := newPrimedCache(t)
93+
require.NoError(t, sc.OnDeltaStreamOpen(context.Background(), 1, ""))
94+
err := sc.OnStreamDeltaRequest(1, &discoveryv3.DeltaDiscoveryRequest{
95+
Node: node,
96+
TypeUrl: resourcev3.ListenerType,
97+
ErrorDetail: errorDetail,
98+
})
99+
require.NoError(t, err)
100+
})
101+
}
102+
53103
// TestOnStreamDeltaResponseConcurrentAccess verifies the same for delta streams.
54104
func TestOnStreamDeltaResponseConcurrentAccess(t *testing.T) {
55105
sc := newTestSnapshotCache(t)

‎release-notes/current.yaml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ security updates: |
1313
new features: |
1414
Added support for authorization path match.
1515
Added a `requestBody` field to the HTTP active health checker in `BackendTrafficPolicy`, allowing a request body payload to be sent during HTTP health checking. The field requires the health check `method` to be `POST` or `PUT`.
16+
Added a `xdsNACKTotal` metric to track the number of NACKs received from Envoy, labeled by node ID and resource type URL. A NACK is a DiscoveryRequest carrying an ErrorDetail, indicating that Envoy rejected the last config update. This metric can be used to alert on config issues causing xDS rejections.
1617
Added the `autoSNIFromEndpointHostname` TLS setting to Backends, allowing the SNI value sent to the backend to be automatically derived from the backend endpoint hostname instead of using a fixed SNI value.
1718
1819
bug fixes: |

‎site/content/en/latest/tasks/observability/gateway-exported-metrics.md‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -57,11 +57,12 @@ Envoy Gateway monitors the cache and xDS connection status in xDS Server.
5757

5858
Envoy Gateway collects the following metrics in xDS Server:
5959

60-
| Name | Description |
61-
|-------------------------------|--------------------------------------------------------|
62-
| `xds_snapshot_create_total` | Total number of xds snapshot cache creates. |
63-
| `xds_snapshot_update_total` | Total number of xds snapshot cache updates by node id. |
64-
| `xds_stream_duration_seconds` | How long a xds stream takes to finish. |
60+
| Name | Description |
61+
|-------------------------------|----------------------------------------------------------------------------------------|
62+
| `xds_snapshot_create_total` | Total number of xds snapshot cache creates. |
63+
| `xds_snapshot_update_total` | Total number of xds snapshot cache updates by node id. |
64+
| `xds_stream_duration_seconds` | How long a xds stream takes to finish. |
65+
| `xds_nack_total` | Total number of xds updates rejected (NACKed) by Envoy, by node id and resource type. |
6566

6667
- For xDS snapshot cache update and xDS stream connection status, each metric includes `nodeID` label to identify the connection peer.
6768
- For xDS stream connection status, each metric also includes `streamID` label to identify the connection stream, and `isDeltaStream` label to identify the delta connection stream.

0 commit comments

Comments
 (0)