[WIP] Extend infrastructure roles handling #1064

erthalion · 2020-07-16T14:54:49Z

Postgres Operator uses infrastructure roles to provide access to a
database for external users e.g. for monitoring purposes. Such
infrastructure roles are expected to be present in the form of k8s
secrets with the following content:

inrole1: some_encrypted_role
password1: some_encrypted_password
user1: some_entrypted_name

inrole2: some_encrypted_role
password2: some_encrypted_password
user2: some_entrypted_name

The format of this content is implied implicitely and not flexible
enough. In case if we do not have possibility to change the format of a
secret we want to use in the Operator, we need to recreate it in this
format.

To address this lets make the format of secret content explicitely. The
idea is to introduce a new configuration option for the Operator.

infrastructure_roles_secrets:
- secret: k8s_secret_name
  name: some_encrypted_name
  password: some_encrypted_password
  role: some_encrypted_role

- secret: k8s_secret_name
  name: some_encrypted_name
  password: some_encrypted_password
  role: some_encrypted_role

This would allow Operator to use any avalable secrets to prepare
infrastructure roles. To make it backward compatible simulate the old
behaviour if the new option is not present.

The new configuration option is intended be used mainly from CRD, but
it's also available via Operator ConfigMap in a limited fashion. For
ConfigMap one can put there only a string with one secret definition in
the following format (as a string):

infrastructure_roles_secret_name: |
    secret: k8s_secret_name,
    name: some_encrypted_name,
    password: some_encrypted_password,
    role: some_encrypted_role

Postgres Operator uses infrastructure roles to provide access to a database for external users e.g. for monitoring purposes. Such infrastructure roles are expected to be present in the form of k8s secrets with the following content: inrole1: some_encrypted_role password1: some_encrypted_password user1: some_entrypted_name inrole2: some_encrypted_role password2: some_encrypted_password user2: some_entrypted_name The format of this content is implied implicitely and not flexible enough. In case if we do not have possibility to change the format of a secret we want to use in the Operator, we need to recreate it in this format. To address this lets make the format of secret content explicitely. The idea is to introduce a new configuration option for the Operator. infrastructure_roles_secrets: - secret: k8s_secret_name name: some_encrypted_name password: some_encrypted_password role: some_encrypted_role - secret: k8s_secret_name name: some_encrypted_name password: some_encrypted_password role: some_encrypted_role This would allow Operator to use any avalable secrets to prepare infrastructure roles. To make it backward compatible simulate the old behaviour if the new option is not present. The new configuration option is intended be used mainly from CRD, but it's also available via Operator ConfigMap in a limited fashion. For ConfigMap one can put there only a string with one secret definition in the following format (as a string): infrastructure_roles_secret_name: | secret: k8s_secret_name, name: some_encrypted_name, password: some_encrypted_password, role: some_encrypted_role

FxKu · 2020-07-23T09:15:54Z

pkg/util/config/config.go

+type InfrastructureRole struct {
+	// Name of a secret which describes the role, and optionally name of a
+	// configmap with an extra information
+	Secret spec.NamespacedName


I wonder if this should also be called SecretName to be clear it's just a reference. What is the common way in K8s to name reference to other resources?

FxKu · 2020-07-23T09:36:54Z

pkg/controller/util.go

+	// To be compatible with the legacy implementation we need to return nil if
+	// the provided secret name is empty. The equivalent situation in the
+	// current implementation is an empty rolesSecrets slice or all its items
+	// are empty.


Maybe I'm overlooking something, but doesn't getInfrastructureRoleDefinitions set the secret name when the old format is used? Can it be empty then?

Yes, it sets if from the old field, which apparently could be empty (judging from the previous tests implementation).

FxKu · 2020-07-23T09:55:37Z

pkg/controller/util.go

 			}
-			delete(secretData, key)
+
+			roles = append(roles, t)


this part here is gone? Only checked in the else part.

if t.Name != "" { if t.Password == "" { c.logger.Warningf("infrastructure role %q has no password defined and is ignored", t.Name) continue } result[t.Name] = t }

