From c63c5fe7720c0dce22369c3e513932108d5f24f7 Mon Sep 17 00:00:00 2001 From: Pieter Callewaert Date: Fri, 26 Sep 2025 08:45:05 +0200 Subject: [PATCH 1/2] Simplify logging --- api/v1alpha1/zz_generated.deepcopy.go | 20 ++++ cmd/main.go | 45 ++++---- .../crd/bases/db.movetokube.com_postgres.yaml | 17 ++- .../db.movetokube.com_postgresusers.yaml | 32 +++--- internal/controller/postgres_controller.go | 20 ++-- .../controller/postgres_controller_test.go | 108 +++++------------- .../controller/postgresuser_controller.go | 8 +- .../postgresuser_controller_test.go | 4 +- pkg/config/config.go | 35 +++++- pkg/postgres/aws.go | 7 +- pkg/postgres/azure.go | 5 +- pkg/postgres/database.go | 13 +-- pkg/postgres/gcp.go | 10 +- pkg/postgres/mock/postgres.go | 41 ++++--- pkg/postgres/postgres.go | 58 +++++----- pkg/postgres/role.go | 5 +- 16 files changed, 211 insertions(+), 217 deletions(-) diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 9d8d57b6a..c21128086 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -160,6 +160,21 @@ func (in *PostgresUser) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PostgresUserAWSSpec) DeepCopyInto(out *PostgresUserAWSSpec) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PostgresUserAWSSpec. +func (in *PostgresUserAWSSpec) DeepCopy() *PostgresUserAWSSpec { + if in == nil { + return nil + } + out := new(PostgresUserAWSSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresUserList) DeepCopyInto(out *PostgresUserList) { *out = *in @@ -202,6 +217,11 @@ func (in *PostgresUserSpec) DeepCopyInto(out *PostgresUserSpec) { (*out)[key] = val } } + if in.AWS != nil { + in, out := &in.AWS, &out.AWS + *out = new(PostgresUserAWSSpec) + **out = **in + } if in.Annotations != nil { in, out := &in.Annotations, &out.Annotations *out = make(map[string]string, len(*in)) diff --git a/cmd/main.go b/cmd/main.go index 14fbecc64..ef524fb6e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -15,6 +15,7 @@ import ( clientgoscheme "k8s.io/client-go/kubernetes/scheme" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" + logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics/filters" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" @@ -28,10 +29,7 @@ import ( // +kubebuilder:scaffold:imports ) -var ( - scheme = runtime.NewScheme() - setupLog = ctrl.Log.WithName("setup") -) +var scheme = runtime.NewScheme() func init() { utilruntime.Must(clientgoscheme.AddToScheme(scheme)) @@ -57,14 +55,12 @@ func main() { "If set, the metrics endpoint is served securely via HTTPS. Use --metrics-secure=false to use HTTP instead.") flag.BoolVar(&enableHTTP2, "enable-http2", true, "If set, HTTP/2 will be enabled for the metrics and webhook servers") - opts := zap.Options{ - Development: true, - } + opts := zap.Options{} opts.BindFlags(flag.CommandLine) flag.Parse() - ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) - + logger := zap.New(zap.UseFlagOptions(&opts)) + logf.SetLogger(logger) // if the enable-http2 flag is false (the default), http/2 should be disabled // due to its vulnerabilities. More specifically, disabling http/2 will // prevent from being vulnerable to the HTTP/2 Stream Cancellation and @@ -72,7 +68,7 @@ func main() { // - https://github.com/advisories/GHSA-qppj-fm5r-hxr3 // - https://github.com/advisories/GHSA-4374-p667-p6c8 disableHTTP2 := func(c *tls.Config) { - setupLog.Info("disabling http/2") + logger.Info("disabling http/2") c.NextProtos = []string{"http/1.1"} } @@ -111,9 +107,9 @@ func main() { cfg := config.Get() lockName := "lock" if cfg.AnnotationFilter == "" { - setupLog.Info("No POSTGRES_INSTANCE set, this instance will only process CRs without an annotation") + logger.Info("No POSTGRES_INSTANCE set, this instance will only process CRs without an annotation") } else { - setupLog.Info("POSTGRES_INSTANCE is set, this instance will only process CRs with the correct annotation", "annotation", cfg.AnnotationFilter) + logger.Info("POSTGRES_INSTANCE is set, this instance will only process CRs with the correct annotation", "annotation", cfg.AnnotationFilter) lockName += "-" + cfg.AnnotationFilter } cacheOpts := cache.Options{} @@ -145,38 +141,45 @@ func main() { // LeaderElectionReleaseOnCancel: true, }) if err != nil { - setupLog.Error(err, "unable to start manager") + logger.Error(err, "unable to start manager") os.Exit(1) } - pg, err := postgres.NewPG(cfg, ctrl.Log) + pg, err := postgres.NewPG(cfg, logger) if err != nil { - setupLog.Error(err, "DB-Connection failed", "cfg", cfg) + // Avoid logging sensitive information like PostgresPass + logger.Error(err, "DB-Connection failed", "cfg", map[string]any{ + "Host": cfg.PostgresHost, + "User": cfg.PostgresUser, + "UriArgs": cfg.PostgresUriArgs, + "CloudProvider": cfg.CloudProvider, + "DefaultDatabase": cfg.PostgresDefaultDb, + }) os.Exit(1) } if err = (controller.NewPostgresReconciler(mgr, cfg, pg)).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "Postgres") + logger.Error(err, "unable to create controller", "controller", "Postgres") os.Exit(1) } if err = (controller.NewPostgresUserReconciler(mgr, cfg, pg)).SetupWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create controller", "controller", "PostgresUser") + logger.Error(err, "unable to create controller", "controller", "PostgresUser") os.Exit(1) } // +kubebuilder:scaffold:builder if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil { - setupLog.Error(err, "unable to set up health check") + logger.Error(err, "unable to set up health check") os.Exit(1) } if err := mgr.AddReadyzCheck("readyz", healthz.Ping); err != nil { - setupLog.Error(err, "unable to set up ready check") + logger.Error(err, "unable to set up ready check") os.Exit(1) } - setupLog.Info("starting manager") + logger.Info("starting manager") if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { - setupLog.Error(err, "problem running manager") + logger.Error(err, "problem running manager") os.Exit(1) } } diff --git a/config/crd/bases/db.movetokube.com_postgres.yaml b/config/crd/bases/db.movetokube.com_postgres.yaml index 209ed20b7..10b1f2585 100644 --- a/config/crd/bases/db.movetokube.com_postgres.yaml +++ b/config/crd/bases/db.movetokube.com_postgres.yaml @@ -20,14 +20,19 @@ spec: description: Postgres is the Schema for the postgres API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object diff --git a/config/crd/bases/db.movetokube.com_postgresusers.yaml b/config/crd/bases/db.movetokube.com_postgresusers.yaml index 0cbd8510a..bb466d47e 100644 --- a/config/crd/bases/db.movetokube.com_postgresusers.yaml +++ b/config/crd/bases/db.movetokube.com_postgresusers.yaml @@ -20,14 +20,19 @@ spec: description: PostgresUser is the Schema for the postgresusers API properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -39,28 +44,23 @@ spec: type: string type: object aws: - description: AWS specific settings for this user. + description: PostgresUserAWSSpec encapsulates AWS specific configuration + toggles. properties: enableIamAuth: - description: Enable IAM authentication for this user (PostgreSQL on AWS RDS only) - default: false type: boolean type: object database: - description: Name of the PostgresDatabase this user will be related to type: string labels: additionalProperties: type: string type: object privileges: - description: List of privileges to grant to this user type: string role: - description: Name of the PostgresRole this user will be associated with type: string secretName: - description: Name of the secret to create with user credentials type: string secretTemplate: additionalProperties: @@ -74,11 +74,10 @@ spec: status: description: PostgresUserStatus defines the observed state of PostgresUser properties: - enableIamAuth: - description: Reflects whether IAM authentication is enabled for this user. - type: boolean databaseName: type: string + enableIamAuth: + type: boolean postgresGroup: type: string postgresLogin: @@ -89,6 +88,7 @@ spec: type: boolean required: - databaseName + - enableIamAuth - postgresGroup - postgresLogin - postgresRole diff --git a/internal/controller/postgres_controller.go b/internal/controller/postgres_controller.go index 96b0b62eb..da19b93e2 100644 --- a/internal/controller/postgres_controller.go +++ b/internal/controller/postgres_controller.go @@ -82,27 +82,27 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c if !instance.GetDeletionTimestamp().IsZero() { if r.shouldDropDB(ctx, instance, reqLogger) && instance.Status.Succeeded { if instance.Status.Roles.Owner != "" { - err := r.pg.DropRole(instance.Status.Roles.Owner, r.pg.GetUser(), instance.Spec.Database, reqLogger) + err := r.pg.DropRole(instance.Status.Roles.Owner, r.pg.GetUser(), instance.Spec.Database) if err != nil { return ctrl.Result{}, err } instance.Status.Roles.Owner = "" } if instance.Status.Roles.Reader != "" { - err = r.pg.DropRole(instance.Status.Roles.Reader, r.pg.GetUser(), instance.Spec.Database, reqLogger) + err = r.pg.DropRole(instance.Status.Roles.Reader, r.pg.GetUser(), instance.Spec.Database) if err != nil { return ctrl.Result{}, err } instance.Status.Roles.Reader = "" } if instance.Status.Roles.Writer != "" { - err = r.pg.DropRole(instance.Status.Roles.Writer, r.pg.GetUser(), instance.Spec.Database, reqLogger) + err = r.pg.DropRole(instance.Status.Roles.Writer, r.pg.GetUser(), instance.Spec.Database) if err != nil { return ctrl.Result{}, err } instance.Status.Roles.Writer = "" } - err = r.pg.DropDatabase(instance.Spec.Database, reqLogger) + err = r.pg.DropDatabase(instance.Spec.Database) if err != nil { return ctrl.Result{}, err } @@ -196,7 +196,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c continue } // Execute create extension SQL statement - err = r.pg.CreateExtension(instance.Spec.Database, extension, reqLogger) + err = r.pg.CreateExtension(instance.Spec.Database, extension) if err != nil { reqLogger.Error(err, fmt.Sprintf("Could not add extensions %s", extension)) continue @@ -224,7 +224,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } // Create schema - err = r.pg.CreateSchema(database, owner, schema, reqLogger) + err = r.pg.CreateSchema(database, owner, schema) if err != nil { reqLogger.Error(err, fmt.Sprintf("Could not create schema %s", schema)) continue @@ -243,7 +243,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c Privs: readerPrivs, CreateSchema: false, } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesReader, reqLogger) + err = r.pg.SetSchemaPrivileges(schemaPrivilegesReader) if err != nil { reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\"", reader, readerPrivs)) continue @@ -257,7 +257,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c FunctionPrivs: writerFunctionPrivs, CreateSchema: true, } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesWriter, reqLogger) + err = r.pg.SetSchemaPrivileges(schemaPrivilegesWriter) if err != nil { reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", writer, writerPrivs, writerSequencePrivs, writerFunctionPrivs)) continue @@ -271,7 +271,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c FunctionPrivs: ownerFunctionPrivs, CreateSchema: true, } - err = r.pg.SetSchemaPrivileges(schemaPrivilegesOwner, reqLogger) + err = r.pg.SetSchemaPrivileges(schemaPrivilegesOwner) if err != nil { reqLogger.Error(err, fmt.Sprintf("Could not give %s permissions \"%s\", sequence privileges \"%s\", and function privileges \"%s\"", owner, ownerPrivs, ownerSequencePrivs, ownerFunctionPrivs)) continue @@ -293,6 +293,7 @@ func (r *PostgresReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c reqLogger.Info("Reconciling done") return ctrl.Result{}, nil } + func (r *PostgresReconciler) addFinalizer(reqLogger logr.Logger, m *dbv1alpha1.Postgres) error { if len(m.GetFinalizers()) < 1 && m.GetDeletionTimestamp() == nil { reqLogger.Info("adding Finalizer for Postgres") @@ -300,6 +301,7 @@ func (r *PostgresReconciler) addFinalizer(reqLogger logr.Logger, m *dbv1alpha1.P } return nil } + func (r *PostgresReconciler) requeue(cr *dbv1alpha1.Postgres, reason error) (ctrl.Result, error) { cr.Status.Succeeded = false return ctrl.Result{}, reason diff --git a/internal/controller/postgres_controller_test.go b/internal/controller/postgres_controller_test.go index b98167be9..824dd246d 100644 --- a/internal/controller/postgres_controller_test.go +++ b/internal/controller/postgres_controller_test.go @@ -144,10 +144,7 @@ var _ = Describe("PostgresReconciler", func() { }) Describe("Checking deletion logic", func() { - - var ( - postgresCR *v1alpha1.Postgres - ) + var postgresCR *v1alpha1.Postgres BeforeEach(func() { postgresCR = &v1alpha1.Postgres{ @@ -167,11 +164,9 @@ var _ = Describe("PostgresReconciler", func() { }, }, } - }) Context("DropOnDelete is unset", func() { - BeforeEach(func() { initClient(postgresCR, true) }) @@ -196,17 +191,15 @@ var _ = Describe("PostgresReconciler", func() { It("should not try to delete roles or database", func() { // Neither DropRole nor DropDatabase should be called - pg.EXPECT().DropRole(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) - pg.EXPECT().DropDatabase(gomock.Any(), gomock.Any()).Times(0) + pg.EXPECT().DropRole(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + pg.EXPECT().DropDatabase(gomock.Any()).Times(0) // Call Reconcile err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) }) - }) Context("DropOnDelete is enabled", func() { - var ( dropGroupRole *gomock.Call dropReaderRole *gomock.Call @@ -217,10 +210,10 @@ var _ = Describe("PostgresReconciler", func() { BeforeEach(func() { // Expected function calls pg.EXPECT().GetUser().Return("pguser").AnyTimes() - dropGroupRole = pg.EXPECT().DropRole(name+"-owner", "pguser", name, gomock.Any()) - dropReaderRole = pg.EXPECT().DropRole(name+"-reader", "pguser", name, gomock.Any()) - dropWriterRole = pg.EXPECT().DropRole(name+"-writer", "pguser", name, gomock.Any()) - dropDatabase = pg.EXPECT().DropDatabase(name, gomock.Any()) + dropGroupRole = pg.EXPECT().DropRole(name+"-owner", "pguser", name) + dropReaderRole = pg.EXPECT().DropRole(name+"-reader", "pguser", name) + dropWriterRole = pg.EXPECT().DropRole(name+"-writer", "pguser", name) + dropDatabase = pg.EXPECT().DropDatabase(name) // Create Postgres with DropOnDelete == true anotherPostgres := postgresCR.DeepCopy() anotherPostgres.Spec.DropOnDelete = true @@ -230,7 +223,6 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Deletion is successful", func() { - It("should remove finalizer", func() { // No method should return error dropGroupRole.Return(nil) @@ -242,7 +234,7 @@ var _ = Describe("PostgresReconciler", func() { // Call Reconcile err := runReconcile(rp, ctx, req) // Patching both the object and its status fails when using the the FakeClient - //if testEnv != nil { + // if testEnv != nil { Expect(err).NotTo(HaveOccurred()) // Check updated Postgres @@ -253,13 +245,10 @@ var _ = Describe("PostgresReconciler", func() { Expect(foundPostgres.GetFinalizers()).To(BeEmpty()) } //} - }) - }) Context("Deletion is not successful", func() { - It("should not remove finalizer when any database action fails", func() { // DropDatabase fails dropDatabase.Return(fmt.Errorf("Could not drop database")) @@ -272,7 +261,6 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.GetFinalizers()).To(ConsistOf("finalizer.db.movetokube.com")) }) - }) Context("Another Postgres exists with same database", func() { @@ -316,11 +304,8 @@ var _ = Describe("PostgresReconciler", func() { err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) }) - }) - }) - }) Describe("Checking creation logic", func() { @@ -340,7 +325,6 @@ var _ = Describe("PostgresReconciler", func() { }) Context("MasterRole is unset", func() { - BeforeEach(func() { initClient(postgresCR, false) }) @@ -357,11 +341,9 @@ var _ = Describe("PostgresReconciler", func() { err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) }) - }) Context("MasterRole is set", func() { - BeforeEach(func() { // Create client modPostgres := postgresCR.DeepCopy() @@ -381,11 +363,9 @@ var _ = Describe("PostgresReconciler", func() { err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) }) - }) Context("Correct annotation filter is set", func() { - BeforeEach(func() { // Create client modPostgres := postgresCR.DeepCopy() @@ -411,7 +391,6 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Incorrect annotation filter is set", func() { - BeforeEach(func() { // Create client modPostgres := postgresCR.DeepCopy() @@ -429,7 +408,6 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Creation is successful", func() { - BeforeEach(func() { initClient(postgresCR, false) // Expected function calls @@ -464,11 +442,9 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.GetFinalizers()).To(ContainElement(expectedFinalizer)) }) - }) Context("Creation is not successful", func() { - BeforeEach(func() { initClient(postgresCR.DeepCopy(), false) // Expected function calls @@ -491,9 +467,7 @@ var _ = Describe("PostgresReconciler", func() { Expect(foundPostgres.Status.Roles).To(Equal(expectedRoles)) Expect(foundPostgres.Status.Succeeded).To(BeFalse()) }) - }) - }) Describe("Checking extensions logic", func() { @@ -515,14 +489,13 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Postgres has no extensions", func() { - BeforeEach(func() { initClient(postgresCR, false) }) It("should not try to create extensions", func() { // CreateExtension should not be called - pg.EXPECT().CreateExtension(name, gomock.Any(), gomock.Any()).Times(0) + pg.EXPECT().CreateExtension(name, gomock.Any()).Times(0) // Call Reconcile err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -537,11 +510,9 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Extensions).To(BeEmpty()) }) - }) Context("Postgres has extensions", func() { - BeforeEach(func() { // Add extensions to Postgres object extPostgres := postgresCR.DeepCopy() @@ -550,11 +521,10 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Creation is successful", func() { - BeforeEach(func() { // Expected method calls - pg.EXPECT().CreateExtension(name, "pg_stat_statements", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().CreateExtension(name, "hstore", gomock.Any()).Return(nil).Times(1) + pg.EXPECT().CreateExtension(name, "pg_stat_statements").Return(nil).Times(1) + pg.EXPECT().CreateExtension(name, "hstore").Return(nil).Times(1) }) It("should update status", func() { @@ -566,15 +536,13 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Extensions).To(ConsistOf("pg_stat_statements", "hstore")) }) - }) Context("Creation is not successful", func() { - BeforeEach(func() { // Expected method calls - pg.EXPECT().CreateExtension(name, "pg_stat_statements", gomock.Any()).Return(fmt.Errorf("Could not create extension")).Times(1) - pg.EXPECT().CreateExtension(name, "hstore", gomock.Any()).Return(nil).Times(1) + pg.EXPECT().CreateExtension(name, "pg_stat_statements").Return(fmt.Errorf("Could not create extension")).Times(1) + pg.EXPECT().CreateExtension(name, "hstore").Return(nil).Times(1) }) It("should update status", func() { @@ -586,13 +554,10 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Extensions).To(ConsistOf("hstore")) }) - }) - }) Context("Subset of extensions already created", func() { - BeforeEach(func() { // Add extensions to Postgres object extPostgres := postgresCR.DeepCopy() @@ -602,11 +567,10 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Creation is successful", func() { - It("should not recreate existing extension", func() { // Expected method calls - pg.EXPECT().CreateExtension(name, "pg_stat_statements", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().CreateExtension(name, "hstore", gomock.Any()).Times(0) + pg.EXPECT().CreateExtension(name, "pg_stat_statements").Return(nil).Times(1) + pg.EXPECT().CreateExtension(name, "hstore").Times(0) // Call reconcile err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -615,11 +579,8 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Extensions).To(ConsistOf("hstore", "pg_stat_statements")) }) - }) - }) - }) Describe("Checking schemas logic", func() { @@ -646,14 +607,13 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Postgres has no schemas", func() { - BeforeEach(func() { initClient(postgresCR, false) }) It("should not try to create schemas", func() { // CreateSchema should not be called - pg.EXPECT().CreateSchema(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Times(0) + pg.EXPECT().CreateSchema(gomock.Any(), gomock.Any(), gomock.Any()).Times(0) // Call Reconcile err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -668,11 +628,9 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Schemas).To(BeEmpty()) }) - }) Context("Postgres has schemas", func() { - BeforeEach(func() { // Add schemas to Postgres object schemaPostgres := postgresCR.DeepCopy() @@ -681,15 +639,14 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Creation is successful", func() { - BeforeEach(func() { // Expected method calls // customers schema - pg.EXPECT().CreateSchema(name, name+"-group", "customers", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().CreateSchema(name, name+"-group", "customers").Return(nil).Times(1) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) // stores schema - pg.EXPECT().CreateSchema(name, name+"-group", "stores", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().CreateSchema(name, name+"-group", "stores").Return(nil).Times(1) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) }) It("should update status", func() { @@ -701,19 +658,17 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Schemas).To(ConsistOf("customers", "stores")) }) - }) Context("Creation is not successful", func() { - BeforeEach(func() { // Expected method calls // customers schema errors - pg.EXPECT().CreateSchema(name, name+"-group", "customers", gomock.Any()).Return(fmt.Errorf("Could not create schema")).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().CreateSchema(name, name+"-group", "customers").Return(fmt.Errorf("Could not create schema")).Times(1) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(0) // stores schema - pg.EXPECT().CreateSchema(name, name+"-group", "stores", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().CreateSchema(name, name+"-group", "stores").Return(nil).Times(1) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) }) It("should update status", func() { @@ -725,13 +680,10 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Schemas).To(ConsistOf("stores")) }) - }) - }) Context("Subset of schema already created", func() { - BeforeEach(func() { // Add schemas to Postgres object schemaPostgres := postgresCR.DeepCopy() @@ -741,13 +693,13 @@ var _ = Describe("PostgresReconciler", func() { }) Context("Creation is successful", func() { - It("should not recreate existing schema", func() { // customers schema - pg.EXPECT().CreateSchema(name, name+"-group", "customers", gomock.Any()).Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().CreateSchema(name, name+"-group", "customers").Return(nil).Times(1) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) // stores schema already exists - pg.EXPECT().CreateSchema(name, name+"-group", "stores", gomock.Any()).Times(0) + pg.EXPECT().CreateSchema(name, name+"-group", "stores").Times(0) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(0) // Call reconcile err := runReconcile(rp, ctx, req) Expect(err).NotTo(HaveOccurred()) @@ -756,11 +708,7 @@ var _ = Describe("PostgresReconciler", func() { Expect(cl.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, foundPostgres)).To(BeNil()) Expect(foundPostgres.Status.Schemas).To(ConsistOf("stores", "customers")) }) - }) - }) - }) - }) diff --git a/internal/controller/postgresuser_controller.go b/internal/controller/postgresuser_controller.go index ad36bf5a7..aeb0dc8bc 100644 --- a/internal/controller/postgresuser_controller.go +++ b/internal/controller/postgresuser_controller.go @@ -33,7 +33,7 @@ type PostgresUserReconciler struct { pgUriArgs string instanceFilter string keepSecretName bool // use secret name as defined in PostgresUserSpec - cloudProvider string + cloudProvider config.CloudProvider } // NewPostgresUserReconciler returns a new reconcile.Reconciler @@ -99,8 +99,7 @@ func (r *PostgresUserReconciler) Reconcile(ctx context.Context, req ctrl.Request if postgres != nil && postgres.GetDeletionTimestamp().IsZero() { db = instance.Status.DatabaseName } - err = r.pg.DropRole(instance.Status.PostgresRole, instance.Status.PostgresGroup, - db, reqLogger) + err = r.pg.DropRole(instance.Status.PostgresRole, instance.Status.PostgresGroup, db) if err != nil { return ctrl.Result{}, err } @@ -120,7 +119,6 @@ func (r *PostgresUserReconciler) Reconcile(ctx context.Context, req ctrl.Request role, login string ) password, err := utils.GetSecureRandomString(15) - if err != nil { return r.requeue(ctx, instance, err) } @@ -178,7 +176,7 @@ func (r *PostgresUserReconciler) Reconcile(ctx context.Context, req ctrl.Request awsConfig := instance.Spec.AWS awsIamRequested := awsConfig != nil && awsConfig.EnableIamAuth - if r.cloudProvider == "AWS" { + if r.cloudProvider == config.CloudProviderAWS { if awsIamRequested && !instance.Status.EnableIamAuth { if err := r.pg.GrantRole("rds_iam", role); err != nil { reqLogger.WithValues("role", role).Error(err, "failed to grant rds_iam role") diff --git a/internal/controller/postgresuser_controller_test.go b/internal/controller/postgresuser_controller_test.go index 543671da4..87072f6ec 100644 --- a/internal/controller/postgresuser_controller_test.go +++ b/internal/controller/postgresuser_controller_test.go @@ -189,7 +189,7 @@ var _ = Describe("PostgresUser Controller", func() { // Expect DropRole to be called pg.EXPECT().GetDefaultDatabase().Return("postgres") pg.EXPECT().DropRole(postgresUser.Status.PostgresRole, postgresUser.Status.PostgresGroup, - databaseName, gomock.Any()).Return(nil) + databaseName).Return(nil) // Call Reconcile err := runReconcile(rp, ctx, req) @@ -209,7 +209,7 @@ var _ = Describe("PostgresUser Controller", func() { // Expect DropRole to fail pg.EXPECT().GetDefaultDatabase().Return("postgres") pg.EXPECT().DropRole(postgresUser.Status.PostgresRole, postgresUser.Status.PostgresGroup, - databaseName, gomock.Any()).Return(fmt.Errorf("failed to drop role")) + databaseName).Return(fmt.Errorf("failed to drop role")) // Call Reconcile err := runReconcile(rp, ctx, req) Expect(err).To(HaveOccurred()) diff --git a/pkg/config/config.go b/pkg/config/config.go index 5bd44a45d..5b63497c2 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -3,6 +3,7 @@ package config import ( "net/url" "strconv" + "strings" "sync" "github.com/movetokube/postgres-operator/pkg/utils" @@ -14,13 +15,24 @@ type Cfg struct { PostgresPass string PostgresUriArgs string PostgresDefaultDb string - CloudProvider string + CloudProvider CloudProvider AnnotationFilter string KeepSecretName bool } -var doOnce sync.Once -var config *Cfg +var ( + doOnce sync.Once + config *Cfg +) + +type CloudProvider string + +const ( + CloudProviderNone CloudProvider = "None" + CloudProviderAWS CloudProvider = "AWS" + CloudProviderAzure CloudProvider = "Azure" + CloudProviderGCP CloudProvider = "GCP" +) func Get() *Cfg { doOnce.Do(func() { @@ -30,7 +42,7 @@ func Get() *Cfg { config.PostgresPass = url.PathEscape(utils.MustGetEnv("POSTGRES_PASS")) config.PostgresUriArgs = utils.MustGetEnv("POSTGRES_URI_ARGS") config.PostgresDefaultDb = utils.GetEnv("POSTGRES_DEFAULT_DATABASE") - config.CloudProvider = utils.GetEnv("POSTGRES_CLOUD_PROVIDER") + config.CloudProvider = ParseCloudProvider(utils.GetEnv("POSTGRES_CLOUD_PROVIDER")) config.AnnotationFilter = utils.GetEnv("POSTGRES_INSTANCE") if value, err := strconv.ParseBool(utils.GetEnv("KEEP_SECRET_NAME")); err == nil { config.KeepSecretName = value @@ -38,3 +50,18 @@ func Get() *Cfg { }) return config } + +// CloudProvider is an enum for supported cloud providers. + +func ParseCloudProvider(s string) CloudProvider { + switch strings.ToLower(s) { + case "aws": + return CloudProviderAWS + case "azure": + return CloudProviderAzure + case "gcp": + return CloudProviderGCP + default: + return CloudProviderNone + } +} diff --git a/pkg/postgres/aws.go b/pkg/postgres/aws.go index 61e732357..a27167a4d 100644 --- a/pkg/postgres/aws.go +++ b/pkg/postgres/aws.go @@ -3,7 +3,6 @@ package postgres import ( "fmt" - "github.com/go-logr/logr" "github.com/lib/pq" ) @@ -54,7 +53,7 @@ func (c *awspg) CreateUserRole(role, password string) (string, error) { return returnedRole, nil } -func (c *awspg) DropRole(role, newOwner, database string, logger logr.Logger) error { +func (c *awspg) DropRole(role, newOwner, database string) error { // On AWS RDS the postgres user isn't really superuser so he doesn't have permissions // to REASSIGN OWNED BY unless he belongs to both roles err := c.GrantRole(role, c.user) @@ -69,12 +68,12 @@ func (c *awspg) DropRole(role, newOwner, database string, logger logr.Logger) er if err != nil && err.(*pq.Error).Code != "0LP01" { if err.(*pq.Error).Code == "42704" { // The group role does not exist, no point of granting roles - logger.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) + c.log.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) return nil } return err } defer c.RevokeRole(newOwner, c.user) - return c.pg.DropRole(role, newOwner, database, logger) + return c.pg.DropRole(role, newOwner, database) } diff --git a/pkg/postgres/azure.go b/pkg/postgres/azure.go index 99628bcb7..a87696a0c 100644 --- a/pkg/postgres/azure.go +++ b/pkg/postgres/azure.go @@ -1,7 +1,6 @@ package postgres import ( - "github.com/go-logr/logr" "github.com/lib/pq" ) @@ -35,7 +34,7 @@ func (azpg *azurepg) CreateDB(dbname, role string) error { return azpg.pg.CreateDB(dbname, role) } -func (azpg *azurepg) DropRole(role, newOwner, database string, logger logr.Logger) error { +func (azpg *azurepg) DropRole(role, newOwner, database string) error { // Grant the role to the user first err := azpg.GrantRole(role, azpg.user) if err != nil && err.(*pq.Error).Code != "0LP01" { @@ -46,5 +45,5 @@ func (azpg *azurepg) DropRole(role, newOwner, database string, logger logr.Logge } // Delegate to parent implementation to perform the actual drop - return azpg.pg.DropRole(role, newOwner, database, logger) + return azpg.pg.DropRole(role, newOwner, database) } diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 7608f8da3..753de6a57 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -3,7 +3,6 @@ package postgres import ( "fmt" - "github.com/go-logr/logr" "github.com/lib/pq" ) @@ -93,7 +92,7 @@ func (c *pg) CreateSchema(db, role, schema string, logger logr.Logger) error { return nil } -func (c *pg) DropDatabase(database string, logger logr.Logger) error { +func (c *pg) DropDatabase(database string) error { _, err := c.db.Exec(fmt.Sprintf(REVOKE_CONNECT, database)) // Error code 3D000 is returned if database doesn't exist if err != nil && err.(*pq.Error).Code != "3D000" { @@ -111,13 +110,13 @@ func (c *pg) DropDatabase(database string, logger logr.Logger) error { return err } - logger.Info(fmt.Sprintf("Dropped database %s", database)) + c.log.Info(fmt.Sprintf("Dropped database %s", database)) return nil } -func (c *pg) CreateExtension(db, extension string, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args, logger) +func (c *pg) CreateExtension(db, extension string) error { + tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args) if err != nil { return err } @@ -130,8 +129,8 @@ func (c *pg) CreateExtension(db, extension string, logger logr.Logger) error { return nil } -func (c *pg) SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, schemaPrivileges.DB, c.args, logger) +func (c *pg) SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges) error { + tmpDb, err := GetConnection(c.user, c.pass, c.host, schemaPrivileges.DB, c.args) if err != nil { return err } diff --git a/pkg/postgres/gcp.go b/pkg/postgres/gcp.go index 94e4dd778..a16faab4d 100644 --- a/pkg/postgres/gcp.go +++ b/pkg/postgres/gcp.go @@ -3,7 +3,6 @@ package postgres import ( "fmt" - "github.com/go-logr/logr" "github.com/lib/pq" ) @@ -18,7 +17,6 @@ func newGCPPG(postgres *pg) PG { } func (c *gcppg) CreateDB(dbname, role string) error { - err := c.GrantRole(role, c.user) if err != nil { return err @@ -30,9 +28,9 @@ func (c *gcppg) CreateDB(dbname, role string) error { return nil } -func (c *gcppg) DropRole(role, newOwner, database string, logger logr.Logger) error { +func (c *gcppg) DropRole(role, newOwner, database string) error { if role == "alloydbsuperuser" || role == "postgres" { - logger.Info(fmt.Sprintf("not dropping %s as it is a reserved AlloyDB role", role)) + c.log.V(1).Info(fmt.Sprintf("not dropping %s as it is a reserved AlloyDB role", role)) return nil } // On AlloyDB the postgres or other alloydbsuperuser members, aren't really superusers so they don't have permissions @@ -51,7 +49,7 @@ func (c *gcppg) DropRole(role, newOwner, database string, logger logr.Logger) er if err != nil && err.(*pq.Error).Code != "0LP01" { if err.(*pq.Error).Code == "42704" { // The group role does not exist, no point of granting roles - logger.Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) + c.log.V(1).Info(fmt.Sprintf("not granting %s to %s as %s does not exist", role, newOwner, newOwner)) return nil } return err @@ -59,5 +57,5 @@ func (c *gcppg) DropRole(role, newOwner, database string, logger logr.Logger) er defer c.RevokeRole(newOwner, c.user) } - return c.pg.DropRole(role, newOwner, database, logger) + return c.pg.DropRole(role, newOwner, database) } diff --git a/pkg/postgres/mock/postgres.go b/pkg/postgres/mock/postgres.go index 3c920cd09..83799779a 100644 --- a/pkg/postgres/mock/postgres.go +++ b/pkg/postgres/mock/postgres.go @@ -12,7 +12,6 @@ package mock_postgres import ( reflect "reflect" - logr "github.com/go-logr/logr" postgres "github.com/movetokube/postgres-operator/pkg/postgres" gomock "go.uber.org/mock/gomock" ) @@ -70,17 +69,17 @@ func (mr *MockPGMockRecorder) CreateDB(dbname, username any) *gomock.Call { } // CreateExtension mocks base method. -func (m *MockPG) CreateExtension(db, extension string, logger logr.Logger) error { +func (m *MockPG) CreateExtension(db, extension string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateExtension", db, extension, logger) + ret := m.ctrl.Call(m, "CreateExtension", db, extension) ret0, _ := ret[0].(error) return ret0 } // CreateExtension indicates an expected call of CreateExtension. -func (mr *MockPGMockRecorder) CreateExtension(db, extension, logger any) *gomock.Call { +func (mr *MockPGMockRecorder) CreateExtension(db, extension any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateExtension", reflect.TypeOf((*MockPG)(nil).CreateExtension), db, extension, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateExtension", reflect.TypeOf((*MockPG)(nil).CreateExtension), db, extension) } // CreateGroupRole mocks base method. @@ -112,17 +111,17 @@ func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock. } // CreateSchema mocks base method. -func (m *MockPG) CreateSchema(db, role, schema string, logger logr.Logger) error { +func (m *MockPG) CreateSchema(db, role, schema string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "CreateSchema", db, role, schema, logger) + ret := m.ctrl.Call(m, "CreateSchema", db, role, schema) ret0, _ := ret[0].(error) return ret0 } // CreateSchema indicates an expected call of CreateSchema. -func (mr *MockPGMockRecorder) CreateSchema(db, role, schema, logger any) *gomock.Call { +func (mr *MockPGMockRecorder) CreateSchema(db, role, schema any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateSchema", reflect.TypeOf((*MockPG)(nil).CreateSchema), db, role, schema, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateSchema", reflect.TypeOf((*MockPG)(nil).CreateSchema), db, role, schema) } // CreateUserRole mocks base method. @@ -141,31 +140,31 @@ func (mr *MockPGMockRecorder) CreateUserRole(role, password any) *gomock.Call { } // DropDatabase mocks base method. -func (m *MockPG) DropDatabase(db string, logger logr.Logger) error { +func (m *MockPG) DropDatabase(db string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DropDatabase", db, logger) + ret := m.ctrl.Call(m, "DropDatabase", db) ret0, _ := ret[0].(error) return ret0 } // DropDatabase indicates an expected call of DropDatabase. -func (mr *MockPGMockRecorder) DropDatabase(db, logger any) *gomock.Call { +func (mr *MockPGMockRecorder) DropDatabase(db any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DropDatabase", reflect.TypeOf((*MockPG)(nil).DropDatabase), db, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DropDatabase", reflect.TypeOf((*MockPG)(nil).DropDatabase), db) } // DropRole mocks base method. -func (m *MockPG) DropRole(role, newOwner, database string, logger logr.Logger) error { +func (m *MockPG) DropRole(role, newOwner, database string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DropRole", role, newOwner, database, logger) + ret := m.ctrl.Call(m, "DropRole", role, newOwner, database) ret0, _ := ret[0].(error) return ret0 } // DropRole indicates an expected call of DropRole. -func (mr *MockPGMockRecorder) DropRole(role, newOwner, database, logger any) *gomock.Call { +func (mr *MockPGMockRecorder) DropRole(role, newOwner, database any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DropRole", reflect.TypeOf((*MockPG)(nil).DropRole), role, newOwner, database, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DropRole", reflect.TypeOf((*MockPG)(nil).DropRole), role, newOwner, database) } // GetDefaultDatabase mocks base method. @@ -253,17 +252,17 @@ func (mr *MockPGMockRecorder) RevokeRole(role, revoked any) *gomock.Call { } // SetSchemaPrivileges mocks base method. -func (m *MockPG) SetSchemaPrivileges(schemaPrivileges postgres.PostgresSchemaPrivileges, logger logr.Logger) error { +func (m *MockPG) SetSchemaPrivileges(schemaPrivileges postgres.PostgresSchemaPrivileges) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "SetSchemaPrivileges", schemaPrivileges, logger) + ret := m.ctrl.Call(m, "SetSchemaPrivileges", schemaPrivileges) ret0, _ := ret[0].(error) return ret0 } // SetSchemaPrivileges indicates an expected call of SetSchemaPrivileges. -func (mr *MockPGMockRecorder) SetSchemaPrivileges(schemaPrivileges, logger any) *gomock.Call { +func (mr *MockPGMockRecorder) SetSchemaPrivileges(schemaPrivileges any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSchemaPrivileges", reflect.TypeOf((*MockPG)(nil).SetSchemaPrivileges), schemaPrivileges, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetSchemaPrivileges", reflect.TypeOf((*MockPG)(nil).SetSchemaPrivileges), schemaPrivileges) } // UpdatePassword mocks base method. diff --git a/pkg/postgres/postgres.go b/pkg/postgres/postgres.go index c4f12e527..4a04592da 100644 --- a/pkg/postgres/postgres.go +++ b/pkg/postgres/postgres.go @@ -3,7 +3,6 @@ package postgres import ( "database/sql" "fmt" - "log" "github.com/go-logr/logr" "github.com/movetokube/postgres-operator/pkg/config" @@ -11,8 +10,8 @@ import ( type PG interface { CreateDB(dbname, username string) error - CreateSchema(db, role, schema string, logger logr.Logger) error - CreateExtension(db, extension string, logger logr.Logger) error + CreateSchema(db, role, schema string) error + CreateExtension(db, extension string) error CreateGroupRole(role string) error RenameGroupRole(currentRole, newRole string) error CreateUserRole(role, password string) (string, error) @@ -23,20 +22,20 @@ type PG interface { SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges, logger logr.Logger) error RevokeRole(role, revoked string) error AlterDefaultLoginRole(role, setRole string) error - DropDatabase(db string, logger logr.Logger) error - DropRole(role, newOwner, database string, logger logr.Logger) error + DropDatabase(db string) error + DropRole(role, newOwner, database string) error GetUser() string GetDefaultDatabase() string } type pg struct { - db *sql.DB - log logr.Logger - host string - user string - pass string - args string - default_database string + db *sql.DB + log logr.Logger + host string + user string + pass string + args string + defaultDatabase string } type PostgresSchemaPrivileges struct { @@ -55,30 +54,29 @@ func NewPG(cfg *config.Cfg, logger logr.Logger) (PG, error) { cfg.PostgresPass, cfg.PostgresHost, cfg.PostgresDefaultDb, - cfg.PostgresUriArgs, - logger) + cfg.PostgresUriArgs) if err != nil { - log.Fatalf("failed to connect to PostgreSQL server: %s", err.Error()) + return nil, err } - logger.Info("connected to postgres server") + logger.V(1).Info("connected to postgres server") postgres := &pg{ - db: db, - log: logger, - host: cfg.PostgresHost, - user: cfg.PostgresUser, - pass: cfg.PostgresPass, - args: cfg.PostgresUriArgs, - default_database: cfg.PostgresDefaultDb, + db: db, + log: logger, + host: cfg.PostgresHost, + user: cfg.PostgresUser, + pass: cfg.PostgresPass, + args: cfg.PostgresUriArgs, + defaultDatabase: cfg.PostgresDefaultDb, } switch cfg.CloudProvider { - case "AWS": + case config.CloudProviderAWS: logger.Info("Using AWS wrapper") return newAWSPG(postgres), nil - case "Azure": + case config.CloudProviderAzure: logger.Info("Using Azure wrapper") return newAzurePG(postgres), nil - case "GCP": + case config.CloudProviderGCP: logger.Info("Using GCP wrapper") return newGCPPG(postgres), nil default: @@ -92,13 +90,13 @@ func (c *pg) GetUser() string { } func (c *pg) GetDefaultDatabase() string { - return c.default_database + return c.defaultDatabase } -func GetConnection(user, password, host, database, uri_args string, logger logr.Logger) (*sql.DB, error) { - db, err := sql.Open("postgres", fmt.Sprintf("postgresql://%s:%s@%s/%s?%s", user, password, host, database, uri_args)) +func GetConnection(user, password, host, database, uriArgs string) (*sql.DB, error) { + db, err := sql.Open("postgres", fmt.Sprintf("postgresql://%s:%s@%s/%s?%s", user, password, host, database, uriArgs)) if err != nil { - log.Fatal(err) + return nil, err } err = db.Ping() return db, err diff --git a/pkg/postgres/role.go b/pkg/postgres/role.go index 0b4d4b240..d6e57d2cc 100644 --- a/pkg/postgres/role.go +++ b/pkg/postgres/role.go @@ -3,7 +3,6 @@ package postgres import ( "fmt" - "github.com/go-logr/logr" "github.com/lib/pq" ) @@ -75,9 +74,9 @@ func (c *pg) RevokeRole(role, revoked string) error { return nil } -func (c *pg) DropRole(role, newOwner, database string, logger logr.Logger) error { +func (c *pg) DropRole(role, newOwner, database string) error { // REASSIGN OWNED BY only works if the correct database is selected - tmpDb, err := GetConnection(c.user, c.pass, c.host, database, c.args, logger) + tmpDb, err := GetConnection(c.user, c.pass, c.host, database, c.args) if err != nil { if err.(*pq.Error).Code == "3D000" { return nil // Database is does not exist (anymore) From 1b4bb72f209856523615ecc1e7e6df5fb79ed546 Mon Sep 17 00:00:00 2001 From: Pieter Callewaert Date: Wed, 17 Dec 2025 09:46:26 +0100 Subject: [PATCH 2/2] Update API definition --- api/v1alpha1/postgresuser_types.go | 14 +++-- .../db.movetokube.com_postgresusers.yaml | 12 ++++- .../controller/postgres_controller_test.go | 6 +-- pkg/postgres/database.go | 8 +-- pkg/postgres/mock/postgres.go | 52 +++++++++---------- pkg/postgres/postgres.go | 4 +- 6 files changed, 57 insertions(+), 39 deletions(-) diff --git a/api/v1alpha1/postgresuser_types.go b/api/v1alpha1/postgresuser_types.go index ac4cff832..a2d49d1a6 100644 --- a/api/v1alpha1/postgresuser_types.go +++ b/api/v1alpha1/postgresuser_types.go @@ -9,12 +9,16 @@ import ( // PostgresUserSpec defines the desired state of PostgresUser type PostgresUserSpec struct { - Role string `json:"role"` - Database string `json:"database"` + // Name of the PostgresRole this user will be associated with + Role string `json:"role"` + // Name of the PostgresDatabase this user will be related to + Database string `json:"database"` + // Name of the secret to create with user credentials SecretName string `json:"secretName"` // +optional SecretTemplate map[string]string `json:"secretTemplate,omitempty"` // key-value, where key is secret field, value is go template // +optional + // List of privileges to grant to this user Privileges string `json:"privileges"` // +optional AWS *PostgresUserAWSSpec `json:"aws,omitempty"` @@ -27,6 +31,8 @@ type PostgresUserSpec struct { // PostgresUserAWSSpec encapsulates AWS specific configuration toggles. type PostgresUserAWSSpec struct { // +optional + // +kubebuilder:default=false + // Enable IAM authentication for this user (PostgreSQL on AWS RDS only) EnableIamAuth bool `json:"enableIamAuth,omitempty"` } @@ -37,7 +43,9 @@ type PostgresUserStatus struct { PostgresLogin string `json:"postgresLogin"` PostgresGroup string `json:"postgresGroup"` DatabaseName string `json:"databaseName"` - EnableIamAuth bool `json:"enableIamAuth"` + // Reflects whether IAM authentication is enabled for this user. + // +optional + EnableIamAuth bool `json:"enableIamAuth"` } // +kubebuilder:object:root=true diff --git a/config/crd/bases/db.movetokube.com_postgresusers.yaml b/config/crd/bases/db.movetokube.com_postgresusers.yaml index bb466d47e..14b8d6e04 100644 --- a/config/crd/bases/db.movetokube.com_postgresusers.yaml +++ b/config/crd/bases/db.movetokube.com_postgresusers.yaml @@ -48,19 +48,28 @@ spec: toggles. properties: enableIamAuth: + default: false + description: Enable IAM authentication for this user (PostgreSQL + on AWS RDS only) type: boolean type: object database: + description: Name of the PostgresDatabase this user will be related + to type: string labels: additionalProperties: type: string type: object privileges: + description: List of privileges to grant to this user type: string role: + description: Name of the PostgresRole this user will be associated + with type: string secretName: + description: Name of the secret to create with user credentials type: string secretTemplate: additionalProperties: @@ -77,6 +86,8 @@ spec: databaseName: type: string enableIamAuth: + description: Reflects whether IAM authentication is enabled for this + user. type: boolean postgresGroup: type: string @@ -88,7 +99,6 @@ spec: type: boolean required: - databaseName - - enableIamAuth - postgresGroup - postgresLogin - postgresRole diff --git a/internal/controller/postgres_controller_test.go b/internal/controller/postgres_controller_test.go index 824dd246d..2f9d62b02 100644 --- a/internal/controller/postgres_controller_test.go +++ b/internal/controller/postgres_controller_test.go @@ -72,7 +72,7 @@ var _ = Describe("PostgresReconciler", func() { mockCtrl = gomock.NewController(GinkgoT()) pg = mockpg.NewMockPG(mockCtrl) pg.EXPECT().AlterDatabaseOwner(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() - pg.EXPECT().ReassignDatabaseOwner(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + pg.EXPECT().ReassignDatabaseOwner(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes() cl = k8sClient // Create runtime scheme sc = scheme.Scheme @@ -668,7 +668,7 @@ var _ = Describe("PostgresReconciler", func() { pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(0) // stores schema pg.EXPECT().CreateSchema(name, name+"-group", "stores").Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).AnyTimes() }) It("should update status", func() { @@ -696,7 +696,7 @@ var _ = Describe("PostgresReconciler", func() { It("should not recreate existing schema", func() { // customers schema pg.EXPECT().CreateSchema(name, name+"-group", "customers").Return(nil).Times(1) - pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(3) + pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).AnyTimes() // stores schema already exists pg.EXPECT().CreateSchema(name, name+"-group", "stores").Times(0) pg.EXPECT().SetSchemaPrivileges(gomock.Any()).Return(nil).Times(0) diff --git a/pkg/postgres/database.go b/pkg/postgres/database.go index 753de6a57..11fe8c432 100644 --- a/pkg/postgres/database.go +++ b/pkg/postgres/database.go @@ -57,12 +57,12 @@ func (c *pg) AlterDatabaseOwner(dbname, owner string) error { return err } -func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error { +func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string) error { if currentOwner == "" || newOwner == "" || currentOwner == newOwner { return nil } - tmpDb, err := GetConnection(c.user, c.pass, c.host, dbName, c.args, logger) + tmpDb, err := GetConnection(c.user, c.pass, c.host, dbName, c.args) if err != nil { return err } @@ -78,8 +78,8 @@ func (c *pg) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger return nil } -func (c *pg) CreateSchema(db, role, schema string, logger logr.Logger) error { - tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args, logger) +func (c *pg) CreateSchema(db, role, schema string) error { + tmpDb, err := GetConnection(c.user, c.pass, c.host, db, c.args) if err != nil { return err } diff --git a/pkg/postgres/mock/postgres.go b/pkg/postgres/mock/postgres.go index 83799779a..23cfdba8f 100644 --- a/pkg/postgres/mock/postgres.go +++ b/pkg/postgres/mock/postgres.go @@ -40,6 +40,20 @@ func (m *MockPG) EXPECT() *MockPGMockRecorder { return m.recorder } +// AlterDatabaseOwner mocks base method. +func (m *MockPG) AlterDatabaseOwner(dbName, owner string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AlterDatabaseOwner", dbName, owner) + ret0, _ := ret[0].(error) + return ret0 +} + +// AlterDatabaseOwner indicates an expected call of AlterDatabaseOwner. +func (mr *MockPGMockRecorder) AlterDatabaseOwner(dbName, owner any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AlterDatabaseOwner", reflect.TypeOf((*MockPG)(nil).AlterDatabaseOwner), dbName, owner) +} + // AlterDefaultLoginRole mocks base method. func (m *MockPG) AlterDefaultLoginRole(role, setRole string) error { m.ctrl.T.Helper() @@ -96,20 +110,6 @@ func (mr *MockPGMockRecorder) CreateGroupRole(role any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateGroupRole", reflect.TypeOf((*MockPG)(nil).CreateGroupRole), role) } -// RenameGroupRole mocks base method. -func (m *MockPG) RenameGroupRole(currentRole, newRole string) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RenameGroupRole", currentRole, newRole) - ret0, _ := ret[0].(error) - return ret0 -} - -// RenameGroupRole indicates an expected call of RenameGroupRole. -func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenameGroupRole", reflect.TypeOf((*MockPG)(nil).RenameGroupRole), currentRole, newRole) -} - // CreateSchema mocks base method. func (m *MockPG) CreateSchema(db, role, schema string) error { m.ctrl.T.Helper() @@ -209,32 +209,32 @@ func (mr *MockPGMockRecorder) GrantRole(role, grantee any) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GrantRole", reflect.TypeOf((*MockPG)(nil).GrantRole), role, grantee) } -// AlterDatabaseOwner mocks base method. -func (m *MockPG) AlterDatabaseOwner(dbName, owner string) error { +// ReassignDatabaseOwner mocks base method. +func (m *MockPG) ReassignDatabaseOwner(dbName, currentOwner, newOwner string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "AlterDatabaseOwner", dbName, owner) + ret := m.ctrl.Call(m, "ReassignDatabaseOwner", dbName, currentOwner, newOwner) ret0, _ := ret[0].(error) return ret0 } -// AlterDatabaseOwner indicates an expected call of AlterDatabaseOwner. -func (mr *MockPGMockRecorder) AlterDatabaseOwner(dbName, owner any) *gomock.Call { +// ReassignDatabaseOwner indicates an expected call of ReassignDatabaseOwner. +func (mr *MockPGMockRecorder) ReassignDatabaseOwner(dbName, currentOwner, newOwner any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AlterDatabaseOwner", reflect.TypeOf((*MockPG)(nil).AlterDatabaseOwner), dbName, owner) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReassignDatabaseOwner", reflect.TypeOf((*MockPG)(nil).ReassignDatabaseOwner), dbName, currentOwner, newOwner) } -// ReassignDatabaseOwner mocks base method. -func (m *MockPG) ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error { +// RenameGroupRole mocks base method. +func (m *MockPG) RenameGroupRole(currentRole, newRole string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ReassignDatabaseOwner", dbName, currentOwner, newOwner, logger) + ret := m.ctrl.Call(m, "RenameGroupRole", currentRole, newRole) ret0, _ := ret[0].(error) return ret0 } -// ReassignDatabaseOwner indicates an expected call of ReassignDatabaseOwner. -func (mr *MockPGMockRecorder) ReassignDatabaseOwner(dbName, currentOwner, newOwner, logger any) *gomock.Call { +// RenameGroupRole indicates an expected call of RenameGroupRole. +func (mr *MockPGMockRecorder) RenameGroupRole(currentRole, newRole any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ReassignDatabaseOwner", reflect.TypeOf((*MockPG)(nil).ReassignDatabaseOwner), dbName, currentOwner, newOwner, logger) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RenameGroupRole", reflect.TypeOf((*MockPG)(nil).RenameGroupRole), currentRole, newRole) } // RevokeRole mocks base method. diff --git a/pkg/postgres/postgres.go b/pkg/postgres/postgres.go index 4a04592da..dd5886a53 100644 --- a/pkg/postgres/postgres.go +++ b/pkg/postgres/postgres.go @@ -18,8 +18,8 @@ type PG interface { UpdatePassword(role, password string) error GrantRole(role, grantee string) error AlterDatabaseOwner(dbName, owner string) error - ReassignDatabaseOwner(dbName, currentOwner, newOwner string, logger logr.Logger) error - SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges, logger logr.Logger) error + ReassignDatabaseOwner(dbName, currentOwner, newOwner string) error + SetSchemaPrivileges(schemaPrivileges PostgresSchemaPrivileges) error RevokeRole(role, revoked string) error AlterDefaultLoginRole(role, setRole string) error DropDatabase(db string) error