Repository: pravega/zookeeper-operator (go)

Issue:
Custom annotations for services created by operator, optional LoadBalancer
### Description

It would be great to have ability to attach custom annotations for services created by the Operator, and to make attaching LoadBalancer for AdminServer service optional

### Importance

This is especially important  for admin-server service, since by default LoadBalancer with default configuration is being attached. For my environment it conflicts with automated provisioning of resources which requires to have specific annotation on resources created by 3rd-party tools. So it's kind of blocker for me.
Additionally, I believe it would be great to make attaching LoadBalancer optional since LB costs money (at least in AWS EKS) and some people do not really use it outside of k8s.

### Location

pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go
pkg/zk/generators.go

### Suggestions for an improvement

I'm going to create PR if nobody minds

Pull request:
Issue 307: Provide a way for customization of annotations for service…
### Change log description

A way for customization of annotations for services and for making LoadBalancer optional for AdminServer service has been added.

### Purpose of the change

Fixes #307 

### What the code does

- clientService, headlessService, adminServerService were added to CRD
- support of new objects added to generators.go
- new code covered with tests

### How to verify it

Adding annotations to clientService/headlessService/adminServerService should lead to attaching corresponding annotations to k8s Service's.
Adding `adminServerService.internal: true` should lead to creating k8s Service with ClusterIP without LoadBalancer.

Patch:
diff --git a/charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml b/charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml
index f506e55..5bcef79 100644
--- a/charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml
+++ b/charts/zookeeper-operator/templates/zookeeper.pravega.io_zookeeperclusters_crd.yaml
@@ -2400,6 +2400,44 @@ spec:
                     type: object
                   type: array
               type: object
+            clientService:
+              description: ClientService defines the policy to create client Service
+                for the zookeeper cluster.
+              properties:
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to
+                    client Service the operator creates.
+                  type: object
+              type: object
+            headlessService:
+              description: HeadlessService defines the policy to create headless Service
+                for the zookeeper cluster.
+              properties:
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to
+                    headless Service the operator creates.
+                  type: object
+              type: object
+            adminServerService:
+              description: AdminServerService defines the policy to create AdminServer Service
+                for the zookeeper cluster.
+              properties:
+                external:
+                  description: External specifies if LoadBalancer should be created for
+                    the AdminServer. True means LoadBalancer will be created, false means ClusterIP
+                    will be used. Default is false.
+                  type: boolean
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to AdminServer
+                    Service the operator creates.
+                  type: object
+              type: object
             ports:
               items:
                 description: ContainerPort represents a network port in a single container.
diff --git a/charts/zookeeper/README.md b/charts/zookeeper/README.md
index 3cf2539..406af2d 100644
--- a/charts/zookeeper/README.md
+++ b/charts/zookeeper/README.md
@@ -82,6 +82,13 @@ The following table lists the configurable parameters of the zookeeper chart and
 | `pod.terminationGracePeriodSeconds` | Amount of time given to the pod to shutdown normally | `30` |
 | `pod.serviceAccountName` | Name for the service account | `zookeeper` |
 | `pod.imagePullSecrets` | ImagePullSecrets is a list of references to secrets in the same namespace to use for pulling any images. | `[]` |
+| `clientService` | Defines the policy to create client Service for the zookeeper cluster. | {} |
+| `clientService.annotations` | Specifies the annotations to attach to client Service the operator creates. | {} |
+| `headlessService` | Defines the policy to create headless Service for the zookeeper cluster. | {} |
+| `headlessService.annotations` | Specifies the annotations to attach to headless Service the operator creates. | {} |
+| `adminServerService` | Defines the policy to create AdminServer Service for the zookeeper cluster. | {} |
+| `adminServerService.annotations` | Specifies the annotations to attach to AdminServer Service the operator creates. | {} |
+| `adminServerService.external` | Specifies if LoadBalancer should be created for the AdminServer. True means LoadBalancer will be created, false - only ClusterIP will be used. | false |
 | `config.initLimit` | Amount of time (in ticks) to allow followers to connect and sync to a leader | `10` |
 | `config.tickTime` | Length of a single tick which is the basic time unit used by Zookeeper (measured in milliseconds) | `2000` |
 | `config.syncLimit` | Amount of time (in ticks) to allow followers to sync with Zookeeper | `2` |
diff --git a/charts/zookeeper/templates/zookeeper.yaml b/charts/zookeeper/templates/zookeeper.yaml
index 606d5eb..5b58628 100644
--- a/charts/zookeeper/templates/zookeeper.yaml
+++ b/charts/zookeeper/templates/zookeeper.yaml
@@ -87,6 +87,30 @@ spec:
     imagePullSecrets:
 {{ toYaml .Values.pod.imagePullSecrets | indent 6 }}
     {{- end }}
