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

feat: upgrade to OCI 1.1 #916

Merged
merged 21 commits into from
Apr 16, 2024
Merged

feat: upgrade to OCI 1.1 #916

merged 21 commits into from
Apr 16, 2024

Conversation

Two-Hearts
Copy link
Contributor

@Two-Hearts Two-Hearts commented Mar 28, 2024

This PR upgrades Notation to OCI 1.1.

Major changes [UPDATE as of 4/2/2024 after community meeting]:

  1. New flag --force-referrers-tag is introduced. And it is only applied to the Sign command. It's default to true, and it's NOT an experimental flag. The original experimental flag --allow-referrers-api will be hidden, i.e., description and example will be hidden from help page. It is kept only for backwards compatibility purpose, and a warning will be printed out when user sets it.
  2. Sign:
    # Default behavior: Use the referrers tag schema for backwards compatibility.
    notation sign ...
    notation sign --force-referrers-tag ...
    
    # With `--force-referrers-tag=false`: Check the Referrers API first, if not supported, automatically fallback to the referrers tag schema.
    notation sign --force-referrers-tag=false ...
    
  3. Verify/List/Inspect: They will always use the Referrers API first, if not supported, automatically fallback to the referrers tag schema. Note: for backwards compatibility reason, the experimental flag --allow-referrers-api is still kept and hidden. When set, a warning will be printed out.

Changes are tested in E2E and ACR.

This PR partially resolves issue #892. Resolves #893. Resolves #926,

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@codecov-commenter
Copy link

codecov-commenter commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.72%. Comparing base (eaa5fb4) to head (ce4915a).
Report is 23 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #916      +/-   ##
==========================================
+ Coverage   64.93%   67.72%   +2.79%     
==========================================
  Files          45       45              
  Lines        2729     2169     -560     
==========================================
- Hits         1772     1469     -303     
+ Misses        795      534     -261     
- Partials      162      166       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Patrick Zheng <[email protected]>
@sudo-bmitch
Copy link

sudo-bmitch commented Mar 28, 2024

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

Edit: note this is a focus on the notation version, rather than the OCI version, since users would more likely know the version of their notation clients while they don't necessarily know the version of their registry.

@priteshbandi
Copy link
Contributor

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

+1, can we add --force-oci-1-1 and make --allow-referrers-api its alias ?

@Two-Hearts
Copy link
Contributor Author

Two-Hearts commented Mar 29, 2024

I'm actually seeing two suggestions here:

  1. Suggested by @sudo-bmitch : --force-1-1-compatibility, here 1-1 means notation 1.1.0 and before. This flag is default to true. In this case:
Sign with the referrers tag schema by default:
notation sign ...
notation sign --force-1-1-compatibility ...

Sign with the referrers API:
notation sign --force-1-1-compatibility=false ...
  1. Suggested by @priteshbandi : --force-oci-1-1, an alias of the current --allow-referrers-api, default to false. Since --allow-referrers-api is currently under experimental, we could safely rename it to --force-oci-1-1. In this case:
Sign with the referrers tag schema by default:
notation sign ...

Sign with the referrers API:
notation sign --force-oci-1-1 ...

Maybe a vote? @sajayantony @shizhMSFT @toddysm @yizha1 @FeynmanZhou @JeyJeyGao @rgnote and other folks.

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts Two-Hearts requested a review from shizhMSFT March 29, 2024 02:00
@shizhMSFT
Copy link
Contributor

This may be too late to change, but if not, I'd suggest renaming the flag to something more clear to end users:

--force-1-1-compatibility: Pushes content to registries required to support notation 1.1.0 and earlier clients

Default the value to true for now, and on a 2.x release, change the default to false.

From an outside view, --allow-referrers-api is confusing since the signing operation isn't calling the API and the client has no say in whether the registry will include the content in the referrers response, that's automatic based on the subject field.

Edit: note this is a focus on the notation version, rather than the OCI version, since users would more likely know the version of their notation clients while they don't necessarily know the version of their registry.

