cmd/entrypoint: do not interpret anything after `--` #5084

vdemeester · 2022-07-06T05:58:45Z

Changes

The way the flag package works, it can eat the flag
"terminator" (aka the double dash --). This means that, in some very
specific cases — where the first item after the -- is also a
subcommand, it would execute the entrypoint subcommand instead of the
actual command.

For example :

$ /ko-app/entrypoint -- init a b
$ /ko-app/entrypoint init a b

This is fixed by making sure we remove anything after -- for the
subcommand processing. And then we pass the rest (after --) to the
entrypointer to be executed.

Note for the reviewers: this is one approach (mutate os.Args) that is relatively contained. The other way to fix that would be to have an exec subcommand that would be used to exec things, but this means changing all containers commands as well which.. might be a bit much for bug fixes at least.
In any case, I think the entrypoint binary needs a bit of love and refactoring.

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Fixes #5080

/kind bug
/priority critial-urgent

This will have to be cherry-pick in all release from 0.35 to 0.37 😅

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

Fix the entrypoint potentially not executing the right command due to flag parsing

tekton-robot · 2022-07-06T05:58:48Z

@vdemeester: The label(s) priority/critial-urgent cannot be applied, because the repository doesn't have them.

In response to this:

Changes

The way the flag package works, it can eat the flag
"terminator" (aka the double dash --). This means that, in some very
specific cases — where the first item after the -- is also a
subcommand, it would execute the entrypoint subcommand instead of the
actual command.

For example :
$ /ko-app/entrypoint -- init a b
$ /ko-app/entrypoint init a b
This is fixed by making sure we remove anything after -- for the
subcommand processing. And then we pass the rest (after --) to the
entrypointer to be executed.

Note for the reviewers: this is one approach (mutate os.Args) that is relatively contained. The other way to fix that would be to have an exec subcommand that would be used to exec things, but this means changing all containers commands as well which.. might be a bit much for bug fixes at least.
In any case, I think the entrypoint binary needs a bit of love and refactoring.

Signed-off-by: Vincent Demeester vdemeest@redhat.com

Fixes #5080

/kind bug
/priority critial-urgent

This will have to be cherry-pick in all release from 0.32 to 0.37 😅

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing

Tests included if any functionality added or changed

Follows the commit message standard

Meets the Tekton contributor standards (including
functionality, content, code)

Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes
Fix the entrypoint potentially not executing the right command due to flag parsing

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 · 2022-07-06T05:59:09Z

/cc @abayer @lbernick @jerop @afrittoli

vdemeester · 2022-07-06T05:59:21Z

This probably need at least one test 🙃 working on it.
Done ✅

tekton-robot · 2022-07-06T06:06:07Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/main.go	14.0%	11.9%	-2.1

tekton-robot · 2022-07-06T06:20:25Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/main.go	14.0%	11.9%	-2.1

vdemeester · 2022-07-06T06:27:25Z

cloudevent: TestSendCloudEventWithRetries/test-send-cloud-event-failed expand_less | 0s
{Failed      cloud_event_controller_test.go:619: error in test test-send-cloud-event-failed: received 0 events but 1 expected. Found events: []string{} WARN	cloudevent/cloud_event_controller.go:162	Failed to send cloudevent: Had to fail. Event ID: 1f6a546b-df34-4e1b-8a72-bfb1fa13414b}

🤔 those are new "unit test" flakes ? 🤔

vdemeester · 2022-07-06T06:27:37Z

/test tekton-pipeline-unit-tests

tekton-robot · 2022-07-06T08:21:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/main.go	14.0%	11.8%	-2.3

tekton-robot · 2022-07-06T08:44:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/main.go	14.0%	11.6%	-2.4

vdemeester · 2022-07-06T09:21:32Z

/retest

abayer · 2022-07-06T11:05:45Z

I believe the unit test flakes are due to my change decreasing requests/limits for that job - tektoncd/plumbing#1124 reverts that change.

abayer

I'm a bit worried that this could cause something else to fail, due to all the fun complicated behaviors around --, but I can't think of anything better. =)

tekton-robot · 2022-07-06T14:17:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abayer

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~~ [abayer]

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

tekton-robot · 2022-07-06T17:01:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/args.go	Do not exist	100.0%
cmd/entrypoint/main.go	14.0%	13.6%	-0.5

afrittoli · 2022-07-06T17:02:07Z

cmd/entrypoint/args_test.go

