github/kubernetes
1
0
Fork 0
mirror of https://github.com/k3s-io/kubernetes.git synced 2025-07-30 15:05:27 +00:00
Code Issues Projects Releases Wiki Activity

Fix a small regression in Service updates

Browse Source 
Prior to 1.22 a user could change NodePort values within a service
during an update, and the apiserver would allocate values for any that
were not specified.

Consider a YAML like:

```
apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  type: NodePort
  ports:
  - name: p
    port: 80
  - name: q
    port: 81
  selector:
    app: foo
```

When this is created, nodeport values will be allocated for each port.
Something like:

```
apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  clusterIP: 10.0.149.11
  type: NodePort
  ports:
  - name: p
    nodePort: 30872
    port: 80
    protocol: TCP
    targetPort: 9376
  - name: q
    nodePort: 31310
    port: 81
    protocol: TCP
    targetPort: 81
  selector:
    app: foo
```

If the user PUTs (kubectl replace) the original YAML, we would see that
`.nodePort = 0`, and allocate new ports.  This was ugly at best.

In 1.22 we fixed this to not allocate new values if we still had the old
values, but instead re-assign them.  Net new ports would still be seen
as `.nodePort = 0` and so new allocations would be made.

This broke a corner case as follows:

Prior to 1.22, the user could PUT this YAML:

```
apiVersion: v1
kind: Service
metadata:
  name: foo
spec:
  type: NodePort
  ports:
  - name: p
    nodePort: 31310 # note this is the `q` value
    port: 80
  - name: q
    # note this nodePort is not specified
    port: 81
  selector:
    app: foo
```

The `p` port would take the `q` port's value.  The `q` port would be
seen as `.nodePort = 0` and a new value allocated.  In 1.22 this results
in an error (duplicate value in `p` and `q`).

This is VERY minor but it is an API regression, which we try to avoid,
and the fix is not too horrible.

This commit adds more robust testing of this logic.
This commit is contained in:
Tim Hockin 2021-08-25 22:21:16 -07:00
parent ff617edd32
commit 73503a4936
3 changed files with 225 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
pkg/api/service/testing/make.go
View File

@ -168,3 +168,10 @@ func SetAllocateLoadBalancerNodePorts(val bool) Tweak {
svc.Spec.AllocateLoadBalancerNodePorts = utilpointer.BoolPtr(val)
}
}
// SetHealthCheckNodePort sets the healthCheckNodePort field for a Service.
func SetHealthCheckNodePort(value int32) Tweak {
return func(svc *api.Service) {
svc.Spec.HealthCheckNodePort = value
}
}

233
pkg/registry/core/service/storage/rest_test.go
View File

