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

gRPC-based KMS plugin service #55684

Merged
merged 9 commits into from Feb 9, 2018

Conversation

@wu-qiang
Contributor

wu-qiang commented Nov 14, 2017

What this PR does / why we need it:
Implement for issue #51965
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #51965

Special notes for your reviewer:
@destijl @sakshamsharma @deads2k @ericchiang
The implementation based on the document https://docs.google.com/document/d/1S_Wgn-psI0Z7SYGvp-83ePte5oUNMr4244uanGLYUmw/edit
Release note:

Implement envelope service with gRPC, so that KMS providers can be pulled out from API server.
@ericchiang

This comment has been minimized.

Member

ericchiang commented Nov 14, 2017

@sakshamsharma

First pass review comments. Also, could you please rename the PR to something like "gRPC-based KMS plugin service"? 😄

}
} else {
// Get gRPC client service with remote config
envelopeService, err = envelope.NewEnvelopeService(

This comment has been minimized.

@sakshamsharma

sakshamsharma Nov 14, 2017

Member

Rename to envelopeService, err := envelope.NewGRPCService(...)? (Refer to comment in grpc_service.go)

var envelopeService envelope.Service
remoteConfig := provider.KMS.RemoteServer
if remoteConfig == nil {
// There should be no KMS provider plugins on API server side in future.

This comment has been minimized.

@sakshamsharma

sakshamsharma Nov 14, 2017

Member

I agree with this comment. Should we remove this block altogether? @destijl

}
// NewEnvelopeService returns an envelope.Service which use gRPC to communicate the remote KMS provider.
func NewEnvelopeService(endpoint, serverCert, clientCert, clientKey string) (Service, error) {

This comment has been minimized.

@sakshamsharma

sakshamsharma Nov 14, 2017

Member

Perhaps renaming this to NewGRPCService or NewRemoteKMSService might be more informative. The current name sounds a bit too generic IMO.

This comment has been minimized.

@wu-qiang

wu-qiang Nov 15, 2017

Contributor

Okay, will modify it to "NewGRPCService".

// Client API for KmsService service
type KmsServiceClient interface {

This comment has been minimized.

@sakshamsharma

sakshamsharma Nov 14, 2017

Member

The Go standard dictates camel case of the form: KMSServiceClient.

This comment has been minimized.

@wu-qiang

wu-qiang Nov 15, 2017

Contributor

Will update the proto file to fix it.

// NewEnvelopeService returns an envelope.Service which use gRPC to communicate the remote KMS provider.
func NewEnvelopeService(endpoint, serverCert, clientCert, clientKey string) (Service, error) {
protocol, addr, err := parseEndpoint(endpoint)

This comment has been minimized.

@sakshamsharma

sakshamsharma Nov 14, 2017

Member

Endpoint might not be provided in the configuration. url.Parse does not give error on an empty input string. You may want to test for it inside parseEndpoint and return a custom error.

This comment has been minimized.

@wu-qiang

wu-qiang Nov 15, 2017

Contributor

Now the error for empty endpoint will be handled in below switch default block. Of course, test endpoint explicitly is more readable. I will add it.

@wu-qiang wu-qiang changed the title from Kms plugin grpc api to gRPC-based KMS plugin service Nov 15, 2017

@wu-qiang

This comment has been minimized.

Contributor

wu-qiang commented Nov 15, 2017

/assign @lavalamp

@wu-qiang

This comment has been minimized.

Contributor

wu-qiang commented Nov 15, 2017

/retest

@timothysc timothysc removed their assignment Nov 15, 2017

@jbeda

This comment has been minimized.

Contributor

jbeda commented Nov 15, 2017

Talked about this in SIG-Auth and wanted to draw attention to #55729. This is another type of extension mechanism where the API server is calling out. I'd love to figure out how this relates to other types of webhook/callout mechanisms.

(I also understand that this is a bit of a different beast in that it is lower level than many of the other web hooks and may have extra performance requirements.)

@kksriram

This comment has been minimized.

kksriram commented Nov 15, 2017

@jbeda I hear you about introducing yet another type of extension mechanism. I am looking at @kubernetes/sig-api-machinery-pr-reviews for input so we can make corrections sooner than later. Making changes to the approach now seems the right time as 1.9 enters slush/freeze and we can get changes ready in time for possible inclusion in 1.10 as alpha.

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 15, 2017

@kksriram: Reiterating the mentions to trigger a notification:
@kubernetes/sig-api-machinery-pr-reviews

In response to this:

@jbeda I hear you about introducing yet another type of extension mechanism. I am looking at @kubernetes/sig-api-machinery-pr-reviews for input so we can make corrections sooner than later. Making changes to the approach now seems the right time as 1.9 enters slush/freeze and we can get changes ready in time for possible inclusion in 1.10 as alpha.

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.

@lavalamp

This comment has been minimized.

Member

lavalamp commented Nov 16, 2017

This is not targeted for 1.9, right?

@kksriram

This comment has been minimized.

kksriram commented Nov 16, 2017

@lavalamp If all tests pass, and @kubernetes/sig-auth @kubernetes/sig-api-machinery-pr-reviews LGTMs by Code freeze (next week, right) - can we target it for 1.9?

I didn't hear a specific concern at sig-auth yesterday beyond @jbeda worry that this is yet another kind of extension.

@kksriram

This comment has been minimized.

kksriram commented Nov 17, 2017

@lavalamp NVM, I see Anthony's post here on sig-release. I guess its up to sig-auth to target it for 1.9.

@dims

This comment has been minimized.

Member

dims commented Nov 17, 2017

Let's see what they say ... @kubernetes/sig-auth-pr-reviews @kubernetes/sig-auth-api-reviews @kubernetes/sig-auth-feature-requests Do we do this in 1.9 or move it to 1.10?

@smarterclayton

This comment has been minimized.

Contributor

smarterclayton commented Nov 17, 2017

@deads2k

This comment has been minimized.

Contributor

deads2k commented Nov 17, 2017

Let's see what they say ... @kubernetes/sig-auth-pr-reviews @kubernetes/sig-auth-api-reviews @kubernetes/sig-auth-feature-requests Do we do this in 1.9 or move it to 1.10?

This is not targeted at 1.9. There hasn't been enough time to review the design and for the appropriate reviewers/approvers to sign off. This is a good target for 1.10.

@wu-qiang

This comment has been minimized.

Contributor

wu-qiang commented Feb 8, 2018

@duglin I will resolve it ASAP.

@wu-qiang

This comment has been minimized.

Contributor

wu-qiang commented Feb 8, 2018

@duglin @deads2k The comment has been resolved.

@duglin

This comment has been minimized.

Contributor

duglin commented Feb 8, 2018

@wu-qiang thanks for the quick response!

@duglin

This comment has been minimized.

Contributor

duglin commented Feb 8, 2018

@deads2k any other comments?

@@ -56,7 +49,7 @@ resources:
secret: dGhpcyBpcyBwYXNzd29yZA==
- kms:
name: testprovider
configfile: testproviderconfig
endpoint: /tmp/testprovider.sock

This comment has been minimized.

@liggitt

liggitt Feb 8, 2018

Member

nit, make the endpoints in the test data actually be valid (unix:///...)

This comment has been minimized.

@wu-qiang

wu-qiang Feb 9, 2018

Contributor

Addressed

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 8, 2018

no other comments from me. I'd be happy to see a working integration with this (I know the google and vault implementations are in progress)

@destijl

This comment has been minimized.

Member

destijl commented Feb 8, 2018

This looks good from our side, @immutableT is just going through the process to get a new repo for the google KMS plugin to live in.

@khenidak

This comment has been minimized.

Contributor

khenidak commented Feb 8, 2018

Folks - can we merge this now? @liggitt @destijl

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 9, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, wu-qiang

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 OWNERS Files:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 9, 2018

Automatic merge from submit-queue (batch tested with PRs 58437, 59490, 55684). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 53207e1 into kubernetes:master Feb 9, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation wu-qiang authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@vineet-garg vineet-garg referenced this pull request Feb 13, 2018

Closed

document kms encryption provider #7399

0 of 1 task complete
package v1beta1;
// This service defines the public APIs for remote KMS provider.
service KMSService {

This comment has been minimized.

@mikedanese

mikedanese Feb 23, 2018

Member

KMSService is redundant. I'd like to fix this this before 1.10. Service names are painful to change.

k8s-merge-robot added a commit that referenced this pull request Feb 23, 2018

Merge pull request #60268 from mikedanese/kmss
Automatic merge from submit-queue (batch tested with PRs 55637, 57461, 60268, 60290, 60210). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

kms: rename KMSService to KeyManagementService

KMSService is redundant. Introduced in #55684

@kubernetes/sig-auth-api-reviews 

```release-note
NONE
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment