[KMSv2] Drop the apimachinery #120225

pegasas · 2023-08-29T06:44:10Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Drop the apimachinery and client-go dependencies from https://github.com/kubernetes/kms/blob/master/go.mod to keep the import lighter.

Which issue(s) this PR fixes:

Fixes #120216

Special notes for your reviewer:

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 · 2023-08-29T06:44:12Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

enj · 2023-08-29T12:57:23Z

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

@@ -64,7 +64,7 @@ func TestGRPCService(t *testing.T) {
 	client := newClient(t, address)

 	// make sure the gRPC server is up before running tests
-	if err := wait.PollImmediateUntilWithContext(ctx, time.Second, func(ctx context.Context) (bool, error) {
+	if err := util.PollImmediateUntilWithContext(ctx, time.Second, func(ctx context.Context) (bool, error) {


Just use a for loop with a sleep. Do not copy the entire implementation from the wait package.

Sure.
I will remove unnecessary logic, split the files according to apimachinery, and add some unit tests in the next steps.

Just make this a simple inline for loop with a sleep.

@pegasas Mo's recommendation was to just make this a inline for loop with a sleep. We don't need to copy any of the poller code or PollImmediateUntilWithContext from the apimachinery util.

sure.
I removed other useless code and keeps a inline for loop with a sleep.

aojea · 2023-09-04T08:44:49Z

staging/src/k8s.io/kms/go.mod

 	google.golang.org/genproto/googleapis/rpc v0.0.0-20230525234030-28d5490b6b19 // indirect
 	google.golang.org/protobuf v1.31.0 // indirect
 	k8s.io/utils v0.0.0-20230406110748-d93618cff8a2 // indirect
 )

 replace (
 	k8s.io/api => ../api
-	k8s.io/apimachinery => ../apimachinery
-	k8s.io/client-go => ../client-go


is client-go an indirect dependency of "k8s.io/apimachinery/pkg/util/wait"?

#120216 (comment)
It seems this targets further complex implementation.

like this: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kms/internal/mock_ratelimit_remote_service.go#L24C1-L24C37

You need to make that into a separate go module so it no longer impacts the deps of the kms module.

ibihim · 2023-10-09T16:30:58Z

/triage accepted

aramase

The CI failures are legit!

The linter failures should go away after you change the implementation to a for loop with sleep and remove the apimachinery code.
Run hack/update-vendor.sh to fix pull-kubernetes-dependencies failures.

aramase · 2023-10-09T23:57:06Z

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

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/base64"
 	"fmt"
+	"k8s.io/kms/pkg/util"


Move the import to line 35. FYI, the imports are grouped this way

import ( stdlib external packages internal packages )

Thanks @aramase !
Corrected!

aramase · 2023-10-09T23:59:12Z

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

@@ -64,7 +64,7 @@ func TestGRPCService(t *testing.T) {
 	client := newClient(t, address)

 	// make sure the gRPC server is up before running tests
-	if err := wait.PollImmediateUntilWithContext(ctx, time.Second, func(ctx context.Context) (bool, error) {
+	if err := util.PollImmediateUntilWithContext(ctx, time.Second, func(ctx context.Context) (bool, error) {


@pegasas Mo's recommendation was to just make this a inline for loop with a sleep. We don't need to copy any of the poller code or PollImmediateUntilWithContext from the apimachinery util.

aramase · 2023-10-10T00:00:00Z

staging/src/k8s.io/kms/pkg/util/util.go

@@ -52,3 +58,105 @@ func ParseEndpoint(endpoint string) (string, error) {

 	return u.Path, nil
 }
+
+func PollImmediateUntilWithContext(ctx context.Context, interval time.Duration, condition func(context.Context) (done bool, err error)) error {


Added a comment above. The previous review comments suggested using a simple inline for loop with sleep instead of copying code from apimachinery/util.

add comments inline.

aramase · 2023-10-10T00:00:15Z

staging/src/k8s.io/kms/pkg/util/util.go

+func HandleCrash() {
+	if r := recover(); r != nil {
+		if r == http.ErrAbortHandler {
+			// honor the http.ErrAbortHandler sentinel panic value:
+			//   ErrAbortHandler is a sentinel panic value to abort a handler.
+			//   While any panic from ServeHTTP aborts the response to the client,
+			//   panicking with ErrAbortHandler also suppresses logging of a stack trace to the server's error log.
+			return
+		}
+		const size = 64 << 10
+		stacktrace := make([]byte, size)
+		stacktrace = stacktrace[:runtime.Stack(stacktrace, false)]
+		if _, ok := r.(string); ok {
+			klog.Errorf("Observed a panic: %s\n%s", r, stacktrace)
+		} else {
+			klog.Errorf("Observed a panic: %#v (%v)\n%s", r, r, stacktrace)
+		}
+	}
+}


This is not required!

k8s-ci-robot · 2023-10-22T19:14:01Z

@pegasas: 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
pull-kubernetes-verify-strict-lint	33ba2aae6f3e24dd3525e9fa2360f77d0ce58f72	link	false	`/test pull-kubernetes-verify-strict-lint`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

enj · 2023-10-23T14:50:51Z

/lgtm
/approve

k8s-ci-robot · 2023-10-23T14:51:00Z

LGTM label has been added.

Git tree hash: 96df378c8aa47696220f66966891d4f54424de38

dims · 2023-10-23T14:53:01Z

/kind cleanup

dims · 2023-10-23T14:53:59Z

/approve
/lgtm

k8s-ci-robot · 2023-10-23T14:54:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, enj, pegasas

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/publishing/OWNERS~~ [dims]
~~staging/src/k8s.io/kms/OWNERS~~ [dims,enj]

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 requested review from enj and lavalamp August 29, 2023 06:45

enj reviewed Aug 29, 2023

View reviewed changes

pegasas force-pushed the kms branch 2 times, most recently from f6d2dcc to 6025cc4 Compare September 4, 2023 03:28

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Sep 4, 2023

pegasas marked this pull request as ready for review September 4, 2023 03:29

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2023

k8s-ci-robot requested review from aramase and enj September 4, 2023 03:30

pegasas force-pushed the kms branch 2 times, most recently from 9a7884c to 871604d Compare September 4, 2023 03:36

pegasas marked this pull request as draft September 4, 2023 07:06

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 4, 2023

pegasas changed the title ~~[KMSv2] Drop the apimachinery and client-go dependencies from k8s.io/kms~~ [DRAFT][KMSv2] Drop the apimachinery and client-go dependencies from k8s.io/kms Sep 4, 2023

aojea reviewed Sep 4, 2023

View reviewed changes

pegasas force-pushed the kms branch from 871604d to 43a11d1 Compare September 4, 2023 13:23

k8s-ci-robot added the area/release-eng Issues or PRs related to the Release Engineering subproject label Sep 4, 2023

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 Oct 9, 2023

aramase suggested changes Oct 10, 2023

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 13, 2023

pegasas force-pushed the kms branch from 33ba2aa to 2cf0606 Compare October 19, 2023 12:34

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2023

pegasas force-pushed the kms branch 2 times, most recently from be4c747 to ac5ecea Compare October 22, 2023 18:20

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2023

pegasas force-pushed the kms branch from ac5ecea to ff938c8 Compare October 23, 2023 14:15

[KMSv2] Drop the apimachinery

d9131d5

pegasas force-pushed the kms branch from ff938c8 to d9131d5 Compare October 23, 2023 14:34

k8s-ci-robot assigned enj Oct 23, 2023

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

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 23, 2023

k8s-ci-robot assigned dims Oct 23, 2023

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

k8s-ci-robot merged commit b327bd7 into kubernetes:master Oct 23, 2023
16 checks passed

k8s-ci-robot added this to the v1.29 milestone Oct 23, 2023

enj mentioned this pull request Oct 26, 2023

Add back k8s.io/kms/internal/plugins/_mock/go.* #121543

Merged

sreeram-venkitesh mentioned this pull request Nov 1, 2023

KMS v2 Improvements kubernetes/enhancements#3299

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[KMSv2] Drop the apimachinery #120225

[KMSv2] Drop the apimachinery #120225

pegasas commented Aug 29, 2023 •

edited

k8s-ci-robot commented Aug 29, 2023

enj Aug 29, 2023

pegasas Aug 29, 2023

enj Sep 4, 2023

pegasas Sep 5, 2023

aramase Oct 9, 2023

pegasas Oct 19, 2023

aojea Sep 4, 2023

pegasas Sep 4, 2023 •

edited

pegasas Sep 4, 2023

enj Sep 4, 2023

pegasas Sep 4, 2023

ibihim commented Oct 9, 2023

aramase left a comment

aramase Oct 9, 2023

pegasas Oct 19, 2023

aramase Oct 9, 2023

aramase Oct 10, 2023

pegasas Oct 19, 2023

aramase Oct 10, 2023

pegasas Oct 19, 2023

k8s-ci-robot commented Oct 22, 2023 •

edited

enj commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

dims commented Oct 23, 2023

dims commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

[KMSv2] Drop the apimachinery #120225

[KMSv2] Drop the apimachinery #120225

Conversation

pegasas commented Aug 29, 2023 • 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 Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pegasas Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibihim commented Oct 9, 2023

aramase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 22, 2023 • edited

enj commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

dims commented Oct 23, 2023

dims commented Oct 23, 2023

k8s-ci-robot commented Oct 23, 2023

pegasas commented Aug 29, 2023 •

edited

pegasas Sep 4, 2023 •

edited

k8s-ci-robot commented Oct 22, 2023 •

edited