Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Interceptor references without a kind don't get a URL set for execution #1499

Closed
abayer opened this issue Dec 20, 2022 · 6 comments · Fixed by #1500
Closed

Interceptor references without a kind don't get a URL set for execution #1499

abayer opened this issue Dec 20, 2022 · 6 comments · Fixed by #1500
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@abayer
Copy link
Contributor

abayer commented Dec 20, 2022

Expected Behavior

Interceptor references without a kind should default to being treated as ClusterInterceptors and have their URL set appropriately on Triggers v0.22.0, which worked properly in v0.21.0 and earlier.

Actual Behavior

On v0.22.0, interceptor references without a kind skip over the if block at

triggers/pkg/sink/sink.go

Lines 510 to 539 in 8797cc0

if i.Ref.Kind == triggersv1.ClusterInterceptorKind {
ic, err := r.ClusterInterceptorLister.Get(i.GetName())
if err != nil {
return nil, nil, nil, fmt.Errorf("url resolution failed for interceptor %s with: %w", i.GetName(), err)
}
if ic.Status.Address != nil && ic.Status.Address.URL != nil {
url = ic.Status.Address.URL
} else if url, err = ic.ResolveAddress(); err != nil {
return nil, nil, nil, fmt.Errorf("url resolution failed for interceptor %s with: %w", i.GetName(), err)
}
if err != nil {
return nil, nil, nil, fmt.Errorf("could not resolve clusterinterceptor URL: %w", err)
}
} else if i.Ref.Kind == triggersv1.NamespacedInterceptorKind {
if r.InterceptorLister == nil {
r.Logger.Debugf("nil lister")
}
ic, err := r.InterceptorLister.Interceptors(r.EventListenerNamespace).Get(i.GetName())
if err != nil {
return nil, nil, nil, fmt.Errorf("url resolution failed for interceptor %s with: %w", i.GetName(), err)
}
if addr := ic.Status.Address; addr != nil && addr.URL != nil {
url = addr.URL
} else if url, err = ic.ResolveAddress(); err != nil {
return nil, nil, nil, fmt.Errorf("url resolution failed for interceptor %s with: %w", i.GetName(), err)
}
if err != nil {
return nil, nil, nil, fmt.Errorf("could not resolve clusterinterceptor URL: %w", err)
}
}
and never get url set, causing errors to show up in the eventlistener logs like:

{"severity":"error","timestamp":"2022-12-20T12:50:32.302Z","logger":"eventlistener","caller":"sink/sink.go:320","message":"Post \"\": unsupported protocol scheme \"\"","eventlistener":"tekton-events","namespace":"default","/triggers-eventid":"7b12fc21-7d0b-4f86-ad11-f33a03b87231","eventlistenerUID":"b37832bb-3e30-4516-9b04-c41d626e9321","/triggers-eventid":"7b12fc21-7d0b-4f86-ad11-f33a03b87231","/triggergroup":"ci-job-triggers"}

Steps to Reproduce the Problem

  1. On Triggers v0.22.0, create an EventListener referencing an interceptor by name without specifying the kind.
  2. Try to trigger that event listener.
  3. See failures.

Additional Info

This was encountered on Tekton's dogfooding cluster after upgrading to v0.22.0.

@abayer abayer added the kind/bug Categorizes issue or PR as related to a bug. label Dec 20, 2022
@abayer
Copy link
Contributor Author

abayer commented Dec 20, 2022

I think #1492 is likely the same root problem.

abayer added a commit to abayer/triggers that referenced this issue Dec 20, 2022
See tektoncd#1499

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
abayer added a commit to abayer/plumbing that referenced this issue Dec 20, 2022
Due to tektoncd/triggers#1499, all our event listeners were failing. I've pushed an image of `eventlistenersink` built from https://github.com/abayer/triggers/tree/default-to-clusterinterceptor (just one commit on top of Triggers v0.22.0) that fixes this and configured the `tekton-triggers-controller` deployment to use that image (gcr.io/abayer-jclouds-test1/eventlistenersink-7ad1faa98cddbcb0c24990303b220bb8@sha256:6d7769173bec31635bc9994fa5ffa3ae40c493315bf4ae7cca5aff24d824a889) as its `-el-image` argument, which has gotten everything working again, but if we'd prefer to run vanilla v0.22.0, this PR adds an explicit `kind: ClusterInterceptor` to every interceptor reference I could find, which should have the same effect.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer
Copy link
Contributor Author

