Load entrypoint from remote #183

tanner-bruce · 2018-10-24T14:41:49Z

Currently, any steps in a Pipeline that rely on the built in entrypointof a container will not have the expected behaviour while running due to the entrypoint being overridden at runtime. This fixes #175.

A major side effect of this work is that the override step will now possibly make HTTP calls every time a step is overridden. There is very rudimentary caching in place which could be improved if performance becomes an issue.

This builds on the work in PR #167

imjasonh · 2018-10-24T14:46:21Z

This is awesome. 👍

We probably need images to be PullPolicy: Always to have this work. If an image is already present on the node and gets pushed with a different entrypoint, this code will assume the new entrypoint, even if the image it runs might not want/have it.
Is there any way to move this up into the controller, rather than running it from the pod each time? This would help us de-duplicate the lookups per build at least, and would enable us to keep a cache of image digest -> entrypoint, which could be reused across runs.
Does this work with private builder images?

imjasonh · 2018-10-24T14:50:46Z

/ok-to-test

tanner-bruce · 2018-10-24T15:02:10Z

We probably need images to be PullPolicy: Always to have this work. If an image is already present on the node and gets pushed with a different entrypoint, this code will assume the new entrypoint, even if the image it runs might not want/have it.

Good point. I don't think it would be possible to check the node itself unless the pod ran as privileged. Adding PullPolicy should be harmless and expected - if you are using overwriting tags on your repo you would likely want Always regardless, and if you are not overwriting tags, this shouldn't be an issue.

Is there any way to move this up into the controller, rather than running it from the pod each time? This would help us de-duplicate the lookups per build at least, and would enable us to keep a cache of image digest -> entrypoint, which could be reused across runs.

For sure, I'll take a look! I maintain the minimal cache per build but it would definitely be better higher up the control chain.

Does this work with private builder images?

Yes, it uses the same authentication mechanism other go-containerregistry tools use.

imjasonh · 2018-10-24T15:39:49Z

Yes, it uses the same authentication mechanism other go-containerregistry tools use.

I'm not sure that's enough to be sure it has the necessary credentials. Among the e2e tests we should have for this, we should make sure this works (and keeps working) with private builder images.

We might find out that supporting entrypoint with private images isn't feasible today, in which case we should at least document that you can't use the two together for now.

pkg/reconciler/v1alpha1/taskrun/taskrun.go

pkg/reconciler/v1alpha1/taskrun/resources/entrypoint.go

bobcatfish · 2018-10-24T17:14:15Z

pkg/reconciler/v1alpha1/taskrun/resources/entrypoint.go

+		ep, err := getEntrypoint(step.Image)
+		if err != nil {
+			return fmt.Errorf("couldn't get entrypoint for %s: %v", step.Image, err)
+		}


my feeling is that if there is an error here we should log it but continue onward

bobcatfish

sounds like @imjasonh has the bulk of the review covered, I just made a couple minor style comments!

bobcatfish · 2018-10-25T18:01:46Z

Sorry for all the conflicts @tanner-bruce there have been a lot of changes going in lately!

bobcatfish

As part of this change, could you also update the image contract docs to explain that entrypoint can be used (and maybe the limitations on it - i.e. if the image pulled by the node uses a different entrypoint than the image pulled by the controller, there will be problems)?

