Proposal: attestations ref map merge #3038
Conversation
frontend/gateway/client/result.go
Outdated
| r.mu.Lock() | ||
| if r.Attestations == nil { | ||
| r.Attestations = map[string][]attestation.Attestation{} | ||
| } | ||
| v.PredicateRefKey = fmt.Sprintf("attestation:%s", identity.NewID()) | ||
| r.Refs[v.PredicateRefKey] = predicateRef | ||
| r.Attestations[k] = append(r.Attestations[k], v) | ||
| r.mu.Unlock() |
There was a problem hiding this comment.
I think this function refactor is sub-optimal - it has to be changed from AddAttestation to a more specific form, so as to allow passing the predicateRef (keeping in mind that other attestation types in the future might require no/multiple refs, so just taking a single one is probably not correct). Not quite sure if I'm missing a trick to make this better.
Additionally, callers would need to perform a caps check against Client.BuildOpts().LLBCaps, which I don't think is possible to push into this function call, since we need access to a client/caps object.
There was a problem hiding this comment.
Suggestion: we can work around the caps issue by attaching the NewResult to the client struct, and doing caps resolution there.
There was a problem hiding this comment.
Another way is to do it in https://github.com/moby/buildkit/blob/master/frontend/gateway/grpcclient/client.go#L114 and filter out attestations if the cap is not present.
There was a problem hiding this comment.
Have added a new gateway capability for this, and done your suggestion.
I think we might be able to rework the AddInTotoAttestation back to a generic AddAttestation with the #3036 proposal, though I'm not too worried about that.
jedevc
changed the title
This patch reworks the attestations code to place its refs into the correponding Result.Refs map, to allow for easier conversions between gateway boundaries. This ensures that almost all conversions (notably, except the tranformation to protobuf), can be performed in a single line, similar to how Metadata is copied. Additionally, this prevents multiple different Attestation types, and allows a single one defined in a common package. To acheive this, instead of including a Ref type directly in each Attestation, we include a key to a ref, which can be looked up in the Result's Ref map. This key must be uniquely generated when attestations are added - note that we cannot use the same Ref ID as seen in the protobuf structure (which would be nice) as the gateway forwarder does not convert to protobuf and simply applies the forwarding in-memory, so we need a uniquely generated id. To distinguish between these IDs and past/present/future contents of the ref map, we use the "attestation:" prefix, which would allow for future extension in a similar way. The gateway protobuf for the in-toto attestation is modified to contain a ref-key instead of a ref directly. Aside from the change in types and conversion logic, the only major changes are in the creation of attestations and the export of attestations. For the creation of attestations, we need to change the AddAttestation function into an InTotoAttestationAttestation, so as to allow passing a Ref alongside, which allows for encapsulation of the prefix logic, and would allow us to possibly change the transport mechanism for attestations in the future. For the export of attestations, we simply introduce a layer of indirection to lookup the attestation in the refs map. Signed-off-by: Justin Chadwell <[email protected]>
jedevc
force-pushed
the
attestations-map-merge
branch
from
8e579e9 to
f2c770e
Compare
This PR requires #2935 - to see only the commits that differ, see here.
This is presented as an alternative possible refactor to #3036 - we should probably attempt to include at least one of those refactors to simplify the attestations work, however, we could include both, as the generics work would simplify even non-attestations code such as duplicated Ref/Refs.
Summary
This patch reworks the attestations code to place its refs into the corresponding
*Result.Refsmap, to allow for easier conversions between gateway boundaries. This ensures that almost all conversions (notably, except the transformation to protobuf), can be performed in a single line, similar to how.Metadatais copied. Additionally, this prevents multiple different Attestation types, and allows a single one defined in a common package.How
To achieve this, instead of including a Ref type directly in each Attestation, we include a key to a ref, which can be looked up in the
Result'sRefsmap. This key must be uniquely generated when attestations are added - note that we cannot use the same Ref ID as seen in the protobuf structure (which would be nice) as the gateway forwarder does not convert to protobuf and simply applies the forwarding in-memory, so we need a uniquely generated id. To distinguish between these IDs and past/present/future contents of the ref map, we use theattestation:prefix, which would allow for future extension in a similar way.Aside from the change in types and conversion logic, the only major changes are in the creation of attestations and the export of attestations. For the creation of attestations, we need to change the
AddAttestationfunction into anAddInTotoAttestation, so as to allow passing aReferencealongside, which allows for encapsulation of the prefix logic, and would allow us to possibly change the transport mechanism for attestations in the future. For the export of attestations, we simply introduce a layer of indirection to lookup the attestation in the refs map.A caveat of this approach is that the behavior of the
Refsmap will change. This will require frontends to detect the presence of specific caps in the buildkit backend (not added yet in this patch), to ensure that attestations are only attached if they are supported, otherwise the local/tar exporters will behave unexpectedly.