Recommend versioning script instead of using configmap 📝

[bobcatfish]

Changes

When discussing some potential ways a Task could be compromised,
@afrittoli mentioned that if a Task refers to some remote source for a
script, there is the potential for that source to be modified and run
something malicious inside a Task. As he described this, he mentioned
that our catalog recommends putting scripts into configmaps, which I was
surprised to see. A script inside a configmap does have this potential
attack vector (though you'd have to have write access to the configmap
to pull it off) but I'm actually a bit more concerned that this isn't
a great solution for maintaining scripts over time. Not to mention that
I'm not how we would support this in the catalog (we'd have to start
supporting applying and versioning ConfigMaps alongside the Tasks that
reference them).

So this commit changes the recommendation to recommend 'graduating'
large scripts from script to tools and treating them like you'd treat
other code. This is still not a fantastic solution as it requires you to
maintain and publish your own images but I think it's a step in the
right direction (and if it helps, I could also see a world where we
support storing and publishing those images via the catalog as well -
though I'm pretty sure @vdemeester disagrees with me here).

@mogsie and @imjasonh i'm curious on your take here also, in #625 it sounds like this recommendation was based on your own experiences + discussions

Submitter Checklist

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

• [yes?] Follows the authoring recommendations
• [n/a] Includes docs (if user facing)
• [n/a] Includes tests (if functionality of task changed or new task added)
• Commit messages follow commit message best practices
• [n/a] Complies with Catalog Organization TEP, see example. Note An issue has been filed to automate this validation

See the contribution guide
for more details.

---

[dibyom]

+1 to this change. Also, one of our recommendation is to create idempotent tasks and pipelines - and IMO having the script in a separate resource makes that harder to achieve. https://github.com/tektoncd/catalog/blob/main/recommendations.md#create-idempotent-tasks-and-pipelines

[vdemeester]

This is still not a fantastic solution as it requires you to
maintain and publish your own images but I think it's a step in the
right direction (and if it helps, I could also see a world where we
support storing and publishing those images via the catalog as well -
though I'm pretty sure @vdemeester disagrees with me here).

😝 You are mostly right. In my opinion, this is not our job to store the content of images our task uses. I kinda like that separation of concern but I also understand that for a task that only needs a ubuntu image and a relatively long script, it feels kind of a lot to have to have/maintain infrastructure to build and push an oci image. But on the other hand, there is some services like GitHub that makes this relatively easy/seamless.

I agree with @dibyom and @bobcatfish on the fact that this recommendation feels a bit weird so I am 👍🏼 on the change.

[mogsie]

@bobcatfish: To answer your question about the motivation for using configmaps, it is essentially a technique to be able to author and maintain shell scripts as stand-alone files rather than in-lined deep inside a YAML file. Essential shell tools like shellcheck, and similar linters for other languages cannot operate on YAML files, and so we found kustomize's ability to convert a file to a configmap which we then can inject into the Task to be a "Good" solution to that problem.

The guidelines were originally conceived as "general authoring guidelines" rather than specifically for tekton catalog. They were also merged prematurely, before much discussion around them (like this) had happened. I'm fine with taking out the configmap as it doesn't fit with the goals of the catalog; I have no opinion on the graduation part. I think for the tekton catalog it makes sense to mention that scripts are linted automatically.

Note that the "idempotency" recommendation is more about enabling "retries" for tasks rather than how a task is completely stand-alone.

[vdemeester]

The guidelines were originally conceived as "general authoring guidelines" rather than specifically for tekton catalog. They were also merged prematurely, before much discussion around them (like this) had happened. I'm fine with taking out the configmap as it doesn't fit with the goals of the catalog; I have no opinion on the graduation part. I think for the tekton catalog it makes sense to mention that scripts are linted automatically.

Agreed, I think at some point we need to distinguish between "general authoring guidelines" and tekton catalog authoring guidelines. We need to see how and where we want to put each of those.

[afrittoli]

Thanks for starting this @bobcatfish - I'm happy with the idea of dropping the ConfigMap recommendation. And +100 larger scripts need to be tested.
#740 gives an option for testing the scripts without having to rebuild a container image, so that would help too once it's merged.

[afrittoli]

There is another option, but it's not merged in catalog yet, so we might update this section once it is - see #740
FYI @chmouel

[afrittoli]

@bobcatfish: To answer your question about the motivation for using configmaps, it is essentially a technique to be able to author and maintain shell scripts as stand-alone files rather than in-lined deep inside a YAML file. Essential shell tools like shellcheck, and similar linters for other languages cannot operate on YAML files, and so we found kustomize's ability to convert a file to a configmap which we then can inject into the Task to be a "Good" solution to that problem.

The guidelines were originally conceived as "general authoring guidelines" rather than specifically for tekton catalog. They were also merged prematurely, before much discussion around them (like this) had happened. I'm fine with taking out the configmap as it doesn't fit with the goals of the catalog; I have no opinion on the graduation part. I think for the tekton catalog it makes sense to mention that scripts are linted automatically.

#740 should help further

[afrittoli]

@tektoncd/catalog-maintainers this LGTM :)

[vinamra28]

/lgtm

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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 [vdemeester]

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