Skip to content

Commit f03d96e

Browse files
endigmaendigma
authored
fix(router): make computeSha256 independent of metric/access log attributes (#2633)
1 parent 0ea3e7d commit f03d96e

File tree

5 files changed

+121
-19
lines changed

5 files changed

+121
-19
lines changed

‎router-tests/cache_warmup_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -462,6 +462,8 @@ func TestCacheWarmup(t *testing.T) {
462462
ValidationMisses: 1,
463463
PlanHits: 4,
464464
PlanMisses: 1,
465+
QueryHashMisses: 2, // 2x miss for safelist queries (raw query body hashed for safelist check)
466+
QueryHashHits: 1, // 1x hit for repeated query body hash
465467
},
466468
},
467469
}, func(t *testing.T, xEnv *testenv.Environment) {

‎router-tests/safelist_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,90 @@ func TestSafelist(t *testing.T) {
117117
})
118118
})
119119

120+
t.Run("safelist with access log sha256 attribute allows a persisted query to run", func(t *testing.T) {
121+
testenv.Run(t, &testenv.Config{
122+
AccessLogFields: []config.CustomAttribute{
123+
{
124+
Key: "operation_sha256",
125+
ValueFrom: &config.CustomDynamicAttribute{
126+
ContextField: core.ContextFieldOperationSha256,
127+
},
128+
},
129+
},
130+
RouterOptions: []core.Option{
131+
core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{
132+
Safelist: config.SafelistConfiguration{Enabled: true},
133+
}),
134+
},
135+
}, func(t *testing.T, xEnv *testenv.Environment) {
136+
header := make(http.Header)
137+
header.Add("graphql-client-name", "my-client")
138+
res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
139+
OperationName: []byte(`"Employees"`),
140+
Header: header,
141+
Query: persistedQuery,
142+
})
143+
require.NoError(t, err)
144+
require.Equal(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, res.Body)
145+
})
146+
})
147+
148+
t.Run("safelist with access log sha256 attribute allows a persisted query run with ID", func(t *testing.T) {
149+
testenv.Run(t, &testenv.Config{
150+
AccessLogFields: []config.CustomAttribute{
151+
{
152+
Key: "operation_sha256",
153+
ValueFrom: &config.CustomDynamicAttribute{
154+
ContextField: core.ContextFieldOperationSha256,
155+
},
156+
},
157+
},
158+
RouterOptions: []core.Option{
159+
core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{
160+
Safelist: config.SafelistConfiguration{Enabled: true},
161+
}),
162+
},
163+
}, func(t *testing.T, xEnv *testenv.Environment) {
164+
header := make(http.Header)
165+
header.Add("graphql-client-name", "my-client")
166+
res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
167+
OperationName: []byte(`"Employees"`),
168+
Extensions: []byte(`{"persistedQuery": {"version": 1, "sha256Hash": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f"}}`),
169+
Header: header,
170+
})
171+
require.NoError(t, err)
172+
require.Equal(t, `{"data":{"employees":[{"id":1},{"id":2},{"id":3},{"id":4},{"id":5},{"id":7},{"id":8},{"id":10},{"id":11},{"id":12}]}}`, res.Body)
173+
})
174+
})
175+
176+
t.Run("safelist with access log sha256 attribute rejects non persisted query", func(t *testing.T) {
177+
testenv.Run(t, &testenv.Config{
178+
AccessLogFields: []config.CustomAttribute{
179+
{
180+
Key: "operation_sha256",
181+
ValueFrom: &config.CustomDynamicAttribute{
182+
ContextField: core.ContextFieldOperationSha256,
183+
},
184+
},
185+
},
186+
RouterOptions: []core.Option{
187+
core.WithPersistedOperationsConfig(config.PersistedOperationsConfig{
188+
Safelist: config.SafelistConfiguration{Enabled: true},
189+
}),
190+
},
191+
}, func(t *testing.T, xEnv *testenv.Environment) {
192+
header := make(http.Header)
193+
header.Add("graphql-client-name", "my-client")
194+
res, err := xEnv.MakeGraphQLRequest(testenv.GraphQLRequest{
195+
OperationName: []byte(`"Employees"`),
196+
Header: header,
197+
Query: queryWithDetails,
198+
})
199+
require.NoError(t, err)
200+
require.Equal(t, persistedNotFoundResp, res.Body)
201+
})
202+
})
203+
120204
t.Run("log unknown operations", func(t *testing.T) {
121205
t.Run("logs non persisted query but allows them to continue", func(t *testing.T) {
122206
testenv.Run(t, &testenv.Config{

‎router-tests/structured_logging_test.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1818,7 +1818,7 @@ func TestFlakyAccessLogs(t *testing.T) {
18181818
"service_name": "service-name", // From request header
18191819
"operation_persisted_hash": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context
18201820
"operation_hash": "1163600561566987607", // From context
1821-
"operation_sha256": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", // From context
1821+
"operation_sha256": "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", // From context
18221822
"operation_name": "Employees", // From context
18231823
"operation_type": "query", // From context
18241824
}
@@ -2029,7 +2029,7 @@ func TestFlakyAccessLogs(t *testing.T) {
20292029
)
20302030
})
20312031

2032-
t.Run("validate request.operation.sha256Hash expression with persisted hash and body", func(t *testing.T) {
2032+
t.Run("validate request.operation.sha256Hash expression with persisted hash only", func(t *testing.T) {
20332033
t.Parallel()
20342034

20352035
testenv.Run(t,
@@ -2062,7 +2062,7 @@ func TestFlakyAccessLogs(t *testing.T) {
20622062

20632063
val, ok := requestContext["operation_sha256_expression"].(string)
20642064
require.True(t, ok)
2065-
require.Equal(t, "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", val)
2065+
require.Equal(t, "dc67510fb4289672bea757e862d6b00e83db5d3cbbcfb15260601b6f29bb2b8f", val)
20662066
},
20672067
)
20682068
})

‎router/core/graph_server.go‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -668,21 +668,25 @@ func (s *graphMux) buildOperationCaches(srv *graphServer) (computeSha256 bool, e
668668
}
669669

670670
// Currently, we only support custom attributes from the context for OTLP metrics
671-
if len(srv.metricConfig.Attributes) > 0 {
671+
if !computeSha256 && len(srv.metricConfig.Attributes) > 0 {
672672
for _, customAttribute := range srv.metricConfig.Attributes {
673673
if customAttribute.ValueFrom != nil && customAttribute.ValueFrom.ContextField == ContextFieldOperationSha256 {
674674
computeSha256 = true
675675
break
676676
}
677677
}
678-
} else if srv.accessLogsConfig != nil {
678+
}
679+
680+
if !computeSha256 && srv.accessLogsConfig != nil {
679681
for _, customAttribute := range append(srv.accessLogsConfig.Attributes, srv.accessLogsConfig.SubgraphAttributes...) {
680682
if customAttribute.ValueFrom != nil && customAttribute.ValueFrom.ContextField == ContextFieldOperationSha256 {
681683
computeSha256 = true
682684
break
683685
}
684686
}
685-
} else if srv.persistedOperationsConfig.Safelist.Enabled || srv.persistedOperationsConfig.LogUnknown {
687+
}
688+
689+
if srv.persistedOperationsConfig.Safelist.Enabled || srv.persistedOperationsConfig.LogUnknown {
686690
// In these case, we'll want to compute the sha256 for every operation, in order to check that the operation
687691
// is present in the Persisted Operation cache
688692
computeSha256 = true

‎router/core/graphql_prehandler.go‎

Lines changed: 25 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -543,22 +543,34 @@ func (h *PreHandler) handleOperation(req *http.Request, httpOperation *httpOpera
543543

544544
// Compute the operation sha256 hash as soon as possible for observability reasons
545545
if h.shouldComputeOperationSha256(operationKit, requestContext) {
546-
if err := operationKit.ComputeOperationSha256(); err != nil {
547-
return &httpGraphqlError{
548-
message: fmt.Sprintf("error hashing operation: %s", err),
549-
statusCode: http.StatusInternalServerError,
546+
if operationKit.parsedOperation.Request.Query == "" && operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() {
547+
// No query body to hash; use the client-provided persisted hash for telemetry.
548+
requestContext.operation.sha256Hash = operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.Sha256Hash
549+
requestContext.expressionContext.Request.Operation.Sha256Hash = requestContext.operation.sha256Hash
550+
551+
setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256)
552+
553+
requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash)
554+
} else {
555+
if err := operationKit.ComputeOperationSha256(); err != nil {
556+
return &httpGraphqlError{
557+
message: fmt.Sprintf("error hashing operation: %s", err),
558+
statusCode: http.StatusInternalServerError,
559+
}
550560
}
551-
}
552-
requestContext.operation.sha256Hash = operationKit.parsedOperation.Sha256Hash
553-
requestContext.expressionContext.Request.Operation.Sha256Hash = operationKit.parsedOperation.Sha256Hash
561+
requestContext.operation.sha256Hash = operationKit.parsedOperation.Sha256Hash
562+
requestContext.expressionContext.Request.Operation.Sha256Hash = operationKit.parsedOperation.Sha256Hash
554563

555-
setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256)
564+
setTelemetryAttributes(req.Context(), requestContext, expr.BucketSha256)
556565

557-
requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash)
558-
if h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled {
559-
// Set the request hash to the parsed hash, to see if it matches a persisted operation
560-
operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{
561-
Sha256Hash: operationKit.parsedOperation.Sha256Hash,
566+
requestContext.telemetry.addCustomMetricStringAttr(ContextFieldOperationSha256, requestContext.operation.sha256Hash)
567+
if h.operationBlocker.safelistEnabled || h.operationBlocker.logUnknownOperationsEnabled {
568+
if !operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery.HasHash() {
569+
// Set the request hash to the parsed hash, to see if it matches a persisted operation
570+
operationKit.parsedOperation.GraphQLRequestExtensions.PersistedQuery = &GraphQLRequestExtensionsPersistedQuery{
571+
Sha256Hash: operationKit.parsedOperation.Sha256Hash,
572+
}
573+
}
562574
}
563575
}
564576
}

0 commit comments

Comments
 (0)