Adjustments for Lifecycle version 0.13.3 #115
Conversation
Starting with lifecycle 0.13.3, it is permitted to have both the old style label-based BOM information and the new style layer-based BOM information. If the buildpack API is 0.6 or older, label-based BOMs only is OK. If the buildpack API is 0.7, you may have both label-based BOM and layer-based BOM or just layer-based BOM. It is permitted to have just label-based BOM, however, that will generate a warning from the lifecycle. Signed-off-by: Daniel Mikusa <dmikusa@vmware.com>
dmikusa
added
semver:patch
build.go
Outdated
| @@ -313,9 +314,18 @@ func Build(builder Builder, options ...Option) { | |||
| } | |||
| } | |||
|
|
|||
| var disableBOMLabel bool | |||
| if val, ok := os.LookupEnv("BP_DISABLE_BOM_LABEL"); ok { | |||
There was a problem hiding this comment.
@dmikusa-pivotal I think the usage of this env var might be buildpacks/platform specific. I would prefer it if we didn't introduce it in libcnb and just relied on the buildpack to toggle this logic. This might be better placed in libpak for eg. LMK what you think.
There was a problem hiding this comment.
I'm not opposed to having a different trigger, but I feel pretty strongly that we should allow for the disabling of the label-based BOM entries. It's known to cause problems.
I went with an env variable because there is some precedent with BP_DEBUG and BP_LOG_LEVEL which are also in libcnb.
Other options that come to mind:
- A BuildContext property: https://github.com/buildpacks/libcnb/blob/main/build.go#L34-L56
- An Option callback:
Line 75 in d3e6e18
There was a problem hiding this comment.
I don't feel strongly about this but in general I agree with @samj1912 that libcnb should avoid being prescriptive about the user->buildpack interface and this might be better in libpack
There was a problem hiding this comment.
@dmikusa-pivotal I think the difference between those log level env vars and the SBOM env var is that the log level env var influences the log lines that the build process outputs which cannot be controlled by the users of libcnb (for eg
Line 179 in d3e6e18
I am concerned because I would like that env var to be set to a value so that labels are turned off by default completely and I imagine paketo might want something else. Given that we are venturing into buildpack author level opinions on configuration that is exposed to the end user(app dev) I think we should avoid introducing this in libcnb.
I might be fine with introducing this in a way that is exposed to the buildpack author and not an end user (i.e. not an env var)
There was a problem hiding this comment.
✅ , I get where you're coming from.
I removed the env variable, added an option that a buildpack author can set when running libcnb.Build, set this to default to false (i.e. don't write the BOM label).
Let me know what you think.
|
Not strictly related to this PR but it looks like libcnb is providing a helpers to create layer-specific SBOMs but not for generic launch and build SBOMs. Seem like we should probably do both? cc @dmikusa-pivotal @samj1912 |
In some environments and with some applications, a long enough BOM label may be generated that it will either break Kubernetes or cause your application to fail to start. With the changes in lifecycle 0.13.3, we are enabling the BOM label again, but due to the issue above we also need a way for users to disable it if there are problems. When running `libcnb.Build` you may now include an `Option` of `WithBOMLabel` that has an argument of either true or false. If not set, it defaults to false. This controls the output of libcnb.BOMEntry items in `launch.toml` and `build.toml`. If true, BOM entries are written. If false, BOM entries are not written, even if they are returned by the buildpack. Signed-off-by: Daniel Mikusa <dmikusa@vmware.com>
| @@ -122,6 +122,7 @@ type Builder interface { | |||
| func Build(builder Builder, options ...Option) { | |||
| config := Config{ | |||
| arguments: os.Args, | |||
| bomLabel: false, | |||
There was a problem hiding this comment.
Should we instead just call this allowDeprecated ?
There was a problem hiding this comment.
IMHO, that's not specific enough. There could be other deprecated things to allow.
I'd be OK with allowDeprecatedBOMLabel and WithDeprecatedBOMLabel. I can make that change and resubmit in the morning if you're Ok with that.
For what it's worth, I did flag WithBOMLabel as deprecated, so if it's used it'll show up as deprecated in your IDE. Is that sufficient? or would you still prefer it in the variable name?
|
@dmikusa-pivotal merging this for now. Was just a nitpick on the naming side. Hopefully we are getting rid of v1 soon anyway. |
Starting with lifecycle 0.13.3, it is permitted to have both the old style label-based BOM information and the new style layer-based BOM information. If the buildpack API is 0.6 or older, label-based BOMs only are OK. If the buildpack API is 0.7, you may have both label-based BOM and layer-based BOM or just layer-based BOM. It is permitted to have just label-based BOM, however, that will generate a warning from the lifecycle.
This PR makes two changes:
BP_DISABLE_BOM_LABELwhich can be used to manually disable the label-based BOM. This is for the case where the label is too large and causes problems with K8s. This defaults to false, so label-based BOM is enabled by default. Setting it to true will result in no label-based BOM being included, even if the buildpacks write that information.