Skip to content

Commit

Permalink
Use the discovery runner in controllers
Browse files Browse the repository at this point in the history
  • Loading branch information
cbandy committed Nov 25, 2024
1 parent a77c141 commit b4d7444
Show file tree
Hide file tree
Showing 9 changed files with 60 additions and 98 deletions.
12 changes: 5 additions & 7 deletions cmd/postgres-operator/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ func main() {
token, _ := registrar.CheckToken()

// add all PostgreSQL Operator controllers to the runtime manager
addControllersToManager(mgr, k8s.IsOpenShift(), log, registrar)
addControllersToManager(mgr, log, registrar)

if features.Enabled(feature.BridgeIdentifiers) {
constructor := func() *bridge.Client {
Expand Down Expand Up @@ -214,10 +214,9 @@ func main() {

// addControllersToManager adds all PostgreSQL Operator controllers to the provided controller
// runtime manager.
func addControllersToManager(mgr runtime.Manager, openshift bool, log logging.Logger, reg registration.Registration) {
func addControllersToManager(mgr runtime.Manager, log logging.Logger, reg registration.Registration) {
pgReconciler := &postgrescluster.Reconciler{
Client: mgr.GetClient(),
IsOpenShift: openshift,
Owner: postgrescluster.ControllerName,
Recorder: mgr.GetEventRecorderFor(postgrescluster.ControllerName),
Registration: reg,
Expand All @@ -242,10 +241,9 @@ func addControllersToManager(mgr runtime.Manager, openshift bool, log logging.Lo
}

pgAdminReconciler := &standalone_pgadmin.PGAdminReconciler{
Client: mgr.GetClient(),
Owner: "pgadmin-controller",
Recorder: mgr.GetEventRecorderFor(naming.ControllerPGAdmin),
IsOpenShift: openshift,
Client: mgr.GetClient(),
Owner: "pgadmin-controller",
Recorder: mgr.GetEventRecorderFor(naming.ControllerPGAdmin),
}

if err := pgAdminReconciler.SetupWithManager(mgr); err != nil {
Expand Down
47 changes: 6 additions & 41 deletions internal/controller/postgrescluster/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,9 @@ import (
policyv1 "k8s.io/api/policy/v1"
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/apimachinery/pkg/api/equality"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/client-go/discovery"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -33,6 +31,7 @@ import (
"github.com/crunchydata/postgres-operator/internal/config"
"github.com/crunchydata/postgres-operator/internal/controller/runtime"
"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/logging"
"github.com/crunchydata/postgres-operator/internal/pgaudit"
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
Expand All @@ -51,11 +50,9 @@ const (

// Reconciler holds resources for the PostgresCluster reconciler
type Reconciler struct {
Client client.Client
DiscoveryClient *discovery.DiscoveryClient
IsOpenShift bool
Owner client.FieldOwner
PodExec func(
Client client.Client
Owner client.FieldOwner
PodExec func(
ctx context.Context, namespace, pod, container string,
stdin io.Reader, stdout, stderr io.Writer, command ...string,
) error
Expand Down Expand Up @@ -94,8 +91,9 @@ func (r *Reconciler) Reconcile(
// from its cache.
cluster.Default()

// TODO(openshift): Separate this into more specific detections elsewhere.
if cluster.Spec.OpenShift == nil {
cluster.Spec.OpenShift = &r.IsOpenShift
cluster.Spec.OpenShift = initialize.Bool(kubernetes.IsOpenShift(ctx))
}

// Keep a copy of cluster prior to any manipulations.
Expand Down Expand Up @@ -482,14 +480,6 @@ func (r *Reconciler) SetupWithManager(mgr manager.Manager) error {
}
}

if r.DiscoveryClient == nil {
var err error
r.DiscoveryClient, err = discovery.NewDiscoveryClientForConfig(mgr.GetConfig())
if err != nil {
return err
}
}

return builder.ControllerManagedBy(mgr).
For(&v1beta1.PostgresCluster{}).
Owns(&corev1.ConfigMap{}).
Expand All @@ -510,28 +500,3 @@ func (r *Reconciler) SetupWithManager(mgr manager.Manager) error {
r.controllerRefHandlerFuncs()). // watch all StatefulSets
Complete(r)
}

// GroupVersionKindExists checks to see whether a given Kind for a given
// GroupVersion exists in the Kubernetes API Server.
func (r *Reconciler) GroupVersionKindExists(groupVersion, kind string) (*bool, error) {
if r.DiscoveryClient == nil {
return initialize.Bool(false), nil
}

resourceList, err := r.DiscoveryClient.ServerResourcesForGroupVersion(groupVersion)
if err != nil {
if apierrors.IsNotFound(err) {
return initialize.Bool(false), nil
}

return nil, err
}

for _, resource := range resourceList.APIResources {
if resource.Kind == kind {
return initialize.Bool(true), nil
}
}

return initialize.Bool(false), nil
}
14 changes: 6 additions & 8 deletions internal/controller/postgrescluster/snapshots.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/crunchydata/postgres-operator/internal/config"
"github.com/crunchydata/postgres-operator/internal/feature"
"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
"github.com/crunchydata/postgres-operator/internal/postgres"
Expand Down Expand Up @@ -56,14 +57,11 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
return nil
}

// Check if the Kube cluster has VolumeSnapshots installed. If VolumeSnapshots
// are not installed, we need to return early. If user is attempting to use
// VolumeSnapshots, return an error, otherwise return nil.
volumeSnapshotKindExists, err := r.GroupVersionKindExists("snapshot.storage.k8s.io/v1", "VolumeSnapshot")
if err != nil {
return err
}
if !*volumeSnapshotKindExists {
// Return early when VolumeSnapshots are not installed in Kubernetes.
// If user is attempting to use VolumeSnapshots, return an error.
if !kubernetes.Has(
ctx, volumesnapshotv1.SchemeGroupVersion.WithKind("VolumeSnapshot"),
) {
if postgrescluster.Spec.Backups.Snapshots != nil {
return errors.New("VolumeSnapshots are not installed/enabled in this Kubernetes cluster; cannot create snapshot.")
} else {
Expand Down
38 changes: 16 additions & 22 deletions internal/controller/postgrescluster/snapshots_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,12 @@ import (
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/discovery"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/crunchydata/postgres-operator/internal/controller/runtime"
"github.com/crunchydata/postgres-operator/internal/feature"
"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
"github.com/crunchydata/postgres-operator/internal/testing/events"
Expand All @@ -33,26 +33,26 @@ import (

func TestReconcileVolumeSnapshots(t *testing.T) {
ctx := context.Background()
cfg, cc := setupKubernetes(t)
_, cc := setupKubernetes(t)
require.ParallelCapacity(t, 1)
discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
assert.NilError(t, err)

recorder := events.NewRecorder(t, runtime.Scheme)
r := &Reconciler{
Client: cc,
Owner: client.FieldOwner(t.Name()),
DiscoveryClient: discoveryClient,
Recorder: recorder,
Client: cc,
Owner: client.FieldOwner(t.Name()),
Recorder: recorder,
}
ns := setupNamespace(t, cc)

// Enable snapshots feature gate
// Enable snapshots feature gate and API
gate := feature.NewGate()
assert.NilError(t, gate.SetFromMap(map[string]bool{
feature.VolumeSnapshots: true,
}))
ctx = feature.NewContext(ctx, gate)
ctx = kubernetes.NewAPIContext(ctx, kubernetes.NewAPISet(
volumesnapshotv1.SchemeGroupVersion.WithKind("VolumeSnapshot"),
))

t.Run("SnapshotsDisabledDeleteSnapshots", func(t *testing.T) {
// Create cluster (without snapshots spec)
Expand Down Expand Up @@ -348,16 +348,13 @@ func TestReconcileVolumeSnapshots(t *testing.T) {

func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
ctx := context.Background()
cfg, cc := setupKubernetes(t)
discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
assert.NilError(t, err)
_, cc := setupKubernetes(t)

recorder := events.NewRecorder(t, runtime.Scheme)
r := &Reconciler{
Client: cc,
Owner: client.FieldOwner(t.Name()),
DiscoveryClient: discoveryClient,
Recorder: recorder,
Client: cc,
Owner: client.FieldOwner(t.Name()),
Recorder: recorder,
}

// Enable snapshots feature gate
Expand Down Expand Up @@ -1253,14 +1250,11 @@ func TestGetLatestReadySnapshot(t *testing.T) {

func TestDeleteSnapshots(t *testing.T) {
ctx := context.Background()
cfg, cc := setupKubernetes(t)
discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
assert.NilError(t, err)
_, cc := setupKubernetes(t)

r := &Reconciler{
Client: cc,
Owner: client.FieldOwner(t.Name()),
DiscoveryClient: discoveryClient,
Client: cc,
Owner: client.FieldOwner(t.Name()),
}
ns := setupNamespace(t, cc)

Expand Down
3 changes: 1 addition & 2 deletions internal/controller/standalone_pgadmin/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ type PGAdminReconciler struct {
ctx context.Context, namespace, pod, container string,
stdin io.Reader, stdout, stderr io.Writer, command ...string,
) error
Recorder record.EventRecorder
IsOpenShift bool
Recorder record.EventRecorder
}

//+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={list,watch}
Expand Down
14 changes: 9 additions & 5 deletions internal/controller/standalone_pgadmin/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package standalone_pgadmin

import (
"context"
"fmt"
"strings"

Expand All @@ -14,6 +15,7 @@ import (

"github.com/crunchydata/postgres-operator/internal/config"
"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/naming"
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
)
Expand Down Expand Up @@ -443,8 +445,8 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f:

// podSecurityContext returns a v1.PodSecurityContext for pgadmin that can write
// to PersistentVolumes.
func podSecurityContext(r *PGAdminReconciler) *corev1.PodSecurityContext {
podSecurityContext := initialize.PodSecurityContext()
func podSecurityContext(ctx context.Context) *corev1.PodSecurityContext {
psc := initialize.PodSecurityContext()

// TODO (dsessler7): Add ability to add supplemental groups

Expand All @@ -454,9 +456,11 @@ func podSecurityContext(r *PGAdminReconciler) *corev1.PodSecurityContext {
// - https://cloud.redhat.com/blog/a-guide-to-openshift-and-uids
// - https://docs.k8s.io/tasks/configure-pod-container/security-context/
// - https://docs.openshift.com/container-platform/4.14/authentication/managing-security-context-constraints.html
if !r.IsOpenShift {
podSecurityContext.FSGroup = initialize.Int64(2)
if !kubernetes.Has(ctx, kubernetes.API{
Group: "security.openshift.io", Kind: "SecurityContextConstraints",
}) {
psc.FSGroup = initialize.Int64(2)
}

return podSecurityContext
return psc
}
14 changes: 9 additions & 5 deletions internal/controller/standalone_pgadmin/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package standalone_pgadmin

import (
"context"
"testing"

"gotest.tools/v3/assert"
Expand All @@ -13,6 +14,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/crunchydata/postgres-operator/internal/initialize"
"github.com/crunchydata/postgres-operator/internal/kubernetes"
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
)
Expand Down Expand Up @@ -434,14 +436,16 @@ func TestPodConfigFiles(t *testing.T) {
}

func TestPodSecurityContext(t *testing.T) {
pgAdminReconciler := &PGAdminReconciler{}

assert.Assert(t, cmp.MarshalMatches(podSecurityContext(pgAdminReconciler), `
ctx := context.Background()
assert.Assert(t, cmp.MarshalMatches(podSecurityContext(ctx), `
fsGroup: 2
fsGroupChangePolicy: OnRootMismatch
`))

pgAdminReconciler.IsOpenShift = true
assert.Assert(t, cmp.MarshalMatches(podSecurityContext(pgAdminReconciler),
ctx = kubernetes.NewAPIContext(ctx, kubernetes.NewAPISet(kubernetes.API{
Group: "security.openshift.io", Version: "v1",
Kind: "SecurityContextConstraints",
}))
assert.Assert(t, cmp.MarshalMatches(podSecurityContext(ctx),
`fsGroupChangePolicy: OnRootMismatch`))
}
6 changes: 3 additions & 3 deletions internal/controller/standalone_pgadmin/statefulset.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ func (r *PGAdminReconciler) reconcilePGAdminStatefulSet(
ctx context.Context, pgadmin *v1beta1.PGAdmin,
configmap *corev1.ConfigMap, dataVolume *corev1.PersistentVolumeClaim,
) error {
sts := statefulset(r, pgadmin, configmap, dataVolume)
sts := statefulset(ctx, pgadmin, configmap, dataVolume)

// Previous versions of PGO used a StatefulSet Pod Management Policy that could leave the Pod
// in a failed state. When we see that it has the wrong policy, we will delete the StatefulSet
Expand Down Expand Up @@ -58,7 +58,7 @@ func (r *PGAdminReconciler) reconcilePGAdminStatefulSet(

// statefulset defines the StatefulSet needed to run pgAdmin.
func statefulset(
r *PGAdminReconciler,
ctx context.Context,
pgadmin *v1beta1.PGAdmin,
configmap *corev1.ConfigMap,
dataVolume *corev1.PersistentVolumeClaim,
Expand Down Expand Up @@ -115,7 +115,7 @@ func statefulset(
// set the image pull secrets, if any exist
sts.Spec.Template.Spec.ImagePullSecrets = pgadmin.Spec.ImagePullSecrets

sts.Spec.Template.Spec.SecurityContext = podSecurityContext(r)
sts.Spec.Template.Spec.SecurityContext = podSecurityContext(ctx)

pod(pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume)

Expand Down
10 changes: 5 additions & 5 deletions internal/postgres/reconcile.go
Original file line number Diff line number Diff line change
Expand Up @@ -276,14 +276,14 @@ func InstancePod(ctx context.Context,
// PodSecurityContext returns a v1.PodSecurityContext for cluster that can write
// to PersistentVolumes.
func PodSecurityContext(cluster *v1beta1.PostgresCluster) *corev1.PodSecurityContext {
podSecurityContext := initialize.PodSecurityContext()
psc := initialize.PodSecurityContext()

// Use the specified supplementary groups except for root. The CRD has
// similar validation, but we should never emit a PodSpec with that group.
// - https://docs.k8s.io/concepts/security/pod-security-standards/
for i := range cluster.Spec.SupplementalGroups {
if gid := cluster.Spec.SupplementalGroups[i]; gid > 0 {
podSecurityContext.SupplementalGroups = append(podSecurityContext.SupplementalGroups, gid)
psc.SupplementalGroups = append(psc.SupplementalGroups, gid)
}
}

Expand All @@ -293,9 +293,9 @@ func PodSecurityContext(cluster *v1beta1.PostgresCluster) *corev1.PodSecurityCon
// - https://cloud.redhat.com/blog/a-guide-to-openshift-and-uids
// - https://docs.k8s.io/tasks/configure-pod-container/security-context/
// - https://docs.openshift.com/container-platform/4.8/authentication/managing-security-context-constraints.html
if cluster.Spec.OpenShift == nil || !*cluster.Spec.OpenShift {
podSecurityContext.FSGroup = initialize.Int64(26)
if !initialize.FromPointer(cluster.Spec.OpenShift) {
psc.FSGroup = initialize.Int64(26)
}

return podSecurityContext
return psc
}

0 comments on commit b4d7444

Please sign in to comment.