@ -622,48 +622,210 @@ func TestServiceRegistryUpdate(t *testing.T) {
}
func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
type proof func(t *testing.T, s *api.Service)
prove := func(proofs ...proof) []proof {
return proofs
}
proveClusterIP := func(idx int, ip string) proof {
return func(t *testing.T, s *api.Service) {
if want, got := ip, s.Spec.ClusterIPs[idx]; want != got {
t.Errorf("wrong ClusterIPs[%d]: want %q, got %q", idx, want, got)
}
}
}
proveNodePort := func(idx int, port int32) proof {
return func(t *testing.T, s *api.Service) {
got := s.Spec.Ports[idx].NodePort
if port > 0 && got != port {
t.Errorf("wrong Ports[%d].NodePort: want %d, got %d", idx, port, got)
} else if port < 0 && got == -port {
t.Errorf("wrong Ports[%d].NodePort: wanted anything but %d", idx, got)
}
}
}
proveHCNP := func(port int32) proof {
return func(t *testing.T, s *api.Service) {
got := s.Spec.HealthCheckNodePort
if port > 0 && got != port {
t.Errorf("wrong HealthCheckNodePort: want %d, got %d", port, got)
} else if port < 0 && got == -port {
t.Errorf("wrong HealthCheckNodePort: wanted anything but %d", got)
}
}
}
testCases := []struct {
name string
svc *api.Service // Need a clusterIP, NodePort, and HealthCheckNodePort allocated
tweak func(*api.Service)
name string
create *api.Service // Needs clusterIP, NodePort, and HealthCheckNodePort allocated
update *api.Service // Needs clusterIP, NodePort, and/or HealthCheckNodePort blank
expectError bool
prove []proof
}{{
name: "single-port",
svc: svctest.MakeService("foo",
name: "single-ip_single-port",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetClusterIPs("1.2.3.4"),
svctest.SetNodePorts(30093),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal)),
tweak: nil,
prove: prove(
proveClusterIP(0, "1.2.3.4"),
proveNodePort(0, 30093),
proveHCNP(30118)),
}, {
name: "multi-port",
svc: svctest.MakeService("foo",
name: "multi-ip_multi-port",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetClusterIPs("1.2.3.4", "2000::1"),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 80, intstr.FromInt(80), api.ProtocolTCP),
svctest.MakeServicePort("q", 443, intstr.FromInt(443), api.ProtocolTCP))),
tweak: nil,
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP))),
prove: prove(
proveClusterIP(0, "1.2.3.4"),
proveClusterIP(1, "2000::1"),
proveNodePort(0, 30093),
proveNodePort(1, 30076),
proveHCNP(30118)),
}, {
name: "shuffle-ports",
svc: svctest.MakeService("foo",
name: "multi-ip_partial",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetClusterIPs("1.2.3.4", "2000::1"),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetClusterIPs("1.2.3.4")),
expectError: true,
}, {
name: "multi-port_partial",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 80, intstr.FromInt(80), api.ProtocolTCP),
svctest.MakeServicePort("q", 443, intstr.FromInt(443), api.ProtocolTCP))),
tweak: func(s *api.Service) {
s.Spec.Ports[0], s.Spec.Ports[1] = s.Spec.Ports[1], s.Spec.Ports[0]
},
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 0)), // provide just 1 value
prove: prove(
proveNodePort(0, 30093),
proveNodePort(1, 30076),
proveHCNP(30118)),
}, {
name: "swap-ports",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
// swapped from above
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP),
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP))),
prove: prove(
proveNodePort(0, 30076),
proveNodePort(1, 30093),
proveHCNP(30118)),
}, {
name: "partial-swap-ports",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30076, 0), // set [0] to [1], omit [1]
svctest.SetHealthCheckNodePort(30118)),
prove: prove(
proveNodePort(0, 30076),
proveNodePort(1, -30076),
proveHCNP(30118)),
}, {
name: "swap-port-with-hcnp",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30076, 30118)), // set [0] to [1], set [1] to HCNP
expectError: true,
}, {
name: "partial-swap-port-with-hcnp",
create: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30093, 30076),
svctest.SetHealthCheckNodePort(30118)),
update: svctest.MakeService("foo",
svctest.SetTypeLoadBalancer,
svctest.SetExternalTrafficPolicy(api.ServiceExternalTrafficPolicyTypeLocal),
svctest.SetPorts(
svctest.MakeServicePort("p", 867, intstr.FromInt(867), api.ProtocolTCP),
svctest.MakeServicePort("q", 5309, intstr.FromInt(5309), api.ProtocolTCP)),
svctest.SetNodePorts(30118, 0)), // set [0] to HCNP, omit [1]
expectError: true,
}}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
ctx := genericapirequest.NewDefaultContext()
storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol})
storage, server := NewTestREST(t, []api.IPFamily{api.IPv4Protocol, api.IPv6Protocol})
defer server.Terminate(t)
svc := tc.svc.DeepCopy()
svc := tc.create.DeepCopy()
obj, err := storage.Create(ctx, svc.DeepCopy(), rest.ValidateAllObjectFunc, &metav1.CreateOptions{})
if err != nil {
t.Fatalf("Expected no error: %v", err)
t.Fatalf("unexpected error on create: %v", err)
}
createdSvc := obj.(*api.Service)
if createdSvc.Spec.ClusterIP == "" {
@ -681,13 +843,21 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
t.Fatalf("expected HealthCheckNodePort to be set")
}
// Update from the original object - just change the selector.
// Update - change the selector to be sure.
svc = tc.update.DeepCopy()
svc.Spec.Selector = map[string]string{"bar": "baz2"}
svc.ResourceVersion = createdSvc.ResourceVersion
obj, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(svc.DeepCopy()), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
obj, _, err = storage.Update(ctx, svc.Name, rest.DefaultUpdatedObjectInfo(svc.DeepCopy()),
rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{})
if tc.expectError {
if err == nil {
t.Fatalf("unexpected success on update")
}
return
}
if err != nil {
t.Fatalf("Expected no error: %v", err)
t.Fatalf("unexpected error on update: %v", err)
}
updatedSvc := obj.(*api.Service)
@ -697,18 +867,9 @@ func TestServiceRegistryUpdateUnspecifiedAllocations(t *testing.T) {
if want, got := createdSvc.Spec.ClusterIPs, updatedSvc.Spec.ClusterIPs; !reflect.DeepEqual(want, got) {
t.Errorf("expected ClusterIPs to not change: wanted %v, got %v", want, got)
}
portmap := func(s *api.Service) map[string]int32 {
ret := map[string]int32{}
for _, p := range s.Spec.Ports {
ret[p.Name] = p.NodePort
}
return ret
}
if want, got := portmap(createdSvc), portmap(updatedSvc); !reflect.DeepEqual(want, got) {
t.Errorf("expected NodePort to not change: wanted %v, got %v", want, got)
}
if want, got := createdSvc.Spec.HealthCheckNodePort, updatedSvc.Spec.HealthCheckNodePort; want != got {
t.Errorf("expected HealthCheckNodePort to not change: wanted %v, got %v", want, got)
for _, proof := range tc.prove {
proof(t, updatedSvc)
}
})
}

22
pkg/registry/core/service/strategy.go
View File

@ -318,6 +318,20 @@ func patchAllocatedValues(newSvc, oldSvc *api.Service) {
}
if needsNodePort(oldSvc) && needsNodePort(newSvc) {
nodePortsUsed := func(svc *api.Service) sets.Int32 {
used := sets.NewInt32()
for _, p := range svc.Spec.Ports {
if p.NodePort != 0 {
used.Insert(p.NodePort)
}
}
return used
}
// Build a set of all the ports in oldSvc that are also in newSvc. We know
// we can't patch these values.
used := nodePortsUsed(oldSvc).Intersection(nodePortsUsed(newSvc))
// Map NodePorts by name. The user may have changed other properties
// of the port, but we won't see that here.
np := map[string]int32{}
@ -325,10 +339,16 @@ func patchAllocatedValues(newSvc, oldSvc *api.Service) {
p := &oldSvc.Spec.Ports[i]
np[p.Name] = p.NodePort
}
// If newSvc is missing values, try to patch them in when we know them and
// they haven't been used for another port.
for i := range newSvc.Spec.Ports {
p := &newSvc.Spec.Ports[i]
if p.NodePort == 0 {
p.NodePort = np[p.Name]
oldVal := np[p.Name]
if !used.Has(oldVal) {
p.NodePort = oldVal
}
}
}
}