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

Jump to bottom

Fix nil pointer exception for nil interceptor #1325

Merged
merged 1 commit into from
Mar 21, 2022
Merged

Fix nil pointer exception for nil interceptor #1325

merged 1 commit into from
Mar 21, 2022

Conversation

savitaashture
Copy link
Contributor

@savitaashture savitaashture commented Mar 16, 2022 •
edited by dibyom
Loading

Changes

Issue: While creating EL if user provide null in Interceptors webhook pod will crash
Root Cause: Failure is because of accessing nil object

github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1.(*TriggerInterceptor).defaultInterceptorKind(...)
	github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1/trigger_types.go:139
github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1.(*EventListener).SetDefaults(0xc0004cd500, 0x22586f8, 0xc000b15920)
	github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1/event_listener_defaults.go:45 +0x144

https://github.com/tektoncd/triggers/blob/main/pkg/apis/triggers/v1alpha1/trigger_types.go#L139

Fix: Added validation to check interceptor is nil or not if nill webhook validation through error

More Info: https://tektoncd.slack.com/archives/CKUSJ2A5D/p1647257237334749

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

NONE

@savitaashture savitaashture requested a review from dibyom March 16, 2022 11:10
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Mar 16, 2022
@tekton-robot tekton-robot requested a review from dlorenc March 16, 2022 11:10
@tekton-robot tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 16, 2022
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 86.7% 87.5% 0.8
pkg/apis/triggers/v1alpha1/event_listener_validation.go 97.8% 97.1% -0.7
pkg/apis/triggers/v1beta1/event_listener_validation.go 98.0% 97.4% -0.6

@khrm
Copy link
Contributor

khrm commented Mar 16, 2022

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Mar 16, 2022
@tekton-robot
Copy link

[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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2022
@savitaashture
Copy link
Contributor Author

/test pull-tekton-triggers-integration-tests

@savitaashture
Copy link
Contributor Author

@dibyom PTAL
Thank you

@dibyom dibyom linked an issue Mar 17, 2022 that may be closed by this pull request
Nil pointer dereference when Interceptors is nil #1328
Closed
dibyom
dibyom reviewed Mar 17, 2022
View reviewed changes
pkg/apis/triggers/v1alpha1/event_listener_validation.go
@@ -286,6 +286,10 @@ func (t *EventListenerTrigger) validate(ctx context.Context) (errs *apis.FieldEr

// Validate optional Interceptors
for i, interceptor := range t.Interceptors {
// No continuation if provided interceptor is nil.
if interceptor == nil {
return errs.Also(apis.ErrInvalidValue(fmt.Sprintf("interceptor '%v' must be a valid value", interceptor), fmt.Sprintf("interceptors[%d]", i)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a quick unit test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dibyom added unit test
PTAL

@dibyom
Copy link
Member

dibyom commented Mar 17, 2022

Thanks @savitaashture This looks good - can we just add a unit test to verify the behavior.
I also opened an issue for the bug since the Slack thread will disappear eventually.

@tekton-robot tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 21, 2022
@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 86.7% 87.5% 0.8
pkg/apis/triggers/v1alpha1/event_listener_validation.go 97.8% 97.8% 0.0
pkg/apis/triggers/v1beta1/event_listener_validation.go 98.0% 98.0% 0.0

@tekton-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/triggers/v1alpha1/event_listener_defaults.go 86.7% 87.5% 0.8
pkg/apis/triggers/v1alpha1/event_listener_validation.go 97.8% 97.8% 0.0
pkg/apis/triggers/v1beta1/event_listener_validation.go 98.0% 98.0% 0.0

@dibyom
Copy link
Member

dibyom commented Mar 21, 2022

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2022
@tekton-robot tekton-robot merged commit 42f8a9d into tektoncd:main Mar 21, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dibyom dibyom dibyom left review comments

@khrm khrm khrm approved these changes

@dlorenc dlorenc Awaiting requested review from dlorenc

Assignees

@dibyom dibyom

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nil pointer dereference when Interceptors is nil
4 participants
@savitaashture @tekton-robot @khrm @dibyom