ytt execution can be interrupted (via a Go Context)

[martyspiewak]

Describe the problem/challenge you have

We would like to use the ytt Go module but we'd like to protect against bad templates that may take down the system (e.g. a template that defines an infinite loop). If we shell out to the executable, we can pass a context with a timeout to protect against this. With the go module, though, there is no way to pass such a context.

Describe the solution you'd like

Allow the ytt Go module to accept a context so we can get a timeout.

Additional Information

Related Slack conversation: https://kubernetes.slack.com/archives/CH8KCCKA5/p1642715223106300

---

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

[pivotaljohn]

This is a very reasonable feature request.
For those who cannot take on this risk can mitigate it by integrating with ytt out-of-process (i.e. as an executable).

The implementation implies some complexity:

• plumbing to carry a "terminate" signal into ytt and
• further to actually cancel Starlark execution.

To perform time-based termination, ytt would need to plumb the means of carrying a terminate signal. Contexts seem like the approach that would need to be ruled out if another approach were taken.
Otherwise, ytt would need to expose whatever configuration Starlark itself provides.

The version of Starlark that ytt currently integrates with does not have such support.
However, Starlark has since gained this capability: google/starlark-go@949cc6f
This work implies upgrading to (at least) that version of Starlark and verifying that there are no breaking changes: carvel-dev/starlark-go@master...google:949cc6f4b09716061bc5c308049b0da811aeca10
ytt would then need to expose this configuration (i.e. maximum number of "steps"/op codes) to integrators.

[pivotaljohn]

This project is moving towards being more in-process integration-friendly: priority = unprioritized backlog. At this time, this item is not on the roadmap, but a PR would be fully supported.

[martyspiewak]

@pivotaljohn If we were to submit a PR for this, what would it take to verify there are no breaking changes when we update the starlark library? Is running all of the tests sufficient? Is there something more we should try to do?

[martyspiewak]

@pivotaljohn just looking through the commits, I notice that this commit is breaking.

If y'all are trying to avoid introducing breaking changes, would you be open to a PR to vmware-tanzu/starlark-go that just adds the cancellation behavior without the rest of the commits from google/starlark-go, or would you not want to further diverge from them?

[pivotaljohn]

@pivotaljohn If we were to submit a PR for this, what would it take to verify there are no breaking changes when we update the starlark library? Is running all of the tests sufficient? Is there something more we should try to do?

Apologies for the long delay in replying, @martyspiewak.

Yes, we are generally confident in the coverage of our test suite.

If y'all are trying to avoid introducing breaking changes, would you be open to a PR to vmware-tanzu/starlark-go that just adds the cancellation behavior without the rest of the commits from google/starlark-go, or would you not want to further diverge from them?

Yeah, we are pretty aggressive about avoiding breaking changes. I need a minute to remind myself of the history here. Is it possible to cherry-pick the cancel functionality... or in someway skip that breaking change?

[martyspiewak]

It looks like y'all are really using the ytt-1-jul-2020 branch in starlark-go (the commit referenced the go.mod file here only exists on that branch).

I was able to cherry-pick the commit that adds this functionality to that branch on a fork (of starlark-go) and it doesn't seem to break anything. I used that in a fork of ytt and the tests seem to pass there as well.

So, I guess the first step is to open a PR with this cherry-picked commit in starlark-go. Do you want me to open that PR against the "ytt-1-jul-2020" branch?

[martyspiewak]

Opened a PR against the ytt-1-jul-2020 branch. happy to change to a different branch if you want me to

[pivotaljohn]

Reviewed the PR, yesterday w/ @cppforlife.

We also spoke in depth about what approach would meet our goals, here: that is what API would ytt expose to afford users and integrators to terminate the invocation.

While Starlark (with the PR you've cherry-picked, thank you) provides the ability to set a maximum "step execution", the way that ytt manages Starlark threads would make it infeasible to select a winning number.

So, this issue's request — to plumb Context through to the Starlark integration — is really the only feasible way we see this working.

Also, we've (Carvel maintainers) prioritized this additional work as it seems of reasonably manageable scope and this is a particularly generally useful feature.

[pivotaljohn]

The contents of carvel-dev/starlark-go#3 are likely GTG, but tests on that branch are not running successfully.

That work needs be complete before this can proceed.