PostgreSQL user creation logic was implemented #542

DoodgeMatvey · 2023-08-31T16:26:01Z

Closes #428

worryg0d · 2023-09-01T07:22:37Z

controllers/clusterresources/postgresqluser_controller.go

+			event = models.Created
+			u.Status.ClustersEvents[namespacedName] = event


You can omit to assign to event var and just do u.Status.ClustersEvents[namespacedName] = models.Created

Agree, fixed

worryg0d · 2023-09-01T07:23:03Z

controllers/clusterresources/postgresqluser_controller.go

+			}
+
+			l.Info("User has been created", "username", newUsername)
+			fmt.Println("userName and ns:", u.Name, namespacedName)


Please delete this log

worryg0d · 2023-09-01T07:27:05Z

controllers/clusterresources/postgresqluser_controller.go

+		Name:      parts[1],
+		Namespace: parts[0],
+	}
+	err := r.Get(context.TODO(), userSecretNamespacedName, defaultUserSecret)


Maybe you should you the context that is passed into your Reconcile method?

It is reconcile, not controller

But this func is used in reconcile func, so you just can pass it's context in createUser func trought signature

Agree, fixed in all such cases

worryg0d · 2023-09-01T07:37:50Z

controllers/clusterresources/postgresqluser_controller.go

+	createUserQuery := fmt.Sprintf(`CREATE USER "%s" WITH PASSWORD '%s'`, newUserName, newPassword)
+	_, err = conn.Exec(context.Background(), createUserQuery)
+	if err != nil {
+		return fmt.Errorf("cannot execute creation user query in postgresql err, err: %w", err)


cannot execute creation user query in postgresql err, err -> cannot execute creation user query in postgresql, err: %w

worryg0d · 2023-09-01T07:51:41Z

controllers/clusterresources/postgresqluser_controller.go

+	createUserQuery := fmt.Sprintf(`CREATE USER "%s" WITH PASSWORD '%s'`, newUserName, newPassword)
+	_, err = conn.Exec(context.Background(), createUserQuery)


conn.Exec requires query and optinal arguments. I think you should omit to create the whole query by yourself. Just do

createUserQuery := `CREATE USER '$1' WITH PASSWORD '$2'` _, err = conn.Exec(ctx, createUserQuery, newUserName, newPassword)

I hope pgx driver also validates passed arguments to avoid SQL injections, which are possible in your case.

We cannot do this because it is important to have " " and ' ' in query

Uhm, I didn't get what you meant. I gave the example with those '' in the query.

I mean that example that you provided won't work. In the comment above, I described why

Oh, I see. You have to use the quotes for the SQL.
If you put those " " and ' ' quotes to the example of mine, does the problem still occur? Because it looks a bit strange, so I'm asking about it.
Edited example:

createUserQuery := `CREATE USER "$1" WITH PASSWORD '$2'`

I already tested your solution and it does not work. That's why I wrote this query in fmt.Sprintf

worryg0d · 2023-09-01T07:53:50Z

controllers/clusterresources/postgresqluser_controller.go

+			APIVersion: models.ClusterresourcesV1beta1APIVersion,
+		},
+		ObjectMeta: ctrl.ObjectMeta{
+			Name:        fmt.Sprintf("%s-%s-%s", models.ClusterNetworkFirewallRulePrefix, clusterID, nodeName),


Please use the previously created var firewallRuleName

worryg0d · 2023-09-01T07:58:15Z

controllers/clusterresources/postgresqluser_controller.go

+		Status: clusterresourcesv1beta1.ClusterNetworkFirewallRuleStatus{
+			FirewallRuleStatus: clusterresourcesv1beta1.FirewallRuleStatus{
+				ID:             "",
+				DeferredReason: "",
+				Status:         "",
+			},
+		},


If I'm not wrong there is no any pointer value, so you can omit this Status initialization

worryg0d · 2023-09-01T08:01:05Z

controllers/clusterresources/postgresqluser_controller.go

+		return false, err
+	}
+
+	return true, err


