chore(apimachinery): deprecate meta.UnsafeGuessKindToResource #119924
Conversation
k8s-ci-robot
added
release-note
|
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Sun Aug 13 10:37:24 UTC 2023. |
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
sig/api-machinery
|
Hi @pmalek. 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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
sig/instrumentation
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| @@ -62,6 +64,8 @@ type DefaultRESTMapper struct { | |||
| kindToScope map[schema.GroupVersionKind]RESTScope | |||
| singularToPlural map[schema.GroupVersionResource]schema.GroupVersionResource | |||
| pluralToSingular map[schema.GroupVersionResource]schema.GroupVersionResource | |||
|
|
|||
| namer namer.Namer | |||
There was a problem hiding this comment.
ACK. What's the fmt tool to check this and how can I trigger it? gofmt doesn't report anything
k8s-ci-robot
added
the
area/dependency
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pmalek 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 |
k8s-ci-robot
added
area/kubectl
sig/cli
|
/test pull-kubernetes-unit |
|
/retest-required |
|
I've checked locally an all UTs pass but there are some build related errors that I'm not sure what they are about. Can someone guide me how to approach this? https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/119924/pull-kubernetes-unit/1691019004237320192 |
| @@ -108,7 +108,7 @@ func NewDiscoveryRESTMapper(groupResources []*APIGroupResources) meta.RESTMapper | |||
| singular := gv.WithResource(resource.SingularName) | |||
| // this is for legacy resources and servers which don't list singular forms. For those we must still guess. | |||
| if len(resource.SingularName) == 0 { | |||
| _, singular = meta.UnsafeGuessKindToResource(gv.WithKind(resource.Kind)) | |||
| singular = gv.WithResource(strings.ToLower(resource.Kind)) | |||
| @@ -24,6 +24,8 @@ import ( | |||
|
|
|||
| "k8s.io/apimachinery/pkg/runtime" | |||
| "k8s.io/apimachinery/pkg/runtime/schema" | |||
| "k8s.io/gengo/namer" | |||
There was a problem hiding this comment.
gengo is for code-generation prior to build, not something we want to rely on at runtime
There was a problem hiding this comment.
I see. What suggestion do you have on this one? namer ability to provide plural names was suggested in #110053 (comment) as a replacement for UnsafeGuessKindToResource. What other options do we have?
| @@ -93,11 +97,15 @@ func NewDefaultRESTMapper(defaultGroupVersions []schema.GroupVersion) *DefaultRE | |||
| defaultGroupVersions: defaultGroupVersions, | |||
| singularToPlural: singularToPlural, | |||
| pluralToSingular: pluralToSingular, | |||
| namer: namer.NewAllLowercasePluralNamer(nil), | |||
There was a problem hiding this comment.
I don't really see how embedding a namer whose exceptions can't be overridden is an improvement on the current code
There was a problem hiding this comment.
Pending response on the alternative for namer one question pops in my head: what improvement would you possibly see here? Make NewDefaultRESTMapper a variadic function accepting exceptions or add second "constructor" which allows to specify the exceptions keeping this intact (as is now)?
| @@ -123,6 +131,9 @@ var unpluralizedSuffixes = []string{ | |||
| // UnsafeGuessKindToResource converts Kind to a resource name. | |||
| // Broken. This method only "sort of" works when used outside of this package. It assumes that Kinds and Resources match | |||
| // and they aren't guaranteed to do so. | |||
| // | |||
| // Deprecated: This is and may yield unwanted and unexpected results. | |||
| // Use k8s.io/gengo/namer's NewAllLowercasePluralNamer() instead. | |||
There was a problem hiding this comment.
same comment about not depending on gengo at runtime
| @@ -228,6 +231,9 @@ func NewObjectTracker(scheme ObjectScheme, decoder runtime.Decoder) ObjectTracke | |||
| decoder: decoder, | |||
| objects: make(map[schema.GroupVersionResource]map[types.NamespacedName]runtime.Object), | |||
| watchers: make(map[schema.GroupVersionResource]map[string][]*watch.RaceFreeFakeWatcher), | |||
| namer: namer.NewAllLowercasePluralNamer(map[string]string{ | |||
| "Endpoints": "endpoints", | |||
There was a problem hiding this comment.
I don't really see how this is an improvement on the current state... this is taking the existing unsafe kind → resource code and replicating the exceptions in multiple other places.
To actually fix this, the kind → resource transformation has to be configurable in the fake clients / testing fixtures
There was a problem hiding this comment.
Got it. I'd assume that you mean all the client "constructors" that utilize NewObjectTracker e.g.
What would you suggest we change/add to support said configurability? We can't introduce a breaking change (even if we wanted NewSimpleClientset already has a variadic parameter set objects). Do we want to add something like NewSimpleClientsetWithExpections? 🤔
There was a problem hiding this comment.
yeah, I think NewObjectTracker and NewSimpleClientset should have variants that allow providing custom kind → resource mappings
There was a problem hiding this comment.
can give ourselves WithOptions signatures to only have to replumb once, with something like
// NewSimpleClientset returns a clientset that will respond with the provided objects.
// It's backed by a very simple object tracker that processes creates, updates and deletions as-is,
// without applying any validations and/or defaults. It shouldn't be considered a replacement
// for a real clientset and is mostly useful in simple unit tests.
// Equivalent to NewSimpleClientsetWithOptions(testing.Options{}, objects...)
func NewSimpleClientset(objects ...runtime.Object) *Clientset {
return NewSimpleClientsetWithOptions(testing.Options{}, objects...)
}
// NewSimpleClientset returns a clientset that will respond with the provided objects.
// It's backed by a very simple object tracker that processes creates, updates and deletions as-is,
// without applying any validations and/or defaults. It shouldn't be considered a replacement
// for a real clientset and is mostly useful in simple unit tests.
func NewSimpleClientsetWithOptions(options testing.Options, objects ...runtime.Object) *Clientset {
o := testing.NewObjectTrackerWithOptions(options, scheme, codecs.UniversalDecoder())and
diff --git a/staging/src/k8s.io/client-go/testing/fixture.go b/staging/src/k8s.io/client-go/testing/fixture.go
index 396840670fd..38ecf9745a6 100644
--- a/staging/src/k8s.io/client-go/testing/fixture.go
+++ b/staging/src/k8s.io/client-go/testing/fixture.go
@@ -216,18 +216,34 @@ type tracker struct {
// watchers' channel. Note that too many unhandled events (currently 100,
// see apimachinery/pkg/watch.DefaultChanSize) will cause a panic.
watchers map[schema.GroupVersionResource]map[string][]*watch.RaceFreeFakeWatcher
+
+ // ...
+ kindToResource map[schema.GroupVersionKind]schema.GroupVersionResource
}
var _ ObjectTracker = &tracker{}
+// ...
+type Options struct {
+ // KindToResource allows customizing the mapping for a given kind.
+ // If unset, UnsafeGuessKindToResource is used to attempt to derive the resource for a given kind.
+ KindToResource map[schema.GroupVersionKind]schema.GroupVersionResource
+}
+
// NewObjectTracker returns an ObjectTracker that can be used to keep track
// of objects for the fake clientset. Mostly useful for unit tests.
+// Equivalent to NewObjectTrackerWithOptions(Options{}, scheme, decoder)
func NewObjectTracker(scheme ObjectScheme, decoder runtime.Decoder) ObjectTracker {
+ return NewObjectTrackerWithOptions(Options{}, scheme, decoder)
+}
+
+func NewObjectTrackerWithOptions(options Options, scheme ObjectScheme, decoder runtime.Decoder) ObjectTracker {
return &tracker{
scheme: scheme,
decoder: decoder,
objects: make(map[schema.GroupVersionResource]map[types.NamespacedName]runtime.Object),
watchers: make(map[schema.GroupVersionResource]map[string][]*watch.RaceFreeFakeWatcher),
+
+ kindToResource: options.KindToResource,
}
}
@@ -333,6 +349,8 @@ func (t *tracker) Add(obj runtime.Object) error {
return fmt.Errorf("no registered kinds for %v", obj)
}
for _, gvk := range gvks {
+ gvr, ok := t.kindToResource[gvk]
+ if !ok {
// NOTE: UnsafeGuessKindToResource is a heuristic and default match. The
// actual registration in apiserver can specify arbitrary route for a
// gvk. If a test uses such objects, it cannot preset the tracker with
// objects via Add(). Instead, it should trigger the Create() function
// of the tracker, where an arbitrary gvr can be specified.
- gvr, _ := meta.UnsafeGuessKindToResource(gvk)
+ gvr, _ = meta.UnsafeGuessKindToResource(gvk)
// Resource doesn't have the concept of "__internal" version, just set it to "".
if gvr.Version == runtime.APIVersionInternal {
gvr.Version = ""
}
+ }
err := t.add(gvr, obj, objMeta.GetNamespace(), false)
if err != nil {| @@ -278,12 +280,17 @@ func TestPatchWithMissingObject(t *testing.T) { | |||
| func TestGetWithExactMatch(t *testing.T) { | |||
| scheme := runtime.NewScheme() | |||
| codecs := serializer.NewCodecFactory(scheme) | |||
| namer := namer.NewAllLowercasePluralNamer(nil) | |||
There was a problem hiding this comment.
using a namer without configurable exceptions just bakes in the special cases at another layer... same comment applies everywhere we're using NewAllLowercasePluralNamer(nil)
|
|
||
| plural, singular := UnsafeGuessKindToResource(version.WithKind(testCase.Kind)) |
There was a problem hiding this comment.
revert the changes to this test... this is exercising UnsafeGuessKindToResource which we don't want to lose coverage of
There was a problem hiding this comment.
Done, reverted this but I retained the change using the subtests and adding the Endpoints test case.
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
…dd Endpoints testcase
|
@pmalek: The following tests failed, say
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. |
k8s-ci-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
dims
added
the
lifecycle/rotten
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. In response to this:
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. |
|
/reopen |
|
@pmalek: Reopened this PR. In response to this:
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
removed
the
lifecycle/rotten
|
/cc Needed for #123938 |
|
Hi @pohly, This one has gotten a little bit stale because I lacked time to work on it but I might find some time these days. It's actually good that other people will also benefit from this being fixed. What's the time horizon for #123938 to get merged so that I roughly can estimate if I'd find some time to work on this |
|
I'm using a workaround in #123938 because it should better get merged before test freeze next week. |
What type of PR is this?
/kind cleanup
/kind deprecation
What this PR does / why we need it:
This PR marks apimachinery's
meta.UnsafeGuessKindToResourceas deprecated given its unreliability and heuristic nature.Which issue(s) this PR fixes:
Related to a discussion in #110053
Special notes for your reviewer:
Should I align all
k8s.io/gengoversions to be the same as before this PR and not bump it or is bumping it fromk8s.io/gengo v0.0.0-20220902162205-c0856e24416dtok8s.io/gengo v0.0.0-20230306165830-ab3349d207d4OK? I gather that I'd need to rungo mod vendorthen as well because running tests inpkg/kubeletcomplains about this.Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: