From e927ce85b6fcd5bfedd7a9e9dec5c35553812bf6 Mon Sep 17 00:00:00 2001
From: Tim Hockin <thockin@google.com>
Date: Fri, 28 Jan 2022 16:38:27 -0800
Subject: [PATCH] service REST: Call Decorator(old) on update path

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 (as it is
documented)

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

Test by aojea
---
 pkg/registry/core/service/storage/storage.go |  5 ++
 test/integration/service/upgrade_test.go     | 81 ++++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 test/integration/service/upgrade_test.go

diff --git a/pkg/registry/core/service/storage/storage.go b/pkg/registry/core/service/storage/storage.go
index d9d77ae6cf4..92da4302a79 100644
--- a/pkg/registry/core/service/storage/storage.go
+++ b/pkg/registry/core/service/storage/storage.go
@@ -367,6 +367,11 @@ func (r *REST) beginUpdate(ctx context.Context, obj, oldObj runtime.Object, opti
 	newSvc := obj.(*api.Service)
 	oldSvc := oldObj.(*api.Service)
 
+	// Make sure the existing object has all fields we expect to be defaulted.
+	// This might not be true if the saved object predates these fields (the
+	// Decorator hook is not called on 'old' in the update path.
+	r.defaultOnReadService(oldSvc)
+
 	// Fix up allocated values that the client may have not specified (for
 	// idempotence).
 	patchAllocatedValues(After{newSvc}, Before{oldSvc})
diff --git a/test/integration/service/upgrade_test.go b/test/integration/service/upgrade_test.go
new file mode 100644
index 00000000000..25366195a3c
--- /dev/null
+++ b/test/integration/service/upgrade_test.go
@@ -0,0 +1,81 @@
+/*
+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,
+			CreationTimestamp: metav1.Now(),
+			UID:               "08675309-9376-9376-9376-086753099999",
+		},
+		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)
+	}
+}