return true, err -> return true, nil

testisnullus · 2023-09-01T09:26:41Z

controllers/clusterresources/helpers.go


 	if len(username) == 0 || len(password) == 0 {
 		return "", "", models.ErrMissingSecretKeys
 	}

-	return username[:len(username)-1], password[:len(password)-1], nil


How can we handle the case if the customer will encode creds without removing the last character?

Added validation for such things, If the user adds an indent, we will handle it and remove it

testisnullus · 2023-09-01T10:03:09Z

controllers/clusterresources/postgresqluser_controller.go

+
+	for namespacedName, event := range u.Status.ClustersEvents {
+		if event == models.CreatingEvent {
+			l.Info("Creating user", "user", u, "namespacedName", namespacedName)


namespacedName -> userSecretNamespacedName or secretNamespacedName

Refactored, changed this to ClusterID and add new field in status DefaultSecretNamespacedName

tengu-alt · 2023-09-01T12:46:03Z

apis/clusters/v1beta1/postgresql_types.go

@@ -622,3 +601,22 @@ func (pgs *PgStatus) DCsFromInstAPI(iDCs []*models.PGDataCentre) (dcs []*DataCen
 	}
 	return
 }
+
+func GetDefaultPgUserSecret(


You can implement it as reconciler method, wdyt?

Because it is a separate function that does not use postgresql cluster fields and gets the name and namespace for the secret. So I think we can leave this as just a function and not a method.

tengu-alt · 2023-09-12T11:18:59Z

apis/clusterresources/v1beta1/postgresqluser_types.go

+	// The keys of the map represent the cluster IDs, while the corresponding values represent the events themselves.
+	ClustersEvents              map[string]string `json:"clustersEvents,omitempty"`
+	DefaultSecretNamespacedName string            `json:"defaultSecretNamespacedName,omitempty"`
+}


We can store NamespacedName as struct wdyt?

yes, we can, changed

tengu-alt · 2023-09-12T11:24:08Z

controllers/clusterresources/postgresqluser_controller.go

+	}
+
+	clusterName := defaultUserSecret.Labels[models.ControlledByLabel]
+	clusterID := defaultUserSecret.Labels[models.ClusterIDLabel]


You can pass ClusterID trought func signature when you iterate by cluster events

tengu-alt · 2023-09-12T11:34:16Z

controllers/clusterresources/postgresqluser_controller.go

+	}
+
+	defaultUserSecret := &k8sCore.Secret{}
+	userSecretNamespacedName := types.NamespacedName{


userSecretNamespacedName -> defaultUserSecretNamespacedName

tengu-alt · 2023-09-12T11:36:53Z

controllers/clusterresources/postgresqluser_controller.go

+	if err != nil {
+		if k8sErrors.IsNotFound(err) {
+			return fmt.Errorf("cannot get default PostgreSQL user secret, username: %s , err: %w", parts[1], err)
+		}


We can use userName as userSecretNamespacedName.Name instead of parts[1] for more readability. If you agree fix in all similar cases

tengu-alt · 2023-09-12T11:42:26Z

controllers/clusterresources/postgresqluser_controller.go

+	exposeServiceList, err := exposeservice.GetExposeService(r.Client, clusterName, parts[0])
+	if err != nil {
+		return fmt.Errorf("cannot list expose services for cluster: %s, err: %w", parts[0], err)
+	}


I guess parts[0] is a secret namespace not a cluster name(also we can use here already created clusterName variable)

logic changed, refactored

tengu-alt · 2023-09-12T12:26:41Z

controllers/clusters/postgresql_controller.go

+		return err
+	}
+
+	secret, err := v1beta1.GetDefaultPgUserSecret(context.TODO(), c.Name, c.Namespace, r.Client)


Why you dont use ctx that we have in func signature?

tengu-alt · 2023-09-12T12:36:33Z

controllers/clusters/postgresql_controller.go

+		u.Status.ClustersEvents = make(map[string]string)
+	}
+
+	u.Status.DefaultSecretNamespacedName = defaultSecretNamespacedName.String()


