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.

Sign up for GitHub

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

Jump to bottom

OTA-916: add SignatureStores to ClusterVersionSpec #1678

Merged
merged 5 commits into from
Dec 6, 2023
Merged

OTA-916: add SignatureStores to ClusterVersionSpec #1678

merged 5 commits into from
Dec 6, 2023

Conversation

PratikMahajan
Copy link
Contributor

@PratikMahajan PratikMahajan commented Nov 21, 2023 •
edited
Loading

SignatureStore will help us define custom signature stores which
can then be used in air-gapped environments. it'll also give admin
flexibility to remove one or more signature stores that they're not
comfortable using.

Enhancement: openshift/enhancements#1485

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 21, 2023

Hello @PratikMahajan! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 21, 2023
@openshift-ci openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 21, 2023
@PratikMahajan PratikMahajan force-pushed the add-custom-sig-stores branch 4 times, most recently from 572da06 to 71f8cb7 Compare November 22, 2023 15:34
JoelSpeed
JoelSpeed reviewed Nov 22, 2023
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

Could you please take a look at the test suite files for this CRD and add both positive and negative test cases that test the validation of this new field

config/v1/types_cluster_version.go Outdated Show resolved Hide resolved
config/v1/types_cluster_version.go
@@ -88,6 +88,21 @@ type ClusterVersionSpec struct {
// +optional
Capabilities *ClusterVersionCapabilitiesSpec `json:"capabilities,omitempty"`

// SignatureStores contains the upstream URIs to verify release signatures.
// By default, CVO will use existing signature stores if this property is empty.
// The CVO will check the release signatures in the local ConfigMaps first. It will search for a valid signature
Copy link
Contributor

Choose a reason for hiding this comment

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

Which local configmaps, are there multiple configmaps? How are these configmaps selected?

Copy link
Member

Choose a reason for hiding this comment

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

Docs for the ConfigMap signature store are here, but in practice this is not user-facing. oc adm release mirror ... (source) and oc-mirror ... (source) pack the signatures into ConfigMap manifests on behalf of the user, who can apply them to their target cluster without having to look inside.

config/v1/types_cluster_version.go Outdated Show resolved Hide resolved
config/v1/types_cluster_version.go Outdated Show resolved Hide resolved
@PratikMahajan PratikMahajan force-pushed the add-custom-sig-stores branch 6 times, most recently from 4fea343 to 627fc08 Compare November 27, 2023 15:55
@PratikMahajan PratikMahajan changed the title add SignatureStores to ClusterVersionSpec OTA-916: add SignatureStores to ClusterVersionSpec Nov 27, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 27, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 27, 2023 •
edited by openshift-ci bot
Loading

@PratikMahajan: This pull request references OTA-916 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

SignatureStore will help us define custom signature stores which
can then be used in air-gapped environments. it'll also give admin
flexibility to remove one or more signature stores that they're not
comfortable using.

Enhancement: openshift/enhancements#1485

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wking
wking approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

storage ClusterOperator Progressing flapping seems unrelated to this change.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2023
@JoelSpeed
Copy link
Contributor

LGTM. Is the implementation of this ready to be merged? I'm conscious we only have a 48 hour window to merge changes into 4.15 now, I would prefer not to merge the API if the feature isn't going to ship this release, and then merge once 4.16 opens next week

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 29, 2023
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 29, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2023
wking
wking approved these changes Dec 6, 2023
View reviewed changes
Copy link
Member

@wking wking left a comment

Choose a reason for hiding this comment

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

Still looks good to me :)

@wking
Copy link
Member

wking commented Dec 6, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 74f9448 and 2 for PR HEAD 52aca23 in total

JoelSpeed
JoelSpeed reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

/approve cancel

This will currently break CustomNoUpgrade jobs, need to fix that before we can merge

config/v1/0000_00_cluster-version-operator_01_clusterversion-TechPreviewNoUpgrade.crd.yaml Outdated Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

@PratikMahajan: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-techpreview
  • /test e2e-aws-serial
  • /test e2e-aws-serial-techpreview
  • /test e2e-upgrade
  • /test images
  • /test integration
  • /test unit
  • /test verify
  • /test verify-client-go
  • /test verify-crd-schema
  • /test verify-deps

The following commands are available to trigger optional jobs:

  • /test e2e-azure
  • /test e2e-gcp

Use /test all to run all jobs.

In response to this:

/test chi/prow/verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@PratikMahajan
Copy link
Contributor Author

/test ci/prow/verify

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