abayer commented Dec 20, 2022

cc @afrittoli @dibyom @savitaashture - this is what borked Dogfooding. =)

@savitaashture
Copy link
Contributor

Hi @abayer i see we have added default kind as ClusterInterceptor here https://github.com/tektoncd/triggers/blob/release-v0.22.x/pkg/apis/triggers/v1beta1/trigger_types.go#L129-L133

And I have tried installing Triggers v0.22.x and installed github example and sent a curl request by following README from here https://github.com/tektoncd/triggers/tree/release-v0.22.x/examples/v1beta1/github and its working without any issues

@abayer
Copy link
Contributor Author

abayer commented Dec 21, 2022

I'd suggest trying to install v0.21.0, create an EventListener/Trigger/etc, and then upgrade to v0.22.0 - I'm pretty sure that the new field isn't being defaulted for existing resources, just new/updated ones.

tekton-robot pushed a commit to tektoncd/plumbing that referenced this issue Dec 21, 2022
Due to tektoncd/triggers#1499, all our event listeners were failing. I've pushed an image of `eventlistenersink` built from https://github.com/abayer/triggers/tree/default-to-clusterinterceptor (just one commit on top of Triggers v0.22.0) that fixes this and configured the `tekton-triggers-controller` deployment to use that image (gcr.io/abayer-jclouds-test1/eventlistenersink-7ad1faa98cddbcb0c24990303b220bb8@sha256:6d7769173bec31635bc9994fa5ffa3ae40c493315bf4ae7cca5aff24d824a889) as its `-el-image` argument, which has gotten everything working again, but if we'd prefer to run vanilla v0.22.0, this PR adds an explicit `kind: ClusterInterceptor` to every interceptor reference I could find, which should have the same effect.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@abayer
Copy link
Contributor Author

abayer commented Dec 21, 2022

Worth mentioning that we call SetDefaults in the PipelineRun reconciler to deal with this sort of problem.

@abayer
Copy link
Contributor Author

abayer commented Dec 21, 2022

...and I see that's called in the EventListener reconciler too...

Ah! I think the problem is that we're using .Spec.TriggerGroups, and the defaulting in EventListener only covers .Spec.Triggers!

abayer added a commit to abayer/triggers that referenced this issue Dec 21, 2022
Fixes tektoncd#1499

While `Kind` is getting defaulted properly for interceptors in `EventListener.Spec.Triggers`, it also needs to be defaulted for interceptors in `EventListener.Spec.TriggerGroups`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
tekton-robot pushed a commit that referenced this issue Jan 3, 2023
Fixes #1499

While `Kind` is getting defaulted properly for interceptors in `EventListener.Spec.Triggers`, it also needs to be defaulted for interceptors in `EventListener.Spec.TriggerGroups`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
@github-project-automation github-project-automation bot moved this from Todo to Done in Tekton Community Roadmap Jan 3, 2023
tekton-robot pushed a commit to tekton-robot/triggers that referenced this issue Jan 13, 2023
Fixes tektoncd#1499

While `Kind` is getting defaulted properly for interceptors in `EventListener.Spec.Triggers`, it also needs to be defaulted for interceptors in `EventListener.Spec.TriggerGroups`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
tekton-robot pushed a commit that referenced this issue Jan 13, 2023
Fixes #1499

While `Kind` is getting defaulted properly for interceptors in `EventListener.Spec.Triggers`, it also needs to be defaulted for interceptors in `EventListener.Spec.TriggerGroups`.

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants