feat(BuildStrategy): use flag to activate insecure registries - #1359
feat(BuildStrategy): use flag to activate insecure registries#1359sgaist wants to merge 7 commits into
Conversation
The Renku admins can now enable/disable the use of insecure registries through the BUILD_INSECURE_OUTPUT_ENABLED environment variable. This flag will be forwarded to Shipwright through the output parameters. The BuildStrategy now follows more closely the implementation from Shipwright and respects that flag.
Coverage Report for CI Build 28015495482Coverage at 86.359% (no base build to compare)Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
@sgaist what is the point of this? What does "insecure registries" mean exactly and what is it used for? |
This tells the buildpack lifecycle to not verify TLS certificates for specific registries (when pulling, pushing, etc.). See also conversation here: #1264 (review) This setting can help test building images end-to-end where the registry is deployed in a non-production setup with certificates not attached to a PKI (self-signed). |
There was a problem hiding this comment.
The script part looks complex now, with argument parsing involved. Do we want to maintain this?
There was a problem hiding this comment.
The idea was to keep it close to the Shipwright implementation however I am not against keeping it simpler if you prefer.
We can move back to a single string containing the list of registries.
| crs.ParamValue( | ||
| name="insecure-registries", | ||
| values=[ | ||
| crs.Value(value=registry) for registry in params.insecure_registries.split(",") | ||
| ], | ||
| ), |
There was a problem hiding this comment.
question: "".split(",") returns [""] will this be a problem?
There was a problem hiding this comment.
I think this will cause trouble further on into the strategy in the bash script in the yaml file.
There was a problem hiding this comment.
It should not however the BuildStrategy does not handle properly an empty value. Not that it fails but it will trigger the export of CNB_INSECURE_REGISTRIES with , as value.
I have refactored that part back to its original implementation as I have simplified the BuildStrategy as well.
Co-authored-by: Tasko Olevski <16360283+olevski@users.noreply.github.com>
- Revert the last changes as setting insecure registries and handling the output as insecure can be independent (e.g. pulling dependencies VS pushing the output). - Add a warning if the registry for the output image is listed as insecure registries while the insecure output option is disabled. - Handle properly empty insecure registry list
…stries The fact that the output shall go to an insecure registry is a separate concern from secure registries providing items for the build.
…dling Some of the checks are now done directly in the configuration of the feature so no need to make them again in the BuildStrategy. The back and forth between string and list and then back to string was not a good idea.
4 participants
Describe your changes
The current implementation of "insecure registries" support for builds is automatically activated by default.
This PR changes that through the use of the
shp-output-insecureoutput parameter from Shipwright.Changes are twofold:
BUILD_INSECURE_OUTPUT_ENABLEDenvironement variableThis will also follow the Shipwright implementation more accurately.