restmapper + discovery: add context-aware APIs #129109

pohly · 2024-12-06T17:15:55Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

The main purpose is to replace context.TODO with a context provided by the caller. A secondary purpose is to enable contextual logging.

Which issue(s) this PR fixes:

Fixes #110810

Special notes for your reviewer:

Modifying the existing interfaces and APIs would have a big impact on the ecosystem. This is a no-go. Instead, the following approach was taken:

All interfaces get duplicated in a *WithContext variant where the methods also have a *WithContext suffix and the ctx parameter. All methods are treated this way except for obvious local get methods (like RESTClient) because it cannot be ruled out entirely that some implementation may need a context.
Implementations of these interfaces implement both method variants, which is possible because the method names are different. The old methods are implemented as thin wrappers around the updated code which is now the body of the new methods or shared helpers. In some cases there is additional overhead (type checks, potentially additional allocations) when using the old methods.
*2Context helpers bridge from the old to the new interfaces. They try a type cast first. Because the in-tree implementations implement both, they can be used directly. For other implementations wrappers are used.
All old APIs and interfaces are marked as deprecated. There is no intent to ever remove them, but consumers should be made aware that there are now better alternatives. Implementations also get marked this way even if nothing ever calls them directly because it shows which code, at least theoretically, could get removed.
Existing unit tests do not get updated to the new APIs. This gives us unit test coverage of the old and new API because the old APIs call the new ones.
In-tree consumers will be updated in follow-up PRs. This is likely to be a longer process. Because of the deprecation comment, hack/golangci-lint.sh -n can be used to find code which needs to be updated.

Does this PR introduce a user-facing change?

client-go now has context-aware APIs for REST mapping and discovery. This addresses a long-standing problem that the implementations under the hood made blocking API calls with `context.TODO`. Consumers of client-go are encouraged to switch to the new APIs and therefore they get marked as `Deprecated`. However, there is no plan to ever remove the old APIs.

/cc @ardaguclu @sttts

k8s-ci-robot · 2024-12-06T17:15:58Z

Please note that we're already in Test Freeze for the release-1.32 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.32.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Fri Dec 6 16:44:50 UTC 2024.

pohly · 2024-12-06T17:26:07Z

staging/src/k8s.io/apimachinery/pkg/api/meta/interfaces.go

