Skip to content

Commit

Permalink
Merge pull request #2062 from mtrmac/oci-1.1-little
Browse files Browse the repository at this point in the history
OCI image-spec / distribution-spec v1.1 updates, first round
  • Loading branch information
rhatdan authored Aug 10, 2023
2 parents cb43107 + 5253f01 commit e626be2
Show file tree
Hide file tree
Showing 15 changed files with 575 additions and 187 deletions.
24 changes: 15 additions & 9 deletions copy/single.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,18 +305,18 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst
options := newOrderedSet()
match := false
for _, wantedPlatform := range wantedPlatforms {
// Waiting for https://github.com/opencontainers/image-spec/pull/777 :
// This currently can’t use image.MatchesPlatform because we don’t know what to use
// for image.Variant.
if wantedPlatform.OS == c.OS && wantedPlatform.Architecture == c.Architecture {
// For a transitional period, this might trigger warnings because the Variant
// field was added to OCI config only recently. If this turns out to be too noisy,
// revert this check to only look for (OS, Architecture).
if platform.MatchesPlatform(c.Platform, wantedPlatform) {
match = true
break
}
options.append(fmt.Sprintf("%s+%s", wantedPlatform.OS, wantedPlatform.Architecture))
options.append(fmt.Sprintf("%s+%s+%q", wantedPlatform.OS, wantedPlatform.Architecture, wantedPlatform.Variant))
}
if !match {
logrus.Infof("Image operating system mismatch: image uses OS %q+architecture %q, expecting one of %q",
c.OS, c.Architecture, strings.Join(options.list, ", "))
logrus.Infof("Image operating system mismatch: image uses OS %q+architecture %q+%q, expecting one of %q",
c.OS, c.Architecture, c.Variant, strings.Join(options.list, ", "))
}
}
return nil
Expand Down Expand Up @@ -460,8 +460,14 @@ func (ic *imageCopier) copyLayers(ctx context.Context) ([]compressiontypes.Algor
encryptAll = len(*ic.c.options.OciEncryptLayers) == 0
totalLayers := len(srcInfos)
for _, l := range *ic.c.options.OciEncryptLayers {
// if layer is negative, it is reverse indexed.
layersToEncrypt.Add((totalLayers + l) % totalLayers)
switch {
case l >= 0 && l < totalLayers:
layersToEncrypt.Add(l)
case l < 0 && l+totalLayers >= 0: // Implies (l + totalLayers) < totalLayers
layersToEncrypt.Add(l + totalLayers) // If l is negative, it is reverse indexed.
default:
return nil, fmt.Errorf("when choosing layers to encrypt, layer index %d out of range (%d layers exist)", l, totalLayers)
}
}

if encryptAll {
Expand Down
86 changes: 79 additions & 7 deletions docker/docker_client.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package docker

import (
"bytes"
"context"
"crypto/tls"
"encoding/json"
Expand All @@ -19,6 +18,7 @@ import (

"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/internal/iolimits"
"github.com/containers/image/v5/internal/set"
"github.com/containers/image/v5/internal/useragent"
"github.com/containers/image/v5/manifest"
"github.com/containers/image/v5/pkg/docker/config"
Expand Down Expand Up @@ -121,6 +121,9 @@ type dockerClient struct {
// Private state for detectProperties:
detectPropertiesOnce sync.Once // detectPropertiesOnce is used to execute detectProperties() at most once.
detectPropertiesError error // detectPropertiesError caches the initial error.
// Private state for logResponseWarnings
reportedWarningsLock sync.Mutex
reportedWarnings *set.Set[string]
}

type authScope struct {
Expand Down Expand Up @@ -281,10 +284,11 @@ func newDockerClient(sys *types.SystemContext, registry, reference string) (*doc
}

return &dockerClient{
sys: sys,
registry: registry,
userAgent: userAgent,
tlsClientConfig: tlsClientConfig,
sys: sys,
registry: registry,
userAgent: userAgent,
tlsClientConfig: tlsClientConfig,
reportedWarnings: set.New[string](),
}, nil
}

Expand Down Expand Up @@ -624,9 +628,76 @@ func (c *dockerClient) makeRequestToResolvedURLOnce(ctx context.Context, method
if err != nil {
return nil, err
}
if warnings := res.Header.Values("Warning"); len(warnings) != 0 {
c.logResponseWarnings(res, warnings)
}
return res, nil
}

// logResponseWarnings logs warningHeaders from res, if any.
func (c *dockerClient) logResponseWarnings(res *http.Response, warningHeaders []string) {
c.reportedWarningsLock.Lock()
defer c.reportedWarningsLock.Unlock()

for _, header := range warningHeaders {
warningString := parseRegistryWarningHeader(header)
if warningString == "" {
logrus.Debugf("Ignored Warning: header from registry: %q", header)
} else {
if !c.reportedWarnings.Contains(warningString) {
c.reportedWarnings.Add(warningString)
// Note that reportedWarnings is based only on warningString, so that we don’t
// repeat the same warning for every request - but the warning includes the URL;
// so it may not be specific to that URL.
logrus.Warnf("Warning from registry (first encountered at %q): %q", res.Request.URL.Redacted(), warningString)
} else {
logrus.Debugf("Repeated warning from registry at %q: %q", res.Request.URL.Redacted(), warningString)
}
}
}
}

// parseRegistryWarningHeader parses a Warning: header per RFC 7234, limited to the warning
// values allowed by opencontainers/distribution-spec.
// It returns the warning string if the header has the expected format, or "" otherwise.
func parseRegistryWarningHeader(header string) string {
const expectedPrefix = `299 - "`
const expectedSuffix = `"`

// warning-value = warn-code SP warn-agent SP warn-text [ SP warn-date ]
// distribution-spec requires warn-code=299, warn-agent="-", warn-date missing
if !strings.HasPrefix(header, expectedPrefix) || !strings.HasSuffix(header, expectedSuffix) {
return ""
}
header = header[len(expectedPrefix) : len(header)-len(expectedSuffix)]

// ”Recipients that process the value of a quoted-string MUST handle a quoted-pair
// as if it were replaced by the octet following the backslash.”, so let’s do that…
res := strings.Builder{}
afterBackslash := false
for _, c := range []byte(header) { // []byte because escaping is defined in terms of bytes, not Unicode code points
switch {
case c == 0x7F || (c < ' ' && c != '\t'):
return "" // Control characters are forbidden
case afterBackslash:
res.WriteByte(c)
afterBackslash = false
case c == '"':
// This terminates the warn-text and warn-date, forbidden by distribution-spec, follows,
// or completely invalid input.
return ""
case c == '\\':
afterBackslash = true
default:
res.WriteByte(c)
}
}
if afterBackslash {
return ""
}
return res.String()
}

// we're using the challenges from the /v2/ ping response and not the one from the destination
// URL in this request because:
//
Expand Down Expand Up @@ -1008,9 +1079,10 @@ func isManifestUnknownError(err error) bool {
if errors.As(err, &e) && e.ErrorCode() == errcode.ErrorCodeUnknown && e.Message == "Not Found" {
return true
}
// ALSO registry.redhat.io as of October 2022
// opencontainers/distribution-spec does not require the errcode.Error payloads to be used,
// but specifies that the HTTP status must be 404.
var unexpected *unexpectedHTTPResponseError
if errors.As(err, &unexpected) && unexpected.StatusCode == http.StatusNotFound && bytes.Contains(unexpected.Response, []byte("Not found")) {
if errors.As(err, &unexpected) && unexpected.StatusCode == http.StatusNotFound {
return true
}
return false
Expand Down
22 changes: 22 additions & 0 deletions docker/docker_client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,28 @@ func TestNeedsNoRetry(t *testing.T) {
}
}

func TestParseRegistryWarningHeader(t *testing.T) {
for _, c := range []struct{ header, expected string }{
{"completely invalid", ""},
{`299 - "trivial"`, "trivial"},
{`100 - "not-299"`, ""},
{`299 localhost "warn-agent set"`, ""},
{`299 - "no-terminating-quote`, ""},
{"299 - \"\x01 control\"", ""},
{"299 - \"\\\x01 escaped control\"", ""},
{"299 - \"e\\scaped\"", "escaped"},
{"299 - \"non-UTF8 \xA1\xA2\"", "non-UTF8 \xA1\xA2"},
{"299 - \"non-UTF8 escaped \\\xA1\\\xA2\"", "non-UTF8 escaped \xA1\xA2"},
{"299 - \"UTF8 žluťoučký\"", "UTF8 žluťoučký"},
{"299 - \"UTF8 \\\xC5\\\xBEluťoučký\"", "UTF8 žluťoučký"},
{`299 - "unterminated`, ""},
{`299 - "warning" "some-date"`, ""},
} {
res := parseRegistryWarningHeader(c.header)
assert.Equal(t, c.expected, res, c.header)
}
}

func TestIsManifestUnknownError(t *testing.T) {
// Mostly a smoke test; we can add more registries here if they need special handling.

Expand Down
5 changes: 5 additions & 0 deletions docker/docker_image_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,11 @@ func (d *dockerImageDestination) TryReusingBlobWithOptions(ctx context.Context,

// Sanity checks:
if reference.Domain(candidateRepo) != reference.Domain(d.ref.ref) {
// OCI distribution spec 1.1 allows mounting blobs without specifying the source repo
// (the "from" parameter); in that case we might try to use these candidates as well.
//
// OTOH that would mean we can’t do the “blobExists” check, and if there is no match
// we could get an upload request that we would have to cancel.
logrus.Debugf("... Internal error: domain %s does not match destination %s", reference.Domain(candidateRepo), reference.Domain(d.ref.ref))
continue
}
Expand Down
7 changes: 6 additions & 1 deletion docker/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ func httpResponseToError(res *http.Response, context string) error {
}

// registryHTTPResponseToError creates a Go error from an HTTP error response of a docker/distribution
// registry
// registry.
//
// WARNING: The OCI distribution spec says
// “A `4XX` response code from the registry MAY return a body in any format.”; but if it is
// JSON, it MUST use the errcode.Error structure.
// So, callers should primarily decide based on HTTP StatusCode, not based on error type here.
func registryHTTPResponseToError(res *http.Response) error {
err := handleErrorResponse(res)
// len(errs) == 0 should never be returned by handleErrorResponse; if it does, we don't modify it and let the caller report it as is.
Expand Down
15 changes: 11 additions & 4 deletions internal/image/docker_schema2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import (
"golang.org/x/exp/slices"
)

const commonFixtureConfigDigest = "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f"

func manifestSchema2FromFixture(t *testing.T, src types.ImageSource, fixture string, mustFail bool) genericManifest {
manifest, err := os.ReadFile(filepath.Join("fixtures", fixture))
require.NoError(t, err)
Expand All @@ -39,7 +41,7 @@ func manifestSchema2FromComponentsLikeFixture(configBlob []byte) genericManifest
return manifestSchema2FromComponents(manifest.Schema2Descriptor{
MediaType: "application/octet-stream",
Size: 5940,
Digest: "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f",
Digest: commonFixtureConfigDigest,
}, nil, configBlob, []manifest.Schema2Descriptor{
{
MediaType: "application/vnd.docker.image.rootfs.diff.tar.gzip",
Expand Down Expand Up @@ -114,7 +116,7 @@ func TestManifestSchema2ConfigInfo(t *testing.T) {
} {
assert.Equal(t, types.BlobInfo{
Size: 5940,
Digest: "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f",
Digest: commonFixtureConfigDigest,
MediaType: "application/octet-stream",
}, m.ConfigInfo())
}
Expand All @@ -123,11 +125,12 @@ func TestManifestSchema2ConfigInfo(t *testing.T) {
// configBlobImageSource allows testing various GetBlob behaviors in .ConfigBlob()
type configBlobImageSource struct {
mocks.ForbiddenImageSource // We inherit almost all of the methods, which just panic()
expectedDigest digest.Digest
f func() (io.ReadCloser, int64, error)
}

func (f configBlobImageSource) GetBlob(ctx context.Context, info types.BlobInfo, _ types.BlobInfoCache) (io.ReadCloser, int64, error) {
if info.Digest.String() != "sha256:9ca4bda0a6b3727a6ffcc43e981cad0f24e2ec79d338f6ba325b4dfd0756fb8f" {
if info.Digest != f.expectedDigest {
panic("Unexpected digest in GetBlob")
}
return f.f()
Expand Down Expand Up @@ -163,7 +166,10 @@ func TestManifestSchema2ConfigBlob(t *testing.T) {
} {
var src types.ImageSource
if c.cbISfn != nil {
src = configBlobImageSource{f: c.cbISfn}
src = configBlobImageSource{
expectedDigest: commonFixtureConfigDigest,
f: c.cbISfn,
}
} else {
src = nil
}
Expand Down Expand Up @@ -350,6 +356,7 @@ func newSchema2ImageSource(t *testing.T, dockerRef string) *schema2ImageSource {

return &schema2ImageSource{
configBlobImageSource: configBlobImageSource{
expectedDigest: commonFixtureConfigDigest,
f: func() (io.ReadCloser, int64, error) {
return io.NopCloser(bytes.NewReader(realConfigJSON)), int64(len(realConfigJSON)), nil
},
Expand Down
Loading

0 comments on commit e626be2

Please sign in to comment.