From 3b10dc645dd3f4112df7d15042f335b86c01b3fc Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Thu, 13 Feb 2020 16:24:15 +0100 Subject: [PATCH 1/2] patch/update services on type change (#824) * use Update when disabling LoadBalancer + added e2e test --- e2e/tests/test_e2e.py | 56 ++++++++++ manifests/operator-service-account-rbac.yaml | 1 + pkg/cluster/resources.go | 103 +++++-------------- pkg/cluster/sync.go | 2 +- 4 files changed, 86 insertions(+), 76 deletions(-) diff --git a/e2e/tests/test_e2e.py b/e2e/tests/test_e2e.py index e92aba11f..2d81a0647 100644 --- a/e2e/tests/test_e2e.py +++ b/e2e/tests/test_e2e.py @@ -58,6 +58,55 @@ class EndToEndTestCase(unittest.TestCase): k8s.create_with_kubectl("manifests/minimal-postgres-manifest.yaml") k8s.wait_for_pod_start('spilo-role=master') + @timeout_decorator.timeout(TEST_TIMEOUT_SEC) + def test_enable_load_balancer(self): + ''' + Test if services are updated when enabling/disabling load balancers + ''' + + k8s = self.k8s + cluster_label = 'version=acid-minimal-cluster' + + # enable load balancer services + pg_patch_enable_lbs = { + "spec": { + "enableMasterLoadBalancer": True, + "enableReplicaLoadBalancer": True + } + } + k8s.api.custom_objects_api.patch_namespaced_custom_object( + "acid.zalan.do", "v1", "default", "postgresqls", "acid-minimal-cluster", pg_patch_enable_lbs) + # wait for service recreation + time.sleep(60) + + master_svc_type = k8s.get_service_type(cluster_label + ',spilo-role=master') + self.assertEqual(master_svc_type, 'LoadBalancer', + "Expected LoadBalancer service type for master, found {}".format(master_svc_type)) + + repl_svc_type = k8s.get_service_type(cluster_label + ',spilo-role=replica') + self.assertEqual(repl_svc_type, 'LoadBalancer', + "Expected LoadBalancer service type for replica, found {}".format(repl_svc_type)) + + # disable load balancer services again + pg_patch_disable_lbs = { + "spec": { + "enableMasterLoadBalancer": False, + "enableReplicaLoadBalancer": False + } + } + k8s.api.custom_objects_api.patch_namespaced_custom_object( + "acid.zalan.do", "v1", "default", "postgresqls", "acid-minimal-cluster", pg_patch_disable_lbs) + # wait for service recreation + time.sleep(60) + + master_svc_type = k8s.get_service_type(cluster_label + ',spilo-role=master') + self.assertEqual(master_svc_type, 'ClusterIP', + "Expected ClusterIP service type for master, found {}".format(master_svc_type)) + + repl_svc_type = k8s.get_service_type(cluster_label + ',spilo-role=replica') + self.assertEqual(repl_svc_type, 'ClusterIP', + "Expected ClusterIP service type for replica, found {}".format(repl_svc_type)) + @timeout_decorator.timeout(TEST_TIMEOUT_SEC) def test_min_resource_limits(self): ''' @@ -362,6 +411,13 @@ class K8s: pod_phase = pods[0].status.phase time.sleep(self.RETRY_TIMEOUT_SEC) + def get_service_type(self, svc_labels, namespace='default'): + svc_type = '' + svcs = self.api.core_v1.list_namespaced_service(namespace, label_selector=svc_labels, limit=1).items + for svc in svcs: + svc_type = svc.spec.type + return svc_type + def check_service_annotations(self, svc_labels, annotations, namespace='default'): svcs = self.api.core_v1.list_namespaced_service(namespace, label_selector=svc_labels, limit=1).items for svc in svcs: diff --git a/manifests/operator-service-account-rbac.yaml b/manifests/operator-service-account-rbac.yaml index a37abe476..4761c145e 100644 --- a/manifests/operator-service-account-rbac.yaml +++ b/manifests/operator-service-account-rbac.yaml @@ -114,6 +114,7 @@ rules: - delete - get - patch + - update # to CRUD the StatefulSet which controls the Postgres cluster instances - apiGroups: - apps diff --git a/pkg/cluster/resources.go b/pkg/cluster/resources.go index c94a7bb46..d6c2149bf 100644 --- a/pkg/cluster/resources.go +++ b/pkg/cluster/resources.go @@ -366,6 +366,11 @@ func (c *Cluster) createService(role PostgresRole) (*v1.Service, error) { } func (c *Cluster) updateService(role PostgresRole, newService *v1.Service) error { + var ( + svc *v1.Service + err error + ) + c.setProcessName("updating %v service", role) if c.Services[role] == nil { @@ -373,70 +378,6 @@ func (c *Cluster) updateService(role PostgresRole, newService *v1.Service) error } serviceName := util.NameFromMeta(c.Services[role].ObjectMeta) - endpointName := util.NameFromMeta(c.Endpoints[role].ObjectMeta) - // TODO: check if it possible to change the service type with a patch in future versions of Kubernetes - if newService.Spec.Type != c.Services[role].Spec.Type { - // service type has changed, need to replace the service completely. - // we cannot use just patch the current service, since it may contain attributes incompatible with the new type. - var ( - currentEndpoint *v1.Endpoints - err error - ) - - if role == Master { - // for the master service we need to re-create the endpoint as well. Get the up-to-date version of - // the addresses stored in it before the service is deleted (deletion of the service removes the endpoint) - currentEndpoint, err = c.KubeClient.Endpoints(c.Namespace).Get(c.endpointName(role), metav1.GetOptions{}) - if err != nil { - return fmt.Errorf("could not get current cluster %s endpoints: %v", role, err) - } - } - err = c.KubeClient.Services(serviceName.Namespace).Delete(serviceName.Name, c.deleteOptions) - if err != nil { - return fmt.Errorf("could not delete service %q: %v", serviceName, err) - } - - // wait until the service is truly deleted - c.logger.Debugf("waiting for service to be deleted") - - err = retryutil.Retry(c.OpConfig.ResourceCheckInterval, c.OpConfig.ResourceCheckTimeout, - func() (bool, error) { - _, err2 := c.KubeClient.Services(serviceName.Namespace).Get(serviceName.Name, metav1.GetOptions{}) - if err2 == nil { - return false, nil - } - if k8sutil.ResourceNotFound(err2) { - return true, nil - } - return false, err2 - }) - if err != nil { - return fmt.Errorf("could not delete service %q: %v", serviceName, err) - } - - // make sure we clear the stored service and endpoint status if the subsequent create fails. - c.Services[role] = nil - c.Endpoints[role] = nil - if role == Master { - // create the new endpoint using the addresses obtained from the previous one - endpointSpec := c.generateEndpoint(role, currentEndpoint.Subsets) - ep, err := c.KubeClient.Endpoints(endpointSpec.Namespace).Create(endpointSpec) - if err != nil { - return fmt.Errorf("could not create endpoint %q: %v", endpointName, err) - } - - c.Endpoints[role] = ep - } - - svc, err := c.KubeClient.Services(serviceName.Namespace).Create(newService) - if err != nil { - return fmt.Errorf("could not create service %q: %v", serviceName, err) - } - - c.Services[role] = svc - - return nil - } // update the service annotation in order to propagate ELB notation. if len(newService.ObjectMeta.Annotations) > 0 { @@ -454,18 +395,30 @@ func (c *Cluster) updateService(role PostgresRole, newService *v1.Service) error } } - patchData, err := specPatch(newService.Spec) - if err != nil { - return fmt.Errorf("could not form patch for the service %q: %v", serviceName, err) - } + // now, patch the service spec, but when disabling LoadBalancers do update instead + // patch does not work because of LoadBalancerSourceRanges field (even if set to nil) + oldServiceType := c.Services[role].Spec.Type + newServiceType := newService.Spec.Type + if newServiceType == "ClusterIP" && newServiceType != oldServiceType { + newService.ResourceVersion = c.Services[role].ResourceVersion + newService.Spec.ClusterIP = c.Services[role].Spec.ClusterIP + svc, err = c.KubeClient.Services(serviceName.Namespace).Update(newService) + if err != nil { + return fmt.Errorf("could not update service %q: %v", serviceName, err) + } + } else { + patchData, err := specPatch(newService.Spec) + if err != nil { + return fmt.Errorf("could not form patch for the service %q: %v", serviceName, err) + } - // update the service spec - svc, err := c.KubeClient.Services(serviceName.Namespace).Patch( - serviceName.Name, - types.MergePatchType, - patchData, "") - if err != nil { - return fmt.Errorf("could not patch service %q: %v", serviceName, err) + svc, err = c.KubeClient.Services(serviceName.Namespace).Patch( + serviceName.Name, + types.MergePatchType, + patchData, "") + if err != nil { + return fmt.Errorf("could not patch service %q: %v", serviceName, err) + } } c.Services[role] = svc diff --git a/pkg/cluster/sync.go b/pkg/cluster/sync.go index fa4fc9ec1..053db9ff7 100644 --- a/pkg/cluster/sync.go +++ b/pkg/cluster/sync.go @@ -116,7 +116,7 @@ func (c *Cluster) syncServices() error { c.logger.Debugf("syncing %s service", role) if err := c.syncEndpoint(role); err != nil { - return fmt.Errorf("could not sync %s endpont: %v", role, err) + return fmt.Errorf("could not sync %s endpoint: %v", role, err) } if err := c.syncService(role); err != nil { From 702a194c414f3fb8ceeb70b4b0cd35d56bd1c5bb Mon Sep 17 00:00:00 2001 From: Felix Kunde Date: Mon, 17 Feb 2020 11:25:07 +0100 Subject: [PATCH 2/2] switch to rbac/v1 (#829) * switch to rbac/v1 --- charts/postgres-operator-ui/templates/serviceaccount.yaml | 6 +++--- charts/postgres-operator/templates/clusterrole.yaml | 2 +- manifests/operator-service-account-rbac.yaml | 2 +- pkg/cluster/cluster.go | 4 ++-- pkg/controller/controller.go | 8 ++++---- pkg/util/k8sutil/k8sutil.go | 6 +++--- ui/manifests/ui-service-account-rbac.yaml | 2 +- 7 files changed, 15 insertions(+), 15 deletions(-) diff --git a/charts/postgres-operator-ui/templates/serviceaccount.yaml b/charts/postgres-operator-ui/templates/serviceaccount.yaml index 4148938b0..7bb715167 100644 --- a/charts/postgres-operator-ui/templates/serviceaccount.yaml +++ b/charts/postgres-operator-ui/templates/serviceaccount.yaml @@ -9,7 +9,7 @@ metadata: app.kubernetes.io/instance: {{ .Release.Name }} --- -apiVersion: rbac.authorization.k8s.io/v1beta1 +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: {{ template "postgres-operator-ui.name" . }} @@ -17,7 +17,7 @@ metadata: app.kubernetes.io/name: {{ template "postgres-operator-ui.name" . }} helm.sh/chart: {{ template "postgres-operator-ui.chart" . }} app.kubernetes.io/managed-by: {{ .Release.Service }} - app.kubernetes.io/instance: {{ .Release.Name }} + app.kubernetes.io/instance: {{ .Release.Name }} rules: - apiGroups: - acid.zalan.do @@ -78,4 +78,4 @@ subjects: # note: the cluster role binding needs to be defined # for every namespace the operator-ui service account lives in. name: {{ template "postgres-operator-ui.name" . }} - namespace: {{ .Release.Namespace }} \ No newline at end of file + namespace: {{ .Release.Namespace }} diff --git a/charts/postgres-operator/templates/clusterrole.yaml b/charts/postgres-operator/templates/clusterrole.yaml index f8550a539..f7fe1634c 100644 --- a/charts/postgres-operator/templates/clusterrole.yaml +++ b/charts/postgres-operator/templates/clusterrole.yaml @@ -1,5 +1,5 @@ {{ if .Values.rbac.create }} -apiVersion: rbac.authorization.k8s.io/v1beta1 +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: {{ include "postgres-operator.serviceAccountName" . }} diff --git a/manifests/operator-service-account-rbac.yaml b/manifests/operator-service-account-rbac.yaml index 4761c145e..5e43cc03b 100644 --- a/manifests/operator-service-account-rbac.yaml +++ b/manifests/operator-service-account-rbac.yaml @@ -5,7 +5,7 @@ metadata: namespace: default --- -apiVersion: rbac.authorization.k8s.io/v1beta1 +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: zalando-postgres-operator diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index c560c4cdf..91e7a5195 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -29,7 +29,7 @@ import ( "github.com/zalando/postgres-operator/pkg/util/patroni" "github.com/zalando/postgres-operator/pkg/util/teams" "github.com/zalando/postgres-operator/pkg/util/users" - rbacv1beta1 "k8s.io/api/rbac/v1beta1" + rbacv1 "k8s.io/api/rbac/v1" ) var ( @@ -45,7 +45,7 @@ type Config struct { RestConfig *rest.Config InfrastructureRoles map[string]spec.PgUser // inherited from the controller PodServiceAccount *v1.ServiceAccount - PodServiceAccountRoleBinding *rbacv1beta1.RoleBinding + PodServiceAccountRoleBinding *rbacv1.RoleBinding } type kubeResources struct { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 831078f3e..f67d99c1d 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -7,7 +7,7 @@ import ( "github.com/sirupsen/logrus" v1 "k8s.io/api/core/v1" - rbacv1beta1 "k8s.io/api/rbac/v1beta1" + rbacv1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" @@ -57,7 +57,7 @@ type Controller struct { workerLogs map[uint32]ringlog.RingLogger PodServiceAccount *v1.ServiceAccount - PodServiceAccountRoleBinding *rbacv1beta1.RoleBinding + PodServiceAccountRoleBinding *rbacv1.RoleBinding } // NewController creates a new controller @@ -198,7 +198,7 @@ func (c *Controller) initRoleBinding() { if c.opConfig.PodServiceAccountRoleBindingDefinition == "" { c.opConfig.PodServiceAccountRoleBindingDefinition = fmt.Sprintf(` { - "apiVersion": "rbac.authorization.k8s.io/v1beta1", + "apiVersion": "rbac.authorization.k8s.io/v1", "kind": "RoleBinding", "metadata": { "name": "%s" @@ -227,7 +227,7 @@ func (c *Controller) initRoleBinding() { case groupVersionKind.Kind != "RoleBinding": panic(fmt.Errorf("role binding definition in the operator config map defines another type of resource: %v", groupVersionKind.Kind)) default: - c.PodServiceAccountRoleBinding = obj.(*rbacv1beta1.RoleBinding) + c.PodServiceAccountRoleBinding = obj.(*rbacv1.RoleBinding) c.PodServiceAccountRoleBinding.Namespace = "" c.logger.Info("successfully parsed") diff --git a/pkg/util/k8sutil/k8sutil.go b/pkg/util/k8sutil/k8sutil.go index c7b2366b0..509b12c19 100644 --- a/pkg/util/k8sutil/k8sutil.go +++ b/pkg/util/k8sutil/k8sutil.go @@ -18,7 +18,7 @@ import ( appsv1 "k8s.io/client-go/kubernetes/typed/apps/v1" corev1 "k8s.io/client-go/kubernetes/typed/core/v1" policyv1beta1 "k8s.io/client-go/kubernetes/typed/policy/v1beta1" - rbacv1beta1 "k8s.io/client-go/kubernetes/typed/rbac/v1beta1" + rbacv1 "k8s.io/client-go/kubernetes/typed/rbac/v1" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" @@ -39,7 +39,7 @@ type KubernetesClient struct { corev1.NamespacesGetter corev1.ServiceAccountsGetter appsv1.StatefulSetsGetter - rbacv1beta1.RoleBindingsGetter + rbacv1.RoleBindingsGetter policyv1beta1.PodDisruptionBudgetsGetter apiextbeta1.CustomResourceDefinitionsGetter clientbatchv1beta1.CronJobsGetter @@ -103,7 +103,7 @@ func NewFromConfig(cfg *rest.Config) (KubernetesClient, error) { kubeClient.StatefulSetsGetter = client.AppsV1() kubeClient.PodDisruptionBudgetsGetter = client.PolicyV1beta1() kubeClient.RESTClient = client.CoreV1().RESTClient() - kubeClient.RoleBindingsGetter = client.RbacV1beta1() + kubeClient.RoleBindingsGetter = client.RbacV1() kubeClient.CronJobsGetter = client.BatchV1beta1() apiextClient, err := apiextclient.NewForConfig(cfg) diff --git a/ui/manifests/ui-service-account-rbac.yaml b/ui/manifests/ui-service-account-rbac.yaml index 4ae218e74..f0a6e8bb7 100644 --- a/ui/manifests/ui-service-account-rbac.yaml +++ b/ui/manifests/ui-service-account-rbac.yaml @@ -5,7 +5,7 @@ metadata: namespace: default --- -apiVersion: rbac.authorization.k8s.io/v1beta1 +apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: name: postgres-operator-ui