Implement TriggerResourceTemplate Spec as []byte blob

[vtereso]

Changes

Implement the TriggerResourceTemplate using []byte blob. Since any field may have a binding, it is impossible to guarantee validation on any. Since arbitrary resources can be templated, this evaluates to arbitrary json. Since we are already parsing json bytes, storing the message in this form is easy and better than an internal map[string]string.

Another option was to disallow interpolation on certain fields to get some form of validation, but I think we can revisit this one we are able to create resources. This way, we can determine if the flexibility of templating any field is worthwhile and if this validation is necessary.

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

_See the contribution guide for more details.

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

• OWNERS [vtereso]

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

[vtereso]

This is a small playground example program that would use this.

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

[googlebot]

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

[bobcatfish]

Two minor requests:

• Looking at the commit message for this one, it would be great to include some more context about why we want to "Implement TriggerResourceTemplate Spec as []byte blob" (e.g. explainging the kinds of functionality we want to support in the future, why this has to be a []byte blob for that, if there are any alternatives considered.
• Could we add a bit of data about this to our docs (https://github.com/tektoncd/triggers/blob/master/docs/triggertemplates.md), e.g. explaining that the body of the resourcetemplates is raw json, and no/little validation will be applied to it (I assume we probably want to do some validation of the ${params....} usage? maybe that should be a follow up issue?)

[bobcatfish]

Thanks for elaborating! 👍

/lgtm
/meow space

[bobcatfish]

👍

[tekton-robot]

@bobcatfish:

In response to this:

Thanks for elaborating! 👍

/lgtm
/meow space

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vdemeester]

@bobcatfish @vtereso Any reason for not using runtime.RawExtension instead ? (see https://godoc.org/k8s.io/apimachinery/pkg/runtime#RawExtension)

[bobcatfish]

@vdemeester in my case I just didn't know about it! Sounds like it's better than the approach we took? Maybe you can give us some pointers? ❤️ 🙏

[gabemontero]

Noting the relative age of this PR, I perhaps may need to register this comment elsewhere (perhaps as a new issue, perhaps in the Tekton Dynamic File Resource proposal ?). But full disclosure I discovered this PR when researching why raw JSON was currently being used.

Please do direct me to other locations to register this if that is more appropriate.

But minimally one advantage of switching TriggerResourceTemplate to https://godoc.org/k8s.io/apimachinery/pkg/runtime#RawExtension as @vdemeester notes is that it gets us on the path to using the k8s dynamic rest mapper APIs, which in turn will allow the trigger controller to employ k8s subject access reviews for the k8s objects to be created.

And if the associated user in question wanting to trigger something in tekton does not have permission to create such object, abort their creation.

Imagine a user creating a TriggerTemplate whose raw json attempts to create a a privileged object that can do nasty things.

I do see from Tekton 2019 Roadmap Reflections that security is still further down the roadmap, and has not been a primary focus per se yet, but it never hurts to start laying some groundwork, right :-) ?

Anyway I've got some cycles, and can start crafting a PR for consideration, at least for simply switching to RawExtension, if folks are amenable. I can throw some temporary commits on what the SAR flow might look like as well if the discussion goes that way.

thanks

[gabemontero]

quick update - I do see some use of unstructured as I've started to prep for my PR ... as I move forward, I'll add comments in the PR around if/how the move to runtime.RawExtension impacts the use of unstructured and ultimately SARs

thanks

[dibyom]

Hey @gabemontero

Thanks for writing this up! I think a new issue would be a great place to discuss this. I think #77 related to SAR checks.

@wlynch also added a Dynamic Client which maybe be related: #238

[gabemontero]

Hey @gabemontero

Thanks for writing this up! I think a new issue would be a great place to discuss this. I think #77 related to SAR checks.

will do @dibyom thanks ... and looking at #77 I probably should have caught the presence of that (as I work with @pmorie ;-) )

I'll respond to the latest comments there ... that said, again to some degree, to leverage runtime.RawExtension vs. what there today is simply best practice, regardless if it helps us get to SARs

@wlynch also added a Dynamic Client which maybe be related: #238

OK cool yes that could help the move to SARs along ... I'll take a close look

[dibyom]

that said, again to some degree, to leverage runtime.RawExtension vs. what there today is simply best practice,

Yes! I agree :)