Skip to content

Commit 0f1f2dc

Browse files
benluddybenluddy
committed
Support InstallPlan steps upgrading existing ClusterIP Services.
If a ClusterIP-type Service manifest omits the immutable spec field "ClusterIP", a value is selected automatically when the Service is created. The catalog operator can patch existing Service resources if the updated manifest either omits "ClusterIP" or sets "ClusterIP" to the same value as that of the existing Service.
1 parent 1259808 commit 0f1f2dc

File tree

3 files changed

+290
-6
lines changed

3 files changed

+290
-6
lines changed

‎pkg/lib/operatorclient/service.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,22 @@ func (c *Client) DeleteService(namespace, name string, options *metav1.DeleteOpt
2828
// UpdateService will update the given Service resource.
2929
func (c *Client) UpdateService(service *v1.Service) (*v1.Service, error) {
3030
klog.V(4).Infof("[UPDATE Service]: %s", service.GetName())
31-
oldSa, err := c.GetService(service.GetNamespace(), service.GetName())
31+
old, err := c.GetService(service.GetNamespace(), service.GetName())
3232
if err != nil {
3333
return nil, err
3434
}
35-
patchBytes, err := createPatch(oldSa, service)
35+
normalized, err := cloneAndNormalizeObject(old)
36+
if err != nil {
37+
return nil, fmt.Errorf("failed to normalize existing Service resource for patch: %w", err)
38+
}
39+
if service.Spec.ClusterIP == old.Spec.ClusterIP {
40+
// Support updating to manifests that specify a
41+
// ClusterIP when its value is the same as that of the
42+
// existing Service.
43+
service = service.DeepCopy()
44+
service.Spec.ClusterIP = ""
45+
}
46+
patchBytes, err := createPatch(normalized, service)
3647
if err != nil {
3748
return nil, fmt.Errorf("error creating patch for Service: %v", err)
3849
}

‎pkg/lib/operatorclient/service_test.go

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package operatorclient
2+
3+
import (
4+
"testing"
5+
6+
corev1 "k8s.io/api/core/v1"
7+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
8+
"k8s.io/apimachinery/pkg/runtime/schema"
9+
"k8s.io/apimachinery/pkg/types"
10+
"k8s.io/client-go/kubernetes/fake"
11+
clienttesting "k8s.io/client-go/testing"
12+
13+
"github.com/stretchr/testify/require"
14+
)
15+
16+
func TestUpdateService(t *testing.T) {
17+
gvr := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "services"}
18+
19+
// In a test expectation, matches any single fake client action.
20+
var wildcard clienttesting.Action = clienttesting.ActionImpl{Verb: "wildcard!"}
21+
22+
for _, tc := range []struct {
23+
Name string
24+
Old *corev1.Service
25+
New *corev1.Service
26+
Expected []clienttesting.Action
27+
}{
28+
{
29+
Name: "no changes",
30+
Old: &corev1.Service{
31+
ObjectMeta: metav1.ObjectMeta{
32+
Name: "name",
33+
Namespace: "namespace",
34+
},
35+
},
36+
New: &corev1.Service{
37+
ObjectMeta: metav1.ObjectMeta{
38+
Name: "name",
39+
Namespace: "namespace",
40+
},
41+
},
42+
Expected: []clienttesting.Action{
43+
wildcard,
44+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{}`)),
45+
},
46+
},
47+
{
48+
Name: "resourceversion not patched",
49+
Old: &corev1.Service{
50+
ObjectMeta: metav1.ObjectMeta{
51+
Name: "name",
52+
Namespace: "namespace",
53+
ResourceVersion: "42",
54+
},
55+
},
56+
New: &corev1.Service{
57+
ObjectMeta: metav1.ObjectMeta{
58+
Name: "name",
59+
Namespace: "namespace",
60+
},
61+
},
62+
Expected: []clienttesting.Action{
63+
wildcard,
64+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{}`)),
65+
},
66+
},
67+
{
68+
Name: "clusterip not patched if omitted",
69+
Old: &corev1.Service{
70+
ObjectMeta: metav1.ObjectMeta{
71+
Name: "name",
72+
Namespace: "namespace",
73+
},
74+
Spec: corev1.ServiceSpec{
75+
ClusterIP: "1.2.3.4",
76+
},
77+
},
78+
New: &corev1.Service{
79+
ObjectMeta: metav1.ObjectMeta{
80+
Name: "name",
81+
Namespace: "namespace",
82+
},
83+
},
84+
Expected: []clienttesting.Action{
85+
wildcard,
86+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{}`)),
87+
},
88+
},
89+
{
90+
Name: "clusterip not patched if unchanged",
91+
Old: &corev1.Service{
92+
ObjectMeta: metav1.ObjectMeta{
93+
Name: "name",
94+
Namespace: "namespace",
95+
},
96+
Spec: corev1.ServiceSpec{
97+
ClusterIP: "1.2.3.4",
98+
},
99+
},
100+
New: &corev1.Service{
101+
ObjectMeta: metav1.ObjectMeta{
102+
Name: "name",
103+
Namespace: "namespace",
104+
},
105+
Spec: corev1.ServiceSpec{
106+
ClusterIP: "1.2.3.4",
107+
},
108+
},
109+
Expected: []clienttesting.Action{
110+
wildcard,
111+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{}`)),
112+
},
113+
},
114+
{
115+
Name: "clusterip patched if changed", // even though the patch will be rejected due to field immutability
116+
Old: &corev1.Service{
117+
ObjectMeta: metav1.ObjectMeta{
118+
Name: "name",
119+
Namespace: "namespace",
120+
},
121+
Spec: corev1.ServiceSpec{
122+
ClusterIP: "1.2.3.4",
123+
},
124+
},
125+
New: &corev1.Service{
126+
ObjectMeta: metav1.ObjectMeta{
127+
Name: "name",
128+
Namespace: "namespace",
129+
},
130+
Spec: corev1.ServiceSpec{
131+
ClusterIP: "4.3.2.1",
132+
},
133+
},
134+
Expected: []clienttesting.Action{
135+
wildcard,
136+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{"spec":{"clusterIP":"4.3.2.1"}}`)),
137+
},
138+
},
139+
{
140+
Name: "spec modified",
141+
Old: &corev1.Service{
142+
ObjectMeta: metav1.ObjectMeta{
143+
Name: "name",
144+
Namespace: "namespace",
145+
},
146+
Spec: corev1.ServiceSpec{
147+
SessionAffinity: "None",
148+
},
149+
},
150+
New: &corev1.Service{
151+
ObjectMeta: metav1.ObjectMeta{
152+
Name: "name",
153+
Namespace: "namespace",
154+
},
155+
Spec: corev1.ServiceSpec{
156+
SessionAffinity: "ClientIP",
157+
},
158+
},
159+
Expected: []clienttesting.Action{
160+
wildcard,
161+
clienttesting.NewPatchAction(gvr, "namespace", "name", types.StrategicMergePatchType, []byte(`{"spec":{"sessionAffinity":"ClientIP"}}`)),
162+
},
163+
},
164+
} {
165+
t.Run(tc.Name, func(t *testing.T) {
166+
require := require.New(t)
167+
168+
kube := fake.NewSimpleClientset(tc.Old)
169+
c := &Client{
170+
Interface: kube,
171+
}
172+
173+
_, err := c.UpdateService(tc.New)
174+
require.NoError(err)
175+
176+
actual := kube.Actions()
177+
require.Len(actual, len(tc.Expected))
178+
179+
for i, action := range kube.Actions() {
180+
if tc.Expected[i] == wildcard {
181+
continue
182+
}
183+
require.Equal(tc.Expected[i], action)
184+
}
185+
})
186+
}
187+
}

‎test/e2e/installplan_e2e_test.go

Lines changed: 90 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package e2e
22

33
import (
4+
"bytes"
45
"context"
56
"encoding/json"
67
"errors"
@@ -9,14 +10,11 @@ import (
910
"sync"
1011
"time"
1112

12-
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog"
13-
14-
"github.com/operator-framework/operator-lifecycle-manager/test/e2e/ctx"
15-
1613
"github.com/blang/semver"
1714
. "github.com/onsi/ginkgo"
1815
"github.com/onsi/ginkgo/extensions/table"
1916
. "github.com/onsi/gomega"
17+
"github.com/onsi/gomega/types"
2018
"github.com/stretchr/testify/assert"
2119
"github.com/stretchr/testify/require"
2220
appsv1 "k8s.io/api/apps/v1"
@@ -27,25 +25,113 @@ import (
2725
k8serrors "k8s.io/apimachinery/pkg/api/errors"
2826
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2927
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
28+
"k8s.io/apimachinery/pkg/runtime"
29+
k8sjson "k8s.io/apimachinery/pkg/runtime/serializer/json"
3030
"k8s.io/apimachinery/pkg/util/wait"
3131
"k8s.io/apimachinery/pkg/watch"
3232
"k8s.io/client-go/util/retry"
33+
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
3334

3435
opver "github.com/operator-framework/api/pkg/lib/version"
3536
operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
3637
operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
3738
"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned"
39+
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/catalog"
3840
"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry"
3941
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/apis/rbac"
4042
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
4143
"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
44+
"github.com/operator-framework/operator-lifecycle-manager/test/e2e/ctx"
4245
)
4346

4447
var _ = Describe("Install Plan", func() {
4548
AfterEach(func() {
4649
TearDown(testNamespace)
4750
})
4851

52+
When("a ClusterIP service exists", func() {
53+
var (
54+
service *corev1.Service
55+
)
56+
57+
BeforeEach(func() {
58+
service = &corev1.Service{
59+
TypeMeta: metav1.TypeMeta{
60+
Kind: "Service",
61+
APIVersion: "v1",
62+
},
63+
ObjectMeta: metav1.ObjectMeta{
64+
Namespace: testNamespace,
65+
Name: "test-service",
66+
},
67+
Spec: corev1.ServiceSpec{
68+
Type: corev1.ServiceTypeClusterIP,
69+
Ports: []corev1.ServicePort{
70+
{
71+
Port: 12345,
72+
},
73+
},
74+
},
75+
}
76+
77+
Expect(ctx.Ctx().Client().Create(context.Background(), service.DeepCopy())).To(Succeed())
78+
})
79+
80+
AfterEach(func() {
81+
Expect(ctx.Ctx().Client().Delete(context.Background(), service)).To(Succeed())
82+
})
83+
84+
It("it can be updated by an InstallPlan step", func() {
85+
scheme := runtime.NewScheme()
86+
Expect(corev1.AddToScheme(scheme)).To(Succeed())
87+
var manifest bytes.Buffer
88+
Expect(k8sjson.NewSerializer(k8sjson.DefaultMetaFactory, scheme, scheme, false).Encode(service, &manifest)).To(Succeed())
89+
90+
plan := &operatorsv1alpha1.InstallPlan{
91+
ObjectMeta: metav1.ObjectMeta{
92+
Namespace: testNamespace,
93+
Name: "test-plan",
94+
},
95+
Spec: operatorsv1alpha1.InstallPlanSpec{
96+
Approval: operatorsv1alpha1.ApprovalAutomatic,
97+
Approved: true,
98+
ClusterServiceVersionNames: []string{},
99+
},
100+
Status: operatorsv1alpha1.InstallPlanStatus{
101+
Phase: operatorsv1alpha1.InstallPlanPhaseInstalling,
102+
CatalogSources: []string{},
103+
Plan: []*operatorsv1alpha1.Step{
104+
{
105+
Status: operatorsv1alpha1.StepStatusUnknown,
106+
Resource: operatorsv1alpha1.StepResource{
107+
Name: service.Name,
108+
Version: "v1",
109+
Kind: "Service",
110+
Manifest: manifest.String(),
111+
},
112+
},
113+
},
114+
},
115+
}
116+
117+
Expect(ctx.Ctx().Client().Create(context.Background(), plan)).To(Succeed())
118+
Expect(ctx.Ctx().Client().Status().Update(context.Background(), plan)).To(Succeed())
119+
120+
key, err := runtimeclient.ObjectKeyFromObject(plan)
121+
Expect(err).ToNot(HaveOccurred())
122+
123+
HavePhase := func(goal operatorsv1alpha1.InstallPlanPhase) types.GomegaMatcher {
124+
return WithTransform(func(plan *operatorsv1alpha1.InstallPlan) operatorsv1alpha1.InstallPlanPhase {
125+
return plan.Status.Phase
126+
}, Equal(goal))
127+
}
128+
129+
Eventually(func() (*operatorsv1alpha1.InstallPlan, error) {
130+
return plan, ctx.Ctx().Client().Get(context.Background(), key, plan)
131+
}).Should(HavePhase(operatorsv1alpha1.InstallPlanPhaseComplete))
132+
})
133+
})
134+
49135
It("with CSVs across multiple catalog sources", func() {
50136

51137
log := func(s string) {

0 commit comments

Comments
 (0)