Set singular names for core types to pass to discovery #113542

ardaguclu · 2022-11-02T11:03:05Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Core types don't set their singular names for discovery endpoint(as opposed to CRDs).
This results in nondeterministic behavior when there is a CRD whose shortname is
one of core types singular name format. Thereby, when user requests resource via this short name,
it becomes unpredictable(because it is determined by the order of which one is requested first).

This PR adds singular names for all core types. As a result, core types will
have always higher precedence when their singular name format is requested
regardless of request order.

Which issue(s) this PR fixes:

Fixes #113227

Special notes for your reviewer:

In terms of backwards compatibility, this change has negative impact only on the use cases where people currently are expecting CRDs via singular form of core types. Isn't this already extreme case and incorrect expectation?

Does this PR introduce a user-facing change?

None

ardaguclu · 2022-11-02T12:51:11Z

test/cmd/discovery.sh

+  set +o errexit
+}
+
+run_assert_singular_name_tests() {


This test is not failing on master. I added it to just prevent future regressions.

ardaguclu · 2022-11-02T12:52:04Z

test/cmd/discovery.sh

+  kube::test::if_has_string "${output_message}" "test-crd-example"
+
+  # test that get pod returns v1/pod instead crd
+  output_message=$(kubectl get pod)


This test should fail on master without this patch.

ardaguclu · 2022-11-02T13:43:25Z

current failing tests don't seem to be related to the changes;
/retest

just to be sure rerun integration tests;
/test pull-kubernetes-integration

ardaguclu · 2022-11-02T17:55:34Z

/retest

ardaguclu · 2022-11-03T08:11:00Z

@deads2k @liggitt could you please take a look at this?. Thanks.

liggitt · 2022-11-03T13:48:00Z

~all storage implementations already set a DefaultQualifiedResource field to the plural version of their resource... I think a peer field SingularQualifiedResource would be a clearer place to set this, making the SingularResource getter be implemented by the Store type

that field could also be validated in Store#CompleteWithOptions

must not be empty
group must match DefaultQualifiedResource

pkg/registry/admissionregistration/mutatingwebhookconfiguration/storage/storage.go

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go

pkg/registry/core/node/storage/storage.go

liggitt

I think we also want an integration test that enables all built-in resources and ensures we set singular names for them which match lower(kind)

Something like this:

diff --git a/test/integration/apiserver/discovery/discovery_test.go b/test/integration/apiserver/discovery/discovery_test.go
index c6b7caaa477..1906f9aa552 100644
--- a/test/integration/apiserver/discovery/discovery_test.go
+++ b/test/integration/apiserver/discovery/discovery_test.go
@@ -27,6 +27,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/require"
+
 	apidiscoveryv2beta1 "k8s.io/api/apidiscovery/v2beta1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensions "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
@@ -690,6 +691,34 @@ func TestGroupPriorty(t *testing.T) {
 	})
 }
 
