Skip to content

Commit 3fc881c

Browse files
ajanikowajanikow
authored
[Feature] Do not change external service ports (#1204)
1 parent 237af92 commit 3fc881c

File tree

5 files changed

+220
-14
lines changed

5 files changed

+220
-14
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
- (Bugfix) Wait for Pod to be Ready in post-restart actions
4141
- (Bugfix) Prevent Runtime update restarts
4242
- (Bugfix) Change member port discovery
43+
- (Feature) Do not change external service ports
4344

4445
## [1.2.20](https://github.com/arangodb/kube-arangodb/tree/1.2.20) (2022-10-25)
4546
- (Feature) Add action progress

‎Makefile‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -499,8 +499,6 @@ tools: update-vendor
499499
@GOBIN=$(GOPATH)/bin go install golang.org/x/tools/cmd/goimports@0bb7e5c47b1a31f85d4f173edc878a8e049764a5
500500
@echo ">> Fetching license check"
501501
@GOBIN=$(GOPATH)/bin go install github.com/google/addlicense@6d92264d717064f28b32464f0f9693a5b4ef0239
502-
@echo ">> Fetching GO Assets Builder"
503-
@GOBIN=$(GOPATH)/bin go install github.com/jessevdk/go-assets-builder@b8483521738fd2198ecfc378067a4e8a6079f8e5
504502
@echo ">> Fetching gci"
505503
@GOBIN=$(GOPATH)/bin go install github.com/daixiang0/gci@v0.3.0
506504
@echo ">> Downloading protobuf compiler..."

‎pkg/deployment/resources/services.go‎

Lines changed: 19 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,7 @@ func (r *Resources) EnsureServices(ctx context.Context, cachedStatus inspectorIn
184184
}
185185
}
186186
} else {
187-
if changed, err := patcher.ServicePatcher(ctx, svcs, s, meta.PatchOptions{}, patcher.PatchServicePorts(clientServicePorts), patcher.PatchServiceSelector(clientServiceSelectors)); err != nil {
187+
if changed, err := patcher.ServicePatcher(ctx, svcs, s, meta.PatchOptions{}, patcher.PatchServiceOnlyPorts(clientServicePorts...), patcher.PatchServiceSelector(clientServiceSelectors)); err != nil {
188188
log.Err(err).Debug("Failed to patch database client service")
189189
return errors.WithStack(err)
190190
} else if changed {
@@ -254,9 +254,12 @@ func (r *Resources) ensureExternalAccessServices(ctx context.Context, cachedStat
254254
log := r.log.Str("section", "service-ea").Str("role", role).Str("service", eaServiceName)
255255
createExternalAccessService := false
256256
deleteExternalAccessService := false
257+
owned := false
257258
eaServiceType := spec.GetType().AsServiceType() // Note: Type auto defaults to ServiceTypeLoadBalancer
258259
if existing, exists := cachedStatus.Service().V1().GetSimple(eaServiceName); exists {
259260
// External access service exists
261+
owned = apiObject.OwnerOf(existing)
262+
260263
updateExternalAccessService := false
261264
loadBalancerIP := spec.GetLoadBalancerIP()
262265
loadBalancerSourceRanges := spec.LoadBalancerSourceRanges
@@ -285,7 +288,7 @@ func (r *Resources) ensureExternalAccessServices(ctx context.Context, cachedStat
285288
} else if existing.Spec.Type == core.ServiceTypeLoadBalancer && (loadBalancerIP != "" && existing.Spec.LoadBalancerIP != loadBalancerIP) {
286289
deleteExternalAccessService = true // LoadBalancerIP is wrong, remove the current and replace with proper one
287290
createExternalAccessService = true
288-
} else if existing.Spec.Type == core.ServiceTypeNodePort && len(existing.Spec.Ports) == 1 && (nodePort != 0 && existing.Spec.Ports[0].NodePort != int32(nodePort)) {
291+
} else if existing.Spec.Type == core.ServiceTypeNodePort && len(existing.Spec.Ports) < 1 || existing.Spec.Ports[0].Name != shared.ServerPortName && (nodePort != 0 && existing.Spec.Ports[0].NodePort != int32(nodePort)) {
289292
deleteExternalAccessService = true // NodePort is wrong, remove the current and replace with proper one
290293
createExternalAccessService = true
291294
}
@@ -300,7 +303,7 @@ func (r *Resources) ensureExternalAccessServices(ctx context.Context, cachedStat
300303
existing.Spec.LoadBalancerSourceRanges = loadBalancerSourceRanges
301304
}
302305
} else if spec.GetType().IsNodePort() {
303-
if existing.Spec.Type != core.ServiceTypeNodePort || len(existing.Spec.Ports) != 1 || (nodePort != 0 && existing.Spec.Ports[0].NodePort != int32(nodePort)) {
306+
if existing.Spec.Type != core.ServiceTypeNodePort || len(existing.Spec.Ports) < 1 || existing.Spec.Ports[0].Name != shared.ServerPortName || (nodePort != 0 && existing.Spec.Ports[0].NodePort != int32(nodePort)) {
304307
deleteExternalAccessService = true // Remove the current and replace with proper one
305308
createExternalAccessService = true
306309
}
@@ -316,7 +319,9 @@ func (r *Resources) ensureExternalAccessServices(ctx context.Context, cachedStat
316319
}
317320
}
318321
if !createExternalAccessService && !deleteExternalAccessService {
319-
if changed, err := patcher.ServicePatcher(ctx, svcs, existing, meta.PatchOptions{}, patcher.PatchServicePorts(eaPorts), patcher.PatchServiceSelector(eaSelector)); err != nil {
322+
if changed, err := patcher.ServicePatcher(ctx, svcs, existing, meta.PatchOptions{},
323+
patcher.PatchServiceSelector(eaSelector),
324+
patcher.Optional(patcher.PatchServiceOnlyPorts(eaPorts...), owned)); err != nil {
320325
log.Err(err).Debug("Failed to patch database client service")
321326
return errors.WithStack(err)
322327
} else if changed {
@@ -331,13 +336,15 @@ func (r *Resources) ensureExternalAccessServices(ctx context.Context, cachedStat
331336
}
332337

333338
if deleteExternalAccessService {
334-
log.Info("Removing obsolete external access service")
335-
err := globals.GetGlobalTimeouts().Kubernetes().RunWithTimeout(ctx, func(ctxChild context.Context) error {
336-
return svcs.Delete(ctxChild, eaServiceName, meta.DeleteOptions{})
337-
})
338-
if err != nil {
339-
log.Err(err).Debug("Failed to remove external access service")
340-
return errors.WithStack(err)
339+
if owned {
340+
log.Info("Removing obsolete external access service")
341+
err := globals.GetGlobalTimeouts().Kubernetes().RunWithTimeout(ctx, func(ctxChild context.Context) error {
342+
return svcs.Delete(ctxChild, eaServiceName, meta.DeleteOptions{})
343+
})
344+
if err != nil {
345+
log.Err(err).Debug("Failed to remove external access service")
346+
return errors.WithStack(err)
347+
}
341348
}
342349
}
343350
if createExternalAccessService {
@@ -368,7 +375,7 @@ func (r *Resources) ensureExternalAccessManagedServices(ctx context.Context, cac
368375

369376
apply := func(svc *core.Service) (bool, error) {
370377
return patcher.ServicePatcher(ctx, cachedStatus.ServicesModInterface().V1(), svc, meta.PatchOptions{},
371-
patcher.PatchServicePorts(ports),
378+
patcher.PatchServiceOnlyPorts(ports...),
372379
patcher.PatchServiceSelector(selectors))
373380
}
374381

‎pkg/util/k8sutil/patcher/service.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,72 @@ func PatchServicePorts(ports []core.ServicePort) ServicePatch {
8383
}
8484
}
8585

86+
func Optional(p ServicePatch, enabled bool) ServicePatch {
87+
return func(in *core.Service) []patch.Item {
88+
if !enabled {
89+
return nil
90+
}
91+
92+
if p != nil {
93+
return p(in)
94+
}
95+
96+
return nil
97+
}
98+
}
99+
100+
func PatchServiceOnlyPorts(ports ...core.ServicePort) ServicePatch {
101+
return func(in *core.Service) []patch.Item {
102+
psvc := in.Spec.DeepCopy()
103+
cp := psvc.Ports
104+
105+
changed := false
106+
107+
for pid := range ports {
108+
got := false
109+
for id := range cp {
110+
if ports[pid].Name == cp[id].Name {
111+
got = true
112+
113+
// Set ignored fields
114+
if ports[pid].NodePort == 0 {
115+
ports[pid].NodePort = cp[id].NodePort
116+
}
117+
if ports[pid].AppProtocol == nil {
118+
ports[pid].AppProtocol = cp[id].AppProtocol
119+
}
120+
if ports[pid].Protocol == "" {
121+
ports[pid].Protocol = cp[id].Protocol
122+
}
123+
if ports[pid].TargetPort.StrVal == "" && ports[pid].TargetPort.IntVal == 0 {
124+
ports[pid].TargetPort = cp[id].TargetPort
125+
}
126+
127+
if !equality.Semantic.DeepEqual(ports[pid], cp[id]) {
128+
q := ports[pid].DeepCopy()
129+
cp[id] = *q
130+
changed = true
131+
break
132+
}
133+
}
134+
}
135+
if !got {
136+
q := ports[pid].DeepCopy()
137+
cp = append(cp, *q)
138+
changed = true
139+
}
140+
}
141+
142+
if !changed {
143+
return nil
144+
}
145+
146+
return []patch.Item{
147+
patch.ItemReplace(patch.NewPath("spec", "ports"), cp),
148+
}
149+
}
150+
}
151+
86152
func PatchServiceSelector(selector map[string]string) ServicePatch {
87153
return func(in *core.Service) []patch.Item {
88154
if equality.Semantic.DeepEqual(in.Spec.Selector, selector) {

‎pkg/util/k8sutil/patcher/service_ports_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ import (
2525

2626
"github.com/stretchr/testify/require"
2727
core "k8s.io/api/core/v1"
28+
"k8s.io/apimachinery/pkg/util/intstr"
29+
30+
"github.com/arangodb/kube-arangodb/pkg/util"
2831
)
2932

3033
func Test_Service_Ports(t *testing.T) {
@@ -67,3 +70,134 @@ func Test_Service_Ports(t *testing.T) {
6770
require.Len(t, q, 1)
6871
})
6972
}
73+
74+
func Test_Service_OnlyPorts(t *testing.T) {
75+
t.Run("Equal", func(t *testing.T) {
76+
q := PatchServiceOnlyPorts(core.ServicePort{
77+
Name: "test",
78+
})(&core.Service{
79+
Spec: core.ServiceSpec{
80+
Ports: []core.ServicePort{
81+
{
82+
Name: "test",
83+
},
84+
},
85+
},
86+
})
87+
88+
require.Len(t, q, 0)
89+
})
90+
91+
t.Run("Missing", func(t *testing.T) {
92+
q := PatchServiceOnlyPorts(core.ServicePort{
93+
Name: "test",
94+
})(&core.Service{
95+
Spec: core.ServiceSpec{
96+
Ports: []core.ServicePort{
97+
{
98+
Name: "test2",
99+
},
100+
},
101+
},
102+
})
103+
104+
require.Len(t, q, 1)
105+
})
106+
107+
t.Run("Different", func(t *testing.T) {
108+
q := PatchServiceOnlyPorts(core.ServicePort{
109+
Name: "test",
110+
Port: 8529,
111+
})(&core.Service{
112+
Spec: core.ServiceSpec{
113+
Ports: []core.ServicePort{
114+
{
115+
Name: "test1",
116+
},
117+
},
118+
},
119+
})
120+
121+
require.Len(t, q, 1)
122+
})
123+
124+
t.Run("Different Port", func(t *testing.T) {
125+
q := PatchServiceOnlyPorts(core.ServicePort{
126+
Name: "test",
127+
Port: 8529,
128+
})(&core.Service{
129+
Spec: core.ServiceSpec{
130+
Ports: []core.ServicePort{
131+
{
132+
Name: "test",
133+
},
134+
},
135+
},
136+
})
137+
138+
require.Len(t, q, 1)
139+
})
140+
141+
t.Run("Changed NodePort", func(t *testing.T) {
142+
q := PatchServiceOnlyPorts(core.ServicePort{
143+
Name: "test",
144+
Port: 8529,
145+
})(&core.Service{
146+
Spec: core.ServiceSpec{
147+
Ports: []core.ServicePort{
148+
{
149+
Name: "test",
150+
Port: 8529,
151+
NodePort: 12345,
152+
},
153+
},
154+
},
155+
})
156+
157+
require.Len(t, q, 0)
158+
})
159+
160+
t.Run("Changed Port", func(t *testing.T) {
161+
q := PatchServiceOnlyPorts(core.ServicePort{
162+
Name: "test",
163+
Port: 8528,
164+
})(&core.Service{
165+
Spec: core.ServiceSpec{
166+
Ports: []core.ServicePort{
167+
{
168+
Name: "test",
169+
Port: 8529,
170+
NodePort: 12345,
171+
},
172+
},
173+
},
174+
})
175+
176+
require.Len(t, q, 1)
177+
})
178+
179+
t.Run("Ignore fields", func(t *testing.T) {
180+
q := PatchServiceOnlyPorts(core.ServicePort{
181+
Name: "test",
182+
Port: 8528,
183+
})(&core.Service{
184+
Spec: core.ServiceSpec{
185+
Ports: []core.ServicePort{
186+
{
187+
Name: "test",
188+
Protocol: core.ProtocolTCP,
189+
AppProtocol: util.NewString("test"),
190+
Port: 8528,
191+
TargetPort: intstr.IntOrString{
192+
StrVal: "TEST",
193+
IntVal: 0,
194+
},
195+
NodePort: 6543,
196+
},
197+
},
198+
},
199+
})
200+
201+
require.Len(t, q, 0)
202+
})
203+
}

0 commit comments

Comments
 (0)