Will it work on multiple cluster?

nice catch, fixed and tested

worryg0d · 2023-09-12T12:53:31Z

controllers/clusterresources/postgresqluser_controller.go

+				ClusterID: clusterID,
+				Type:      models.PgAppType,
+			},
+			Network: fmt.Sprintf("%s/%s", nodeAddress, "32"),


You can just do

fmt.Sprintf("%s/32", nodeAddress)

It also can be, but I prefer my variant to move it in const in future

worryg0d · 2023-09-12T13:06:49Z

controllers/clusters/postgresql_controller.go

+		u.Status.ClustersEvents = make(map[string]string)
+	}
+
+	u.Status.DefaultSecretNamespacedName = defaultSecretNamespacedName.String()


You need the reference to the default user in the PostgreSQLUserController to create a new one. But what if there is more than 1 cluster reference to the given user resource?
I think it would be better if we save the reference in another map like map[ClusterID]DefaultSecretNamespacedName. Or modify ClusterEvents to something like that:

type ClusterEvent struct { Event string `json:"event,omitempty"` DefaultSecretRef types.NamespacedName `json:"defautSecretRef,omitempty"` } type PostgreSQLUserStatus struct { ClusterEvents map[ClusterID]ClusterEvent `json:"clusterEvents,omitempty" }

nice catch, fixed and tested

testisnullus · 2023-09-14T18:40:53Z

controllers/clusterresources/postgresqluser_controller.go

+import (
+	"context"
+	"errors"
+	"fmt"
+	"github.com/jackc/pgx/v5"
+	k8sCore "k8s.io/api/core/v1"
+	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/tools/record"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/log"


Sort imports please

testisnullus · 2023-09-14T19:03:52Z

controllers/clusters/postgresql_controller.go

+
+		l.Error(err, "Cannot get PostgreSQL user", "user", u.Spec)
+		r.EventRecorder.Eventf(c, models.Warning, models.CreationFailed,
+			"Cannot get PostgreSQL user. user reference: %v", uRef)


Cannot get PostgreSQL user. user reference -> Cannot get PostgreSQL user. User reference

ribaraka · 2023-09-15T09:27:13Z

apis/clusters/v1beta1/postgresql_types.go

@@ -89,6 +89,7 @@ type PgSpec struct {
 	ClusterConfigurations map[string]string `json:"clusterConfigurations,omitempty"`
 	Description           string            `json:"description,omitempty"`
 	SynchronousModeStrict bool              `json:"synchronousModeStrict,omitempty"`
+	UserRefs              []*UserReference  `json:"userRef,omitempty"`


json:userRefs

ribaraka · 2023-09-15T09:33:52Z

apis/clusterresources/v1beta1/postgresqluser_types.go

+type NamespacedName struct {
+	Namespace string `json:"namespace"`
+	Name      string `json:"name"`
+}


I think there is no need to create this struct, please just use the existing types.NamespacedName.

Here we need to create ours because types.NamespacedName does not have json representation

ribaraka · 2023-09-15T09:45:41Z

controllers/clusterresources/postgresqluser_controller.go

+	newUsername, newPassword, err := getUserCreds(s)
+	if err != nil {
+		l.Error(err, "Cannot get the PostgreSQL user credentials from the secret",
+			"secret name", s.Name,
+			"secret namespace", s.Namespace)
+		r.EventRecorder.Eventf(u, models.Warning, models.CreatingEvent,
+			"Cannot get the PostgreSQL user credentials from the secret. Reason: %v", err)
+
+		return models.ReconcileRequeue, nil
+	}


Please move this block of code before adding finalizers

and name vars just username and password

Moved this block before finalazers, we have default username and password and new password. We need to clearly distinguish them for better readability

ribaraka · 2023-09-15T09:49:45Z

controllers/clusterresources/postgresqluser_controller.go

+			err = r.Status().Patch(ctx, u, patch)
+			if err != nil {
+				l.Error(err, "Cannot patch PostgreSQL user status")
+				r.EventRecorder.Eventf(u, models.Warning, models.PatchFailed,
+					"Resource patch is failed. Reason: %v", err)
+
+				return models.ReconcileRequeue, nil
+			}


please make messages more detailed

added details

DoodgeMatvey added the enhancement New feature or request label Aug 31, 2023

DoodgeMatvey requested review from RostislavPorohnya, ribaraka, testisnullus, tengu-alt, OleksiienkoMykyta and worryg0d August 31, 2023 16:26

DoodgeMatvey self-assigned this Aug 31, 2023

DoodgeMatvey force-pushed the issue-134-create-user branch 2 times, most recently from 7b03af4 to 0fc2d03 Compare August 31, 2023 16:35

worryg0d requested changes Sep 1, 2023

View reviewed changes

DoodgeMatvey force-pushed the issue-134-create-user branch from 0fc2d03 to bea26b8 Compare September 1, 2023 09:34

DoodgeMatvey requested a review from worryg0d September 1, 2023 09:51

testisnullus requested changes Sep 1, 2023

View reviewed changes

tengu-alt reviewed Sep 1, 2023

View reviewed changes

DoodgeMatvey force-pushed the issue-134-create-user branch 2 times, most recently from 7b4b068 to 625feaf Compare September 11, 2023 11:38

DoodgeMatvey requested review from tengu-alt and testisnullus September 11, 2023 13:47

DoodgeMatvey force-pushed the issue-134-create-user branch from 625feaf to d129a66 Compare September 11, 2023 15:59

tengu-alt reviewed Sep 12, 2023

View reviewed changes

worryg0d reviewed Sep 12, 2023

View reviewed changes

vendor

e62448c

DoodgeMatvey force-pushed the issue-134-create-user branch from d129a66 to 564a055 Compare September 14, 2023 16:37

testisnullus requested changes Sep 14, 2023

View reviewed changes

DoodgeMatvey force-pushed the issue-134-create-user branch from 564a055 to 4854c32 Compare September 14, 2023 19:19

DoodgeMatvey requested review from testisnullus, worryg0d and tengu-alt September 14, 2023 19:24

testisnullus approved these changes Sep 14, 2023

View reviewed changes

worryg0d approved these changes Sep 15, 2023

View reviewed changes

ribaraka requested changes Sep 15, 2023

View reviewed changes

PostgreSQL user creation logic was implemented

4e030b4

DoodgeMatvey force-pushed the issue-134-create-user branch from 4854c32 to 4e030b4 Compare September 15, 2023 10:15

DoodgeMatvey requested a review from ribaraka September 15, 2023 10:21

ribaraka approved these changes Sep 15, 2023

View reviewed changes

DoodgeMatvey merged commit 0753266 into main Sep 15, 2023
1 check passed

		event = models.Created
		u.Status.ClustersEvents[namespacedName] = event

		createUserQuery := fmt.Sprintf(`CREATE USER "%s" WITH PASSWORD '%s'`, newUserName, newPassword)
		_, err = conn.Exec(context.Background(), createUserQuery)

PostgreSQL user creation logic was implemented #542

PostgreSQL user creation logic was implemented #542

Conversation

DoodgeMatvey commented Aug 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoodgeMatvey Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worryg0d Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

DoodgeMatvey Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DoodgeMatvey Sep 1, 2023 •

edited

Loading

worryg0d Sep 11, 2023 •

edited

Loading

DoodgeMatvey Sep 12, 2023 •

edited

Loading