@PratikMahajan: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build
  • /test e2e-aws-ovn
  • /test e2e-aws-ovn-techpreview
  • /test e2e-aws-serial
  • /test e2e-aws-serial-techpreview
  • /test e2e-upgrade
  • /test images
  • /test integration
  • /test unit
  • /test verify
  • /test verify-client-go
  • /test verify-crd-schema
  • /test verify-deps

The following commands are available to trigger optional jobs:

  • /test e2e-azure
  • /test e2e-gcp

Use /test all to run all jobs.

In response to this:

/test ci/prow/verify

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@PratikMahajan
Copy link
Contributor Author

/test verify

@JoelSpeed
Copy link
Contributor

/override ci/prow/verify-crd-schema

Overriding existing boolean failures caused by renames

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, JoelSpeed, PratikMahajan, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to this:

/override ci/prow/verify-crd-schema

Overriding existing boolean failures caused by renames

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 74f9448 and 2 for PR HEAD b994043 in total

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 6, 2023

@PratikMahajan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-azure b994043 link false /test e2e-azure

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit f356bd9 into openshift:master Dec 6, 2023
15 of 16 checks passed
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.16.0-202312061931.p0.gf356bd9.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 6, 2023
@wking
vendor: Bump openshift/api to pick up signatureStores
dfec7af 
Pulling in openshift/api@f356bd9e2ff6 (Merge pull request
openshift/api#1678 from PratikMahajan/add-custom-sig-stores,
2023-12-06).

  $ go get -u github.com/openshift/api@master
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

all using:

  $ go version
  go version go1.21.1 linux/amd64

I also bumped the Dockerfile by hand, so now we'll pick up everything
under 0000_00_cluster-version-operator_* (importantly for this bump,
both the Default and TechPreviewNoUpgrade versions of the
ClusterVersion CRD).

And I bumped hack/test-prerequisites.go to use the new path to the
default CRD.
wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 6, 2023
@wking
vendor: Bump openshift/api to pick up signatureStores
2bb754d 
Pulling in openshift/api@f356bd9e2ff6 (Merge pull request
openshift/api#1678 from PratikMahajan/add-custom-sig-stores,
2023-12-06).

  $ go get -u github.com/openshift/api@master
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

all using:

  $ go version
  go version go1.21.1 linux/amd64

I also bumped the Dockerfile by hand, so now we'll pick up everything
under 0000_00_cluster-version-operator_* (importantly for this bump,
both the Default and TechPreviewNoUpgrade versions of the
ClusterVersion CRD).

And I bumped hack/test-prerequisites.go to use the new path to the
default CRD.
@wking wking mentioned this pull request Dec 6, 2023
Merged
wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 7, 2023
@wking
vendor: Bump openshift/api to pick up signatureStores
c736be2 
Pulling in openshift/api@f356bd9e2ff6 (Merge pull request
openshift/api#1678 from PratikMahajan/add-custom-sig-stores,
2023-12-06).

  $ go get -u github.com/openshift/api@master
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

all using:

  $ go version
  go version go1.21.1 linux/amd64

I also bumped the Dockerfile by hand, so now we'll pick up everything
under 0000_00_cluster-version-operator_* (importantly for this bump,
both the Default and TechPreviewNoUpgrade versions of the
ClusterVersion CRD).

And I bumped hack/test-prerequisites.go to use the new path to the
default CRD.
wking added a commit to wking/cluster-version-operator that referenced this pull request Dec 7, 2023
@wking
vendor: Bump openshift/api to pick up signatureStores
a98232f 
Pulling in openshift/api@f356bd9e2ff6 (Merge pull request
openshift/api#1678 from PratikMahajan/add-custom-sig-stores,
2023-12-06).

  $ go get -u github.com/openshift/api@master
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

all using:

  $ go version
  go version go1.21.1 linux/amd64

I also bumped the Dockerfile by hand, so now we'll pick up everything
under 0000_00_cluster-version-operator_* (importantly for this bump,
both the Default and TechPreviewNoUpgrade versions of the
ClusterVersion CRD).

And I bumped hack/test-prerequisites.go to use the new path to the
default CRD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@deads2k deads2k deads2k left review comments

@LalatenduMohanty LalatenduMohanty LalatenduMohanty left review comments

@petr-muller petr-muller petr-muller left review comments

@wking wking wking approved these changes

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@wking wking

@deads2k deads2k

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@PratikMahajan @openshift-ci-robot @JoelSpeed @deads2k @wking @LalatenduMohanty @petr-muller @openshift-bot @openshift-merge-robot