Return clear error for unsupported interceptor kind in sink - #2096
Open
pujitha24 wants to merge 3 commits into
Open
Return clear error for unsupported interceptor kind in sink#2096pujitha24 wants to merge 3 commits into
pujitha24 wants to merge 3 commits into
Conversation
Motivation: Users report the eventlistener sink logging the confusing error `Post "": unsupported protocol scheme ""` while processing trigger interceptors, notably in clusters (e.g. ArgoCD-managed) where a Trigger can be created before the admission webhook has defaulted its interceptor kind. When a TriggerInterceptor's Ref.Kind is neither ClusterInterceptor nor NamespacedInterceptor, Sink.ExecuteInterceptors left its local `url` variable nil and passed it straight to interceptors.Execute. apis.URL.String() nil-checks and returns "", so the sink ends up POSTing to an empty URL and surfaces Go's raw transport error instead of a message that identifies the actual problem. Approach: Add the missing else branch alongside the existing Kind checks in ExecuteInterceptors (pkg/sink/sink.go) so an unrecognized/unset Ref.Kind returns a clear, immediate error naming the offending interceptor kind and name, instead of silently falling through to a nil URL. This does not change what happens to the request (the interceptor still cannot run and the trigger still will not fire either way) - it only replaces a cryptic low-level HTTP error with an actionable one and fails fast rather than after an HTTP round-trip attempt. Validation: - go build ./... - go vet ./pkg/sink/... - gofmt -l pkg/sink/sink.go pkg/sink/sink_test.go (no output) - make test-unit (go test ./...) passes across the full repo, including the new pkg/sink/sink_test.go:TestExecuteInterceptor_UnsupportedKind - Reproduced the exact reported error: temporarily reverted the sink.go change (keeping only the new test) and confirmed the test observes `Post "": unsupported protocol scheme ""`, byte-for-byte matching the error in the issue. Restored the fix and confirmed the same test now observes the new clear error message instead. Report: tektoncd#1772 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
tekton-robot
added
the
release-note
tekton-robot
added
the
size/S
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2096 +/- ##
===========================================
+ Coverage 32.14% 80.08% +47.94%
===========================================
Files 260 93 -167
Lines 11967 4510 -7457
===========================================
- Hits 3847 3612 -235
+ Misses 7797 651 -7146
+ Partials 323 247 -76
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
golangci-lint's dogsled linter flagged the 3-blank-identifier declaration added by this PR's new test. Name the []byte return value and include it in the failure message, consistent with the pattern already used elsewhere in this test file. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Contributor
Author
|
/assign @savitaashture |
khrm
reviewed
Aug 18, 2026
| resp, _, _, err := s.ExecuteTriggerInterceptors(trigger, &http.Request{URL: url}, json.RawMessage(`{"head": "blah"}`), s.Logger, "eventID", map[string]interface{}{}) | ||
| if err == nil { | ||
| t.Fatalf("ExecuteInterceptor() expected an error for an unsupported interceptor kind, got none: %+v", resp) | ||
| } |
Contributor
There was a problem hiding this comment.
Can we also assert that the error message contains "unsupported interceptor kind"?
Per review feedback, also verify the returned error names the unsupported interceptor kind rather than just checking that an error occurred. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Contributor
Author
|
Good catch, added — the test now also asserts |
khrm
reviewed
Sep 2, 2026
khrm
left a comment
khrm
left a comment
Contributor
There was a problem hiding this comment.
/approve
@savitaashture Let's review and merge this.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: khrm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
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
4 participants
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.
Changes
Fixes a confusing error surfaced by the eventlistener sink when a
TriggerInterceptor'sRef.Kindis neitherClusterInterceptornorNamespacedInterceptor(for example, when aTriggeris created before the admission webhook has defaulted the interceptor kind, as seen in ArgoCD-managed clusters). Previously,Sink.ExecuteInterceptorsleft the localurlvariablenilin this case and passed it straight tointerceptors.Execute, resulting in the sink POSTing to an empty URL and surfacing Go's raw transport errorPost "": unsupported protocol scheme ""instead of a message identifying the actual problem.This adds the missing
elsebranch alongside the existingKindchecks inExecuteInterceptors(pkg/sink/sink.go) so an unrecognized/unsetRef.Kindreturns a clear, immediate error naming the offending interceptor kind and name, and fails fast rather than after an HTTP round-trip attempt. This does not change the outcome for the request (the interceptor still cannot run and the trigger still will not fire either way) — it only replaces the cryptic low-level HTTP error with an actionable one.Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
AI assistance: this change was drafted with Claude Code.
Fixes #1772