+// unique across groups.
+type RESTMapperWithContext interface {
+	// KindFor takes a partial resource and returns the single match.  Returns an error if there are multiple matches
+	KindForWithContext(ctx context.Context, resource schema.GroupVersionResource) (schema.GroupVersionKind, error)


I am using the *WithContext pattern consistently because that's what we settled on in Kubernetes. However, it leads to long names which in this case are also a bit odd.

In Go, *Context is used more often than *WithContext:

# e.g. database/sql/sql.go:func (db *DB) PingContext(ctx context.Context) error { $ grep -r 'func.*Context(ctx' /nvme/gopath/go/src/ | grep -v WithContext | wc -l 55 # e.g. net/http/request.go:func NewRequestWithContext(ctx context.Context, method, url string, body io.Reader) (*Request, error) $ grep -r 'func.*WithContext(ctx' /nvme/gopath/go/src/ | wc -l 5

Should we follow that practice and use the shorter *Context pattern?

Then this becomes:

type RESTMapperContext interface { KindForContext(ctx context.Context, resource schema.GroupVersionResource) (schema.GroupVersionKind, error)

Although *Context is used far more and makes sense, RESTMapperContext may mislead people about it behaves as a Context rather than having a Context.

That's a good point.

So let's stick with *WithContext?

I will admit I'm not super thrilled by the lengthiness of this, but I'm always for consistency. So 👍 for *WithContext.

ardaguclu

Without any in-depth code review, overall method seems to be the most plausible one for least disruption and makes sense to me.
We may need to ask additional opinions from;
openapi/discovery
@seans3 @Jefftree
restmapper
@deads2k @liggitt

ardaguclu · 2024-12-09T07:57:51Z

staging/src/k8s.io/apimachinery/pkg/api/meta/interfaces.go

+// unique across groups.
+type RESTMapperWithContext interface {
+	// KindFor takes a partial resource and returns the single match.  Returns an error if there are multiple matches
+	KindForWithContext(ctx context.Context, resource schema.GroupVersionResource) (schema.GroupVersionKind, error)


Although *Context is used far more and makes sense, RESTMapperContext may mislead people about it behaves as a Context rather than having a Context.

pohly · 2024-12-09T10:44:57Z

/test pull-kubernetes-apidiff

Jefftree · 2024-12-10T21:09:32Z

/triage accepted

The Lister and Watcher interfaces only supported methods without context, but were typically implemented with client-go API calls which need a context. New interfaces get added using the same approach as in kubernetes#129109.

k8s-ci-robot · 2025-01-30T11:01:42Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pohly
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the 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

pohly · 2025-01-30T13:45:00Z

/test pull-kubernetes-integration

#129908

The Lister and Watcher interfaces only supported methods without context, but were typically implemented with client-go API calls which need a context. New interfaces get added using the same approach as in kubernetes/kubernetes#129109. Kubernetes-commit: 6688adae142e37114d9dfa8d94cd1d8a91fbcc13

soltysh

I left a few comments, but the major ones are:

I believe we agreed to NOT do deprecation on methods w/o context.
What should we do with tests in staging/src/k8s.io/apimachinery/pkg/api/meta/ directory, currently they only work with non-context variants. Should we duplicate them for context variants?

The rest seems reasonable, but I will want to do another pass.

soltysh · 2025-02-07T18:53:13Z

staging/src/k8s.io/apimachinery/pkg/api/meta/interfaces.go

+	if m, ok := m.(RESTMapperWithContext); ok {
+		return m
+	}
+	return &restMapperWrapper{


In other place (#129341) you used value receiver. So for consistency, and based on what you're writing above we should also update

kubernetes/staging/src/k8s.io/client-go/tools/cache/listwatch.go

Lines 88 to 99 in 39f1c90

func ToWatcherWithContext(w Watcher) WatcherWithContext {

if w, ok := w.(WatcherWithContext); ok {

return w

}

return watcherWrapper{

parent: w,

}

}

type watcherWrapper struct {

parent Watcher

}

to use pointer as well.

soltysh · 2025-02-07T18:54:40Z

staging/src/k8s.io/apimachinery/pkg/api/meta/interfaces.go

+// unique across groups.
+type RESTMapperWithContext interface {
+	// KindFor takes a partial resource and returns the single match.  Returns an error if there are multiple matches
+	KindForWithContext(ctx context.Context, resource schema.GroupVersionResource) (schema.GroupVersionKind, error)


I will admit I'm not super thrilled by the lengthiness of this, but I'm always for consistency. So 👍 for *WithContext.

soltysh · 2025-02-07T18:55:44Z

staging/src/k8s.io/apimachinery/pkg/api/meta/multirestmapper.go

+	_ ResettableRESTMapper            = MultiRESTMapper{}
+	_ fmt.Stringer                    = MultiRESTMapper{}
+	_ ResettableRESTMapperWithContext = MultiRESTMapperWithContext{}
+	_ fmt.Stringer                    = MultiRESTMapperWithContext{}


soltysh · 2025-02-07T18:57:24Z

staging/src/k8s.io/apimachinery/pkg/api/meta/priority.go

@@ -30,11 +31,14 @@ const (
 )

 var (
-	_ ResettableRESTMapper = PriorityRESTMapper{}
+	_ ResettableRESTMapper            = PriorityRESTMapper{}
+	_ ResettableRESTMapperWithContext = PriorityRESTMapperWithContext{}


Probably also worth adding fmt.Stringer like you did with MultiRESTMapper above.

Note that I decided to keep using PriorityRESTMapperWithContext as value: that's because ToPriorityRESTMapperWithContext returns the actual struct, not an interface, and the struct is small enough that passing it by value should be better, in particular because all callers immediately discard it again.

soltysh · 2025-02-07T18:58:10Z

staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go

@@ -72,6 +73,7 @@ func (m *DefaultRESTMapper) String() string {
 }

 var _ RESTMapper = &DefaultRESTMapper{}
+var _ RESTMapperWithContext = &DefaultRESTMapper{}


Ditto about fmt.Stringer

soltysh · 2025-02-07T19:09:21Z

staging/src/k8s.io/apimachinery/pkg/api/meta/interfaces.go

@@ -109,7 +111,7 @@ type RESTMapping struct {
 // to API groups. In other words, kinds and resources should not be assumed to be
 // unique across groups.
 //
-// TODO: split into sub-interfaces
+// Deprecated: use RESTMapperWithContext instead.


This applies to other files as well. I remember we agreed NOT to deprecate these, but rather put a message like:

// Contextual logging: RESTMapperWithContext should be used instead of RESTMapper in code which supports contextual logging.

That was for functions where the original version is functionally sufficient for its intended purpose.

Here we have a case of an interface that looks like it works locally (no context) but under the hood often must call blocking functions which really should be called with a context if the user of this interface has one. I think this is a case where we should nudge the ecosystem towards updating their code by declaring the old functions as deprecated.

I wasn't entirely sure whether this should return a value or a pointer to satisfy the interface. Both works, so I benchmarked it elsewhere (REST mapper). Mem allocs are the same (one alloc/call), but returning a value is 10% slower when calling one method. What I then benchmarked is whether pointer vs value receiver in the wrapper makes a difference. Converting from value receiver (what I had before) to pointer receiver reduced call overhead by 6%. That's because with a value receiver, Go has to auto-generate a variant with pointer receiver and calls the value receiver through that. That can be seen in a debugger (call stack) and when setting breakpoints: (dlv) b restMapperWrapper.KindForWithContext Command failed: Location "restMapperWrapper.KindForWithContext" ambiguous: k8s.io/apimachinery/pkg/api/meta.restMapperWrapper.KindForWithContext, k8s.io/apimachinery/pkg/api/meta.(*restMapperWrapper).KindForWithContext… Conventional wisdom is to define types with value receiver because those can be called also on unmutable instances, making them more flexible. But for types which will only ever be used via a pointer, I think pointer receiver is better for the reasons above (small performance difference, easier to debug).

The main purpose is to replace context.TODO with a context provided by the caller. A secondary purpose is to enable contextual logging. Modifying the existing interfaces and APIs would have a big impact on the ecosystem. This is a no-go. Instead, the following approach was taken: - All interfaces get duplicated in a *WithContext variant where the methods also have a *WithContext suffix and the ctx parameter. All methods are treated this way except for obvious local get methods (like RESTClient) because it cannot be ruled out entirely that some implementation may need a context. - Implementations of these interfaces implement both method variants which is possible because the method names are different. The old methods are implemented as thin wrappers around the updated code which is now the body of the new methods or shared helpers. In some cases there is additional overhead (type checks, potentially additional allocations) when using the old methods. - To*WithContext helpers bridge from the old to the new interfaces. They try a type cast first. Because the in-tree implementations implement both, they can be used directly. For other implementations wrappers are used. - All old APIs and interfaces are marked as deprecated. There is no intent to ever remove them, but consumers should be made aware that there are now better alternatives. Implementations also get marked this way even if nothing ever calls them directly because it shows which code, at least theoretically, could get removed. - Existing unit tests do not get updated to the new APIs. This gives us unit test coverage of the old and new API because the old APIs call the new ones. - In-tree consumers will be updated in follow-up PRs. This is likely to be a longer process. Because of the deprecation comment, `hack/golangci-lint.sh -n` can be used to find code which needs to be updated.

k8s-ci-robot · 2025-02-13T11:30:11Z

@pohly: 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-linter-hints	`74102a6`	link	false	`/test pull-kubernetes-linter-hints`

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-sigs/prow repository. I understand the commands that are listed here.

The Lister and Watcher interfaces only supported methods without context, but were typically implemented with client-go API calls which need a context. New interfaces get added using the same approach as in kubernetes#129109.

k8s-triage-robot · 2025-05-14T12:37:25Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

pohly · 2025-05-15T11:42:46Z

/remove-lifecycle stale

harche · 2025-05-30T13:57:14Z

LGTM from my side, but @soltysh do the changes look okay to you?

k8s-ci-robot requested review from ardaguclu and sttts December 6, 2024 17:15

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 6, 2024

pohly commented Dec 6, 2024

View reviewed changes

pohly force-pushed the log-client-go-restmapper-discovery branch from 89a610e to 7af7a6a Compare December 9, 2024 07:11

ardaguclu reviewed Dec 9, 2024

View reviewed changes

pohly mentioned this pull request Dec 9, 2024

client-go/tools/cache: add APIs with context parameter #126387

Merged

pohly force-pushed the log-client-go-restmapper-discovery branch from 7af7a6a to 71efc9a Compare December 9, 2024 17:05

pohly mentioned this pull request Dec 9, 2024

WIP: client-go: finish context support #129125

Open

pohly force-pushed the log-client-go-restmapper-discovery branch 2 times, most recently from 638a635 to edb084b Compare December 10, 2024 10:27

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 Dec 10, 2024

pohly force-pushed the log-client-go-restmapper-discovery branch from edb084b to 3366e33 Compare December 12, 2024 14:17

pohly added a commit to pohly/kubernetes that referenced this pull request Dec 12, 2024

Merge kubernetes#129109.

9c458ce

k8s-ci-robot added do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. area/code-generation area/dependency Issues or PRs related to dependency changes area/test labels Dec 12, 2024

pohly added a commit to pohly/kubernetes that referenced this pull request Jan 20, 2025

Merge kubernetes#129109.

8f7ad14

pohly added a commit to pohly/kubernetes that referenced this pull request Jan 22, 2025

Merge kubernetes#129109.

4dbaf88

pohly added a commit to pohly/kubernetes that referenced this pull request Jan 28, 2025

Merge kubernetes#129109.

2d546df

pohly force-pushed the log-client-go-restmapper-discovery branch from 21e9af1 to 852983a Compare January 30, 2025 11:01

pohly added a commit to pohly/kubernetes that referenced this pull request Feb 7, 2025

Merge kubernetes#129109.

ef9b650

soltysh reviewed Feb 7, 2025

View reviewed changes

hashim21223445 approved these changes Feb 9, 2025

View reviewed changes

pohly added 2 commits February 13, 2025 12:00

pohly force-pushed the log-client-go-restmapper-discovery branch from 852983a to 74102a6 Compare February 13, 2025 11:09

pohly added a commit to pohly/kubernetes that referenced this pull request Apr 16, 2025

Merge kubernetes#129109.

acf0a3b

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 14, 2025

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 15, 2025

manusa mentioned this pull request May 29, 2025

feat(auth): Authorize user from custom SSE header containers/kubernetes-mcp-server#96

Merged

manusa mentioned this pull request Jul 2, 2025

Accept standard oauth header by keeping the current header containers/kubernetes-mcp-server#158

Merged

	func ToWatcherWithContext(w Watcher) WatcherWithContext {
	if w, ok := w.(WatcherWithContext); ok {
	return w
	}
	return watcherWrapper{
	parent: w,
	}
	}

	type watcherWrapper struct {
	parent Watcher
	}

restmapper + discovery: add context-aware APIs #129109

Are you sure you want to change the base?

restmapper + discovery: add context-aware APIs #129109

Uh oh!

Conversation

pohly commented Dec 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

k8s-ci-robot commented Dec 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented Dec 9, 2024

Uh oh!

Jefftree commented Dec 10, 2024

Uh oh!

k8s-ci-robot commented Jan 30, 2025

Uh oh!

pohly commented Jan 30, 2025

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Feb 13, 2025

Uh oh!

k8s-triage-robot commented May 14, 2025

Uh oh!

pohly commented May 15, 2025

Uh oh!

harche commented May 30, 2025

Uh oh!

Uh oh!

pohly commented Dec 6, 2024 •

edited

Loading