+  {{- if .Values.clientService }}
+  clientService:
+    {{- if .Values.clientService.annotations }}
+    annotations:
+{{ toYaml .Values.clientService.annotations | indent 6 }}
+    {{- end }}
+  {{- end }}
+  {{- if .Values.headlessService }}
+  headlessService:
+    {{- if .Values.headlessService.annotations }}
+    annotations:
+{{ toYaml .Values.headlessService.annotations | indent 6 }}
+    {{- end }}
+  {{- end }}
+  {{- if .Values.adminServerService }}
+  adminServerService:
+    {{- if .Values.adminServerService.annotations }}
+    annotations:
+{{ toYaml .Values.adminServerService.annotations | indent 6 }}
+    {{- end }}
+    {{- if .Values.adminServerService.external }}
+    external: {{ .Values.adminServerService.external }}
+    {{- end }}
+  {{- end }}
   {{- if .Values.config }}
   config:
     {{- if .Values.config.initLimit }}
diff --git a/charts/zookeeper/values.yaml b/charts/zookeeper/values.yaml
index cab8daa..f3f5aef 100644
--- a/charts/zookeeper/values.yaml
+++ b/charts/zookeeper/values.yaml
@@ -40,6 +40,16 @@ pod:
   serviceAccountName: zookeeper
   # imagePullSecrets: []
 
+adminServerService: {}
+  # annotations: {}
+  # external: false
+
+clientService: {}
+  # annotations: {}
+
+headlessService: {}
+  # annotations: {}
+
 config: {}
   # initLimit: 10
   # tickTime: 2000
diff --git a/deploy/crds/zookeeper.pravega.io_zookeeperclusters_crd.yaml b/deploy/crds/zookeeper.pravega.io_zookeeperclusters_crd.yaml
index 873aab7..c37e0a2 100644
--- a/deploy/crds/zookeeper.pravega.io_zookeeperclusters_crd.yaml
+++ b/deploy/crds/zookeeper.pravega.io_zookeeperclusters_crd.yaml
@@ -2396,6 +2396,44 @@ spec:
                     type: object
                   type: array
               type: object
+            clientService:
+              description: ClientService defines the policy to create client Service
+                for the zookeeper cluster.
+              properties:
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to
+                    client Service the operator creates.
+                  type: object
+              type: object
+            headlessService:
+              description: HeadlessService defines the policy to create headless Service
+                for the zookeeper cluster.
+              properties:
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to
+                    headless Service the operator creates.
+                  type: object
+              type: object
+            adminServerService:
+              description: AdminServerService defines the policy to create AdminServer Service
+                for the zookeeper cluster.
+              properties:
+                external:
+                  description: External specifies if LoadBalancer should be created for
+                    the AdminServer. True means LoadBalancer will be created, false means ClusterIP
+                    will be used. Default is false.
+                  type: boolean
+                annotations:
+                  additionalProperties:
+                    type: string
+                  description: Annotations specifies the annotations to attach to AdminServer
+                    Service the operator creates.
+                  type: object
+              type: object
             ports:
               items:
                 description: ContainerPort represents a network port in a single container.