It makes sense as pushing manifests with subjects does not really call the referrers API as defined in distribution-spec v1.1.0 although it did call as defined in distribution-spec RC versions.

The --force-1-1-compatibility is good but confusing since it means more than registry interactions.
Therefore, I'd suggest --force-referrers-tag. In notation 1.x, the default value is --force-referrers-tag=true for backward compatibility. In notation 2.x, the default value can set to false.

@yizha1
Copy link
Contributor

yizha1 commented Apr 1, 2024

I vote for the flag name --force-referrers-tag.

@priteshbandi
Copy link
Contributor

As discussed in Today's call, I'm fine with both --force-referrers-tag or --force-tag-schema` with defualt value of true in. (1.2 release). This serves the intent of using tags for storing signature and doesn't uses oci version.

@FeynmanZhou
Copy link
Member

FeynmanZhou commented Apr 2, 2024

I am kind of @shizhMSFT 's proposal, i.e.,

  • --force-referrers-tag and its default value is true:
    From end users' point of view, they care about how signature can be stored in the registry. Apparently, the signature will be pushed and stored as a "sha256-xxxxx" tag in the registry with the default --force-referrers-tag=true. So --force-referrers-tag seems much more straightforward for end users to understand how the signature is stored in the registry.
  • Keep --allow-referrers-api as an alias:
    For some popular registries that already supported OCI v1.1 and integrated Notation, such as Harbor and Zot, --allow-referrers-api has been referenced in their documentation. Keep the original flag name will be friendly and non-breaking for those.

For @sudo-bmitch and @priteshbandi 's proposals above, --force-1-1-compatibility and force-oci-1-1 sound a bit vague and technical because most of end users are not aware of the OCI spec and different versions. In general, users care about which signature format they can use to store the signature in registry.

@yizha1
Copy link
Contributor

yizha1 commented Apr 2, 2024

I think --force-referrers-tag is better than --force-tag-schema, as it shows that the tag is related to referrers, and also a short version of the term Referrers Tag Schema used in OCI specification, see the screenshot below

image

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
building.md Outdated Show resolved Hide resolved
cmd/notation/registry.go Outdated Show resolved Hide resolved
cmd/notation/registry.go Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts Two-Hearts requested a review from JeyJeyGao April 3, 2024 05:55
Signed-off-by: Patrick Zheng <[email protected]>
JeyJeyGao
JeyJeyGao previously approved these changes Apr 3, 2024
Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/notation/inspect.go Outdated Show resolved Hide resolved
cmd/notation/list.go Outdated Show resolved Hide resolved
cmd/notation/sign.go Outdated Show resolved Hide resolved
cmd/notation/sign.go Outdated Show resolved Hide resolved
cmd/notation/sign_test.go Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
test/e2e/suite/command/inspect.go Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
cmd/notation/inspect.go Outdated Show resolved Hide resolved
cmd/notation/inspect.go Outdated Show resolved Hide resolved
cmd/notation/list.go Outdated Show resolved Hide resolved
cmd/notation/list.go Show resolved Hide resolved
cmd/notation/registry.go Outdated Show resolved Hide resolved
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts Two-Hearts requested a review from shizhMSFT April 10, 2024 00:31
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@JeyJeyGao JeyJeyGao left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@priteshbandi priteshbandi left a comment

Choose a reason for hiding this comment

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

LGTM

@Two-Hearts Two-Hearts merged commit 0f556be into notaryproject:main Apr 16, 2024
5 checks passed
@Two-Hearts Two-Hearts deleted the oci branch April 16, 2024 06:15
JeyJeyGao added a commit to JeyJeyGao/notation that referenced this pull request Jun 6, 2024
JeyJeyGao added a commit to JeyJeyGao/notation that referenced this pull request Jun 6, 2024
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.

Improve output message for signing in the OCI v1.0 compliant registry bump: update to go 1.22
8 participants