another possible source of sensitive data to strip would be the observed connection details of the resources. Those get passed in the request also and we should consider stripping those.

Looks like you've also added stripping out any Secret composed resources, which makes sense given that is the recommended path for exposing XR connection details in v2.

However, I was referring to the connection details that are included for every observed resource, e.g. the connection_details field in the protobuf. That should get stripped too to be consistent, right?

This will be using unary RPC right? https://grpc.io/docs/what-is-grpc/core-concepts/#unary-rpc

I think the EmitRequest() call in the InspectingFunctionRunner would technically be blocking then - the client gRPC call will block until the server sends a response or the context times out.

This likely will not cause any problems (haven't heard of it causing any problems in changelogs that are in the middle of the managed reconciler), because it is just over a local unix socket and responses should have very minimal latency. But it may still be good to acknowledge that this isn't technically non-blocking.

we could consider to use a pretty short context/timeout then just for the gRPC calls, so they can't ever have too much of slow down effect on the overall function pipeline.

do i have that right about the blocking nature of unary gRPC? what do you think about a shorter context for them?

Good catch! I initially thought of handling them from a separate goroutine, but I ended up deciding not to. Just continuing on error could be enough indeed, I'll update the doc accordingly.

yep that makes sense to continue on error. i did want to point out, just to be sure you've seen it, the approach taken for change logs that uses a separate shorter context instead of the outer parent context:
https://github.com/crossplane/crossplane-runtime/blob/main/pkg/reconciler/managed/changelogger.go#L114-L117

Perhaps this is what you were referring to later in the doc with this?

The default emit timeout is 100ms. If the sidecar doesn't respond within this time, the emit fails and pipeline execution continues.

Yes, that's what I meant 👍

⚠️ Potential issue | 🟠 Major

Consider redacting/controlling pipeline context data.

Functions can write arbitrary data into the pipeline context, which is included in RunFunctionRequest. If that data can be sensitive, emitting it verbatim risks leaking secrets/PII. Should we define a sanitization strategy (e.g., allowlist, denylist, or opt‑in emission for context keys) in this section? Thanks for considering.

-The `RunFunctionRequest` includes `credentials` and connection details fields that may contain sensitive data, **this field must be cleared before emission**.
+The `RunFunctionRequest` includes `credentials` and connection details fields that may contain sensitive data, **this field must be cleared before emission**.
+Additionally, the pipeline `context` can contain arbitrary data written by functions; consider a
+clear policy for sanitizing or selectively emitting context keys to avoid leaking secrets/PII.

In `@design/one-pager-pipeline-inspector.md` around lines 302 - 307, Update the
"Security: Credential Stripping" section to explicitly require sanitization of
the pipeline RunFunctionRequest.context in addition to clearing
RunFunctionRequest.credentials and Secret.data: define the chosen strategy
(allowlist, denylist, or opt‑in emission) and the exact behavior (which keys are
emitted, which are redacted, and default policy), state who controls the policy
and how to opt in/out, and add a brief example of sanitization rules so
implementers know to filter context keys before emitting RunFunctionRequest.

⚠️ Potential issue | 🟡 Minor

# Search for existing Secret kind detection patterns in the codebase
rg -nP --type=go -C3 '\bkind.*[Ss]ecret|[Ss]ecret.*kind' | head -100

# Also search for how resources are typically checked for their kind/type in Go
rg -nP --type=go -C2 'GetKind\(\)|\.Kind' | head -80

# Read the specific lines in the design document to understand the context
cat -n design/one-pager-pipeline-inspector.md | sed -n '200,220p'

# Read the sanitization section mentioned in the review
cat -n design/one-pager-pipeline-inspector.md | sed -n '360,380p'

# Read the BuildMetadata section completely to understand what it extracts
cat -n design/one-pager-pipeline-inspector.md | sed -n '150,230p'

Clarify the Secret detection mechanism in the sanitization section.

Good catch that BuildMetadata extracts the CompositeResourceKind at line 208! However, the sanitization logic at lines 362-363 and 371-372 is shown as placeholders (// ...) for detecting and clearing Secret data. Could you briefly explain how the sanitizer will identify which resources are Secrets (e.g., by checking kind == "Secret")? Also, clarifying what "drop data" means for Secrets would be helpful—does it mean clearing the entire data field in the Kubernetes Secret structure, or specific sub-fields?

In `@design/one-pager-pipeline-inspector.md` around lines 204 - 217, The sanitizer
must detect Kubernetes Secrets by checking the extracted metadata: if
meta.CompositeResourceKind (from BuildMetadata) equals "Secret" (compare
case-insensitively and optionally verify apiVersion/group indicates core/v1),
then treat it as sensitive; implement the sanitizer branch to "drop data" by
clearing the Secret payload fields (set the Secret's data and stringData fields
to an empty map or nil) and avoid returning any secret values (you can also
remove binaryData if present), and record/annotate that data was stripped so
callers know content was redacted. Ensure the check references
meta.CompositeResourceKind and meta.CompositeResourceAPIVersion and the
sanitizer function that currently contains the Secret placeholders so the logic
replaces the // ... with these explicit checks and field clears.

the implementation will address that.

@phisco, understood! It makes sense to work out those implementation details when coding the actual sanitizer. The design document establishes the intent to redact Secret data, and the specifics of detection and field handling can be refined during development.

Thanks for clarifying! 👍

Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs

Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Discuss changes in a GitHub issue before starting implementation; new features can only be merged during the active development period of a Crossplane release cycle

⚠️ Potential issue | 🟠 Major

Reconsider the security boundary for the context field.

The current design passes the pipeline context field through as-is with the reasoning that "functions already have access to it and it typically contains non-sensitive configuration data." However, I'm concerned about this for a few reasons:

1. Different trust boundaries: Functions run as part of the pipeline with certain security constraints, while the sidecar is a separate component that may have different access patterns or be implemented by third parties.

2. User-defined data: Functions can write arbitrary data to the context, and we can't guarantee it's non-sensitive. For example, a custom function might cache intermediate computed values, tokens, or other data.

3. "Typically" isn't "always": The phrase "typically contains non-sensitive configuration data" acknowledges there could be exceptions.

Could you clarify the decision here? Options might include:

• Define an allowlist of context keys that are safe to emit
• Add a configuration option to control context emission
• Document that implementations should sanitize context
• Provide guidance on what should/shouldn't be written to context

Past reviews flagged this concern; I want to make sure we've thought through the security implications. Thanks!

Based on learnings, security review is critical for Crossplane.

In `@design/one-pager-pipeline-inspector.md` around lines 347 - 349, The design
currently emits the pipeline "context" field as-is which creates a security
risk; update the document to treat context as potentially sensitive by (1)
proposing an allowlist approach for safe context keys and an explicit
API/configuration flag to control context emission, (2) requiring that the
sidecar or pipeline implementation sanitize or redact context entries (esp.
user-written values) before emitting composed resources, and (3) adding clear
guidance for authors about what may never be written to context (secrets,
tokens, PII) and documenting default behavior and override mechanism; reference
the "context" field, composed resource handling for Secrets (clear `data`), and
the sidecar/pipeline emission path in the text so reviewers can find and apply
these changes.

that's a fair concern, but there is little we can do here unfortunately, i think that's where we should draw the boundary between what we need to solve on crossplane side and what downstream implementations should solve.

@phisco, that makes sense—I appreciate the clarification on where the boundary lies. Following the pattern of minimal upstream hooks with downstream implementations handling the specifics is architecturally consistent with other Crossplane designs like change logs.

Since the responsibility for sanitizing the context field falls to downstream implementations, would it make sense to add a note in the design document highlighting this? Something like:

Note on Context Security: The pipeline context field is emitted as-is. Downstream sidecar implementations are responsible for sanitizing or filtering context entries based on their security requirements and trust boundaries. Platform teams should review what data functions write to the context and configure their inspector implementations accordingly.

This would help ensure downstream implementers are aware of the security considerations when building their sidecars. But if you feel the current documentation is sufficient, I'm good with the decision as-is. Thanks for thinking through the design boundary with me! 🙏

Learnt from: phisco
Repo: crossplane/crossplane PR: 7025
File: design/one-pager-pipeline-inspector.md:347-349
Timestamp: 2026-01-22T17:27:31.137Z
Learning: In the Pipeline Inspector design for Crossplane, the pipeline `context` field is intentionally emitted as-is to downstream implementations. The security boundary is drawn such that Crossplane provides the minimal hook mechanism, and downstream sidecar implementations are responsible for sanitizing or filtering the context based on their security requirements.

Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Review code for functional correctness, proper documentation and intent explanation, adherence to Coding Style guidelines, idiomatic alignment with existing Crossplane APIs, sufficient test coverage, DCO sign-off, and passing CI jobs

Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Discuss changes in a GitHub issue before starting implementation; new features can only be merged during the active development period of a Crossplane release cycle

What's 0?

The iteration index, starts from zero, it's increased if unsatisfied requirements are received and another call is needed.

dropped from the signature, given that it's always going to be initialized to 0 there

Worth using ulid or some other ordered UID for span and trace IDs?

They have no such guarantee in OTEL, and although I agree it would be nice, I don't want to give more guarantees than required. It should be completely opaque for now.

What if it's an operation function? Worth having distinct sub-messages for composition and operation?

The tradeoff of JSON bytes is more space on the wire and potentially on disk, right? Is expecting consumers to have the protobuf schema unreasonable?

I don't think they'd need the same schema as Crossplane. Though to be fair if they didn't they might miss fields that were actually in the request (which JSON wouldn't).

This is going to the sidecar, so the wire size is less of an issue. Then if the sidecar wants to convert it back to a more storage/network optimized encoding for its own purposes, it can. I think it makes sense to use plain json here and have as little assumptions as possible for sidecars.

Does it? It's more likely to be within the limit, but I imagine a request very close to the limit + the step meta could push it over.