fix(BuildStrategy): follow shp-output-insecure properly with buildpacks

[sgaist]

Changes

This PR refactors the implementation to:

• Better handle empty insecure-registries both as empty list or empty values in it.
• Adds a warning if the registry used for the insecure output is listed in the insecure-registries
• Disable the upload stage if that is the case

Related Issue

Fixes #2237

Type of PR

/kind bug

Submitter Checklist

• Includes tests if functionality changed/was added
• Includes docs if changes are user-facing
• Kind label has been set
• Release notes block has been filled in, or marked NONE

See the contributor guide
for details on coding conventions, github and prow interactions, and the code review process.

Release Notes

Fixed buildpacks-v3 BuildStrategy examples: improved `shp-output-insecure` handling.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign heavywombat for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[SaschaSchwarze0]

/hold

I am not agreeing with your statement

The current implementation has one flaw: it creates the CNB_INSECURE_REGISTRIES environment variable in all cases which should not happen when shp-output-insecure is set to false.

The build user, can specify insecure on build.spec.output and for Buildpacks, the user can pass (additional) insecure registries by host using build.spec.paramValues.

Imo, it is totally legitimate to have a secure output registry (and therefore have build.spec.output.insecure = false) while still having the need to consume from insecure registries (for example a custom run-image) and therefore pass that in param values.

Your proposed change would mean that the param value is then ignored.

Am I missing something?

[sgaist]

/hold

I am not agreeing with your statement

The current implementation has one flaw: it creates the CNB_INSECURE_REGISTRIES environment variable in all cases which should not happen when shp-output-insecure is set to false.

The build user, can specify insecure on build.spec.output and for Buildpacks, the user can pass (additional) insecure registries by host using build.spec.paramValues.

Imo, it is totally legitimate to have a secure output registry (and therefore have build.spec.output.insecure = false) while still having the need to consume from insecure registries (for example a custom run-image) and therefore pass that in param values.

Your proposed change would mean that the param value is then ignored.

Am I missing something?

I think I get you. I misunderstood something and while my fix is wrong, it may point to something else.

One thing I somehow missed was that CNB_INSECURE_REGISTRIES works both ways, for pulling and pushing as I essentially worked from the premise that the feature would be used only for pushing the output image.

While I was doing some work to implement a similar logic in a different BuildStrategy, I passed the output image prefix in params.insecure-registries which meant that with or without shp-output-insecure set, it wouldn't change the outcome: the image would be successfully built and pushed thus something I was not expecting and the creation of this patch.

With that in mind, should we add a check that will make the build fail if the image prefix can be found in the params.insecure-registries while shp-output-insecure is set to false ?

[SaschaSchwarze0]

With that in mind, should we add a check that will make the build fail if the image prefix can be found in the params.insecure-registries while shp-output-insecure is set to false ?

Gut feeling ... I tend to not fail the build, but logging a warning could make sense. Something like "You use the insecure-registries parameter for the registry host of the output image. Consider using the insecure flag on the output definition instead."

[sgaist]

Sounds good

One last question before I refactor the patch: should the push not happen in this case to avoid accidentally sending the image to a registry that was not meant to be used in the first place for that ?
This will keep the build working but also make the person involved check the logs to understand why they did not get the image in the registry.