New secret syncing #385

erthalion · 2018-09-12T13:48:49Z

First implementation of idea to make the operator more transparent.

Since now it's done before through the plan

golangcibot · 2018-09-12T13:49:21Z

pkg/cluster/actions.go

+
+type MetaData struct {
+	cluster   *Cluster
+	namespace string


namespace is unused

coveralls · 2018-09-12T13:52:30Z

Coverage remained the same at 23.766% when pulling b44bf1d on feature/actions-for-secret-syncing into 9c75588 on master.

sdudoladov · 2018-09-13T15:24:27Z

pkg/cluster/actions.go

+
+var NoActions []Action = []Action{}
+
+type MetaData struct {


probably "ActionMetaData" reads better?

sdudoladov · 2018-09-13T15:26:19Z

pkg/controller/postgresql.go

+
+	if hasNewName {
+		return util.NameFromMeta(event.NewSpec.ObjectMeta)
+	} else {


is this else really needed ? just leave the 2nd return on its own.

sdudoladov · 2018-09-13T15:29:29Z

pkg/cluster/actions.go

+
+type Action interface {
+	Name() string
+	Validate() error


so the intention is to validate before applying but it is up to the interface user to make so ?

sdudoladov · 2018-09-13T15:31:24Z

pkg/cluster/actions.go

+
+	// if this secret belongs to the infrastructure role and the password has
+	// changed - replace it in the secret
+	updateSecret := (user.Password != string(action.secret.Data["password"]) &&


mustUpdateInfraRoleSecret?

sdudoladov · 2018-09-13T15:36:03Z

pkg/cluster/actions.go

+			msg = "Could not update infrastructure role secret for role %q: %v"
+			return fmt.Errorf(msg, action.secretUsername, err)
+		}
+	} else {


that's a bit confusing in a sense that if in one branch I see secret update, in the other branch I excpect to see logic describing what happens if a secret is not updated, and not the udpate of a secret of another type

sdudoladov · 2018-09-13T15:38:16Z

pkg/cluster/cluster.go

@@ -200,6 +200,45 @@ func (c *Cluster) initUsers() error {
 	return nil
 }

+func CreateSecrets() []Action {


that deserves a comment because I'd expect this function to return sth secret-related and not simply nil

sdudoladov · 2018-09-13T15:40:40Z

pkg/cluster/cluster.go

+}
+
+// TODO: mind the secrets of the deleted/new users
+func (c *Cluster) PlanForSecrets() (plan []Action) {


it is worth commenting here what a plan actually means (sequence of actions i assume? ) It is not immediately obvious for a person w/o operator and DB background

sdudoladov · 2018-09-13T15:44:02Z

@erthalion can you please add an overview of the idea so that other people can easier join reviewing ?

Add user initialization since it's required for secrets

golangcibot · 2018-09-14T13:24:37Z

pkg/cluster/actions.go

+
+type MetaData struct {
+	cluster   *Cluster
+	namespace string


namespace is unused

erthalion · 2018-10-09T12:09:24Z

So, @zerg-junior @alexeyklyukin what do you think about this feature?

sdudoladov

during debugging is there a way to see a generated plan without applying it first ?
this change needs some overview documentation to ensure we are all on the same page

sdudoladov · 2018-10-11T13:55:49Z

pkg/controller/postgresql.go

+}
+
+func (c *Controller) validatePlan(plan cluster.Plan) (err error) {
+	for _, action := range plan {


i'd prefer to have all actions validates first and reported to the end user at once, to avoid re-running the validation just to discover an error after an error .

sdudoladov · 2018-10-11T14:02:28Z

pkg/controller/postgresql.go

+		// build plan
+		actions := c.generatePlan(event)
+		if err := c.validatePlan(actions); err != nil {
+			c.logger.Errorf("Invalid plan: %v", err)


so we nevertheless apply the erroneous plan ?

sdudoladov · 2018-10-11T14:18:01Z

pkg/cluster/actions.go

+}
+
+func (action ActionSecret) Validate() error {
+	if action.secret.Data["username"] == nil {


do all the secrets handled by the operator always have the username field ?

k1ng440 · 2019-01-19T19:50:28Z

@erthalion you might want to answer those questions above?

golangcibot · 2019-01-23T16:28:57Z

pkg/controller/postgresql_test.go

-	False = false
+	True   = true
+	False  = false
+	logger = logrus.New().WithField("test", "cluster")


logger is unused (from deadcode)

golangcibot · 2019-01-23T16:28:57Z

pkg/controller/postgresql_test.go

+)
+
+const (
+	superUserName       = "postgres"


superUserName is unused (from deadcode)

golangcibot · 2019-01-23T16:28:58Z

pkg/controller/postgresql_test.go

+
+const (
+	superUserName       = "postgres"
+	replicationUserName = "standby"


replicationUserName is unused (from deadcode)

sdudoladov · 2019-02-27T11:56:15Z

@erthalion @Jan-M what is our position on this change ? do we continue ?

erthalion · 2019-02-27T12:28:33Z

Of course we need to continue.

sdudoladov · 2019-05-31T13:50:27Z

closed for now due to the lack of activity

erthalion added 2 commits September 12, 2018 15:40

Add action-based version of secret sync

5563078

Remove syncSecrets for cluster creation

e588fdd

Since now it's done before through the plan

erthalion requested review from alexeyklyukin, avaczi, CyberDem0n, Jan-M and sdudoladov as code owners September 12, 2018 13:48

erthalion mentioned this pull request Sep 12, 2018

[WIP] Actions and comparison logic #336

Closed

golangcibot reviewed Sep 12, 2018

View reviewed changes

pkg/cluster/actions.go Outdated

type MetaData struct {

cluster *Cluster

namespace string

Copy link

golangcibot Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace is unused

sdudoladov reviewed Sep 13, 2018

View reviewed changes

erthalion added 4 commits September 14, 2018 11:46

Remove unused function

65c49e0

Add user initialization since it's required for secrets

Accept multiple clients on delve

8bc1e7b

Various bufixes

9039029

Improvements per review

43a0449

golangcibot reviewed Sep 14, 2018

View reviewed changes

pkg/cluster/actions.go Outdated

type MetaData struct {

cluster *Cluster

namespace string

Copy link

golangcibot Sep 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

namespace is unused

Avoid annoying notification abount unused fields

ba1f886

sdudoladov mentioned this pull request Sep 17, 2018

RFC: operator should register all PG manifests #382

Open

erthalion added 3 commits September 21, 2018 11:36

Add a simple tests for CreateSecret

b0756a1

Merge branch 'master' into feature/actions-for-secret-syncing

dc8f169

Fix import

3c0a6b8

erthalion changed the title ~~[WIP] New secret syncing~~ New secret syncing Oct 4, 2018

erthalion added 2 commits October 8, 2018 14:11

Add tests for secrets update

31725a6

Add more tests for secrets verity/apply logic

a31c69e

sdudoladov reviewed Oct 11, 2018

View reviewed changes

golangcibot reviewed Jan 23, 2019

View reviewed changes

Merge branch 'master' into feature/actions-for-secret-syncing

b44bf1d

Remove unused variables

8c052c5

FxKu added this to the v1.2 milestone May 20, 2019

sdudoladov closed this May 31, 2019

sdudoladov removed this from the v1.2 milestone May 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New secret syncing #385

New secret syncing #385

erthalion commented Sep 12, 2018

golangcibot Sep 12, 2018

coveralls commented Sep 12, 2018 •

edited

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov Sep 13, 2018

sdudoladov commented Sep 13, 2018

golangcibot Sep 14, 2018

erthalion commented Oct 9, 2018

sdudoladov left a comment

sdudoladov Oct 11, 2018

sdudoladov Oct 11, 2018

sdudoladov Oct 11, 2018

k1ng440 commented Jan 19, 2019

golangcibot Jan 23, 2019

golangcibot Jan 23, 2019

golangcibot Jan 23, 2019

sdudoladov commented Feb 27, 2019

erthalion commented Feb 27, 2019

sdudoladov commented May 31, 2019

New secret syncing #385

New secret syncing #385

Conversation

erthalion commented Sep 12, 2018

Choose a reason for hiding this comment

coveralls commented Sep 12, 2018 • edited

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

sdudoladov commented Sep 13, 2018

Choose a reason for hiding this comment

erthalion commented Oct 9, 2018

sdudoladov left a 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

k1ng440 commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdudoladov commented Feb 27, 2019

erthalion commented Feb 27, 2019

sdudoladov commented May 31, 2019

coveralls commented Sep 12, 2018 •

edited