diff --git a/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go b/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go
index a06d657..9ee5a7e 100644
--- a/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go
+++ b/pkg/apis/zookeeper/v1beta1/zookeepercluster_types.go
@@ -100,6 +100,12 @@ type ZookeeperClusterSpec struct {
 	// Updating the Pod does not take effect on any existing pods.
 	Pod PodPolicy `json:"pod,omitempty"`
 
+	AdminServerService AdminServerServicePolicy `json:"adminServerService,omitempty"`
+
+	ClientService ClientServicePolicy `json:"clientService,omitempty"`
+
+	HeadlessService HeadlessServicePolicy `json:"headlessService,omitempty"`
+
 	//StorageType is used to tell which type of storage we will be using
 	//It can take either Ephemeral or persistence
 	//Default StorageType is Persistence storage
@@ -488,6 +494,26 @@ func (p *PodPolicy) withDefaults(z *ZookeeperCluster) (changed bool) {
 	return changed
 }
 
+type AdminServerServicePolicy struct {
+	// Annotations specifies the annotations to attach to AdminServer service the operator
+	// creates.
+	Annotations map[string]string `json:"annotations,omitempty"`
+
+	External bool `json:"external,omitempty"`
+}
+
+type ClientServicePolicy struct {
+	// Annotations specifies the annotations to attach to client service the operator
+	// creates.
+	Annotations map[string]string `json:"annotations,omitempty"`
+}
+
+type HeadlessServicePolicy struct {
+	// Annotations specifies the annotations to attach to headless service the operator
+	// creates.
+	Annotations map[string]string `json:"annotations,omitempty"`
+}
+
 func (s *Probes) withDefaults() (changed bool) {
 	if s.ReadinessProbe == nil {
 		changed = true
diff --git a/pkg/controller/zookeepercluster/zookeepercluster_controller.go b/pkg/controller/zookeepercluster/zookeepercluster_controller.go
index 1a154aa..f05f3ec 100644
--- a/pkg/controller/zookeepercluster/zookeepercluster_controller.go
+++ b/pkg/controller/zookeepercluster/zookeepercluster_controller.go
@@ -722,7 +722,7 @@ func (r *ReconcileZookeeperCluster) cleanupOrphanPVCs(instance *zookeeperv1beta1
 
 func (r *ReconcileZookeeperCluster) getPVCList(instance *zookeeperv1beta1.ZookeeperCluster) (pvList corev1.PersistentVolumeClaimList, err error) {
 	selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
-		MatchLabels: map[string]string{"app": instance.GetName()},
+		MatchLabels: map[string]string{"app": instance.GetName(), "uid": string(instance.UID)},
 	})
 	pvclistOps := &client.ListOptions{
 		Namespace:     instance.Namespace,
diff --git a/pkg/zk/generators.go b/pkg/zk/generators.go
index 3f6f8ba..b588be9 100644
--- a/pkg/zk/generators.go
+++ b/pkg/zk/generators.go
@@ -57,7 +57,7 @@ func MakeStatefulSet(z *v1beta1.ZookeeperCluster) *appsv1.StatefulSet {
 				Name: zkDataVolume,
 				Labels: mergeLabels(
 					z.Spec.Labels,
-					map[string]string{"app": z.GetName()},
+					map[string]string{"app": z.GetName(), "uid": string(z.UID)},
 				),
 				Annotations: z.Spec.Persistence.Annotations,
 			},
@@ -191,7 +191,7 @@ func MakeClientService(z *v1beta1.ZookeeperCluster) *v1.Service {
 	svcPorts := []v1.ServicePort{
 		{Name: "tcp-client", Port: ports.Client},
 	}
-	return makeService(z.GetClientServiceName(), svcPorts, true, false, z)
+	return makeService(z.GetClientServiceName(), svcPorts, true, false, z.Spec.ClientService.Annotations, z)
 }
 
 // MakeAdminServerService returns a service which provides an interface
@@ -201,7 +201,9 @@ func MakeAdminServerService(z *v1beta1.ZookeeperCluster) *v1.Service {
 	svcPorts := []v1.ServicePort{
 		{Name: "tcp-admin-server", Port: ports.AdminServer},
 	}
-	return makeService(z.GetAdminServerServiceName(), svcPorts, true, true, z)
+	external := z.Spec.AdminServerService.External
+	annotations := z.Spec.AdminServerService.Annotations
+	return makeService(z.GetAdminServerServiceName(), svcPorts, true, external, annotations, z)
 }
 
 // MakeConfigMap returns a zookeeper config map
@@ -236,7 +238,7 @@ func MakeHeadlessService(z *v1beta1.ZookeeperCluster) *v1.Service {
 		{Name: "tcp-metrics", Port: ports.Metrics},
 		{Name: "tcp-admin-server", Port: ports.AdminServer},
 	}
-	return makeService(headlessSvcName(z), svcPorts, false, false, z)
+	return makeService(headlessSvcName(z), svcPorts, false, false, z.Spec.HeadlessService.Annotations, z)
 }
 
 func makeZkConfigString(z *v1beta1.ZookeeperCluster) string {
@@ -300,9 +302,9 @@ func makeZkEnvConfigString(z *v1beta1.ZookeeperCluster) string {
 		"CLUSTER_SIZE=" + fmt.Sprint(z.Spec.Replicas) + "\n"
 }
 
-func makeService(name string, ports []v1.ServicePort, clusterIP bool, external bool, z *v1beta1.ZookeeperCluster) *v1.Service {
+func makeService(name string, ports []v1.ServicePort, clusterIP bool, external bool, annotations map[string]string, z *v1beta1.ZookeeperCluster) *v1.Service {
 	var dnsName string
-	var annotationMap map[string]string
+	var annotationMap = copyMap(annotations)
 	if !clusterIP && z.Spec.DomainName != "" {
 		domainName := strings.TrimSpace(z.Spec.DomainName)
 		if strings.HasSuffix(domainName, dot) {
@@ -310,9 +312,7 @@ func makeService(name string, ports []v1.ServicePort, clusterIP bool, external b
 		} else {
 			dnsName = name + dot + domainName + dot
 		}
-		annotationMap = map[string]string{externalDNSAnnotationKey: dnsName}
-	} else {
-		annotationMap = map[string]string{}
+		annotationMap[externalDNSAnnotationKey] = dnsName
 	}
 	service := v1.Service{
 		TypeMeta: metav1.TypeMeta{
@@ -334,7 +334,7 @@ func makeService(name string, ports []v1.ServicePort, clusterIP bool, external b
 		},
 	}
 	if external {
-		service.Spec.Type = "LoadBalancer"
+		service.Spec.Type = v1.ServiceTypeLoadBalancer
 	}
 	if !clusterIP {
 		service.Spec.ClusterIP = v1.ClusterIPNone
@@ -388,3 +388,13 @@ func mergeLabels(l ...map[string]string) map[string]string {
 	}
 	return res
 }
+
+// Make a copy of map
+func copyMap(s map[string]string) map[string]string {
+	res := make(map[string]string)
+
+	for lKey, lValue := range s {
+		res[lKey] = lValue
+	}
+	return res
+}


Test patch:
diff --git a/pkg/zk/generators_test.go b/pkg/zk/generators_test.go
index 8afee47..607f201 100644
--- a/pkg/zk/generators_test.go
+++ b/pkg/zk/generators_test.go
@@ -297,6 +297,11 @@ var _ = Describe("Generators Spec", func() {
 					Labels: map[string]string{
 						"exampleLabel": "exampleValue",
 					},
+					ClientService: v1beta1.ClientServicePolicy{
+						Annotations: map[string]string{
+							"exampleAnnotation": "exampleValue",
+						},
+					},
 				},
 			}
 			z.WithDefaults()
@@ -318,8 +323,7 @@ var _ = Describe("Generators Spec", func() {
 		})
 
 		It("should not set the dns annotation", func() {
-			mapLength := len(s.GetAnnotations())
-			Ω(mapLength).To(Equal(0))
+			Expect(s.GetAnnotations()).NotTo(HaveKey("external-dns.alpha.kubernetes.io/hostname"))
 		})
 
 		It("should have custom labels set", func() {
@@ -327,6 +331,12 @@ var _ = Describe("Generators Spec", func() {
 				"exampleLabel",
 				"exampleValue"))
 		})
+
+		It("should have custom annotations set", func() {
+			Ω(s.GetAnnotations()).To(HaveKeyWithValue(
+				"exampleAnnotation",
+				"exampleValue"))
+		})
 	})
 
 	Context("#MakeHeadlessService", func() {
@@ -345,6 +355,11 @@ var _ = Describe("Generators Spec", func() {
 					Labels: map[string]string{
 						"exampleLabel": "exampleValue",
 					},
+					HeadlessService: v1beta1.HeadlessServicePolicy{
+						Annotations: map[string]string{
+							"exampleAnnotation": "exampleValue",
+						},
+					},
 				},
 			}
 			z.WithDefaults()
@@ -394,6 +409,12 @@ var _ = Describe("Generators Spec", func() {
 				"exampleLabel",
 				"exampleValue"))
 		})
+
+		It("should have custom annotations set", func() {
+			Ω(s.GetAnnotations()).To(HaveKeyWithValue(
+				"exampleAnnotation",
+				"exampleValue"))
+		})
 	})
 
 	Context("#MakeHeadlessService dnsname without dot", func() {
@@ -435,6 +456,11 @@ var _ = Describe("Generators Spec", func() {
 					Labels: map[string]string{
 						"exampleLabel": "exampleValue",
 					},
+					AdminServerService: v1beta1.AdminServerServicePolicy{
+						Annotations: map[string]string{
+							"exampleAnnotation": "exampleValue",
+						},
+					},
 				},
 			}
 			z.WithDefaults()
@@ -460,6 +486,40 @@ var _ = Describe("Generators Spec", func() {
 				"exampleLabel",
 				"exampleValue"))
 		})
+
+		It("should have custom annotations set", func() {
+			Ω(s.GetAnnotations()).To(HaveKeyWithValue(
+				"exampleAnnotation",
+				"exampleValue"))
+		})
+
+		It("should have no LoadBalancer attached by default", func() {
+			Ω(s.Spec.Type).NotTo(Equal(v1.ServiceTypeLoadBalancer))
+		})
+	})
+
+	Context("#MakeAdminServerService external with LoadBalancer", func() {
+		var s *v1.Service
+
+		BeforeEach(func() {
+			z := &v1beta1.ZookeeperCluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "example",
+					Namespace: "default",
+				},
+				Spec: v1beta1.ZookeeperClusterSpec{
+					AdminServerService: v1beta1.AdminServerServicePolicy{
+						External: true,
+					},
+				},
+			}
+			z.WithDefaults()
+			s = zk.MakeAdminServerService(z)
+		})
+
+		It("should have LoadBalancer attached", func() {
+			Ω(s.Spec.Type).To(Equal(v1.ServiceTypeLoadBalancer))
+		})
 	})
 
 	Context("#MakePodDisruptionBudget", func() {