Remove image push steps from the build strategies

[sbose78]

Given that every build should eventually end up with an image being pushed, should we avoid making the build strategy author specify how to push an image ?

How about having our controller append the tekton steps on the fly to push the image in a strategy agnostic way, if feasible?

[SaschaSchwarze0]

@sbose78 how would this then look like? We would then need to define that all build strategies have to put the build output on some volume mount so that a following step can pick it up and push it, right? Two questions I see:

• Do we know whether all strategies that we support today can do this ? Clearly possible with buildah and s2i because this uses buildah. Kaniko can be configured to run without a push and store a tarBall instead. One would need to find out if that's suitable for the needs. Buildpacks, not sure if the exporter can be configured to produce a container image without pushing. If we come to the conclusion that it can be done with all these strategies, are we certain that all strategies that somebody else wants to use support it as well? And this question can be asked in the other direction as well because today we define that build strategies do the push.
• Does it make things easier for the user who writes the build strategy? We do not have personas defines somewhere in this project but the person who creates a build strategy imo is not the same person that creates a build or build run. A build strategy author requires more knowledge and deep insight into the build tool that the user wants to use. So, I think that no matter whether we define that the build strategy must only create the image and place it at a well known location or whether we define that the build strategy must take care of the push as well, the user will get it done - if the technology that the user plans to use supports the path that we define.

In general, creating the image means one step less for the buildstrategy author to take care. But I am somehow also coming to the conclusion that it might make sense to allow the author of the build strategy to define whether the push is included in the strategy already or not. But this aspect also feels a little over-engineered.

So, in short: I agree with the idea. I am uncertain about whether it makes things easier or not.

[zhangtbj]

I think we should keep our original way that uses the official and verified image push step in each build strategy. Below are my opinions:

1. I am not sure if we can extract a common shared push step which can use the same USER, same permission, same step to push image for all strategies, Or maybe different buildstrategies have different requirements or contexts. But at least, there is risk for future update.

2. We need to maintain a shared push step in our controller separately, and when there is any step or any buildstrategy changed, we need additional effect to make sure the shared push step works fine for all build strategies

3. I think it is better to keep the sync or the same knowledge with open-source community or tekton catalog, the end-user can use the buildstrategy without any dependency or use it directly. (Maybe better for future)

4. Some of buildstrateies, like kaniko or buildah can build and push by using one step/command, I think we don't need to separate it as two steps.

5. I think each build tool has its community and test to make sure the build and push can work fine, for new build tools in future, the community will also provide good integration with build and push. If we use our own build step, there maybe more and more gaps with communities that we may don't know.

6. And I don't see too many benefits behind the change and maybe there is a risk or additional effect to make it work for all buildstratgies. OR is there any other background or customer requirement behind it?

So I suggest we still keep the original way, for example:

• if buildpacks, just use its officially and verified exporter to push image
• if kaniko, just use itis executor to build and export together

I think it also makes our life easier right now :) , if in future, all build tools have more commons or we are confident that they are good enough, I think we can plan it in future.

But of course, we can have some discussions here. :)

[sbose78]

Thank you very much for weighing in!

1. If the build strategy author wishes to specify a push step : we will support it.
2. If the build strategy author does not wish to specify a push step : we should consider supporting that too.

Could the build strategy author skip the step of pushing if it it adhered to guidelines about where the image must placed so that something could pick it up and push it to the registry ?

Here's why:

1. The BuildStrategy can only focus on having "how to build an image" information. A build strategy author shouldn't have to define the same number of steps as those in a tekton Task.

2. Pushing images to registries is a redundant activity which every BuildRun execution has to eventually do. That should not need explicit definition.

3. Today we do a TLS_VERIFY=false while pushing images and that parameter varies for every build tool/strategy. But, should a BuildStrategy author really be worried about that ? The controller should be smart enough to discover certificates in the cluster and use them to talk to secure/insecure/enterprise registries.

[SaschaSchwarze0]

Quick comment regarding TLS_VERIFY=false. I noticed that already and put a todo on my mind (and should have opened an issue instead) that this needs to go away or made configurable with verification enabled by default. I consider it a security vulnerability if we ship something where certificate validation is disabled by default.

[zhangtbj]

Thanks for the clarification. :)

• When talking about the build strategy author, I think it is fine that we can allow that for an opensource project, but for both of our systems, I think we need to be careful because we still don't know what permission we need to open for a tenant user and if it is risky.

• But if a build strategy author already knows the build strategy and build tool very well, I think he should be an expert on the build and he should know or prefer to use the official/original push function in the build tool which they already familiar with (For example I am the fan of buildpacks.io, and before I used it to build and push my image, if I use Build v2, I may still prefer to use the exporter of it for me and I understand it standard and function well). :)

• And there is a problem for us that how to detect if the new created buildstratgy includes the push step or not? if we allow end-user to define his own buildstrategy, we cannot ensure what step looks like, it will be very difficult for us.

• So I like to follow what the community provides us, we also officially announce we support buildah, buildpacks, etc.... builders, and we should use all of them.
  Today, we think the push step can be extracted as a shared step, maybe in future, if some builder deads, for example if nobody uses buildbacks and we drop it, then we only have Dockerfile build, then we may think we can extract all build step as one, then I think we are creating a new build tool which is out of any community. But I think we are a build framework which uses/consumes the builders from the community. :)

• For the TLS_VERIFY=false, Hi @SaschaSchwarze0 can you help or show us the opened issue? I went through all build strategies:

1. buildah: --tls-verify=false
2. kaniko: --skip-tls-verify=true
3. buildpacks: none
4. s2i: --tls-verify=false
   I think it should be another story and Yes, I agree that we should solve them one by one, maybe need to co-work with each community team.

But yes, I agree, although now we use different commands to push the image in different buildstrategies, but the push step may or should be similar or same.
I think in future, maybe there is one of case that a namespaced end-user just want to build the image, and want us to help push without any push step (such as he just provides docker build command, or a shell script to do the build). But I think it should be one of very advanced case if our build framework is very popular and robust in the future, and then once we have that requirement, then we can plan and do it at that time. :)

I am not sure if it is ok for us or did I still misunderstand something?

For sure, I also like the idea which improves our framework!:)

[SaschaSchwarze0]

@zhangtbj here is the TLS verify issue: Sample build strategies should be secure by default #169

[sbose78]

We could figure out how a user could 'decorate' the push/export step if present so that the build controller could be make aware of it

In short, the success of the BuildStrategy API / CRD depends on how simple we make it for build strategy authors to describe how to do an image build.

But I think it should be one of very advanced case if our build framework is very popular and robust in the future.

It will be ;)

This is more of a discussion at the moment.

[zhangtbj]

Glad to see your comment in the repo again, Shoubhik. :)

Yes, I agree, I think we can think about it first.
If in future, we can support both of the build: build+push, build_only, I think that is also great.

[blairdrummond]

This is maybe a silly idea, but could you support this by just setting a "/dev/null" output registry (metaphorically)? I.e., given

apiVersion: shipwright.io/v1alpha1
kind: Build
metadata:
  name: s2i-nodejs-build
spec:
...
  output: {}

Have the controller notice the empty output, and spin up an ephemeral registry tmp-shipwright-registry.svc.cluster.local as a sink, and set $(params.shp-output-image) to tmp-shipwright-registry.svc.cluster.local/nullimage:latest. Then all of the build code runs as usual and the image does "push" it just doesn't go anywhere. This has the advantage that all of the build/push pipelines can support output: {} without modifications, though obviously it does create unnecessary network traffic and consumes more resources than changing the pipelines to actually not push in the first-place. (edit: see later comment)