Create cross namespace secrets #1490

RafiaSabih · 2021-05-14T15:14:30Z

If namespace is provided with the username in manifest,
then create the secret for that user in that namespace.
Also, the name of the secret will contain the namespace.

pkg/cluster/cluster.go

FxKu · 2021-05-19T07:56:20Z

pkg/cluster/cluster.go

+		if strings.Contains(username, ".") {
+			splits := strings.Split(username, ".")
+			name = splits[1]
+			namespace = splits[0]


what if the username is .user? Then namespace is "" and should become the default namespace

Right now, such a username is considered invalid username and I think this should continue. I don't see a reason why this should be allowed, if no namespace is provided then username only should be provided.

- include namespace in secret name only when namespace is provided - use username.namespace as key to pgUsers only when namespace is provided - avoid conflict in the role creation in db by checking namespace alongwith the username

Jan-M · 2021-05-26T13:50:14Z

pkg/cluster/cluster.go

+		name := username
+		namespace := c.Namespace
+
+		if strings.Contains(username, ".") {


Needs some attention.

Also include update to regex in manifest schema.

- update regular expression for usernames - add test to allow check for valid usernames - create pg roles with namespace (if any) appended in rolename

sdudoladov · 2021-05-27T13:52:24Z

docs/reference/operator_parameters.md

-  TPR) and `{tprgroup}` with the group of the CRD. No other placeholders are
-  allowed. The default is
-  `{username}.{cluster}.credentials.{tprkind}.{tprgroup}`.
+  operator. `{namesapce}` is replaced with name of the namespace (if any,


typo: namesApce -> namespace

sdudoladov · 2021-05-27T13:57:39Z

docs/reference/operator_parameters.md

-  allowed. The default is
-  `{username}.{cluster}.credentials.{tprkind}.{tprgroup}`.
+  operator. `{namesapce}` is replaced with name of the namespace (if any,
+  otherwise empty), `{username}` is replaced with name of the secret,


That is an ambiguous description because a secret always lives in a namespace.
My best guess is that what is meant here is

This part of the secret name is empty if the secret and a PG cluster it belongs to are in the same namespace

sdudoladov · 2021-05-27T14:02:03Z

e2e/tests/test_e2e.py

+        '''
+        app_namespace = "appspace"
+        k8s = self.k8s
+        k8s.api.core_v1.create_namespace(app_namespace)


I am unsure if kind always creates a new namespace fast enough. It is better to have some type of timeout / waiting here to avoid spurious test failures.

sdudoladov · 2021-05-27T14:04:21Z

e2e/tests/test_e2e.py

+                }
+            })
+        self.eventuallyEqual(lambda: k8s.count_secrets_in_namespace(app_namespace),
+                             1, "Secret not created in user namespace")


To make it more informative, please log the name of the secret and the namespace

sdudoladov · 2021-05-27T14:12:53Z

pkg/cluster/cluster_test.go

+
+	err := cluster.initRobotUsers()
+	if err != nil {
+		t.Errorf("%s Could not create namespaced users with error: %s", testName, err)


Does namespaced user here really mean namespaced secret ?

I doubt there is a need to log a test name, the golang test framework likely reports it automatically

sdudoladov · 2021-05-27T14:15:01Z

pkg/cluster/cluster_test.go

+func TestValidUsernames(t *testing.T) {
+	testName := "test username validity"
+
+	invalidUsernames := []string{"_", ".", ".user", "appspace.", "appspace.user.extra", "user_", "_user", "-user", "user-", ",", ",user", "user,", "namespace,user"}


Is the name - disallowed by other means ?

sdudoladov · 2021-05-27T14:20:47Z

pkg/cluster/sync.go

-	for _, cachedUser := range c.pgUsersCache {
-		if _, exists := c.pgUsers[cachedUser.Name]; !exists {
-			userNames = append(userNames, cachedUser.Name)
+		pg_role := u.Name


pg_role should be pgRole because of the golang naming conventions.

sdudoladov · 2021-05-27T14:27:08Z

pkg/cluster/sync.go

-		userNames = append(userNames, u.Name)
-		// add team member role name with rename suffix in case we need to rename it back
-		if u.Origin == spec.RoleOriginTeamsAPI && c.OpConfig.EnableTeamMemberDeprecation {
-			deletedUsers[u.Name+c.OpConfig.RoleDeletionSuffix] = u.Name
-			userNames = append(userNames, u.Name+c.OpConfig.RoleDeletionSuffix)
-		}
-	}
-
-	// add team members that exist only in cache
-	// to trigger a rename of the role in ProduceSyncRequests
-	for _, cachedUser := range c.pgUsersCache {
-		if _, exists := c.pgUsers[cachedUser.Name]; !exists {
-			userNames = append(userNames, cachedUser.Name)


What happened to these lines ? they are still needed for renaming roles removed fro PostgresTeam CRD

sdudoladov · 2021-05-27T14:32:04Z

pkg/cluster/sync.go

+		if u.Namespace != c.Namespace {
+			// to avoid the conflict of having multiple users of same name
+			// but each in different namespace.
+			pg_role = fmt.Sprintf("%s.%s", u.Name, u.Namespace)


so the template for the secret name is namespace.username but (in case of this name conflict) the role also gets a namespace as part of its name ? Will the resultant secret contain the namespace twice ?

…into cross_namespace_secret

sdudoladov · 2021-06-01T11:04:27Z

charts/postgres-operator/values-crd.yaml

-  # template for database user secrets generated by the operator
+  # template for database user secrets generated by the operator,
+  # here username contains the namespace in the format namespace.username
+  # if the user is in different namespace than cluster


I think it is the secret in the different namespace not the user.

sdudoladov · 2021-06-01T11:18:38Z

e2e/tests/test_e2e.py

+        k8s = self.k8s
+        v1_appnamespace = client.V1Namespace(metadata=client.V1ObjectMeta(name=app_namespace))
+        try:
+            k8s.api.core_v1.create_namespace(v1_appnamespace)


It is unlikely the actual namespace creation will timeout. What I meant in my previous review was the following:

The API call to create namespace may take some time, especially on kind where we know from experience even basic k8s operations sometimes take minutes

the patch at line 603 may run into problems because of that

Since we run e2e tests in different environments (locally, GitHub Action, CDP etc) that may lead to unwanted spurious test failures

Earlier I used simple timeouts in such cases, but now we have function like self.k8s.wait_for_pod_start and so on. You may just add a new one named self.k8s.wait_for_namespace_creation

sdudoladov · 2021-06-01T11:25:20Z

e2e/tests/test_e2e.py

+                }
+            })
+        self.eventuallyEqual(lambda: k8s.count_secrets_with_label("cluster_name=acid-minimal-cluster,application=spilo", app_namespace),
+                             1, "Secret not created for user in namespace", app_namespace)


To which parameter of eventuallyEqual does app_namespace correspond ?

sdudoladov · 2021-06-01T11:32:14Z

pkg/cluster/sync.go

+		if u.Namespace != c.Namespace {
+			// to avoid the conflict of having multiple users of same name
+			// but each in different namespace.
+			pgRole = fmt.Sprintf("%s.%s", u.Name, u.Namespace)


I feel that behavior needs to be logged and mentioned in the docs. Just because it may happen an end user unaware of the name conflict expects to get username but gets instead namespace.username

e2e/tests/test_e2e.py

pkg/cluster/cluster.go

FxKu · 2021-06-08T17:58:58Z

pkg/cluster/sync.go

 	for _, u := range c.pgUsers {
-		userNames = append(userNames, u.Name)
+		pgRole := u.Name
+		if u.Namespace != c.Namespace {


make sure u.Namespace is always set. Currently it's missing in initTeamMembers and readPgUsersFromDatabase methods which leads to the wrong name appended here. This makes the second e2e test fail atm (it wants to create a user that already exists).

scratch initTeamMembers and readPgUsersFromDatabase. It's easier to just add && u.Namespace != "" here to not touch more unit tests. I pushed the change now.

Jan-M · 2021-06-10T13:46:53Z

👍

RafiaSabih · 2021-06-10T14:41:51Z

👍

FxKu · 2021-06-11T08:35:16Z

👍

Create cross namespace secrets

8cf76d8

RafiaSabih requested review from CyberDem0n, erthalion, FxKu, Jan-M and sdudoladov as code owners May 14, 2021 15:14

add test cases

6205020

FxKu added the zalando label May 19, 2021

FxKu reviewed May 19, 2021

View reviewed changes

pkg/cluster/cluster.go Outdated Show resolved Hide resolved

FxKu reviewed May 19, 2021

View reviewed changes

Rafia Sabih added 4 commits May 19, 2021 17:39

fixes

af719c0

Fixes

43154ba

- include namespace in secret name only when namespace is provided - use username.namespace as key to pgUsers only when namespace is provided - avoid conflict in the role creation in db by checking namespace alongwith the username

Update unit tests

188e812

Fix test case

f572047

Jan-M reviewed May 26, 2021

View reviewed changes

Rafia Sabih added 4 commits May 27, 2021 13:25

Fixes

fd5edea

- update regular expression for usernames - add test to allow check for valid usernames - create pg roles with namespace (if any) appended in rolename

merge master

4e9816b

add more test cases for valid usernames

25a6417

update docs

0f31b5b

sdudoladov reviewed May 27, 2021

View reviewed changes

Rafia Sabih added 3 commits May 31, 2021 16:07

fixes as per review comments

09039e8

update e2e

c0bfca9

Merge branch 'master' of https://github.com/zalando/postgres-operator …

b2171be

…into cross_namespace_secret

sdudoladov reviewed Jun 1, 2021

View reviewed changes

Rafia Sabih added 4 commits June 4, 2021 16:53

fixes

f0472fd

Add toggle to allow namespaced secrets

a992494

update docs

9e1d906

comment update

2f89354

FxKu reviewed Jun 8, 2021

View reviewed changes

e2e/tests/test_e2e.py Outdated Show resolved Hide resolved

Update e2e/tests/test_e2e.py

1ed916f

FxKu reviewed Jun 8, 2021

View reviewed changes

pkg/cluster/cluster.go Show resolved Hide resolved

FxKu reviewed Jun 8, 2021

View reviewed changes

FxKu and others added 6 commits June 8, 2021 19:59

update codegen and merge with master

775aa34

few minor fixes

fcd9d27

fix unit tests

27c6ac2

fix e2e

8f183c2

fix e2e attempt 2

880c8a8

fix e2e

4ca2c56

FxKu merged commit 75a9e2b into master Jun 11, 2021

FxKu added this to the 1.7 milestone Jun 11, 2021

sigi4 mentioned this pull request Jun 16, 2021

How to properly upgrade CRD and cluster #1527

Closed

FxKu mentioned this pull request Jul 27, 2021

fix searching for users with namespace in name #1569

Merged

Create cross namespace secrets #1490

Create cross namespace secrets #1490

Uh oh!

Conversation

RafiaSabih commented May 14, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FxKu Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jan-M commented Jun 10, 2021

Uh oh!

RafiaSabih commented Jun 10, 2021

Uh oh!

FxKu commented Jun 11, 2021

Uh oh!

Uh oh!

FxKu Jun 9, 2021 •

edited

Loading