kmsv2: add grpc service #114678

ibihim · 2022-12-23T15:59:41Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This is a smaller version of #112898, it only contains a GRPC Service and an in-memory kms.Service.

Which issue(s) this PR fixes:

Fixes #112859 partially.

Fixes #

Special notes for your reviewer:

This is a subset of #114315 with slight changes to accommodate discussions from #sig-auth-kms-dev on kubernetes Slack.

Does this PR introduce a user-facing change?

NONE

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 · 2022-12-23T15:59:49Z

@ibihim: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2022-12-23T15:59:50Z

Hi @ibihim. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

enj · 2022-12-23T16:29:59Z

staging/src/k8s.io/kms/service/grpc_service.go

+	}
+
+	return s, nil
+}


This cannot error.

enj · 2022-12-23T16:30:38Z

staging/src/k8s.io/kms/service/grpc_service.go

+
+// ListenAndServe accepts incoming connections on a Unix socket. Returns non-nil
+// error unless Close or Shutdown is called.
+func (s *GRPCService) ListenAndServe() error {


This blocks right? Document that behavior.

enj · 2022-12-23T16:34:33Z

staging/src/k8s.io/kms/service/grpc_service.go

+// blocks until all pending  RPCs are finished.
+func (s *GRPCService) Shutdown() {
+	if s.server != nil {
+		s.server.GracefulStop()


If this returns an error, ignore it explicitly.

s.server.GracefulStop() has the signature: func (s *Server) GracefulStop(). It doesn't return an error.

staging/src/k8s.io/kms/service/grpc_service.go

enj · 2022-12-23T16:46:45Z

staging/src/k8s.io/kms/service/grpc_service_test.go

+	})
+}
+
+func newClient(t *testing.T, address string) (kmsapi.KeyManagementServiceClient, func() error) {


This should only return the client, use t.Cleanup to avoid funneling up.

enj · 2022-12-23T16:48:19Z

staging/src/k8s.io/kms/service/grpc_service_test.go

+
+var _ Service = (*testService)(nil)
+
+func (ts *testService) Decrypt(ctx context.Context, uid string, req *DecryptRequest) ([]byte, error) {


Use one letter receiver names.

enj · 2022-12-23T16:50:28Z

staging/src/k8s.io/kms/service/grpc_service_test.go

+			t.Errorf("want: len(keyID) > 0, have: %d", len(status.KeyId))
+		}
+		if status.Version != version {
+			t.Errorf("want %q, have: %q", "v2alpha1", status.Version)


Suggested change

t.Errorf("want %q, have: %q", "v2alpha1", status.Version)

t.Errorf("want %q, have: %q", version, status.Version)

enj · 2022-12-23T16:51:40Z

staging/src/k8s.io/kms/service/grpc_service_test.go

+		}
+
+		decRes, err := client.Decrypt(ctx, &kmsapi.DecryptRequest{
+			Ciphertext:  encRes.Ciphertext,


Assert that ciphertext is not equal to plaintext.

enj · 2022-12-23T16:53:33Z

/ok-to-test

enj · 2023-01-05T19:23:01Z

/reopen

k8s-ci-robot · 2023-01-05T19:23:06Z

@enj: Reopened this PR.

In response to this:

/reopen

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.

Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>

enj · 2023-01-06T13:01:49Z

/lgtm
/approve

k8s-ci-robot · 2023-01-06T13:01:55Z

LGTM label has been added.

Git tree hash: f49e8a3666d6a8642f5d601bb7d9ff16d658aa01

k8s-ci-robot · 2023-01-06T13:02:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, ibihim

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:

~~staging/src/k8s.io/kms/OWNERS~~ [enj]

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

enj · 2023-01-06T15:08:23Z

/retest

liggitt · 2023-01-06T20:10:19Z

the test added in this PR is flaking (saw an immediate failure in #114766 - https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/114766/pull-kubernetes-unit/1611428013864390656 and https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/114766/pull-kubernetes-unit-go-canary/1611428103538610176)

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 23, 2022

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Dec 23, 2022

ibihim mentioned this pull request Dec 23, 2022

[KMSv2] GRPC Service with in-memory KMS Service #114315

Closed

k8s-ci-robot requested review from enj and wojtek-t December 23, 2022 16:00

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 23, 2022

enj suggested changes Dec 23, 2022

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 23, 2022

ibihim force-pushed the kmsv2-plugin-service-grpc branch from 3af7d3c to 026325c Compare January 4, 2023 18:39

ibihim mentioned this pull request Jan 5, 2023

staging/publishing: open up kms package for sub packages and utils #114850

Merged

k8s-ci-robot closed this in #114850 Jan 5, 2023

k8s-ci-robot reopened this Jan 5, 2023

ibihim force-pushed the kmsv2-plugin-service-grpc branch 3 times, most recently from 57dd645 to 4bb35c8 Compare January 6, 2023 11:03

kmsv2: add grpc service

371b3b3

Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>

ibihim force-pushed the kmsv2-plugin-service-grpc branch from 4bb35c8 to 371b3b3 Compare January 6, 2023 12:43

k8s-ci-robot assigned enj Jan 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 6, 2023

k8s-ci-robot merged commit 98dae66 into kubernetes:master Jan 6, 2023

k8s-ci-robot added this to the v1.27 milestone Jan 6, 2023

ibihim deleted the kmsv2-plugin-service-grpc branch January 6, 2023 18:12

aramase mentioned this pull request Jan 6, 2023

[KMSv2] Add metrics and tracing in k8s.io/kms for plugins #112859

Closed

ibihim mentioned this pull request Jan 6, 2023

REQUEST: New membership for ibihim kubernetes/org#3921

Closed

9 tasks

liggitt mentioned this pull request Jan 6, 2023

[Prepare for go1.20] *: Bump versions and fix tests #114766

Merged

liggitt mentioned this pull request Mar 16, 2023

KMS v2 Improvements kubernetes/enhancements#3299

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kmsv2: add grpc service #114678

kmsv2: add grpc service #114678

ibihim commented Dec 23, 2022 •

edited

k8s-ci-robot commented Dec 23, 2022

k8s-ci-robot commented Dec 23, 2022

enj Dec 23, 2022

enj Dec 23, 2022

enj Dec 23, 2022

ibihim Jan 2, 2023 •

edited

enj Dec 23, 2022

enj Dec 23, 2022

enj Dec 23, 2022

enj Dec 23, 2022

enj commented Dec 23, 2022

enj commented Jan 5, 2023

k8s-ci-robot commented Jan 5, 2023

enj commented Jan 6, 2023

k8s-ci-robot commented Jan 6, 2023

k8s-ci-robot commented Jan 6, 2023

enj commented Jan 6, 2023

liggitt commented Jan 6, 2023 •

edited


		var _ Service = (*testService)(nil)

		func (ts testService) Decrypt(ctx context.Context, uid string, req DecryptRequest) ([]byte, error) {

	t.Errorf("want %q, have: %q", "v2alpha1", status.Version)
	t.Errorf("want %q, have: %q", version, status.Version)

kmsv2: add grpc service #114678

kmsv2: add grpc service #114678

Conversation

ibihim commented Dec 23, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

k8s-ci-robot commented Dec 23, 2022

k8s-ci-robot commented Dec 23, 2022

enj Dec 23, 2022

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

ibihim Jan 2, 2023 • edited

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

enj Dec 23, 2022

Choose a reason for hiding this comment

enj commented Dec 23, 2022

enj commented Jan 5, 2023

k8s-ci-robot commented Jan 5, 2023

enj commented Jan 6, 2023

k8s-ci-robot commented Jan 6, 2023

k8s-ci-robot commented Jan 6, 2023

enj commented Jan 6, 2023

liggitt commented Jan 6, 2023 • edited

ibihim commented Dec 23, 2022 •

edited

ibihim Jan 2, 2023 •

edited

liggitt commented Jan 6, 2023 •

edited