+	"github.com/google/go-cmp/cmp"
+)
+
+func TestExtractArgs(t *testing.T) {


afrittoli

Thank you for adding the tests!
/lgtm

tekton-robot · 2022-07-06T17:54:15Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/args.go	Do not exist	100.0%
cmd/entrypoint/main.go	14.0%	13.6%	-0.5

afrittoli · 2022-07-06T18:00:06Z

/lgtm

afrittoli · 2022-07-06T21:02:30Z

/restest

The way the `flag` package works, it can eat the flag "terminator" (aka the double dash `--`). This means that, in some very specific cases — where the first item after the `--` is also a subcommand, it would execute the entrypoint subcommand instead of the actual command. For example : ``` $ /ko-app/entrypoint -- init a b $ /ko-app/entrypoint init a b ``` This is fixed by making sure we remove anything after `--` for the subcommand processing. And then we pass the rest (after `--`) to the entrypointer to be executed. Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2022-07-07T07:20:18Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/args.go	Do not exist	100.0%
cmd/entrypoint/main.go	14.0%	13.6%	-0.5

afrittoli

/lgtm

tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jul 6, 2022

tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jul 6, 2022

tekton-robot requested review from dlorenc and pritidesai July 6, 2022 05:58

vdemeester requested review from imjasonh and removed request for pritidesai and dlorenc July 6, 2022 05:58

tekton-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 6, 2022

vdemeester requested a review from a user July 6, 2022 05:58

tekton-robot requested review from abayer, afrittoli, jerop and lbernick July 6, 2022 05:59

vdemeester force-pushed the 5080-entrypoint-init-fix branch from 4e7e445 to e006882 Compare July 6, 2022 06:14

tekton-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 6, 2022

vdemeester force-pushed the 5080-entrypoint-init-fix branch 3 times, most recently from 913dfc7 to faa9f89 Compare July 6, 2022 08:13

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 6, 2022

vdemeester force-pushed the 5080-entrypoint-init-fix branch from faa9f89 to bcfa0cf Compare July 6, 2022 08:38

abayer approved these changes Jul 6, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2022

vdemeester force-pushed the 5080-entrypoint-init-fix branch from bcfa0cf to ab5633f Compare July 6, 2022 16:57

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 6, 2022

afrittoli reviewed Jul 6, 2022

View reviewed changes

tekton-robot assigned afrittoli Jul 6, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2022

vdemeester force-pushed the 5080-entrypoint-init-fix branch from ab5633f to 9aa5e5c Compare July 6, 2022 17:49

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 6, 2022

vdemeester force-pushed the 5080-entrypoint-init-fix branch from 9aa5e5c to 5a5703c Compare July 7, 2022 07:16

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022

afrittoli reviewed Jul 7, 2022

View reviewed changes

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022

tekton-robot merged commit da91bf3 into tektoncd:main Jul 7, 2022

vdemeester deleted the 5080-entrypoint-init-fix branch July 7, 2022 07:57

vdemeester added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/entrypoint: do not interpret anything after `--` #5084

cmd/entrypoint: do not interpret anything after `--` #5084

vdemeester commented Jul 6, 2022 •

edited

Loading

tekton-robot commented Jul 6, 2022

Changes

Submitter Checklist

Release Notes

vdemeester commented Jul 6, 2022

vdemeester commented Jul 6, 2022 •

edited

Loading

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

vdemeester commented Jul 6, 2022

vdemeester commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

vdemeester commented Jul 6, 2022

abayer commented Jul 6, 2022

abayer left a comment

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

afrittoli Jul 6, 2022

afrittoli left a comment

tekton-robot commented Jul 6, 2022

afrittoli commented Jul 6, 2022

afrittoli commented Jul 6, 2022

tekton-robot commented Jul 7, 2022

afrittoli left a comment

cmd/entrypoint: do not interpret anything after -- #5084

cmd/entrypoint: do not interpret anything after -- #5084

Conversation

vdemeester commented Jul 6, 2022 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jul 6, 2022

Changes

Submitter Checklist

Release Notes

vdemeester commented Jul 6, 2022

vdemeester commented Jul 6, 2022 • edited Loading

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

vdemeester commented Jul 6, 2022

vdemeester commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

vdemeester commented Jul 6, 2022

abayer commented Jul 6, 2022

abayer left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 6, 2022

tekton-robot commented Jul 6, 2022

afrittoli Jul 6, 2022

Choose a reason for hiding this comment

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented Jul 6, 2022

afrittoli commented Jul 6, 2022

afrittoli commented Jul 6, 2022

tekton-robot commented Jul 7, 2022

afrittoli left a comment

Choose a reason for hiding this comment

cmd/entrypoint: do not interpret anything after `--` #5084

cmd/entrypoint: do not interpret anything after `--` #5084

vdemeester commented Jul 6, 2022 •

edited

Loading

vdemeester commented Jul 6, 2022 •

edited

Loading