Repository: kubernetes/kubernetes (language: go)

Issue:
API emulation versioning honors cohabitating resources
#### What type of PR is this?

/kind bug
/kind regression
/sig api-machinery


#### What this PR does / why we need it:
API emulation versioning seems break Cohabitating Resources overwriting. it seems a regression introduced in https://github.com/kubernetes/kubernetes/commit/403301bfdf2c7312591077827abd2e72f445a53a#diff-df88db6cbb681f73f6ac0a2979ce70b6d8f812e05c5064de760cd7d90d9b1f1bR248



#### Which issue(s) this PR fixes:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #127232

#### Special notes for your reviewer:

#### Does this PR introduce a user-facing change?
```release-note
Fixes a 1.31 regression with API emulation versioning honors cohabitating resources
```

#### Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

<!--
This section can be blank if this pull request does not require a release note.

When adding links which point to resources within git repositories, like
KEPs or supporting documentation, please reference a specific commit and avoid
linking directly to the master branch. This ensures that links reference a
specific point in time, rather than a document that may change over time.

See here for guidance on getting permanent links to files: https://help.github.com/en/articles/getting-permanent-links-to-files

Please use the following format for linking documentation:
- [KEP]: <link>
- [Usage]: <link>
- [Other doc]: <link>
-->
```docs

```


Discussion / hints:
/cc @aaron-prindle 
/triage accepted
@seans3: GitHub didn't allow me to request PR reviews from the following users: aaron-prindle.