(I think these docs were added while you were working on this @tanner-bruce sorry about that! Hopefully we can get this in asap so you don't have to keep updating the PR!)

I have a couple remaining questions:

I'm not sure that's enough to be sure it has the necessary credentials. Among the e2e tests we should have for this, we should make sure this works (and keeps working) with private builder images.
We might find out that supporting entrypoint with private images isn't feasible today, in which case we should at least document that you can't use the two together for now.

@tanner-bruce any update about this?

I think the existing e2e tests would cover this, since they push to a private registry afaik? But I would think you'd run into problems if the controller didn't have the right credentials to pull from the private registry - what's the behavior in that case? I would think it should log an error but still continue on without failing the TaskRun?

Is there any way to move this up into the controller, rather than running it from the pod each time?

I'm confused by this @imjasonh - I thought this logic was in the controller all along?

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go

pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint_test.go

pkg/reconciler/v1alpha1/taskrun/taskrun_test.go

tanner-bruce · 2018-10-31T16:20:25Z

@bobcatfish @imjasonh I tested that a private image works with the entrypoint lookup, however to accomplish it I set the HOME envvar on the controller to be /root (just to be explicit), made a ~/.docker/config.json` that had a _json_key/service account username/password combo and then tested with the private image.

It successfully pulled the entrypoint and the tests pass, but it did require modifying the runtime of the controller itself to have the docker creds. This could likely be done in a nicer way - maybe go-containerregistry can have creds loaded via code and we could allow config for this, or have the task itself contain a ref to the secret and the controller would load it.

For testing this as it stands we would need to add some sort of creds to the controller deployment.

I think the existing e2e tests would cover this, since they push to a private registry afaik? But I would think you'd run into problems if the controller didn't have the right credentials to pull from the private registry - what's the behavior in that case? I would think it should log an error but still continue on without failing the TaskRun?

Right now we log a warning message and carry on.

I'm confused by this @imjasonh - I thought this logic was in the controller all along?

Previously it was only caching at the build level. I moved the entrypoint.NewCache up to the reconciler level but am not persisting the cache inside the actual reconciler at this moment

docs/using.md

bobcatfish · 2018-10-31T22:35:38Z

pkg/reconciler/v1alpha1/taskrun/taskrun.go

+			ep, err := entrypoint.GetRemoteEntrypoint(cache, step.Image)
+			if err != nil {
+				c.Logger.Warnf("could not get remote entrypoint for %s: %v", step.Image, err)
+				continue


nice, perfect 👍

only improvement i can think of is the log message could elaborate on what this means, e.g. something like "could not get remote entrypoint for %s, if %s relies on entrypoint behaviour may be blah blah blah blah: %v"

i just realized we'd only in this if the command is not specified - maybe failing silently isn't the right way to go since there's pretty much no chance the behaviour you'd end up with is what the user wants in this case

do you have a preference @tanner-bruce re. whether we fail the whole TaskRun in this case or keep going?

Good point, I think it should definitely fail.

The error message returned is pretty explicit:

couldn't get config for image gcr.io/build-crd-testing/secret-sauce: UNAUTHORIZED: "You don't have the needed permissions to perform this operation, and you may have invalid credentials.

bobcatfish

just a couple minor comments!

/lgtm

remove if you want to merge as-is:
/hold

or ping me if you make changes and need another lgtm

tanner-bruce · 2018-11-01T03:54:26Z

/test pull-knative-build-pipeline-unit-tests

tanner-bruce · 2018-11-01T15:28:02Z

/hold cancel

bobcatfish · 2018-11-01T18:02:07Z

docs/using.md

+      command: [docker]
+```
+
+However, if the steps specified a custom `command`, that is what would be used.


this line is a little odd now since the example above specifies a custom command as well, but we can iterate on it!

bobcatfish · 2018-11-01T18:02:25Z

/lgtm

tejal29 · 2018-11-01T20:03:06Z

@tanner-bruce can you please rebase from latest master and resolve conflict.

tanner-bruce · 2018-11-01T22:00:47Z

/test pull-knative-build-pipeline-integration-tests
/test pull-knative-build-pipeline-unit-tests

Currently, any steps in a Pipeline that rely on the built in entrypoint of a container will not have the expected behaviour while running due to the entrypoint being overridden at runtime. This fixes tektoncd#175. A major side effect of this work is that the override step will now possibly make HTTP calls every time a step is overridden. There is very rudimentary caching in place, however this could likely be improved if performance becomes an issue. Fixes tektoncd#175

knative-metrics-robot · 2018-11-01T22:55:35Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go	Do not exist	84.2%
pkg/reconciler/v1alpha1/taskrun/taskrun.go	72.7%	71.4%	-1.3

bobcatfish · 2018-11-05T22:32:18Z

looks like the test coverage went down slightly but lets go ahead with this anyway, we can keep iterating on it.

/lgtm

bobcatfish · 2018-11-06T19:01:38Z

🤦‍♀️
/approve

knative-prow-robot · 2018-11-06T19:01:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish, tanner-bruce

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

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

knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 24, 2018

knative-prow-robot requested review from aaron-prindle and imjasonh October 24, 2018 14:41

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 24, 2018

knative-prow-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 24, 2018

tanner-bruce mentioned this pull request Oct 24, 2018

Build steps that rely on entrypoint wont work properly with logging, i.e. Command is required currently #175

Closed

bobcatfish reviewed Oct 24, 2018

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/taskrun.go Show resolved Hide resolved

bobcatfish reviewed Oct 24, 2018

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/resources/entrypoint.go Outdated Show resolved Hide resolved

bobcatfish reviewed Oct 24, 2018

View reviewed changes

tanner-bruce force-pushed the load-entrypoint-from-remote branch from bf8be01 to dfbed84 Compare October 25, 2018 02:18

tanner-bruce force-pushed the load-entrypoint-from-remote branch 2 times, most recently from 90f82e8 to f874817 Compare October 29, 2018 19:39

tanner-bruce mentioned this pull request Oct 30, 2018

Add command to Task.BuildSpec.Steps in all example Task definitions. #202

Closed

bobcatfish reviewed Oct 30, 2018

View reviewed changes

tanner-bruce force-pushed the load-entrypoint-from-remote branch 2 times, most recently from 7fd99fb to c5d7706 Compare October 31, 2018 18:40

bobcatfish reviewed Oct 31, 2018

View reviewed changes

docs/using.md Show resolved Hide resolved

bobcatfish reviewed Oct 31, 2018

View reviewed changes

knative-prow-robot assigned bobcatfish Oct 31, 2018

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 31, 2018

bobcatfish mentioned this pull request Oct 31, 2018

Add command to Task.BuildSep.Steps in all example… #209

Merged

tanner-bruce force-pushed the load-entrypoint-from-remote branch from c5d7706 to 449dffe Compare November 1, 2018 01:44

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

tanner-bruce force-pushed the load-entrypoint-from-remote branch from 449dffe to 6721e8f Compare November 1, 2018 01:46

tanner-bruce force-pushed the load-entrypoint-from-remote branch from 6721e8f to 4159d84 Compare November 1, 2018 14:55

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2018

bobcatfish reviewed Nov 1, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

tejal29 closed this Nov 1, 2018

tejal29 reopened this Nov 1, 2018

tanner-bruce force-pushed the load-entrypoint-from-remote branch from 4159d84 to 3de01a1 Compare November 1, 2018 21:43

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018

tanner-bruce force-pushed the load-entrypoint-from-remote branch 2 times, most recently from 5bbcd2f to 2ebd459 Compare November 1, 2018 22:44

tanner-bruce force-pushed the load-entrypoint-from-remote branch from 2ebd459 to a40eb0c Compare November 1, 2018 22:53

bobcatfish added this to the 0.0.1 Alpha release milestone Nov 3, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 6, 2018

knative-prow-robot merged commit 8fa4437 into tektoncd:master Nov 6, 2018

aaron-prindle mentioned this pull request Nov 13, 2018

No init containers knative/build#470

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load entrypoint from remote #183

Load entrypoint from remote #183

tanner-bruce commented Oct 24, 2018

imjasonh commented Oct 24, 2018 •

edited

Loading

imjasonh commented Oct 24, 2018

tanner-bruce commented Oct 24, 2018

imjasonh commented Oct 24, 2018

bobcatfish Oct 24, 2018

bobcatfish left a comment

bobcatfish commented Oct 25, 2018

bobcatfish left a comment

tanner-bruce commented Oct 31, 2018 •

edited

Loading

bobcatfish Oct 31, 2018

bobcatfish Oct 31, 2018

tanner-bruce Nov 1, 2018

tanner-bruce Nov 1, 2018

bobcatfish left a comment

tanner-bruce commented Nov 1, 2018

tanner-bruce commented Nov 1, 2018

bobcatfish Nov 1, 2018

bobcatfish commented Nov 1, 2018

tejal29 commented Nov 1, 2018

tanner-bruce commented Nov 1, 2018

knative-metrics-robot commented Nov 1, 2018

bobcatfish commented Nov 5, 2018

bobcatfish commented Nov 6, 2018

knative-prow-robot commented Nov 6, 2018

Load entrypoint from remote #183

Load entrypoint from remote #183

Conversation

tanner-bruce commented Oct 24, 2018

imjasonh commented Oct 24, 2018 • edited Loading

imjasonh commented Oct 24, 2018

tanner-bruce commented Oct 24, 2018

imjasonh commented Oct 24, 2018

bobcatfish Oct 24, 2018

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish commented Oct 25, 2018

bobcatfish left a comment

Choose a reason for hiding this comment

tanner-bruce commented Oct 31, 2018 • edited Loading

bobcatfish Oct 31, 2018

Choose a reason for hiding this comment

bobcatfish Oct 31, 2018

Choose a reason for hiding this comment

tanner-bruce Nov 1, 2018

Choose a reason for hiding this comment

tanner-bruce Nov 1, 2018

Choose a reason for hiding this comment

bobcatfish left a comment

Choose a reason for hiding this comment

tanner-bruce commented Nov 1, 2018

tanner-bruce commented Nov 1, 2018

bobcatfish Nov 1, 2018

Choose a reason for hiding this comment

bobcatfish commented Nov 1, 2018

tejal29 commented Nov 1, 2018

tanner-bruce commented Nov 1, 2018

knative-metrics-robot commented Nov 1, 2018

bobcatfish commented Nov 5, 2018

bobcatfish commented Nov 6, 2018

knative-prow-robot commented Nov 6, 2018

imjasonh commented Oct 24, 2018 •

edited

Loading

tanner-bruce commented Oct 31, 2018 •

edited

Loading