+func TestSingularNames(t *testing.T) {
+	server := kubeapiservertesting.StartTestServerOrDie(t, nil, []string{"--runtime-config=api/all=true"}, framework.SharedEtcd())
+	t.Cleanup(server.TearDownFn)
+
+	kubeClientSet, err := kubernetes.NewForConfig(server.ClientConfig)
+	require.NoError(t, err)
+
+	_, resources, err := kubeClientSet.Discovery().ServerGroupsAndResources()
+	require.NoError(t, err)
+
+	for _, rr := range resources {
+		for _, r := range rr.APIResources {
+			if strings.Contains(r.Name, "/") {
+				continue
+			}
+			if r.SingularName == "" {
+				t.Errorf("missing singularName for resource %q in %q", r.Name, rr.GroupVersion)
+				continue
+			}
+			if r.SingularName != strings.ToLower(r.Kind) {
+				t.Errorf("expected singularName for resource %q in %q to be %q, got %q", r.Name, rr.GroupVersion, strings.ToLower(r.Kind), r.SingularName)
+				continue
+			}
+		}
+	}
+
+}
+
 func makeCRDSpec(group string, kind string, namespaced bool, versions []string, categories ...string) apiextensionsv1.CustomResourceDefinitionSpec {
 	scope := apiextensionsv1.NamespaceScoped
 	if !namespaced {

pkg/registry/core/pod/storage/storage.go

liggitt · 2022-11-18T15:53:52Z

I think we also want an integration test that enables all built-in resources and ensures we set singular names for them which match lower(kind)

@deads2k does that restriction for built-in types match your expectation?

ardaguclu · 2022-11-18T16:03:25Z

Reminder to myself: assure that aggregated discovery populates singular names.

ardaguclu · 2022-11-21T07:53:33Z

Failure seems unrelated to this change;

/test pull-kubernetes-e2e-kind-ipv6

liggitt · 2022-11-21T16:05:43Z

the registry and integration test changes lgtm, would like a second from @deads2k

will defer to @soltysh or @seans3 on the test-cmd test

ardaguclu · 2022-12-15T15:06:25Z

@deads2k would you mind taking a look at this one?. Thanks.

sftim · 2022-12-20T15:59:44Z

Does this PR introduce a user-facing change?

I think it does, even though you can only directly observe it via the API and not via tools. The user visible change is that the nondeterministic behavior:

This results in nondeterministic behavior when there is a CRD whose shortname is
one of core types singular name format. Thereby, when user requests resource via this short name,
it becomes unpredictable(because it is determined by the order of which one is requested first).

becomes deterministic.

deads2k · 2022-12-20T20:37:47Z

pkg/registry/core/namespace/storage/storage.go

@@ -94,6 +95,12 @@ func (r *REST) NamespaceScoped() bool {
 	return r.store.NamespaceScoped()
 }

+var _ rest.SingularNameProvider = &REST{}
+
+func (r *REST) GetSingularName() string {


this stands out. Why does this one need it?

because it doesn't inline r.store and inherit the default implementation (see all other standard rest.Storage implementations forwarding to r.store in this file)

it does not inherit default store implementation as said ^

kubernetes/pkg/registry/core/namespace/storage/storage.go

Line 44 in 43a889f

type REST struct {

deads2k · 2022-12-20T21:06:43Z

staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go

@@ -1320,6 +1325,12 @@ func (e *Store) CompleteWithOptions(options *generic.StoreOptions) error {
 	if e.DefaultQualifiedResource.Empty() {
 		return fmt.Errorf("store %#v must have a non-empty qualified resource", e)
 	}
+	if e.SingularQualifiedResource.Empty() {


deads2k · 2022-12-20T21:07:50Z

namespace storage stood out. lgtm once I know why that needed a special case.

deads2k · 2023-01-03T16:22:10Z

/lgtm
/approve

k8s-ci-robot · 2023-01-03T16:22:16Z

LGTM label has been added.

Git tree hash: 3edda9b27f6880059cbb9d46d884a8d3f6744d6b

k8s-ci-robot · 2023-01-03T16:22:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, deads2k

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:

~~pkg/registry/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]
~~staging/src/k8s.io/sample-apiserver/OWNERS~~ [deads2k]
~~test/cmd/OWNERS~~ [ardaguclu,deads2k]
~~test/integration/apiserver/OWNERS~~ [deads2k]

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

nabokihms · 2023-03-05T11:18:00Z

pkg/registry/authentication/selfsubjectreview/rest.go

+var _ rest.SingularNameProvider = &REST{}
+
+func (r *REST) GetSingularName() string {
+	return "selfsubjectrulesreview"


@ardaguclu Hello, it seems like a bug, but I don't know why the tests are passing. I will fix this on promoting selfsubjectreviews to beta, jfyi.

The name is selfsubjectreview.

k8s-ci-robot requested review from apelisse and caesarxuchao November 2, 2022 11:04

ardaguclu changed the title ~~WIP: Fix shortname disperancy~~ WIP: Set singular names for core types to pass discovery Nov 2, 2022

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 Nov 2, 2022

ardaguclu changed the title ~~WIP: Set singular names for core types to pass discovery~~ WIP: Set singular names for core types to pass to discovery Nov 2, 2022

ardaguclu commented Nov 2, 2022

View reviewed changes

ardaguclu changed the title ~~WIP: Set singular names for core types to pass to discovery~~ Set singular names for core types to pass to discovery Nov 2, 2022

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 Nov 2, 2022

liggitt reviewed Nov 3, 2022

View reviewed changes

pkg/registry/admissionregistration/mutatingwebhookconfiguration/storage/storage.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 3, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/endpoints/installer.go Outdated Show resolved Hide resolved

minor integration test fixes and more singular resource

1f54f61

ardaguclu force-pushed the fix-shortname-disperancy branch from e676db1 to 1f54f61 Compare November 18, 2022 10:12

ardaguclu changed the title ~~WIP: Set singular names for core types to pass to discovery~~ Set singular names for core types to pass to discovery Nov 18, 2022

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 Nov 18, 2022

liggitt reviewed Nov 18, 2022

View reviewed changes

pkg/registry/core/node/storage/storage.go Outdated Show resolved Hide resolved

liggitt reviewed Nov 18, 2022

View reviewed changes

pkg/registry/core/pod/storage/storage.go Outdated Show resolved Hide resolved

Add integration test to test singularnames for all resources

43a889f

deads2k reviewed Dec 20, 2022

View reviewed changes

k8s-ci-robot assigned deads2k Jan 3, 2023

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

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

k8s-ci-robot merged commit 12c71fd into kubernetes:master Jan 3, 2023

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

ardaguclu deleted the fix-shortname-disperancy branch January 3, 2023 17:38

nabokihms reviewed Mar 5, 2023

View reviewed changes

nabokihms mentioned this pull request Mar 5, 2023

KEP-3325: Promote SelfSubjectReview to Beta #116274

Merged

irbekrm mentioned this pull request May 5, 2023

Bump k8s.io dependencies cert-manager/cert-manager#5961

Merged

ardaguclu mentioned this pull request May 10, 2023

WRKLDS-700: bump(k8s) to v0.27.1 openshift/run-once-duration-override#19

Merged

RainbowMango mentioned this pull request Jun 29, 2023

Bump Kubernetes dependencies to v1.27.3 karmada-io/karmada#3730

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set singular names for core types to pass to discovery #113542

Set singular names for core types to pass to discovery #113542

ardaguclu commented Nov 2, 2022 •

edited

ardaguclu Nov 2, 2022

ardaguclu Nov 2, 2022

ardaguclu commented Nov 2, 2022

ardaguclu commented Nov 2, 2022

ardaguclu commented Nov 3, 2022

liggitt commented Nov 3, 2022 •

edited

liggitt left a comment

liggitt commented Nov 18, 2022

ardaguclu commented Nov 18, 2022

ardaguclu commented Nov 21, 2022

liggitt commented Nov 21, 2022

ardaguclu commented Dec 15, 2022

sftim commented Dec 20, 2022 •

edited

deads2k Dec 20, 2022

liggitt Dec 20, 2022 •

edited

ardaguclu Dec 21, 2022

deads2k Dec 20, 2022

deads2k commented Dec 20, 2022

deads2k commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

nabokihms Mar 5, 2023

nabokihms Mar 5, 2023

Set singular names for core types to pass to discovery #113542

Set singular names for core types to pass to discovery #113542

Conversation

ardaguclu commented Nov 2, 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?

ardaguclu Nov 2, 2022

Choose a reason for hiding this comment

ardaguclu Nov 2, 2022

Choose a reason for hiding this comment

ardaguclu commented Nov 2, 2022

ardaguclu commented Nov 2, 2022

ardaguclu commented Nov 3, 2022

liggitt commented Nov 3, 2022 • edited

liggitt left a comment

Choose a reason for hiding this comment

liggitt commented Nov 18, 2022

ardaguclu commented Nov 18, 2022

ardaguclu commented Nov 21, 2022

liggitt commented Nov 21, 2022

ardaguclu commented Dec 15, 2022

sftim commented Dec 20, 2022 • edited

deads2k Dec 20, 2022

Choose a reason for hiding this comment

liggitt Dec 20, 2022 • edited

Choose a reason for hiding this comment

ardaguclu Dec 21, 2022

Choose a reason for hiding this comment

deads2k Dec 20, 2022

Choose a reason for hiding this comment

deads2k commented Dec 20, 2022

deads2k commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

nabokihms Mar 5, 2023

Choose a reason for hiding this comment

nabokihms Mar 5, 2023

Choose a reason for hiding this comment

ardaguclu commented Nov 2, 2022 •

edited

liggitt commented Nov 3, 2022 •

edited

sftim commented Dec 20, 2022 •

edited

liggitt Dec 20, 2022 •

edited