-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Set the scripts dir to readonly after init #4161
Set the scripts dir to readonly after init #4161
Conversation
Hi @06kellyjac. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
/ok-to-test /approve |
The scripts dir only needs to be writable during `place-scripts` Previously you could replace the script of other steps before they're ran and this mitigates that issue
5d11d83
to
142cdcf
Compare
Ok, my unit tests were passing but that was just a cached result 😕 I've dropped the debug scripts part |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @tektoncd/core-maintainers
/retest |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dibyom, sbwsg, 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:
Approvers can indicate their approval by writing |
/lgtm |
I can't at a glance tell what the cause of the failure is. Is this known flakeyness? |
Here's the data race log in full:
But I don't see how your change has caused this. 😕 |
/test tekton-pipeline-unit-tests |
Looks like the UT race was a flake :/ /test pull-tekton-pipeline-alpha-integration-tests |
Changes
Make the script volume mount read-only for all mounts excluding the init container that creates the scripts
This resolves issue #4160 and prevents overwriting the scripts from subsequent steps leading to anomalous, obscure, or dangerous results.
I made these changes a while ago before the debug scripts volume was added. I added an extra commit to deal with debug scripts too. I've ran the unit tests and I've deployed with
ko
and done some normal runs (using the tests from catalog) in a k3d cluster but I'm not very familiar with this debug scripts feature so some deeper testing might be necessary.Edit: the debug scripts need to be written to each step like
/tekton/tools
with it's post/wait files/kind bug
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes
Note: I can modify the release-note to "action required" if we think someone might actually be overwriting the scripts in their use of tekton
Their resolution action would be to either an alternative mentioned at the bottom of #4160 or to mount their own volume to write other scripts to