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

Add OCI-Referrers header #463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sudo-bmitch
Copy link
Contributor

This adds an optional header that servers can use to inform clients about the lack of a referrers listing for a given manifest, enabling clients to skip some requests.

Related to #454.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

If we know they don't have reference we also know whether they do. Further, we should provide notice regarding the point in time element. If you like could go into more detail explaining that the manifest being referred to does not change when being referenced, thus the latest tag, for example, does not change at all thus providing no indication that a new referrer is available.

spec.md Outdated
@@ -173,6 +173,9 @@ If the digest does differ, it MAY be the case that the hashing algorithms used d
See [Content Digests](https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests) <sup>[apdx-3](#appendix)</sup> for information on how to detect the hashing algorithm in use.
Most clients MAY ignore the value, but if it is used, the client MUST verify the value against the uploaded blob data.

A registry that implements the [Referrers Listing](#listing-referrers) MAY include the `OCI-Referrers: absent` header on manifests which do not have any referrers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
A registry that implements the [Referrers Listing](#listing-referrers) MAY include the `OCI-Referrers: absent` header on manifests which do not have any referrers.
A registry that implements the [Referrers Listing](#listing-referrers) MAY include the `OCI-Referrers: absent` or `OCI-Referrers: present` headers on manifests to indicate whether they, presently, have any referrers.

@@ -173,6 +173,9 @@ If the digest does differ, it MAY be the case that the hashing algorithms used d
See [Content Digests](https://github.com/opencontainers/image-spec/blob/v1.0.1/descriptor.md#digests) <sup>[apdx-3](#appendix)</sup> for information on how to detect the hashing algorithm in use.
Most clients MAY ignore the value, but if it is used, the client MUST verify the value against the uploaded blob data.

A registry that implements the [Referrers Listing](#listing-referrers) MAY include the `OCI-Referrers: absent` header on manifests which do not have any referrers.
Clients MAY skip requests to the [Referrers Listing](#listing-referrers) for manifests that include the `OCI-Referrers: absent` header.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Clients MAY skip requests to the [Referrers Listing](#listing-referrers) for manifests that include the `OCI-Referrers: absent` header.
Clients should recognize that receipt of the `OCI-Referrers: absent` or `OCI-Referrers: present` headers, are a point in time notice. At any time, manifests with the subject references that cause the notice may be removed or added after the notice has been provided.

@sudo-bmitch
Copy link
Contributor Author

sudo-bmitch commented Sep 16, 2023

If we know they don't have reference we also know whether they do.

@mikebrow Since this is a "MAY", the absence of the header needs to be treated as if referrers could exist. For a client that is pulling referrers along with the rest of the image, the workflow is the same if there was an OCI-Referrers: present header as if it didn't exist. Are there use cases where adding that header would change the client behavior? If we can't find use cases today but discover them later, we could also add this to the spec later.

From the previous discussions and linked issue, the workflow we were considering for referrers that exists is to add a Link header giving clients a direct URL to pull the referrers from a CDN. The Link header would avoid querying the referrers API to get the 307 (redirect) to the CDN location. I'm avoiding closing the linked #454 with this PR so the Link header can be added in a separate PR.

For the "point in time", that applies to every referrers API request. That would make more sense in documenting the referrers API itself, and not the manifest GET response.

@mikebrow
Copy link
Member

If we know they don't have reference we also know whether they do.

@mikebrow Since this is a "MAY", the absence of the header needs to be treated as if referrers could exist. For a client that is pulling referrers along with the rest of the image, the workflow is the same if there was an OCI-Referrers: present header as if it didn't exist. Are there use cases where adding that header would change the client behavior? If we can't find use cases today but discover them later, we could also add this to the spec later.

isn't it more expensive to force all clients to assume existence every time make the extra referrers requests?

From the previous discussions and linked issue, the workflow we were considering for referrers that exists is to add a Link header giving clients a direct URL to pull the referrers from a CDN. The Link header would avoid querying the referrers API to get the 307 (redirect) to the CDN location. I'm avoiding closing the linked #454 with this PR so the Link header can be added in a separate PR.

fair to say a link header would be one way/mode to provide a positive indicator.. still not sure why avoiding link or positive notice for local is encouraging clients to not call referrers, when no negative response header is provided.. though I do expect clients to configure yes/no look for artifacts even if no response header is provided indicating artifact existence..

For the "point in time", that applies to every referrers API request. That would make more sense in documenting the referrers API itself, and not the manifest GET response.

the lack of subject refers to the manifest would be a point in time notice.. not following why making that point here and in the referrers api will not make sense to the reader...

@mikebrow
Copy link
Member

client1.. pull image with unpack... trace log includes that artifacts have been reported for the following:

client1.. reports upstream the notified existence of artifacts for...

upstream client.. pulls said artifacts for reported.. applying upstream filters etc..

upstream client requests client1 run the container

@sudo-bmitch
Copy link
Contributor Author

isn't it more expensive to force all clients to assume existence every time make the extra referrers requests?

It's very expensive and will be at least half of the referrers requests for clients that implement recursive queries, which is why this PR is here. But making the opposite assumption, that a lack of the header means there are no referrers, cannot be safely done.

We have the following decision logic:

  • Client doesn't want referrers (i.e. today's runtimes that aren't referrers aware)
    • 1: Just pull the manifest and ignore any headers if they exist
  • Client wants referrers
    • Registry is 1.0 and client will fallback
      • 2a: Referrers query fails, fallback tag get fails, no referrers
      • 2b: Referrers query fails, fallback tag finds referrers
    • Registry is 1.1 but does not implement the header
      • 3: Referrers query returns list, possibly empty if none exist
    • Registry is 1.1 and implements the absent header
      • 4a: If there are no referrers, client sees the absent header and skips the referrers query
      • 4b: Referrers query returns listing
    • Registry is 1.1 and implements the absent and present header
      • 5a: If there are no referrers, client sees the absent header and skips the referrers query
      • 5b: If there are referrers, present header exists and referrers query returns listing

The client behavior between the 4 and 5 is the same. And a client must run the referrers query if there is no header to handle scenarios 2 and 3. Effectively when the client doesn't get an absent header, it may be 4b, 3, or 2, and the client doesn't know until it runs the referrers query.

Is there a scenario I'm missing where adding the present header would help clients?

@michaelb990
Copy link
Contributor

I would prefer not encouraging registries to do more (potentially unnecessary) work on any of the APIs in the "pull" workflow. Some clients will need this information, but (IMO) most won't, so it seems like our goal should be to keep the manifest GET API should be as simple as possible.

@sudo-bmitch
Copy link
Contributor Author

I would prefer not encouraging registries to do more (potentially unnecessary) work on any of the APIs in the "pull" workflow. Some clients will need this information, but (IMO) most won't, so it seems like our goal should be to keep the manifest GET API should be as simple as possible.

@michaelb990 agreed, that's why this was only a MAY. If/when registries think that the added work will reduce their overhead, they can add it. And until then, there won't be any header so clients that need to pull referrers will always query for them.

jdolitsky
jdolitsky previously approved these changes Jan 4, 2024
@jonjohnsonjr
Copy link
Contributor

I would prefer not encouraging registries to do more (potentially unnecessary) work on any of the APIs in the "pull" workflow. Some clients will need this information, but (IMO) most won't, so it seems like our goal should be to keep the manifest GET API should be as simple as possible.

Agree with this.

How do we feel about a client header that positively indicates that the client wants referrers information? Then the registry doesn't have to guess about whether it's "worth" calculating this header for the client.

For clients that don't care about referrers, the registry doesn't have to do any additional work to query referrers.

Signed-off-by: Brandon Mitchell <[email protected]>
@sudo-bmitch
Copy link
Contributor Author

How do we feel about a client header that positively indicates that the client wants referrers information? Then the registry doesn't have to guess about whether it's "worth" calculating this header for the client.

For clients that don't care about referrers, the registry doesn't have to do any additional work to query referrers.

@jonjohnsonjr @michaelb990 I've updated with the client header. Let me know if that covers your concerns.

@mikebrow have your concerns above been addressed?

@avtakkar
Copy link

avtakkar commented Jan 5, 2024

We are leaning towards not implementing this header in Azure Container Registry due to the added processing cost of consistently calculating this value.

@sudo-bmitch sudo-bmitch modified the milestones: v1.1.0, v1.2.0 Jan 11, 2024
@sudo-bmitch
Copy link
Contributor Author

Based on the Jan 11th call, we're postponing any decision to 1.2

@mikebrow
Copy link
Member

if not included in OCI 1.1 clients and the registries can proof of concept the header optimization ...

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.

6 participants