Skip to content

Commit 8c88a0b

Browse files
jmckulkjmckulk
committed
Check ownership of pgAdmin services
Before this change PGO would force ownership on any service provided through PGAdmin.Spec.ServiceName, assuming the Service existed in the environment. With this change, we will check ownership and only reconcile a service if it is either not owned or owned by the associated PGAdmin object.
1 parent ae5beed commit 8c88a0b

File tree

8 files changed

+186
-9
lines changed

8 files changed

+186
-9
lines changed

‎internal/controller/standalone_pgadmin/service.go

Lines changed: 46 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,16 @@ import (
1919

2020
corev1 "k8s.io/api/core/v1"
2121
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
22+
"k8s.io/apimachinery/pkg/types"
2223
"k8s.io/apimachinery/pkg/util/intstr"
2324
"sigs.k8s.io/controller-runtime/pkg/client"
2425

25-
"github.com/pkg/errors"
26+
apierrors "k8s.io/apimachinery/pkg/api/errors"
2627

2728
"github.com/crunchydata/postgres-operator/internal/logging"
2829
"github.com/crunchydata/postgres-operator/internal/naming"
2930
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
31+
"github.com/pkg/errors"
3032
)
3133

3234
// +kubebuilder:rbac:groups="",resources="services",verbs={get}
@@ -45,10 +47,12 @@ func (r *PGAdminReconciler) reconcilePGAdminService(
4547
// need to delete any existing service(s). At the start of every reconcile
4648
// get all services that match the current pgAdmin labels.
4749
services := corev1.ServiceList{}
48-
if err := r.Client.List(ctx, &services, client.MatchingLabels{
49-
naming.LabelStandalonePGAdmin: pgadmin.Name,
50-
naming.LabelRole: naming.RolePGAdmin,
51-
}); err != nil {
50+
if err := r.Client.List(ctx, &services,
51+
client.InNamespace(pgadmin.Namespace),
52+
client.MatchingLabels{
53+
naming.LabelStandalonePGAdmin: pgadmin.Name,
54+
naming.LabelRole: naming.RolePGAdmin,
55+
}); err != nil {
5256
return err
5357
}
5458

@@ -64,16 +68,49 @@ func (r *PGAdminReconciler) reconcilePGAdminService(
6468
}
6569
}
6670

67-
// TODO (jmckulk): check if the requested services exists without our pgAdmin
68-
// as the owner. If this happens, don't take over ownership of the existing svc.
69-
7071
// At this point only a service defined by spec.ServiceName should exist.
71-
// Update the service or create it if it does not exist
72+
// Check if the user has requested a service through ServiceName
7273
if pgadmin.Spec.ServiceName != "" {
74+
// Look for an existing service with name ServiceName in the namespace
75+
existingService := &corev1.Service{}
76+
err := r.Client.Get(ctx, types.NamespacedName{
77+
Name: pgadmin.Spec.ServiceName,
78+
Namespace: pgadmin.GetNamespace(),
79+
}, existingService)
80+
if client.IgnoreNotFound(err) != nil {
81+
return err
82+
}
83+
84+
// If we found an existing service in our namespace with ServiceName
85+
if !apierrors.IsNotFound(err) {
86+
87+
// Check if the existing service has ownerReferences.
88+
// If it doesn't we can go ahead and reconcile the service.
89+
// If it does then we need to check if we are the controller.
90+
if len(existingService.OwnerReferences) != 0 {
91+
92+
// If the service is not controlled by this pgAdmin then we shouldn't reconcile
93+
if !metav1.IsControlledBy(existingService, pgadmin) {
94+
err := errors.New("Service is controlled by another object")
95+
log.V(1).Error(err, "PGO does not force ownership on existing services",
96+
"ServiceName", pgadmin.Spec.ServiceName)
97+
r.Recorder.Event(pgadmin,
98+
corev1.EventTypeWarning, "InvalidServiceWarning",
99+
"Failed to reconcile Service ServiceName: "+pgadmin.Spec.ServiceName)
100+
101+
return err
102+
}
103+
}
104+
}
105+
106+
// A service has been requested and we are allowed to create or reconcile
73107
service := service(pgadmin)
108+
109+
// Set the controller reference on the service
74110
if err := errors.WithStack(r.setControllerReference(pgadmin, service)); err != nil {
75111
return err
76112
}
113+
77114
return errors.WithStack(r.apply(ctx, service))
78115
}
79116

‎testing/kuttl/e2e/standalone-pgadmin-service/00-assert.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ metadata:
55
labels:
66
postgres-operator.crunchydata.com/role: pgadmin
77
postgres-operator.crunchydata.com/pgadmin: pgadmin
8+
ownerReferences:
9+
- apiVersion: postgres-operator.crunchydata.com/v1beta1
10+
controller: true
11+
kind: PGAdmin
12+
name: pgadmin
813
spec:
914
selector:
1015
postgres-operator.crunchydata.com/pgadmin: pgadmin

‎testing/kuttl/e2e/standalone-pgadmin-service/10--manual-service.yaml

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# Manually create a service that should be taken over by pgAdmin
2+
# The manual service is of type LoadBalancer
3+
# Cnce taken over, the type should change to ClusterIP
4+
apiVersion: v1
5+
kind: Service
6+
metadata:
7+
name: manual-pgadmin-service
8+
spec:
9+
ports:
10+
- name: pgadmin-port
11+
port: 5050
12+
protocol: TCP
13+
selector:
14+
postgres-operator.crunchydata.com/pgadmin: rhino
15+
type: LoadBalancer
16+
---
17+
# Create a pgAdmin that points to an existing un-owned service
18+
apiVersion: postgres-operator.crunchydata.com/v1beta1
19+
kind: PGAdmin
20+
metadata:
21+
name: manual-svc-pgadmin
22+
spec:
23+
serviceName: manual-pgadmin-service
24+
dataVolumeClaimSpec:
25+
accessModes:
26+
- "ReadWriteOnce"
27+
resources:
28+
requests:
29+
storage: 1Gi
30+
serverGroups: []

‎testing/kuttl/e2e/standalone-pgadmin-service/10-assert.yaml

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
# Check that the manually created service has the correct ownerReference
2+
apiVersion: v1
3+
kind: Service
4+
metadata:
5+
name: manual-pgadmin-service
6+
labels:
7+
postgres-operator.crunchydata.com/role: pgadmin
8+
postgres-operator.crunchydata.com/pgadmin: manual-svc-pgadmin
9+
ownerReferences:
10+
- apiVersion: postgres-operator.crunchydata.com/v1beta1
11+
controller: true
12+
kind: PGAdmin
13+
name: manual-svc-pgadmin
14+
spec:
15+
selector:
16+
postgres-operator.crunchydata.com/pgadmin: manual-svc-pgadmin
17+
ports:
18+
- port: 5050
19+
targetPort: 5050
20+
protocol: TCP
21+
name: pgadmin-port
22+
type: ClusterIP

‎testing/kuttl/e2e/standalone-pgadmin-service/20--owned-service.yaml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# Create a pgAdmin that will create and own a service
2+
apiVersion: postgres-operator.crunchydata.com/v1beta1
3+
kind: PGAdmin
4+
metadata:
5+
name: pgadmin-service-owner
6+
spec:
7+
serviceName: pgadmin-owned-service
8+
dataVolumeClaimSpec:
9+
accessModes:
10+
- "ReadWriteOnce"
11+
resources:
12+
requests:
13+
storage: 1Gi
14+
serverGroups: []

‎testing/kuttl/e2e/standalone-pgadmin-service/20-assert.yaml

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
apiVersion: v1
2+
kind: Service
3+
metadata:
4+
name: pgadmin-owned-service
5+
labels:
6+
postgres-operator.crunchydata.com/role: pgadmin
7+
postgres-operator.crunchydata.com/pgadmin: pgadmin-service-owner
8+
ownerReferences:
9+
- apiVersion: postgres-operator.crunchydata.com/v1beta1
10+
controller: true
11+
kind: PGAdmin
12+
name: pgadmin-service-owner
13+
spec:
14+
selector:
15+
postgres-operator.crunchydata.com/pgadmin: pgadmin-service-owner
16+
ports:
17+
- port: 5050
18+
targetPort: 5050
19+
protocol: TCP
20+
name: pgadmin-port
21+
type: ClusterIP

‎testing/kuttl/e2e/standalone-pgadmin-service/21-assert.yaml

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
# Original service should still have owner reference
2+
apiVersion: v1
3+
kind: Service
4+
metadata:
5+
name: pgadmin-owned-service
6+
labels:
7+
postgres-operator.crunchydata.com/role: pgadmin
8+
postgres-operator.crunchydata.com/pgadmin: pgadmin-service-owner
9+
ownerReferences:
10+
- apiVersion: postgres-operator.crunchydata.com/v1beta1
11+
controller: true
12+
kind: PGAdmin
13+
name: pgadmin-service-owner
14+
spec:
15+
selector:
16+
postgres-operator.crunchydata.com/pgadmin: pgadmin-service-owner
17+
ports:
18+
- port: 5050
19+
targetPort: 5050
20+
protocol: TCP
21+
name: pgadmin-port
22+
type: ClusterIP
23+
---
24+
apiVersion: v1
25+
involvedObject:
26+
apiVersion: postgres-operator.crunchydata.com/v1beta1
27+
kind: PGAdmin
28+
name: pgadmin-service-thief
29+
kind: Event
30+
message: 'Failed to reconcile Service ServiceName: pgadmin-owned-service'
31+
reason: InvalidServiceWarning
32+
source:
33+
component: pgadmin-controller
34+
type: Warning

‎testing/kuttl/e2e/standalone-pgadmin-service/21-steal-service.yaml

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
# Create a second pgAdmin that attempts to steal the service
2+
apiVersion: postgres-operator.crunchydata.com/v1beta1
3+
kind: PGAdmin
4+
metadata:
5+
name: pgadmin-service-thief
6+
spec:
7+
serviceName: pgadmin-owned-service
8+
dataVolumeClaimSpec:
9+
accessModes:
10+
- "ReadWriteOnce"
11+
resources:
12+
requests:
13+
storage: 1Gi
14+
serverGroups: []

0 commit comments

Comments
 (0)