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

API-1509: Add aesgcm to encryption config #1407

Closed
wants to merge 1 commit into from
Closed

API-1509: Add aesgcm to encryption config #1407

wants to merge 1 commit into from

Conversation

dgrisonnet
Copy link
Member

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 16, 2023

Hello @dgrisonnet! 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.

@dgrisonnet dgrisonnet changed the title WIP: config: add aesgcm to encryption config Add aesgcm to encryption config Feb 21, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2023
@dgrisonnet dgrisonnet changed the title Add aesgcm to encryption config API-1509: Add aesgcm to encryption config Feb 21, 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 Feb 21, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Feb 21, 2023 •
edited by openshift-ci bot

@dgrisonnet: This pull request references API-1509 which is a valid jira issue.

In response to this:

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.

dinhxuanvu
dinhxuanvu reviewed Feb 21, 2023
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

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

tkashem commented Feb 22, 2023

/assign @deads2k

deads2k
deads2k reviewed Feb 22, 2023
View reviewed changes
config/v1/types_apiserver.go Outdated
@@ -184,7 +184,7 @@ type APIServerEncryption struct {
Type EncryptionType `json:"type,omitempty"`
}

// +kubebuilder:validation:Enum="";identity;aescbc
// +kubebuilder:validation:Enum="";identity;aescbc;aesgcm
Copy link
Contributor

@deads2k deads2k Feb 22, 2023 •
edited

Choose a reason for hiding this comment

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

use a technique for techpreview only https://github.com/openshift/api/blob/master/example/v1/types_stable.go#L72-L73 and get insta-merge.

You'll be able to remove the techpreview category when the testing works. This can happen in 4.13 if its ready.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2023
@dgrisonnet
config: add aesgcm to encryption config
ae90cb1 
Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 22, 2023

@dgrisonnet: all tests passed!

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.

dgrisonnet
dgrisonnet commented Feb 22, 2023
View reviewed changes
config/v1/types_apiserver.go
@@ -184,7 +184,8 @@ type APIServerEncryption struct {
Type EncryptionType `json:"type,omitempty"`
}

// +kubebuilder:validation:Enum="";identity;aescbc
Copy link
Member Author

Choose a reason for hiding this comment

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

@deads2k I removed the kubebuilder validation because it felt redundant to have both validations in place, is that correct?

dinhxuanvu
dinhxuanvu reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

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

openshift-ci bot commented Feb 22, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dgrisonnet, dinhxuanvu
Once this PR has been reviewed and has the lgtm label, please ask for approval from deads2k. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@deads2k deads2k mentioned this pull request Feb 23, 2023
Merged
@deads2k
Copy link
Contributor

deads2k commented Feb 23, 2023

this lgtm, but the generation didn't quite work. I fixed it in the middle/last commit of #1413

@dgrisonnet
Copy link
Member Author

Closing in favor of #1413.

@dgrisonnet dgrisonnet closed this Feb 23, 2023
@dgrisonnet dgrisonnet deleted the aesgcm branch February 23, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu left review comments

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@deads2k deads2k

@dinhxuanvu dinhxuanvu

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dgrisonnet @openshift-ci-robot @tkashem @deads2k @dinhxuanvu