FxKu · 2020-07-23T15:58:39Z

pkg/util/k8sutil/k8sutil.go

+	newFormatConfigmap := &v1.ConfigMap{}
+	newFormatConfigmap.Name = "testcluster"
+	newFormatConfigmap.Data = map[string]string{
+		"new-foobar": "{}",


shouldn't we also test with fields like user_flags to test if a proper pgUser struct is marshalled?

Postgres Operator uses infrastructure roles to provide access to a database for external users e.g. for monitoring purposes. Such infrastructure roles are expected to be present in the form of k8s secrets with the following content: inrole1: some_encrypted_role password1: some_encrypted_password user1: some_entrypted_name inrole2: some_encrypted_role password2: some_encrypted_password user2: some_entrypted_name The format of this content is implied implicitely and not flexible enough. In case if we do not have possibility to change the format of a secret we want to use in the Operator, we need to recreate it in this format. To address this lets make the format of secret content explicitely. The idea is to introduce a new configuration option for the Operator. infrastructure_roles_secrets: - secret: k8s_secret_name name: some_encrypted_name password: some_encrypted_password role: some_encrypted_role - secret: k8s_secret_name name: some_encrypted_name password: some_encrypted_password role: some_encrypted_role This would allow Operator to use any avalable secrets to prepare infrastructure roles. To make it backward compatible simulate the old behaviour if the new option is not present. The new configuration option is intended be used mainly from CRD, but it's also available via Operator ConfigMap in a limited fashion. For ConfigMap one can put there only a string with one secret definition in the following format (as a string): infrastructure_roles_secret_name: | secret: k8s_secret_name, name: some_encrypted_name, password: some_encrypted_password, role: some_encrypted_role

FxKu · 2020-07-27T14:25:34Z

pkg/util/config/config.go

+	// configmap with an extra information
+	SecretName spec.NamespacedName
+
+	Name     string


Should this field better be called User? I got confused while configuring the infrastructure_roles_secret field in the config map (where I used user:<username> first, and then name:user and finally name:<username>).

FxKu · 2020-07-31T13:22:32Z

pkg/controller/util.go

+			name := strings.TrimSpace(values[0])
+			value := strings.TrimSpace(values[1])
+
+			switch name {


if you would also extract the secretname key here, too, we could mark the infrastructure_roles_secret_name as deprecated and one could solely rely on infrastructure_roles_secrets. The surrounding conditions need to be adapted then:

if c.opConfig.InfrastructureRolesSecretName == emptyName { if c.opConfig.InfrastructureRolesDefs == "" { return rolesDefs } else { do extraction from configmap } else { older format }

Now, it's a bit confusing that you have to use both infrastructure_roles... config options together when dealing with configmap. But if you do the same for CRD only the old format is used and the new format is ignored (see if c.opConfig.InfrastructureRolesSecretName == emptyName)

…alando/postgres-operator into feature/infrastructure-roles-extension

This parameter should be a detail of internal implementation and only facilitate compatibility with the legacy behaviour if we want to recommend only using the new format.

docs/reference/operator_parameters.md

RafiaSabih · 2020-08-05T09:34:17Z

docs/reference/operator_parameters.md

+  and specify the keys for extract credentials from them such as user name,
+  password and role membership. For the configmap this has to be a string
+  which allows referencing only one infrastructure roles secret.
+  The default is empty.


I am finding first part of the sentence hard to follow, array of infrastructure role definitions that references existing secrets and specify the keys to extract credentials from them such as user name, password, and role membership, does this phrasing says what you meant.
Also, when you mention for the configmap, can it be specified somewhere else also...?
Also, is it role's secret...?

it's roles in this case. Did some rephrasing.

RafiaSabih · 2020-08-05T09:37:53Z

docs/user.md

+parameter where you can reference multiple existing secrets. Prior to `v1.6.0`
+the operator could only reference one secret with the
+`infrastructure_roles_secret_name` option. However, this secret could contain
+multiple roles using the same set of keys with incremented indexes.


Indexes? Does it refers to array indexes? Does it need to be mentioned like this...?

slightly rewritten, hopefully clearer now ... I thought the example shows it nicely what I'm referring to

docs/user.md

erthalion · 2020-08-05T11:35:32Z

👍

FxKu · 2020-08-05T11:40:57Z

👍

erthalion requested review from avaczi, CyberDem0n, FxKu, Jan-M, RafiaSabih and sdudoladov as code owners July 16, 2020 14:54

erthalion force-pushed the feature/infrastructure-roles-extension branch 3 times, most recently from 6c962e9 to bd57694 Compare July 22, 2020 10:34

erthalion and others added 2 commits July 22, 2020 12:36

update codegen

3b82af4

FxKu reviewed Jul 23, 2020

View reviewed changes

erthalion force-pushed the feature/infrastructure-roles-extension branch from 0636883 to d5b7c94 Compare July 27, 2020 08:35

FxKu reviewed Jul 27, 2020

View reviewed changes

Felix Kunde and others added 3 commits July 31, 2020 10:02

enable validation and extend doc reference

998279a

Merge branch 'master' into feature/infrastructure-roles-extension

1257b88

Add e2e test for infrastructure roles secrets

53e049a

FxKu reviewed Jul 31, 2020

View reviewed changes

erthalion and others added 5 commits July 31, 2020 15:39

Add e2e test for infrastructure roles secrets

52e0569

Add e2e test for infrastructure roles secrets

6464609

rename fields, allow secretname, update docs

d6c686a

Merge branch 'feature/infrastructure-roles-extension' of github.com:z…

d4fee0d

…alando/postgres-operator into feature/infrastructure-roles-extension

reflect changes in e2e test

a9cfaf3

erthalion force-pushed the feature/infrastructure-roles-extension branch 3 times, most recently from a71f571 to 1805b83 Compare August 3, 2020 12:14

erthalion force-pushed the feature/infrastructure-roles-extension branch 5 times, most recently from b7f018b to 602f26c Compare August 3, 2020 13:42

erthalion and others added 3 commits August 3, 2020 15:45

E2E test working directory is e2e

602f26c

Hide template

e85c21a

This parameter should be a detail of internal implementation and only facilitate compatibility with the legacy behaviour if we want to recommend only using the new format.

update docs and CRD validation

ac79faa

RafiaSabih reviewed Aug 5, 2020

View reviewed changes

reflect feedback

79b53d8

erthalion merged commit 7cf2fae into master Aug 5, 2020

FxKu added this to the 1.6 milestone Dec 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Extend infrastructure roles handling #1064

[WIP] Extend infrastructure roles handling #1064

Uh oh!

erthalion commented Jul 16, 2020

Uh oh!

FxKu Jul 23, 2020

Uh oh!

FxKu Jul 23, 2020

Uh oh!

erthalion Jul 23, 2020

Uh oh!

FxKu Jul 23, 2020 •

edited

Loading

Uh oh!

FxKu Jul 23, 2020

Uh oh!

FxKu Jul 27, 2020 •

edited

Loading

Uh oh!

FxKu Jul 31, 2020 •

edited

Loading

Uh oh!

Uh oh!

RafiaSabih Aug 5, 2020

Uh oh!

FxKu Aug 5, 2020

Uh oh!

RafiaSabih Aug 5, 2020

Uh oh!

FxKu Aug 5, 2020

Uh oh!

Uh oh!

erthalion commented Aug 5, 2020

Uh oh!

FxKu commented Aug 5, 2020

Uh oh!

Uh oh!

[WIP] Extend infrastructure roles handling #1064

[WIP] Extend infrastructure roles handling #1064

Uh oh!

Conversation

erthalion commented Jul 16, 2020

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!

FxKu Jul 23, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

FxKu Jul 27, 2020 • 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!

FxKu Jul 31, 2020 • 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!

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!

erthalion commented Aug 5, 2020

Uh oh!

FxKu commented Aug 5, 2020

Uh oh!

Uh oh!

FxKu Jul 23, 2020 •

edited

Loading

FxKu Jul 27, 2020 •

edited

Loading

FxKu Jul 31, 2020 •

edited

Loading