Note that only [kubernetes members](https://github.com/orgs/kubernetes/people) and repo collaborators can review this PR, and authors cannot review their own PRs.

<details>

In response to [this](https://github.com/kubernetes/kubernetes/pull/127239#issuecomment-2341930657):

>/cc @aaron-prindle 
>/triage accepted


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
@jpbetz If possible, would you please take a look? I will be really appreicated that.

What I want to know most is that does community assert this as a bug?  or `cohabitating resources` mechanism is not guaranteed in this case and I have to find another way to deal with that. I'm blocked on this issue to upgrading to 1.31 right now and if the answer is positive, I can do a local workaround (such as a templary patch) and move on until this PR is merged.
This looks like something that got missed in the Compatibility Version implementation work. This fix looks reasonable to me.

As a follow up, is there anything we can add to kubernetes/kubernetes (ideally a test) that would catch issues with this use case in the future?

/lgtm
/approve
LGTM label has been added.  <details>Git tree hash: 6b93ff17c9558ae4ba05ca37b605b37d0f7d3d80</details>
[APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes/kubernetes/pull/127239#issuecomment-2344672553" title="Approved">jpbetz</a>*, *<a href="https://github.com/kubernetes/kubernetes/pull/127239#" title="Author self-approved">xuzhenglun</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes%2Fkubernetes).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[staging/src/k8s.io/apiserver/OWNERS](https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/OWNERS)~~ [jpbetz]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
@jpbetz  

Should we cherry-pick this fix into v1.31 ? 

> As a follow up, is there anything we can add to kubernetes/kubernetes (ideally a test) that would catch issues with this use case in the future?

I'm glad to do so, do you have any suggestion to achieve this? I'm not sure if UT can cover this scenario, may be a e2e test is required?
> Should we cherry-pick this fix into v1.31 ?

It is a regression, which is one of the criteria for cherry-picks.  Some additional assurance that the fix works would be helpful.  sig-release will make the decision, but I'm okay with proposing it.  More evidence of test coverage will help make the case here.

Maybe a integration test somewhere in test/integration/apiserver?  It's relatively cheap to start an apiserver in integration tests with custom options/configuration (v.s. e2e, where the tests are build to be run against a running cluster). 

Fix patch:
diff --git a/staging/src/k8s.io/apiserver/pkg/apis/example2/install/install.go b/staging/src/k8s.io/apiserver/pkg/apis/example2/install/install.go
index 473895998f14c..276a24199a3ff 100644
--- a/staging/src/k8s.io/apiserver/pkg/apis/example2/install/install.go
+++ b/staging/src/k8s.io/apiserver/pkg/apis/example2/install/install.go
@@ -22,12 +22,14 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apiserver/pkg/apis/example"
+	"k8s.io/apiserver/pkg/apis/example2"
 	example2v1 "k8s.io/apiserver/pkg/apis/example2/v1"
 )
 
 // Install registers the API group and adds types to a scheme
 func Install(scheme *runtime.Scheme) {
 	utilruntime.Must(example.AddToScheme(scheme))
+	utilruntime.Must(example2.AddToScheme(scheme))
 	utilruntime.Must(example2v1.AddToScheme(scheme))
 	utilruntime.Must(scheme.SetVersionPriority(example2v1.SchemeGroupVersion))
 }


Test patch:
diff --git a/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go b/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go
index a12e865f05954..b8628f0506c9e 100644
--- a/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go
+++ b/staging/src/k8s.io/apiserver/pkg/server/storage/storage_factory_test.go
@@ -30,6 +30,8 @@ import (
 	"k8s.io/apiserver/pkg/apis/example"
 	exampleinstall "k8s.io/apiserver/pkg/apis/example/install"
 	examplev1 "k8s.io/apiserver/pkg/apis/example/v1"
+	"k8s.io/apiserver/pkg/apis/example2"
+	example2install "k8s.io/apiserver/pkg/apis/example2/install"
 	"k8s.io/apiserver/pkg/storage/storagebackend"
 	"k8s.io/apiserver/pkg/util/version"
 )
@@ -52,6 +54,7 @@ func init() {
 	)
 
 	exampleinstall.Install(scheme)
+	example2install.Install(scheme)
 }
 
 type fakeNegotiater struct {
@@ -107,28 +110,144 @@ func (n *fakeNegotiater) DecoderToVersion(serializer runtime.Decoder, gv runtime
 	return n.serializer
 }
 
-func TestConfigurableStorageFactory(t *testing.T) {
-	ns := &fakeNegotiater{types: []string{"test/test"}}
-	f := NewDefaultStorageFactory(storagebackend.Config{}, "test/test", ns, NewDefaultResourceEncodingConfig(scheme), NewResourceConfig(), nil)
-	f.AddCohabitatingResources(example.Resource("test"), schema.GroupResource{Resource: "test2", Group: "2"})
-	called := false
-	testEncoderChain := func(e runtime.Encoder) runtime.Encoder {
-		called = true
-		return e
-	}
-	f.AddSerializationChains(testEncoderChain, nil, example.Resource("test"))
-	f.SetEtcdLocation(example.Resource("*"), []string{"/server2"})
-	f.SetEtcdPrefix(example.Resource("test"), "/prefix_for_test")
+type newStorageCodecFn func(opts StorageCodecConfig) (codec runtime.Codec, encodeVersioner runtime.GroupVersioner, err error)
+type newStorageCodecRecorder struct {
+	opt StorageCodecConfig
+}
 
-	config, err := f.NewConfig(example.Resource("test"), nil)
-	if err != nil {
-		t.Fatal(err)
+func (c *newStorageCodecRecorder) Decorators(f newStorageCodecFn) newStorageCodecFn {
+	return func(opts StorageCodecConfig) (codec runtime.Codec, encodeVersioner runtime.GroupVersioner, err error) {
+		c.opt = opts
+		return f(opts)
 	}
-	if config.Prefix != "/prefix_for_test" || !reflect.DeepEqual(config.Transport.ServerList, []string{"/server2"}) {
-		t.Errorf("unexpected config %#v", config)
+}
+
+func TestConfigurableStorageFactory(t *testing.T) {
+	var emptyResource schema.GroupResource
+
+	testCases := []struct {
+		name                  string
+		enabledResources      []schema.GroupVersionResource
+		overrideResource      schema.GroupResource
+		cohabitatingResources []schema.GroupResource
+		resourceForConfig     schema.GroupResource
+		exampleForConfig      runtime.Object
+		wantStorageVersion    schema.GroupVersion
+	}{
+		{
+			name: "config resource is primary cohabitating resources",
+			enabledResources: []schema.GroupVersionResource{
+				example.Resource("replicasets").WithVersion("v1"),
+				example2.Resource("replicasets").WithVersion("v1"),
+			},
+			cohabitatingResources: []schema.GroupResource{example.Resource("replicasets"), example2.Resource("replicasets")},
+			resourceForConfig:     example.Resource("replicasets"),
+			exampleForConfig:      &example.ReplicaSet{},
+			wantStorageVersion:    schema.GroupVersion{Group: example.GroupName, Version: "v1"},
+		},
+		{
+			name: "config resource is secondary cohabitating resources",
+			enabledResources: []schema.GroupVersionResource{
+				example.Resource("replicasets").WithVersion("v1"),
+				example2.Resource("replicasets").WithVersion("v1"),
+			},
+			cohabitatingResources: []schema.GroupResource{example.Resource("replicasets"), example2.Resource("replicasets")},
+			resourceForConfig:     example2.Resource("replicasets"),
+			exampleForConfig:      &example.ReplicaSet{},
+			wantStorageVersion:    schema.GroupVersion{Group: example.GroupName, Version: "v1"},
+		},
+		{
+			name: "config resource is primary cohabitating resources and not enabled",
+			enabledResources: []schema.GroupVersionResource{
+				// example.Resource("replicasets").WithVersion("v1"), // <- disabled
+				example2.Resource("replicasets").WithVersion("v1"),
+			},
+			cohabitatingResources: []schema.GroupResource{example.Resource("replicasets"), example2.Resource("replicasets")},
+			resourceForConfig:     example.Resource("replicasets"),
+			exampleForConfig:      &example.ReplicaSet{},
+			wantStorageVersion:    schema.GroupVersion{Group: example2.GroupName, Version: "v1"},
+		},
+		{
+			name: "config resource is secondary cohabitating resources and not enabled",
+			enabledResources: []schema.GroupVersionResource{
+				example.Resource("replicasets").WithVersion("v1"),
+				// example2.Resource("replicasets").WithVersion("v1"),  // <- disabled
+			},
+			cohabitatingResources: []schema.GroupResource{example.Resource("replicasets"), example2.Resource("replicasets")},
+			resourceForConfig:     example2.Resource("replicasets"),
+			exampleForConfig:      &example.ReplicaSet{},
+			wantStorageVersion:    schema.GroupVersion{Group: example.GroupName, Version: "v1"},
+		},
+		{
+			name: "override config for one resource of group",
+			enabledResources: []schema.GroupVersionResource{
+				example.Resource("replicasets").WithVersion("v1"),
+				example2.Resource("replicasets").WithVersion("v1"),
+			},
+			overrideResource:   example.Resource("replicasets"),
+			resourceForConfig:  example.Resource("replicasets"),
+			exampleForConfig:   &example.ReplicaSet{},
+			wantStorageVersion: schema.GroupVersion{Group: example.GroupName, Version: "v1"},
+		},
+		{
+			name: "override config for all resource of group",
+			enabledResources: []schema.GroupVersionResource{
+				example.Resource("replicasets").WithVersion("v1"),
+				example2.Resource("replicasets").WithVersion("v1"),
+			},
+			cohabitatingResources: []schema.GroupResource{example.Resource("replicasets"), example2.Resource("replicasets")},
+			overrideResource:      example.Resource("*"),
+			resourceForConfig:     example.Resource("replicasets"),
+			exampleForConfig:      &example.ReplicaSet{},
+			wantStorageVersion:    schema.GroupVersion{Group: example.GroupName, Version: "v1"},
+		},
 	}
-	if !called {
-		t.Errorf("expected encoder chain to be called")
+
+	for _, test := range testCases {
+		t.Run(test.name, func(t *testing.T) {
+			ns := &fakeNegotiater{types: []string{"test/test"}}
+			resourceConfig := NewResourceConfig()
+			resourceConfig.EnableResources(test.enabledResources...)
+
+			f := NewDefaultStorageFactory(storagebackend.Config{}, "test/test", ns, NewDefaultResourceEncodingConfig(scheme), resourceConfig, nil)
+
+			encoderChainCalled := false
+			recorder := newStorageCodecRecorder{}
+
+			f.newStorageCodecFn = recorder.Decorators(f.newStorageCodecFn)
+			f.AddCohabitatingResources(test.cohabitatingResources...)
+
+			if test.overrideResource != emptyResource {
+				testEncoderChain := func(e runtime.Encoder) runtime.Encoder {
+					encoderChainCalled = true
+					return e
+				}
+
+				f.SetEtcdLocation(test.overrideResource, []string{"/server2"})
+				f.AddSerializationChains(testEncoderChain, nil, test.overrideResource)
+				f.SetEtcdPrefix(test.overrideResource, "/prefix_for_test")
+			}
+
+			config, err := f.NewConfig(test.resourceForConfig, test.exampleForConfig)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// check override resources config
+			if test.overrideResource != emptyResource {
+				if config.Prefix != "/prefix_for_test" || !reflect.DeepEqual(config.Transport.ServerList, []string{"/server2"}) {
+					t.Errorf("unexpected config %#v", config)
+				}
+				if !encoderChainCalled {
+					t.Errorf("expected encoder chain to be called")
+				}
+			}
+
+			// check cohabitating resources config
+			if recorder.opt.StorageVersion != test.wantStorageVersion {
+				t.Errorf("unexpected encoding version %#v, but expected %#v", recorder.opt.StorageVersion, test.wantStorageVersion)
+			}
+		})
 	}
 }