bdoverlay suggestions #1
Conversation
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| mediaType := comp.Type.MediaType() | ||
| if sr.cm.Snapshotter.Name() == "overlaybd" { | ||
| snStat, err := sr.cm.Snapshotter.Stat(ctx, sr.getSnapshotID()) | ||
| if err != nil { | ||
| return nil, errors.Wrapf(err, "failed to Stat overlaybd") | ||
| } | ||
| if bdPath := snStat.Labels[obdlabel.LocalOverlayBDPath]; bdPath != "" { | ||
| if err := commitOverlayBD(ctx, sr, &desc); err != nil { | ||
| return nil, err | ||
| } | ||
| mediaType = desc.MediaType | ||
| } | ||
| } |
There was a problem hiding this comment.
I tried to move this closer to where it's used, but wasn't sure if enableOverlay (below) is possible with this snapshotter. It's disabled by default for overlaybd, but can be enabled with the BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF env-var.
If it is not supported, then probably there should be an error, and otherwise we may have to split this block (for the mediaType part), and move the commit back to below.
There was a problem hiding this comment.
Acturally, commitOverlayBD do similar things like tryComputeOverlayBlob but is for block device. If enableOverlay is true, commitOverlayBD should overcover desc generated by tryComputeOverlayBlob. We set enableOverlay to false to avoid this (because it will make overlaybd build slower).
There was a problem hiding this comment.
@HileQAQ Thanks, yes that was the part I was wondering about; should we ignore BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF for overlaybd altogether? We disable enableOverlay by default, but (technically) the BUILDKIT_DEBUG_FORCE_OVERLAY_DIFF could still enable this (and perhaps that should either produce an error, or a warning and be ignored?)
There was a problem hiding this comment.
Enabling enableOverlay has no effect on the result of the overlaybd build, it just makes it a little slower(in compute diff between lower and upper). So I think it shouldn't be an error, even don't need a warning.
There was a problem hiding this comment.
Gotcha; so, yes, perhaps we should disable it altogether, or put it in a branch where it's != "overlaybd"
There was a problem hiding this comment.
Yes, I will merge this PR and make some changes related to this.
| } | ||
| desc.Digest = dgst | ||
| desc.Size = sz | ||
| desc.MediaType = ocispecs.MediaTypeImageLayer |
There was a problem hiding this comment.
I moved this here, as commitOverlayBD was already mutating the descriptor 🤔
There was a problem hiding this comment.
Yes, I think it is more appropriate to put together the modification of overlaybd for desc.
| bdPath := snStat.Labels[obdlabel.LocalOverlayBDPath] | ||
| if bdPath == "" { | ||
| return errors.New("missing overlaybd path label") | ||
| } | ||
| dir := path.Dir(bdPath) |
There was a problem hiding this comment.
Maybe redundant; mostly thought; perhaps we need to make sure that this code path isn't hit if there's no label
There was a problem hiding this comment.
This label is generated from overlaybd.Prepare, but I think it needs to be judged here too.
contains a fix for CVE-2024-45338 / https://go.dev/issue/70906, but it doesn't affect our codebase: govulncheck -show=verbose ./... ... Vulnerability #1: GO-2024-3333 Non-linear parsing of case-insensitive content in golang.org/x/net/html More info: https://pkg.go.dev/vuln/GO-2024-3333 Module: golang.org/x/net Found in: golang.org/x/net@v0.29.0 Fixed in: golang.org/x/net@v0.33.0 Your code is affected by 0 vulnerabilities. This scan also found 0 vulnerabilities in packages you import and 1 vulnerability in modules you require, but your code doesn't appear to call these vulnerabilities. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2 participants
some draft / ideas while reviewing moby#3867