Add support for the X-Registry-Supports-Signatures API extension #255
Conversation
mtrmac
force-pushed
the
x-registry-supports-signatures
branch
from
fad4f6d
to
e625e38
Compare
docker/docker_client.go
Outdated
| func (c *dockerClient) ping() error { | ||
| // ensurePropertiesAreDetected detects various properties of the registry. | ||
| // See the dockerClient documentation for members which are affected by this. | ||
| func (c *dockerClient) ensurePropertiesAreDetected() error { |
There was a problem hiding this comment.
too long even if such expressive. GatherProperties | GetRegistryProperties or such would be better to me (and I'm not a native english speaker/writer, I just know Golang doesn't need such expressiveness according to the official doc I couldn't find a link for 😄 )
There was a problem hiding this comment.
ACK, detectProperties works just as well (and the callers don’t care that we cache the data, no need to expose that in the name.)
| Type: extensionSignatureTypeAtomic, | ||
| Content: newSig, | ||
| } | ||
| body, err := json.Marshal(sig) |
There was a problem hiding this comment.
Good catch, fixed. Thanks!
| defer res.Body.Close() | ||
| if res.StatusCode != http.StatusCreated { | ||
| body, err := ioutil.ReadAll(res.Body) | ||
| if err == nil { |
There was a problem hiding this comment.
if we're just logging here I would rather do:
body, _ := ioutil.ReadAll(res.Body)
logrus.Debugf("Error body %v", string(body))
even if it's nil, no need to check that unless you really think logging nil will clutter the logs. Up to your preference.
There was a problem hiding this comment.
This is a copy&paste from PutManifest, and the extra check does not really hurt at all. You are right that it would be fine not to have it, but when we already do, removing it would not make the code noticeably better IMHO.
(Perhaps one day we might consolidate all the “res.StatusCode is unexpected” error handling into a helper for consistency, but that would be a separate PR.)
|
👍 |
mtrmac
force-pushed
the
x-registry-supports-signatures
branch
2 times, most recently
from
6bec0dc
to
ccfeee9
Compare
mtrmac
changed the title
mtrmac
changed the title
mtrmac
force-pushed
the
x-registry-supports-signatures
branch
from
ccfeee9
to
3c58c93
Compare
Apparently we have never been able to push signatures to sigstore, and nobody noticed?! FIXME: This needs a test Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…Request This will allow using paths starting with /extensions/v2/ in the future. Also, we can now name the parameter “path” instead of the incorrect “url”, and the “path” name actually matches (there is no magic /v2/ adding involved). Also eliminates duplication of the ping paths (in logging code only). Does not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This only moves the client.scheme == "" check inside the method; but the naming will make it more appropriate to call the method from dockerImagesource / dockerImageDestination when they need to depend on the results of the properties detection in methods which may be called before dockerClient.makeRequest is ever called (detecting the properties as a side effect). Does not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is provided by the OpenShift-integrated registry. This is equivalent to the atomic: transport (in the “openshift”) subpackage, but it requires less code and notably does not require an OpenShift login context to be configured. See openshift/origin#12504 and openshift/openshift-docs#3556 for more information on this API extension. To preserve compatibility, we always check for a configured lookaside sigstore first; if that is set up, we use the lookaside and ignore the registry-native signature storage. Usually the user would not bother to set up the lookaside, and use the native mechanism. The code is mostly trivial; the only non-obvious aspect is the loop in putSignaturesToAPIExtension, which is a pretty direct translation of openshiftImageDestination.PutSignatures. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
mtrmac
force-pushed
the
x-registry-supports-signatures
branch
from
3c58c93
to
6c17ca3
Compare
|
@mtrmac LGTM |
|
👍 @runcom Note that there are still outstanding questions on how this interacts with the |
This is provided by the OpenShift-integrated registry. This is equivalent to the atomic: transport (in the “openshift”) subpackage, but it requires less code and notably does not require an OpenShift login context to be configured.
See openshift/origin#12504 and openshift/openshift-docs#3556 for more information on this API extension.
To preserve compatibility, we always check for a configured lookaside sigstore first; if that is set up, we use the lookaside and ignore the registry-native signature storage. Usually the user would not bother to set up the lookaside, and use the native mechanism.
See the individual commits for more details.
NOTE: This includes tests using the recording mechanism proposed in #254. They are in separate commits, so they can be dropped if that mechanism is rejected or controversial and would block merging this.