Allow services update when changing annotations (#818)
The current implementations for `pkg.util.k8sutil.SameService` considers only service annotations change on the default annotations created by the operator. Custom annotations are not compared and consequently not applied after the first service creation. This commit introduces a complete annotations comparison between the current service created by the operator and the new one generated based on the configs. Also, it adds tests on the above-mentioned function.
This commit is contained in:
Jonathan Juares Beber
GitHub
parent
00f00af2e8
commit
744c71d16b
|
|
@ -270,7 +270,16 @@ class EndToEndTestCase(unittest.TestCase):
|
|||
}
|
||||
k8s.update_config(patch_custom_service_annotations)
|
||||
|
||||
k8s.create_with_kubectl("manifests/postgres-manifest-with-service-annotations.yaml")
|
||||
pg_patch_custom_annotations = {
|
||||
"spec": {
|
||||
"serviceAnnotations": {
|
||||
"annotation.key": "value"
|
||||
}
|
||||
}
|
||||
}
|
||||
k8s.api.custom_objects_api.patch_namespaced_custom_object(
|
||||
"acid.zalan.do", "v1", "default", "postgresqls", "acid-minimal-cluster", pg_patch_custom_annotations)
|
||||
|
||||
annotations = {
|
||||
"annotation.key": "value",
|
||||
"foo": "bar",
|
||||
|
|
|
|||
|
|
@ -1,20 +0,0 @@
|
|||
apiVersion: "acid.zalan.do/v1"
|
||||
kind: postgresql
|
||||
metadata:
|
||||
name: acid-service-annotations
|
||||
spec:
|
||||
teamId: "acid"
|
||||
volume:
|
||||
size: 1Gi
|
||||
numberOfInstances: 2
|
||||
users:
|
||||
zalando: # database owner
|
||||
- superuser
|
||||
- createdb
|
||||
foo_user: [] # role for application foo
|
||||
databases:
|
||||
foo: zalando # dbname: owner
|
||||
postgresql:
|
||||
version: "11"
|
||||
serviceAnnotations:
|
||||
annotation.key: value
|
||||
|
|
@ -9,7 +9,6 @@ import (
|
|||
batchv1beta1 "k8s.io/api/batch/v1beta1"
|
||||
clientbatchv1beta1 "k8s.io/client-go/kubernetes/typed/batch/v1beta1"
|
||||
|
||||
"github.com/zalando/postgres-operator/pkg/util/constants"
|
||||
v1 "k8s.io/api/core/v1"
|
||||
policybeta1 "k8s.io/api/policy/v1beta1"
|
||||
apiextclient "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
|
||||
|
|
@ -136,21 +135,37 @@ func SameService(cur, new *v1.Service) (match bool, reason string) {
|
|||
}
|
||||
}
|
||||
|
||||
oldDNSAnnotation := cur.Annotations[constants.ZalandoDNSNameAnnotation]
|
||||
newDNSAnnotation := new.Annotations[constants.ZalandoDNSNameAnnotation]
|
||||
oldELBAnnotation := cur.Annotations[constants.ElbTimeoutAnnotationName]
|
||||
newELBAnnotation := new.Annotations[constants.ElbTimeoutAnnotationName]
|
||||
match = true
|
||||
|
||||
if oldDNSAnnotation != newDNSAnnotation {
|
||||
return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
|
||||
constants.ZalandoDNSNameAnnotation, newDNSAnnotation, oldDNSAnnotation)
|
||||
}
|
||||
if oldELBAnnotation != newELBAnnotation {
|
||||
return false, fmt.Sprintf("new service's %q annotation value %q doesn't match the current one %q",
|
||||
constants.ElbTimeoutAnnotationName, oldELBAnnotation, newELBAnnotation)
|
||||
reasonPrefix := "new service's annotations doesn't match the current one:"
|
||||
for ann := range cur.Annotations {
|
||||
if _, ok := new.Annotations[ann]; !ok {
|
||||
match = false
|
||||
if len(reason) == 0 {
|
||||
reason = reasonPrefix
|
||||
}
|
||||
reason += fmt.Sprintf(" Removed '%s'.", ann)
|
||||
}
|
||||
}
|
||||
|
||||
return true, ""
|
||||
for ann := range new.Annotations {
|
||||
v, ok := cur.Annotations[ann]
|
||||
if !ok {
|
||||
if len(reason) == 0 {
|
||||
reason = reasonPrefix
|
||||
}
|
||||
reason += fmt.Sprintf(" Added '%s' with value '%s'.", ann, new.Annotations[ann])
|
||||
match = false
|
||||
} else if v != new.Annotations[ann] {
|
||||
if len(reason) == 0 {
|
||||
reason = reasonPrefix
|
||||
}
|
||||
reason += fmt.Sprintf(" '%s' changed from '%s' to '%s'.", ann, v, new.Annotations[ann])
|
||||
match = false
|
||||
}
|
||||
}
|
||||
|
||||
return match, reason
|
||||
}
|
||||
|
||||
// SamePDB compares the PodDisruptionBudgets
|
||||
|
|
|
|||
|
|
@ -0,0 +1,310 @@
|
|||
package k8sutil
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/zalando/postgres-operator/pkg/util/constants"
|
||||
|
||||
v1 "k8s.io/api/core/v1"
|
||||
)
|
||||
|
||||
func newsService(ann map[string]string, svcT v1.ServiceType, lbSr []string) *v1.Service {
|
||||
svc := &v1.Service{
|
||||
Spec: v1.ServiceSpec{
|
||||
Type: svcT,
|
||||
LoadBalancerSourceRanges: lbSr,
|
||||
},
|
||||
}
|
||||
svc.Annotations = ann
|
||||
return svc
|
||||
}
|
||||
|
||||
func TestServiceAnnotations(t *testing.T) {
|
||||
tests := []struct {
|
||||
about string
|
||||
current *v1.Service
|
||||
new *v1.Service
|
||||
reason string
|
||||
match bool
|
||||
}{
|
||||
{
|
||||
about: "two equal services",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeClusterIP,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeClusterIP,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: true,
|
||||
},
|
||||
{
|
||||
about: "services differ on service type",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeClusterIP,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's type "LoadBalancer" doesn't match the current one "ClusterIP"`,
|
||||
},
|
||||
{
|
||||
about: "services differ on lb source ranges",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"185.249.56.0/22"}),
|
||||
match: false,
|
||||
reason: `new service's LoadBalancerSourceRange doesn't match the current one`,
|
||||
},
|
||||
{
|
||||
about: "new service doesn't have lb source ranges",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{}),
|
||||
match: false,
|
||||
reason: `new service's LoadBalancerSourceRange doesn't match the current one`,
|
||||
},
|
||||
{
|
||||
about: "services differ on DNS annotation",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "new_clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: 'external-dns.alpha.kubernetes.io/hostname' changed from 'clstr.acid.zalan.do' to 'new_clstr.acid.zalan.do'.`,
|
||||
},
|
||||
{
|
||||
about: "services differ on AWS ELB annotation",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: "1800",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: 'service.beta.kubernetes.io/aws-load-balancer-connection-idle-timeout' changed from '3600' to '1800'.`,
|
||||
},
|
||||
{
|
||||
about: "service changes existing annotation",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "baz",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: 'foo' changed from 'bar' to 'baz'.`,
|
||||
},
|
||||
{
|
||||
about: "service changes multiple existing annotations",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
"bar": "foo",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "baz",
|
||||
"bar": "fooz",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
// Test just the prefix to avoid flakiness and map sorting
|
||||
reason: `new service's annotations doesn't match the current one:`,
|
||||
},
|
||||
{
|
||||
about: "service adds a new custom annotation",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: Added 'foo' with value 'bar'.`,
|
||||
},
|
||||
{
|
||||
about: "service removes a custom annotation",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: Removed 'foo'.`,
|
||||
},
|
||||
{
|
||||
about: "service removes a custom annotation and adds a new one",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"bar": "foo",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: Removed 'foo'. Added 'bar' with value 'foo'.`,
|
||||
},
|
||||
{
|
||||
about: "service removes a custom annotation, adds a new one and change another",
|
||||
current: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"foo": "bar",
|
||||
"zalan": "do",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
"bar": "foo",
|
||||
"zalan": "do.com",
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
reason: `new service's annotations doesn't match the current one: Removed 'foo'. Added 'bar' with value 'foo'. 'zalan' changed from 'do' to 'do.com'`,
|
||||
},
|
||||
{
|
||||
about: "service add annotations",
|
||||
current: newsService(
|
||||
map[string]string{},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
new: newsService(
|
||||
map[string]string{
|
||||
constants.ZalandoDNSNameAnnotation: "clstr.acid.zalan.do",
|
||||
constants.ElbTimeoutAnnotationName: constants.ElbTimeoutAnnotationValue,
|
||||
},
|
||||
v1.ServiceTypeLoadBalancer,
|
||||
[]string{"128.141.0.0/16", "137.138.0.0/16"}),
|
||||
match: false,
|
||||
// Test just the prefix to avoid flakiness and map sorting
|
||||
reason: `new service's annotations doesn't match the current one: Added `,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.about, func(t *testing.T) {
|
||||
match, reason := SameService(tt.current, tt.new)
|
||||
if match && !tt.match {
|
||||
t.Errorf("expected services to do not match: '%q' and '%q'", tt.current, tt.new)
|
||||
return
|
||||
}
|
||||
if !match && tt.match {
|
||||
t.Errorf("expected services to be the same: '%q' and '%q'", tt.current, tt.new)
|
||||
return
|
||||
}
|
||||
if !match && !tt.match {
|
||||
if !strings.HasPrefix(reason, tt.reason) {
|
||||
t.Errorf("expected reason '%s', found '%s'", tt.reason, reason)
|
||||
return
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
Loading…
Reference in New Issue