chipingress: send resource attributes as prefixed gRPC metadata - #2288
Draft
pkcll wants to merge 2 commits into
Draft
chipingress: send resource attributes as prefixed gRPC metadata#2288pkcll wants to merge 2 commits into
pkcll wants to merge 2 commits into
Conversation
Resource attributes describe the producer, not any individual event. Carrying
them as per-event CloudEvent extensions repeated identical bytes for every
event in a batch — for a thousand events with ten attributes, roughly 300 KB
that counts against maxGRPCRequestSize and so reduces how many events fit per
batch. OTLP factors resource out of the payload for the same reason. Send them
once per request as gRPC metadata instead.
Every emitted key is ResourceHeaderPrefix ("resource_") followed by a key
normalized to grpc's charset, which grpc-go gives as [0-9a-z-_.]
(internal/metadata.ValidateKey). Structure therefore survives —
csa_public_key becomes resource_csa_public_key rather than collapsing to
csapublickey — which is what lets chip-ingress emit the forwarded header
verbatim. Values still go through SanitizeMetadataValue, because grpc-go fails
an entire RPC, auth header included, on one non-printable value. A trailing
"-bin" is rewritten so grpc does not try to base64-decode a plain-text
attribute.
The prefix is a wire contract with chip-ingress, which forwards metadata
carrying it onto every Kafka record a request produces. Requiring it inbound
and preserving it outbound keeps the namespace closed, and that is what
removes the need for a reserved-key set on either side. The header interceptor
appends to outgoing metadata rather than replacing, so an attribute named
X-Beholder-Node-Auth-Token would have sent a second value under the key
carrying the CSA node auth token and broken authentication; prefixed, it
becomes resource_x-beholder-node-auth-token and collides with nothing. The
same holds for authorization, te, content-type, the grpc- prefix and
pseudo-headers, so reservedMetadataHeaderNames, reservedMetadataKeys and
isReservedMetadataKey are all deleted rather than extended. A test asserts the
property directly, in place of the set it replaces.
Removes EventOpt, NewEventWithOpts and WithResourceAttributeExtensions, which
existed only for the extension path and have no callers in either repository;
NewEvent returns to being the single event constructor. With
SanitizeMetadataHeaders the sole consumer of the shared key helper, fold it in
and delete resource_attributes.go and the resourceAttrKey pair type — the
sanitized output map doubles as the dedupe set. SanitizeMetadataKey becomes
unexported, since nothing outside the package used it.
Adds ResourceHeaderPrefix. The same constant exists in chip-ingress as
constants.ResourceHeaderPrefix; duplicating a wire contract across
repositories matches how authHeaderKey is already spelled in both
pkg/beholder and pkg/chipingress, and the two must stay byte-identical or
forwarding silently stops.
Contributor
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates pkg/chipingress resource-attribute propagation to use request-scoped gRPC metadata only, namespaced under a resource_ prefix, and removes the prior per-event CloudEvent extension mechanism. This aligns resource attributes with their producer-scoped semantics and avoids repeated per-event payload bloat while ensuring attributes cannot collide with reserved/auth metadata keys.
Changes:
- Introduces
ResourceHeaderPrefix = "resource_"as the wire contract for resource attributes sent via gRPC metadata. - Reworks
SanitizeMetadataHeadersto (1) structure-preserving normalize keys to gRPC’s allowed charset and (2) always prefix withresource_, replacing the previous reserved-key deny-list approach. - Removes the CloudEvent extension path (
EventOpt,NewEventWithOpts,WithResourceAttributeExtensions) and deletes the shared resource-attribute helper file, consolidating logic around metadata.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/chipingress/types.go | Adds ResourceHeaderPrefix constant and removes reserved-key sets tied to the deleted CE-extension path. |
| pkg/chipingress/resource_attributes.go | Deletes the shared resource-attribute key helper now made redundant by the new metadata sanitization approach. |
| pkg/chipingress/header_provider.go | Implements structure-preserving metadata key normalization, applies resource_ prefix to all emitted keys, and removes reliance on reserved-key lists. |
| pkg/chipingress/header_provider_test.go | Updates/expands tests to assert prefixing, normalization rules, -bin rewrite, deterministic collision handling, and non-collision with reserved/auth keys. |
| pkg/chipingress/client.go | Removes CE-extension event opts and documents that resource attributes are request-scoped via WithResourceAttributeHeaders; small refactor for NOP lookup header key. |
| pkg/chipingress/client_test.go | Removes CE-extension tests and adds an integration-style test asserting auth token coexists with prefixed resource attributes without collision. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
This PR is stale because it has been open 30 days with no activity. |
SanitizeMetadataHeaders now omits an invalid key, non-printable value, duplicate, or over-limit attribute rather than rewriting it, so two distinct configured keys can never collapse into one gRPC metadata key and a non-printable value can never be silently byte-mangled. Adds the 32-attribute / 128B-key / 512B-value / 4096B-total caps, and warns plus meters every dropped attribute via WithResourceAttributeHeaders.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2267, which added resource-attribute support to the ChipIngress client via two
mechanisms: per-event CloudEvent extensions and per-request gRPC metadata. This keeps the metadata
path, gives it a namespace, and deletes the extension path.
Why metadata rather than per-event extensions
Resource attributes describe the producer, not any individual event. That is why OTLP factors
resourceout of its payload rather than repeating it on every log record, and the same reasoningapplies here.
Per-event stamping also duplicated bytes: ~10 attributes across a 1,000-event batch is roughly 300 KB
of identical content, which counts against
maxGRPCRequestSizeand therefore reduces how many eventsfit per batch.
Chip-ingress already fans connection-scoped values out onto every Kafka record — the verified auth
token becomes
ce_csapublickey, the NOP lookup becomesce_nodeoperatorname— viabaseKafkaHeaders,cached per
(domain, entity, specVersion). Resource attributes now follow that same path.The
resource_contractEvery emitted metadata key is
resource_+ a key normalized to grpc's charset, which grpc-go gives as[0-9a-z-_.](internal/metadata.ValidateKey). Structure therefore survives —csa_public_keybecomes
resource_csa_public_keyrather than collapsing tocsapublickey— which is what letschip-ingress emit the forwarded header verbatim.
The prefix is why neither side needs a reserved-key set. The header interceptor appends to
outgoing metadata rather than replacing it, so an attribute landing on an existing header name sends
two values under one key. For the CSA auth token that breaks authentication. Prefixed, that attribute
becomes
resource_x-beholder-node-auth-tokenand collides with nothing — and the same holds forauthorization,te,content-type, thegrpc-prefix and pseudo-headers.So
reservedMetadataHeaderNames,reservedMetadataKeysandisReservedMetadataKeyare deletedrather than extended, replaced by a test asserting the property directly. Enumerated deny-lists have
to be maintained correctly forever; this one is closed by construction.
Validate, don't rewrite
An earlier revision of this PR normalized an invalid key by replacing disallowed characters with
_,and an invalid value by replacing each non-printable byte with
?. Both were reverted in favor ofvalidating and omitting:
sanitizeMetadataKeynow returns("", false)for anything that doesn'talready match
[0-9a-z-_.]+(once lower-cased) or that ends in-bin, and a non-printable value isdropped rather than byte-mangled.
Rewriting silently collapses distinct operator-configured keys into one gRPC metadata key — e.g. two
attributes that only differ by an illegal character both landing on the same sanitized name — which is
worse than dropping the offending attribute outright.
SanitizeMetadataHeadersnow returns(map[string]string, []DroppedAttribute);DroppedAttribute{Key, Reason}records what was omitted andwhy (
invalid_key,invalid_value,duplicate_key,limit_exceeded), so a caller can warn and meterinstead of silently losing data.
WithResourceAttributeHeadersdoes exactly that: it logs a warning(via an overridable, no-op-by-default
*zap.Loggerset throughSetResourceAttributeLogger) andincrements an otel counter (
chipingress.resource_attribute.dropped, tagged byreason, against theglobal
MeterProvider) for every dropped attribute.Limits
SanitizeMetadataHeadersnow caps resource attributes at 32 entries, 128 bytes per key, 512 bytes pervalue, and 4096 bytes of combined accepted key+value bytes (prefix excluded) — reserving headroom in
the gRPC HEADERS frame for authentication and normal gRPC metadata. Keys are processed in sorted order,
so which entries get dropped once a limit is hit is deterministic rather than map-iteration-order
dependent.
Changes
header_provider.go—sanitizeMetadataKeyvalidates instead of rewriting; addsisPrintableASCII; adds the fourmaxResourceAttribute*limit constants; addsDroppedAttributeand the
reason*constants;SanitizeMetadataHeadersnow validates, dedupes, enforces limits, andreturns the dropped list alongside the accepted headers.
SanitizeMetadataValue(the old byte-by-byterewriter) is deleted — it had no callers outside this file's own tests once the check became a
pass/fail predicate.
client.go—WithResourceAttributeHeaderswarns and meters every dropped attribute; adds thepackage-level
resourceAttributeLogger(no-op by default) andSetResourceAttributeLoggertooverride it; adds the lazily-constructed
resourceAttributeDropsCounteragainst the global otelmeter. Also (from the prior revision): deletes
EventOpt,NewEventWithOptsandWithResourceAttributeExtensions;NewEventis the single event constructor again.types.go— addsResourceHeaderPrefix; deletes the reserved-key set and its predicate.resource_attributes.go— deleted; folded intoheader_provider.gonow thatSanitizeMetadataHeadersis the sole consumer of the shared key/dedupe logic.Exported API
EventOpt,NewEventWithOpts,WithResourceAttributeExtensions,SanitizeMetadataValueResourceHeaderPrefix,DroppedAttribute,SetResourceAttributeLoggerSanitizeMetadataHeadersnow returns(map[string]string, []DroppedAttribute)and validates instead of rewritingAll removals were added by #2267 and have zero callers in either chainlink-common or chainlink —
verified by grep across both repos.
NewStaticHeaderProviderandWithResourceAttributeHeaderskeeptheir signatures;
WithResourceAttributeHeaders's behavior changes only in what it warns/meters.Paired changes
resource_onto every Kafkarecord. Independent of this PR; consumers see nothing until both are deployed.
resource_prefix — it callsWithResourceAttributeHeaders, so both the prefixing and the new validate/limit/observabilitybehavior happen transparently in here. It does need its
pkg/chipingresspin bumped once thismerges.
ResourceHeaderPrefixis deliberately duplicated asconstants.ResourceHeaderPrefixinchip-ingress — the same cross-repo duplication
authHeaderKeyalready has betweenpkg/beholderandpkg/chipingress. The two must stay byte-identical or forwarding silently stops.Related PRs
This is the upstream PR in the resource-attribute stack:
primitives:
resource_-prefixed gRPC metadata, validation, caps)Config.ResourceAttributesinto this PR'sWithResourceAttributeHeaders; needs apkg/chipingresspin bump once this merges)server; forwards
resource_-prefixed metadata onto Kafka headers; independent of beholder: send resource attributes to chip ingress as gRPC metadata #2216, dependsonly on this PR's wire contract)
Test plan
Tests bind
127.0.0.1:0, so they need a sandbox that permits listening. Notable cases: prefixing andstructure preservation; validate-don't-rewrite for both invalid keys and non-printable values (an
invalid entry is omitted with a reason, never rewritten); deterministic collapse of keys that validate
to the same name; each of the four limits (oversized key, oversized value, >32 attributes, >4096
combined bytes) dropping deterministically in sorted-key order; and — on a live gRPC connection — that
the CSA auth token arrives exactly once alongside prefixed attributes, including one deliberately named
after the auth header.