apiserver: Call .Decorator inside update func

[thockin]

This is causing a bug when upgrading from older releases to 1.23 because
of Service's maybe-too-clever default-on-read logic.

Service depends on .Decorator() to be called upon read, to
back-populate old saved objects which do not have .clusterIPs[] set.
This works on read, but the cache saves the pre-decorated type.

In 1.23, this code was refactored and it seems some edge-case handling
was inadvertently removed (I have not confirmed exactly what happened).

The simplest fix is this one - it will catch anyone who follows this
pattern. Alternately, we could patch this in Service registry, but if
anyone else follows this default-on-read pattern (and I know there are
some) they will also be susceptible to this sort of bug.

/kind bug
/kind regression

Fixes a regression in 1.23 where update requests to previously persisted `Service` objects that have not been modified since 1.19 can be rejected with an incorrect `spec.clusterIPs: Required value` error

[thockin]

cc @khenidak @aojea

[lavalamp]

I didn't expect default-on-read code to be in a decorator. I found

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

Line 128 in 1945829

store.Decorator = genericStore.defaultOnRead

which seems to do that, but this seems to be the type that has the bug, so it's not helping me change my mind. I'd really expect everything to be defaulted by a defaulting function?

@deads2k might recall something useful off the top of his head.

[liggitt]

there are two types of back-filling service does, iirc...

1. mechanical, stateless translations like {clusterIP:"foo",clusterIPs:nil} → {clusterIP:"foo", clusterIPs:["foo"]}
2. population of values that depend on server config, like dual stack enablement and preferred stack

for the first category, using the standard defaulting path makes them apply far more consistently (on read from etcd, from incoming requests, and after applying a patch from a mutating webhook)

for the second category, plumbing config into the standard defaulting path wasn't really workable, so I think that pushed services to use storage strategy Decorator

[thockin]

We wanted to handle cases like a client who doesn't understand clusterIPs seeming to clear it and that ending up in losing the 2nd IP in a dual-stack service. We have a million test cases that permute this in all directions (though apparently not enough, given the bug at hand)

[liggitt]

tl;dr:

• I'm really unsure about calling Decorator(existing) inside GuaranteedUpdate in the update path
• I think Service should collapse onto standard defaulting for anything stateless it can in the future
• I think Service should resolve the issue in 1.23 by calling r.defaultOnRead(oldSvc) at the top of REST#beginUpdate instead:

diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go
index d9d77ae6cf4..f470e39cf38 100644
--- a/pkg/registry/core/service/storage/storage.go
+++ b/pkg/registry/core/service/storage/storage.go
@@ -365,7 +365,10 @@ func (r *REST) beginCreate(ctx context.Context, obj runtime.Object, options *met
 
 func (r *REST) beginUpdate(ctx context.Context, obj, oldObj runtime.Object, options *metav1.UpdateOptions) (genericregistry.FinishFunc, error) {
        newSvc := obj.(*api.Service)
        oldSvc := oldObj.(*api.Service)
+
+       // make sure the existing object has all fields we expect to be defaulted set
+       r.defaultOnRead(oldSvc)
 
        // Fix up allocated values that the client may have not specified (for
        // idempotence).

---

Decorator() is documented as useful for populating values that aren't actually stored, before an object is returned from the API. It is currently called in the right places to accomplish that.

---

Observation A: Decorator() is not called in the following places internal logic has visibility to objects:

1. Create: on the new object before it is sent to admission, or passed to BeginCreate/PrepareForCreate/Validate
2. Update: inside the GuaranteedUpdate loop on the existing object before it is sent to admission, or used to derive the new object, or passed to BeginUpdate/PrepareForUpdate/ValidateUpdate
3. Update: inside the GuaranteedUpdate loop on the new object before it is sent to admission, or passed to BeginUpdate/PrepareForUpdate/ValidateUpdate
4. Delete calls: inside the GuaranteedUpdate loop on the existing object before it is sent to admission

Observation B: Decorator() is also not called on a new object in a create or update request before it is stored, only after (just before returning the response to the API user). That's consistent with its documented purpose of populating fields that aren't stored.

Observation C: Service uses Decorator() to apply defaults to the following fields (avoiding automatic defaulting of incoming requests so it can better tell what the user submitted in beginCreate/beginUpdate):

1. ClusterIPs (statelessly using normalizeClusterIPs)
2. IPFamilyPolicy and IPFamilies (statefully in defaultOnReadIPFamilies using server config)
3. to clear InternalTrafficPolicy for external names (statelessly)

Observation D: Service has logic in beginCreate to apply defaults to the following fields:

1. ClusterIPs (statelessly, using normalizeClusterIPs)
2. IPFamilyPolicy and IPFamilies (statefully in initIPFamilyFields using server config)

Observation E: Service has logic in beginUpdate to apply defaults to the following fields:

1. ClusterIPs (statefully, using normalizeClusterIPs passing in the existing object)
2. IPFamilyPolicy and IPFamilies (statefully in initIPFamilyFields using server config and the existing object)

---

Because of A2 (existing object is not decorated on update), E1 (new object defaults clusterIPs by copying from existing, rather than defaulting as [clusterIP]) and maybe E2 don't work properly.

---

This PR adds a Decorator() call to A2 to make the existing object seen by beginUpdate consistent with what is returned by a Get() API call.

It seems arbitrary to decorate earlier internally to fix up this one internal use case for BeginUpdate and leave A1, A3, and A4 undecorated and BeginCreate / Admission / Validation handling undecorated objects. Concretely, that means admission webhooks can't rely on clusterIPs being set in objects sent to them (new objects in create or update calls, existing object in delete calls).

It also seems incorrect to decorate objects in A1, A3, and A4 because that pushes decoration data into storage (directly in opposition to the "this is for setting unstored values" docs). But if we look at how Service is using Decorator(), it wants to persist the data it is setting there... in fact, it implements duplicate logic in BeginCreate/BeginUpdate to set the same fields so they'll be persisted.

It also seems incorrect to decorate existing and new object inconsistently (to change A2 and not change A3)... it doesn't happen to matter in the end for Service because of the duplicate logic that sets the same fields in BeginCreate/BeginUpdate, but for a type to set Decorator() which says it is for setting unstored values, then have those values show up only in the old object in BeginUpdate / ValidateUpdate seems... surprising to me.

I think to fix the immediate issue, service and pvc storage should apply their own defaultOnRead decoration to the old object at the top of beginUpdate.

[smarterclayton]

Decorators were definitely not intended to be used before update (and adding this would break the semantics of someone using a decorator). Basically, decorators are only for fields that cannot and should not be persisted, are not watchable, and should deliberately be changeable outside of storage (for whatever reason someone was using them).

Any watchable field should definitely be a pre update call, but we shouldn’t change the meaning of a decorator. We definitely should update doc to indicate what decorator cannot be used for.

I expected those resources to have their strategy set these during on before create and on before update.

EDIT: one more decorator use case - for synthetic fields that would be expensive to store at rest which can be calculated from fields stored at rest (which would be watchable because the inputs are only in the object)

[thockin]

I think Service should collapse onto standard defaulting for anything stateless it can in the future

No disagreement. I don't think clusterIPs can (because of old clients) and I
don't think IP families can. The other one was a SNAFU may be it can.

I think Service should resolve the issue in 1.23 by calling r.defaultOnRead(oldSvc) at the top of REST#beginUpdate instead:

Plausible. I'll try it.

Decorator() is documented as useful for populating values that aren't actually stored,

True. What I think Service really wants is a Modernize() hook, whose
semantics are explicitly "bring an existing valid object up to current norms".
This would specifically apply on the Get() path and to the "old" object on the
Update() path and probably the Delete() path.

I don't think it would apply on the Create() path - that's what the existing
mechanisms are for (and in all likelihood would call the same code).

WebHooks are an interesting one. There are 3 classes of fields: those the user
set, those the user did not set, and those the user did not set but took some
default. Do you want the webhook to know which fields were set by the user or
which fields were set by either the user or the defaults? I posit it's the
former, most of the time. If I want to change defaults in my own webhook, I
need to know the difference between "set by user" and "not set by user,
defaulted".

It's also worth noting that "Modernize" is pretty much what we need to "touch"
saved objects and bring them up to newer API versions, isn't it?

Anyway, I ACK that this is somewhat an abuse of Decorator(), but it was the
only way we could find to do what we needed. :)

[thockin]

@bswartz - you did the PVC equivalent of this, but it's not clear to me if you will have the same bugs (since this centers on Update())

[thockin]

pushed with alternate fix, which seems to work, too.

[aojea]

/test pull-kubernetes-integration

this keeps failing with timeouts, is probably not related

[bswartz]

@bswartz - you did the PVC equivalent of this, but it's not clear to me if you will have the same bugs (since this centers on Update())

I'll take a look at this.

[aojea]

@thockin test with reproducer, I've also verified that this fixes the problem, can you please incorporate it?

diff --git a/test/integration/service/upgrade_test.go b/test/integration/service/upgrade_test.go
new file mode 100644
index 00000000000..1df58674d2f
--- /dev/null
+++ b/test/integration/service/upgrade_test.go
@@ -0,0 +1,80 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package service
+
+import (
+       "context"
+       "testing"
+
+       v1 "k8s.io/api/core/v1"
+       metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+       "k8s.io/apimachinery/pkg/runtime"
+       "k8s.io/client-go/kubernetes"
+       kubeapiservertesting "k8s.io/kubernetes/cmd/kube-apiserver/app/testing"
+       "k8s.io/kubernetes/pkg/api/legacyscheme"
+       "k8s.io/kubernetes/test/integration/framework"
+)
+
+func Test_UpgradeService(t *testing.T) {
+       etcdOptions := framework.SharedEtcd()
+       apiServerOptions := kubeapiservertesting.NewDefaultTestServerOptions()
+       s := kubeapiservertesting.StartTestServerOrDie(t, apiServerOptions, nil, etcdOptions)
+       defer s.TearDownFn()
+       serviceName := "test-old-service"
+       ns := "old-service-ns"
+
+       kubeclient, err := kubernetes.NewForConfig(s.ClientConfig)
+       if err != nil {
+               t.Fatalf("Unexpected error: %v", err)
+       }
+       if _, err := kubeclient.CoreV1().Namespaces().Create(context.TODO(), (&v1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}), metav1.CreateOptions{}); err != nil {
+               t.Fatal(err)
+       }
+
+       // Create a service and store it in etcd with missing fields representing an old version
+       svc := &v1.Service{
+               ObjectMeta: metav1.ObjectMeta{
+                       Name:      serviceName,
+                       Namespace: ns,
+               },
+               Spec: v1.ServiceSpec{
+                       ClusterIP: "10.0.0.1",
+                       Ports: []v1.ServicePort{
+                               {
+                                       Name: "test-port",
+                                       Port: 81,
+                               },
+                       },
+               },
+       }
+       svcJSON, err := runtime.Encode(legacyscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), svc)
+       if err != nil {
+               t.Fatalf("Failed creating service JSON: %v", err)
+       }
+       key := "/" + etcdOptions.Prefix + "/services/specs/" + ns + "/" + serviceName
+       if _, err := s.EtcdClient.Put(context.Background(), key, string(svcJSON)); err != nil {
+               t.Error(err)
+       }
+       t.Logf("Service stored in etcd %v", string(svcJSON))
+
+       // Try to update the service
+       _, err = kubeclient.CoreV1().Services(ns).Update(context.TODO(), svc, metav1.UpdateOptions{})
+       if err != nil {
+               t.Error(err)
+       }
+
+}

@liggitt we should have one of this for APIs that add new fields, maybe generalize it as the ones that test the etcd storage data?

[liggitt]

+1 for adding this test (I'd also recommend setting metadata.creationTimestamp and metadata.uid to match what all persisted objects have)

@liggitt we should have one of this for APIs that add new fields, maybe generalize it as the ones that test the etcd storage data?

Since most types use standard defaulting which is applied much more consistently and straightforwardly, I don't think they have to test to the same degree service does.

[thockin]

Added test, with UID and creation set. Thanks, @aojea.

[aojea]

LGTM, but better if Jordan, Clayton or Daniel approve

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, thockin

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 [liggitt,thockin]
• test/OWNERS [liggitt,thockin]

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

[liggitt]

lgtm, pick to 1.23 is next

[lavalamp]

LGTM, this looks much better

I think patch/apply go through this code path, so you've fixed them too, but might be good to double check.

[liggitt]

It does and I checked.

[fedebongio]

/triage accepted