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

Implement KMS v2alpha1 #111126

Merged
merged 1 commit into from Aug 3, 2022
Merged

Implement KMS v2alpha1 #111126

merged 1 commit into from Aug 3, 2022

Conversation

aramase
Copy link
Member

@aramase aramase commented Jul 14, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements kubernetes/enhancements#3302

  1. Implements the new KMS v2alpha1 API (TransformFromStorage and TransformToStorage)
  2. Adds EncryptedObject type for new storage format using protobuf
  3. Modifies KMSConfiguration to configure the API version for kms provider (Allowed values: v2, empty or anything else will default fallback to v1)
  4. Adds KMSv2 feature gate
  5. Configures new health check probes for kmsv2 provider using Status RPC
  6. Adds integration tests to validate the kmsv2 feature
  7. Updates autogen scripts to generate manifests for kmsv2

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Adds KMS v2alpha1 support

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

KEP: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3299-kms-v2-improvements

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 14, 2022
@aramase
Copy link
Member Author

aramase commented Jul 14, 2022

/sig auth
/cc @enj @ritazh

@k8s-ci-robot k8s-ci-robot requested review from enj and ritazh July 14, 2022 00:29
@k8s-ci-robot k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. area/apiserver kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 14, 2022
@aramase aramase force-pushed the kms-v2alpha1-impl branch 2 times, most recently from 4c06744 to 92c4f2e Compare July 14, 2022 06:47
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 14, 2022
@aramase aramase force-pushed the kms-v2alpha1-impl branch 2 times, most recently from 5db3533 to 3a153d3 Compare July 14, 2022 06:52
@leilajal
Copy link
Contributor

/cc @deads2k
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 14, 2022
@aramase aramase force-pushed the kms-v2alpha1-impl branch 2 times, most recently from 073b88e to 9ce0015 Compare July 14, 2022 21:42
@aramase aramase requested a review from liggitt August 2, 2022 23:17
@cici37
Copy link
Contributor

cici37 commented Aug 3, 2022

Hello 👋, 1.25 Release Lead here.

The exception request is approved and your updated deadline to make any changes to your PR is 9:00 AM PST Tuesday 9th August 2022. Thank you!

/milestone v1.25

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 3, 2022
@aramase aramase force-pushed the kms-v2alpha1-impl branch 2 times, most recently from bab41a5 to f74ab3e Compare August 3, 2022 18:44
@aramase
Implement KMS v2alpha1
f19f3f4 
- add feature gate
- add encrypted object and run generated_files
- generate protobuf for encrypted object and add unit tests
- move parse endpoint to util and refactor
- refactor interface and remove unused interceptor
- add protobuf generate to update-generated-kms.sh
- add integration tests
- add defaulting for apiVersion in kmsConfiguration
- handle v1/v2 and default in encryption config parsing
- move metrics to own pkg and reuse for v2
- use Marshal and Unmarshal instead of serializer
- add context for all service methods
- check version and keyid for healthz

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@aramase aramase mentioned this pull request Aug 3, 2022
Closed
liggitt
liggitt reviewed Aug 3, 2022
View reviewed changes
staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/envelope.go

const (
// KMSAPIVersion is the version of the KMS API.
KMSAPIVersion = "v2alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

so we require grpc implementors to return this value in v2 responses currently? and we'll bump this to beta and then to just v2 at GA?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's right! This is the same behavior as v1beta1 today. In case of v1 we perform this check before every RPC call to the plugin. In v2 this is only part of the health check.

Copy link
Member

Choose a reason for hiding this comment

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

huh. since v1beta1 never got to v1, I'm having a bit of a hard time picturing how we'll simultaneously support v2beta1 and v2, but can resolve that before the beta point where we have to have some cross-release compatibility story

@liggitt
Copy link
Member

liggitt commented Aug 3, 2022

/approve
/lgtm

for API bits and vendor.txt change
@enj approval at #111126 (comment)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, enj, liggitt

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit 0a2ae7a into kubernetes:master Aug 3, 2022
@aramase aramase deleted the kms-v2alpha1-impl branch August 3, 2022 23:42
This was referenced Aug 17, 2022
Closed
Closed
Closed
@enj enj mentioned this pull request Aug 18, 2022
Closed
@aramase aramase mentioned this pull request Aug 18, 2022
Closed
@harshanarayana harshanarayana mentioned this pull request Aug 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtaufen mtaufen mtaufen left review comments

@liggitt liggitt liggitt left review comments

@ritazh ritazh ritazh left review comments

@aojea aojea aojea left review comments

@enj enj enj requested changes

@dgrisonnet dgrisonnet dgrisonnet left review comments

@lavalamp lavalamp Awaiting requested review from lavalamp

@deads2k deads2k Awaiting requested review from deads2k

@mikedanese mikedanese Awaiting requested review from mikedanese

Assignees

@liggitt liggitt

@mikedanese mikedanese

@enj enj

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth: KMS v2
Status: ✅ Done
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet