Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Enable TriggerBindings to validate requests #89

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

khrm
Copy link
Contributor

@khrm khrm commented Sep 1, 2019

This PR resolves the issue #45.
It assumes that a task has been defined which can validate requests.
That task will receive header and payload as params.
Before the creation of resources, task will be called alongwith serviceaccount
which has github-secret used to create webhook.
Assumption:

  1. Task is defined in such a way that it can use headers and payload received as params.
  2. Apart from serviceaccount, payload and headers, task doesn't need anything else.
  3. Task gives us non zero exit if validation failed. A sample task and main.go is provided.

PS: This is still a work in progress. Need to test out further. Will do on Tuesday as Monday is holiday.

Changes

Submitter Checklist

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

See the contribution guide for more details.

Release Notes

- API changes: Added Validate Trigger Struct
- Any changes in behavior: Factor-out execute trigger to run concurrently.

@tekton-robot tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 1, 2019
@tekton-robot tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 1, 2019
cmd/securetrigger/main.go Outdated Show resolved Hide resolved
@khrm khrm force-pushed the issue45 branch 6 times, most recently from bbc8ead to 32f130f Compare September 4, 2019 09:57
@akihikokuroda
Copy link
Member

akihikokuroda commented Sep 4, 2019

This is not a part of the assumptions but I believe that it is an useful capability.

Would you add functions that set params into the event listener and pass them to the validation task? Otherwise the param values must be hard coded into the task and task is not reusable.

Use case: I specify the secret name (that has the secret key of the github) when I create the eventlistener and the secret name is passed to the validation task as one of the params so that the task can mount the secret.

@khrm
Copy link
Contributor Author

khrm commented Sep 4, 2019

Good idea, will add that.

@khrm khrm changed the title WIP: Enable TriggerBindings to validate requests Enable TriggerBindings to validate requests Sep 4, 2019
@tekton-robot tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2019
@khrm khrm force-pushed the issue45 branch 4 times, most recently from 6188fcc to c27bc5b Compare September 5, 2019 15:14
ncskier pushed a commit to ncskier/triggers that referenced this pull request Sep 5, 2019
Fixes tektoncd#89 so the EventListener sink Deployment and Service have the proper
permissions to set the EventListener as their owner.
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

I really like the approach especially the fact that we are reusing tasks and taskruns!! Added some comments on the polling approach for checking if the taskrun is complete. In addition, I'd like to see some basic docs around the TriggerValidate type as well as an example showing it in action. But overall, looking good 😃

pkg/sink/sink.go Outdated Show resolved Hide resolved
pkg/sink/sink.go Outdated Show resolved Hide resolved
pkg/sink/sink.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/event_listener_types.go Outdated Show resolved Hide resolved
docs/validate-webhook.yaml Outdated Show resolved Hide resolved
docs/validate-webhook.yaml Outdated Show resolved Hide resolved
cmd/securetrigger/main.go Outdated Show resolved Hide resolved
cmd/securetrigger/main.go Outdated Show resolved Hide resolved
cmd/securetrigger/main.go Outdated Show resolved Hide resolved
pkg/apis/triggers/v1alpha1/event_listener_types.go Outdated Show resolved Hide resolved
docs/validate-webhook.yaml Outdated Show resolved Hide resolved
@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 11, 2019
@khrm
Copy link
Contributor Author

khrm commented Sep 11, 2019

@dibyom I feel second option as better. I want to avoid resources. I am changing it to that.

@dibyom
Copy link
Member

dibyom commented Sep 11, 2019

@khrm cool, I like @vtereso 's idea as well:

Joining the array on space or some other delimiter was what I meant behind flattening. If we are limited by arrays, maybe this is preferable?

I do agree at the moment, resources are probably not we want but later when we are done with all the cool resource extensibility stuff, it is something we could consider using

@khrm
Copy link
Contributor Author

khrm commented Sep 11, 2019

I do agree at the moment, resources are probably not we want but later when we are done with all the cool resource extensibility stuff, it is something we could consider using

Yes, let's wait for resource extensibility. For now, I have done json encoding. Will update the docs with that instruction.

docs/eventlisteners.md Outdated Show resolved Hide resolved
docs/validate-event.md Outdated Show resolved Hide resolved
This PR resolves the issue tektoncd#45.
It assumes that a task has been defined which can validate requests.
That task will receive header and payload as params.
Before the creation of resources, task will be called alongwith serviceaccount
which has github-secret used to create webhook.

Assumption:
    1. Task is defined in such a way that it can use headers and payload received as params.
    2. Apart from params, serviceaccount, payload and headers, task doesn't need anything else.
    3. Task gives us non zero exit if validation failed. A sample task and main.go is provided.

PS: This is still a work in progress. Need to test out further. Will do on Tuesday as Monday is holiday.
@tekton-robot tekton-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 12, 2019
Copy link
Member

@dibyom dibyom left a comment

Choose a reason for hiding this comment

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

Looks good to me. Holding so that @vtereso can take another look 👼

/lgtm
/hold

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, khrm, vtereso

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

@vtereso
Copy link

vtereso commented Sep 12, 2019

/hold cancel

@tekton-robot tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2019
@tekton-robot tekton-robot merged commit f51a54c into tektoncd:master Sep 12, 2019
@khrm khrm deleted the issue45 branch September 12, 2019 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants