Add resource attribute support to ChipIngress client#2216
Draft
pkcll wants to merge 9 commits into
Draft
Conversation
Contributor
✅ API Diff Results -
|
Propagate node resource attributes (from beholder.Config.ResourceAttributes) to the cre topic via gRPC metadata headers and sanitized CloudEvent extensions, so ChipIngress events carry the same node/environment identification already present on the legacy beholder__platform__messages topic.
grpc.WithUnaryInterceptor is last-one-wins, so configuring both a header provider and NOP lookup silently dropped one; switch to WithChainUnaryInterceptor so they compose. Also reserve the CloudEvents core context attribute names (id, source, type, specversion, data, etc.) so a resource attribute can never collide with them.
grpc-go hard-fails the entire RPC when an outgoing metadata pair fails its charset/printability validation, unlike the CE-extension path where an invalid entry is just dropped. Add SanitizeMetadataHeaders (reusing SanitizeExtensionName for keys, plus a printable-ASCII value sanitizer) and apply it once when building the static header provider, so a malformed resource-attribute key or value can no longer take down chip-ingress emission for the node.
After [a-z0-9] key sanitization, "te" is the only grpc-reserved header name reachable (every other reserved header contains a ':' or '-' that sanitization strips). Handle it deterministically via an explicit skip set rather than relying on grpc's own handling of a reserved header.
pkcll
force-pushed
the
chip-ingress-resource-attributes
branch
from
e6eabcc to
fdb6dee
Compare
Remove an unnecessary blank line in NewEvent and use net.ListenConfig in tests instead of deprecated net.Listen. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Introduce NewEventWithOpts for variadic EventOpt without changing NewEvent's exported signature, update beholder call sites, and bump the root module's pkg/chipingress dependency to include the new API. Co-authored-by: Cursor <cursoragent@cursor.com>
pkcll
force-pushed
the
chip-ingress-resource-attributes
branch
from
606a264 to
ada7f9b
Compare
2 tasks
Point pkg/chipingress at the squashed commit and drop stale go.sum entries so check-tidy passes make gomodtidy.
Keep the require pinned to the main-branch pseudo-version so go-mod validation passes, and replace to ./pkg/chipingress so root-module builds pick up this PR's chipingress API changes.
Adding a map field to a previously-comparable exported struct breaks apidiff. ChipIngressEmitterConfig gets a new NewWithResourceAttributes method instead of a ResourceAttributes field; New(client) is unchanged and delegates with nil attrs. ChipIngressEmitter stores resource attrs behind a pointer (resourceAttrs *resourceAttrExtensions) instead of a bare map, keeping the struct comparable.
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
1 participant
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.
Summary
beholder.Config.ResourceAttributesto the ChipIngresscretopic via two mechanisms: (1) gRPC metadata headers (chipingress.WithHeaderProvider/NewStaticHeaderProvider), raw key names, parity withbeholder__platform__messages; (2) CloudEvent extensions (chipingress.WithResourceAttributeExtensions) with keys sanitized to^[a-z0-9]+$per the CloudEvents spec, surfacing asce_<name>Kafka headers.ChipIngressEmitterandChipIngressBatchEmitterServiceto stamp resource attributes on every emitted event.Known gaps / follow-ups (called out for reviewers)
baseKafkaHeaderslogic does not appear to forward arbitrary incoming gRPC metadata onto Kafka headers today. The CE-extension mechanism guarantees headers immediately; the gRPC-metadata mechanism is shipped for observability now, with full parity once/if a chip-ingress server-side change forwards it. A follow-up ticket against chip-ingress should track that.pkg/durableemitteris out of scope. It has its ownNewEventcall site and config, and wiring it up would also requirechainlinkcore changes (application.go,shell.go,plugins/loop_registry.go). This assumes production CRE-topic traffic goes through the beholder emitters, notDurableEmitter— unconfirmed, worth a sanity check before merge.pkg/chipingressis its own Go module;go.mod's pinnedpkg/chipingresspseudo-version needs bumping to pick these changes up inpkg/beholder(standard two-step process per this repo's history).Test plan
go test ./pkg/chipingress/...andgo test ./pkg/beholder/...passSanitizeExtensionName,WithResourceAttributeExtensions(sanitization, reserved-name skip, deterministic dedup),NewStaticHeaderProvider,resourceAttributesToStringMap, and emitter-level tests assertingce_<sanitized>extensions land on published/batched events