Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A more spec-compliant/resilient OCI distribution implementation #310

Merged
merged 18 commits into from
Dec 7, 2022

Conversation

joshspicer
Copy link
Member

@joshspicer joshspicer commented Dec 2, 2022

Exercises the devcontainer features publish command against the OCI registry specification's Reference Implementation , and updates our implementation to more closely follow the specification.

More specifically, starts a registry container (with hardcoded auth generated with htpasswd).

const startRegistryCmd = `docker run -d -p 5000:5000 \
   -v ${resolvedTmpPath}/auth.htpasswd:/etc/docker/registry/auth.htpasswd \
   -e REGISTRY_AUTH="{htpasswd: {realm: localhost, path: /etc/docker/registry/auth.htpasswd}}" \
   --name registry \
   registry`;

And then executing the following publish command against the registry server running on localhost.

DEVCONTAINERS_OCI_AUTH='localhost:5000|myuser|mypass' ./${cli} features publish --log-level trace -r localhost:5000 -n octocat/features ${collectionFolder}/src

A new class of unit tests will spin up an ephemeral registry container and exercise the push and pull operations of our OCI distribution implementation.

Details on changes made to support Reference Implementation

  • Create a new postUploadSessionId for each uploaded blob layer. Previously, a single session ID was created for each manifest, but our implementation now matches the specification where it should be generated per blob. GHCR tolerates re-using the upload ID, but the reference implementation does not.
  • Fallback to Basic auth if a session token cannot be fetched from the registry. For GHCR, operations are only accepted with a valid session token from the /session endpoint of the registry, whereas the reference implementation talks only with Basic auth credentials. I don't see the session auth built in the official specification, so my implementation supports both methods, only using Basic auth if the /token endpoint does not return a success code and a valid token from it's JSON response body.
  • Accurately set the Content-Length header when uploading blobs, per the specification.

Additional Changes

  • Previously, we were reading the .tgz files from disk several times throughout the process. Now, the file is read from disk into a buffer one time, and the buffer is passed around. This is more efficient.
  • The DEVCONTAINERS_OCI_AUTH environment variable was always broken (a condition would always result to false). Since there is no way this could be used by any active users (and it is not yet documented), I changed the contract a bit to include a username field, which is necessary to get this variable working with the reference implementation (GHCR ignores this field, which is why it was originally omitted). This variable is exercised in the provided test.
  • More clearly surfaces error responses from the registry in logs. Errors are parsed as JSON per the specification.

image

  • Break out existing features info command two new, simple commands devcontainer info tags and devcontainer info manifest. These are useful utilities to break out for understanding published Features. They are used in the newly added test.
  • The http(s) family of functions will now default to using http when communicating with localhost, or if the parsed protocol is http.

image

@joshspicer joshspicer changed the title Wire up tests against Reference Implementation OCI Registry A more spec-compliant/resilient OCI distribution implementation Dec 2, 2022
@joshspicer joshspicer marked this pull request as ready for review December 3, 2022 00:24
@joshspicer joshspicer requested a review from a team as a code owner December 3, 2022 00:24
@joshspicer joshspicer self-assigned this Dec 5, 2022
Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 This is awesome, thanks! ✨

src/spec-configuration/containerCollectionsOCI.ts Outdated Show resolved Hide resolved
src/spec-utils/httpRequest.ts Show resolved Hide resolved
Co-authored-by: Samruddhi Khandale <skhandale@microsoft.com>
Copy link
Contributor

@chrmarti chrmarti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a question regarding 'localhost' implying 'http'. LGTM otherwise!

@joshspicer joshspicer merged commit 2855a9d into main Dec 7, 2022
@joshspicer joshspicer deleted the joshspicer/tests-with-reference-registry branch December 7, 2022 17:56
joshspicer added a commit that referenced this pull request Dec 7, 2022
joshspicer added a commit that referenced this pull request Dec 7, 2022
@joshspicer joshspicer restored the joshspicer/tests-with-reference-registry branch December 7, 2022 17:58
@joshspicer joshspicer deleted the joshspicer/tests-with-reference-registry branch December 7, 2022 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants