Watch model's cloud credential for validity. #8586

anastasiamac · 2018-04-12T08:35:09Z

Description of change

This is major part of the work that allows models to react when cloud credential becomes invalid.
This PR introduces a worker and its manifold, configured for models.

anastasiamac · 2018-04-12T08:35:29Z

cloud credential validity watcher

wallyworld

A few things to look at. Nice to see this stuff progressing.

wallyworld · 2018-04-13T00:20:29Z

apiserver/facades/agent/credentialvalidator/credentialvalidator.go

@@ -17,7 +17,8 @@ var logger = loggo.GetLogger("juju.api.credentialvalidator")
 // CredentialValidator defines the methods on credentialvalidator API endpoint.
 type CredentialValidator interface {
 	ModelCredential() (params.ModelCredential, error)
-	WatchCredential(string) (params.NotifyWatchResult, error)
+	WatchCredential(params.Entity) (params.NotifyWatchResult, error)
+	//WatchCredential(string) (params.NotifyWatchResult, error)


wallyworld · 2018-04-13T00:22:08Z

apiserver/facades/agent/credentialvalidator/credentialvalidator.go

@@ -48,16 +49,15 @@ func internalNewCredentialValidatorAPI(backend Backend, resources facade.Resourc
 // changes to the given cloud credentials.


Comment is plural but api call is single.
We should stick to convention and implement as a bulk call, even though we'll just be passing in one credential at the moment.

wallyworld · 2018-04-13T00:25:18Z

worker/credentialvalidator/manifold.go

+
+import (
+	"github.com/juju/errors"
+	worker "gopkg.in/juju/worker.v1"


delete alias. someone introduced this a while back, not sure why

wallyworld · 2018-04-13T00:27:03Z

worker/credentialvalidator/manifold_test.go

+	"github.com/juju/testing"
+	jc "github.com/juju/testing/checkers"
+	gc "gopkg.in/check.v1"
+	worker "gopkg.in/juju/worker.v1"


and here etc

wallyworld · 2018-04-13T00:28:16Z

worker/credentialvalidator/util_test.go

+}
+
+// modelUUID is the model tag we're using in the tests.
+var modelUUID = "01234567-89ab-cdef-0123-456789abcdef"


we have a predefined model tag for tests we could use

wallyworld · 2018-04-13T00:29:02Z

worker/credentialvalidator/worker.go

+
+// ErrModelCredentialChanged indicates that a Worker has bounced
+// because model credential was replaced.
+var ErrModelCredentialChanged = errors.New("unexpected model credential")


"model credential changed"
?

wallyworld · 2018-04-13T00:32:57Z

worker/credentialvalidator/util_test.go

+
+// checkManifoldNotValid checks that the supplied ManifoldConfig creates
+// a manifold that cannot be started.
+func checkManifoldNotValid(c *gc.C, config credentialvalidator.ManifoldConfig, expect string) {


Interesting approach. In other workers, we use an exported Validate() method on the config and just call that directly. There's a bit more boilerplate with this approach.

i followed whatever was in migration flag. I am not big fun of exporting functions that are not otherwise exported just for testing...

what function would you be exporting that isn't otherwise exported?

Sure, ok. Can go either way with this one. We sometimes export methods to make testing easier. But this way is fine, was just thinking out loud. I wish we didn't have so many ways things that are the same have been implemented over time.

I don't think it's worth putting the foot down. I'll export Validate() and will change the test to call it directly rather than going through Start().

I do not see a significant difference in implementation - nothing gained through exporting it from behavioral or performance consideration.
However, I agree that the test is simpler and leaner and this can be a metrics of quality by itself :D

wallyworld · 2018-04-13T00:34:28Z

worker/credentialvalidator/worker.go

+				return errors.Trace(err)
+			}
+			if !exists {
+				// Really should not happen in practice since the uninstall


given the comment should we at least log that this has occurred as a Warning

wallyworld · 2018-04-13T00:37:12Z

worker/credentialvalidator/worker.go

+	if err := config.Validate(); err != nil {
+		return nil, errors.Trace(err)
+	}
+	modelCredential, exists, err := config.Facade.ModelCredential()


These steps should all be at the top of the loop method (before the for{})
Besides following current practice, it means that modelCredential and watcher don't need to be worker attributes. The watcher would be added to the catacomb when created in the loop.

Getting model credential is the biggest and not a simple operation. There is no need to get it all the time only if a valid and interesting change is reported.
The same approach is used in migration flag which to me was a better and very similar example to what I was doing than any other worker we have.

it won't be got ll the time - it's just moving the code from here to the top of the worker loop (not inside the for). 99% of workers have all their logic inside the loop(). Migration worker would be the outlier here. It's better to keep all the worker logic together in the loop function.

While it has been idiomatic in other places to have the New method just initialise the structure and start the loop, I don't see anything wrong with the initialisation including getting the credentials from the DB.

Consistency is good. If there's no real reason to stray, why introduce the cognitive overhead. Code is write once, read many.

Plus the suggested approach removes the need for unnecessary worker attributes. Less LOC = less bugs by definition.

I think consistency is good. Ideally new people coming into the code will be able to see the patterns of how things are written. I'd agree that moving the code into the loop function for consistency is worth it.

wallyworld · 2018-04-13T00:38:21Z

worker/credentialvalidator/worker_test.go

+	"github.com/juju/juju/worker/workertest"
+)
+
+type WorkerSuite struct {


There need to be tests that the worker and watcher are properly stopped.
See other worker packages for examples.

Again, all these are similar to model migration flag. All the tests here that get a valid watcher/worker have workertest.CheckKilled etc... Did you look at:
TestModelCredentialErrorWhileRunning l. 64;
TestCredentialChangeToInvalid l.91;
TestCredentialChangeFromInvalid l.110;
TestModelCredentialReplaced l. 129;
TestNoRelevantCredentialChange l.147.

I have also ran all the tests in other packages that test workers start and stop (like cmd/jujud/agent).

In addition, I have live tested.

We often have a simple test like this:

func (s *WorkerSuite) TestStartStop(c *gc.C) {
w, err := caasoperator.NewWorker(s.config)
c.Assert(err, jc.ErrorIsNil)
workertest.CheckAlive(c, w)
workertest.CleanKill(c, w)
}

Also, there's no check that I can see that the watcher is killed when the worker dies.

Don't necessarily assume that what's in migration flag or elsewhere is current best practice. Sadly this stuff tends to evolve and there's a bit of legacy code all around.

I do agree with Ian on this one, traditionally workers have a simple test that just has the start and a clean kill.

howbazaar · 2018-04-13T03:25:26Z

cmd/jujud/agent/machine_test.go

@@ -1041,8 +1041,20 @@ func (s *MachineSuite) TestHostedModelWorkers(c *gc.C) {
 		return &minModelWorkersEnviron{}, nil
 	})

-	st, closer := s.setUpNewModel(c)
-	defer closer()
+	st := s.Factory.MakeModel(c, &factory.ModelParams{


Why do we care about the ConfigAttrs? Are the defaults not good enough?

NO, here we care about cloud credential being on the model, otherwise cred-validator will not start and it has too since it's in the list of valid model workers that this suite maintains.

I might need to write cleaner narrow test cases (like 'model (+live/not/dying/not dead/not migrating/etc variations) on a cloud that requires cred must have a worker', 'model on a loud that does not require cred, does not have a worker') but I'd prefer to do these in a follow-up PR. That PR will clean up this mess a bit.

howbazaar · 2018-04-13T03:27:54Z

worker/credentialvalidator/manifold.go

+}
+
+// start is a StartFunc for a Worker manifold.
+func (config ManifoldConfig) start(context dependency.Context) (worker.Worker, error) {


I quite like the idea of breaking start out as a config method. seems cleaner.

howbazaar · 2018-04-13T03:29:12Z

worker/credentialvalidator/manifold.go

+
+func filterErrors(err error) error {
+	cause := errors.Cause(err)
+	if cause == ErrChanged || cause == ErrModelCredentialChanged {


What about the situation where the credentails become invalid?

ErrChanged will be thrown specifically when credential is invalid. This is defined in the worker :D

I'll rename the error since when we throw ErrChanged, we are specifically reacting to 'invalid credential' only.

howbazaar · 2018-04-13T03:29:45Z

worker/credentialvalidator/manifold_test.go

@@ -0,0 +1,177 @@
+// Copyright 2018 Canonical Ltd.
+// Licensed under the LGPLv3, see LICENCE file for details.


The file you copied from had the wrong licence - AGPL

howbazaar · 2018-04-13T03:32:17Z

worker/credentialvalidator/manifold_test.go

+	c.Check(err, gc.ErrorMatches, "expected out to be a \\*Flag; got a .*")
+}
+
+func (*ManifoldSuite) TestOutputBadInput(c *gc.C) {


Is this bad imput?

howbazaar · 2018-04-13T03:43:05Z

worker/credentialvalidator/util_test.go

+	err := config.Validate()
+	check(err)
+
+	worker, err := credentialvalidator.New(config)


You only need to check that the config is validated in the new worker function once. This violates the "test it once" idea.

howbazaar · 2018-04-13T03:44:08Z

worker/credentialvalidator/util_test.go

+
+// checkManifoldNotValid checks that the supplied ManifoldConfig creates
+// a manifold that cannot be started.
+func checkManifoldNotValid(c *gc.C, config credentialvalidator.ManifoldConfig, expect string) {


what function would you be exporting that isn't otherwise exported?

howbazaar · 2018-04-13T03:47:48Z

worker/credentialvalidator/worker.go

+	if err := config.Validate(); err != nil {
+		return nil, errors.Trace(err)
+	}
+	modelCredential, exists, err := config.Facade.ModelCredential()


While it has been idiomatic in other places to have the New method just initialise the structure and start the loop, I don't see anything wrong with the initialisation including getting the credentials from the DB.

howbazaar · 2018-04-13T03:50:51Z

worker/credentialvalidator/worker_test.go

+
+	workertest.CheckAlive(c, worker)
+	workertest.CleanKill(c, worker)
+	s.facade.CheckCallNames(c, "ModelCredential", "WatchCredential", "ModelCredential", "ModelCredential", "ModelCredential")


This feels racey to me. How do we know that the right number of calls have been made befor we kill the worker?

howbazaar · 2018-04-13T03:52:46Z

worker/credentialvalidator/worker_test.go

+	"github.com/juju/juju/worker/workertest"
+)
+
+type WorkerSuite struct {


I do agree with Ian on this one, traditionally workers have a simple test that just has the start and a clean kill.

wallyworld

Overall this is really good. There's just the few nit picky things. With many new starters, we're trying to be a little stricter on code consistency to make it easier for folks to understand the code when the read it. Once Tim is happy, the LGTM.

howbazaar · 2018-04-13T04:31:24Z

I'm happy in general. Overall I think the quality of the work is very high, and the actual issues are very minor. Almost all of the changes are to be more consistent with what has emerged as best practice for workers and their config.

Good job @anastasiamac , it really is good.

anastasiamac · 2018-04-13T06:19:08Z

Thank you for your reviews. Once all the checks pass, I'll land this with some changes and will follow up with a separate PR that has tighter tests both of manifold within worker pkg and outside, specifically in cmd/juju/agent.

anastasiamac · 2018-04-13T06:33:13Z

$$merge$$

…worker #8594 ## Description of change Based on review comments in PR #8586, this PR improves credential validator worker tests: prdesc in worker pkg itself - watcher is constructed deterministically and sends definitive changes; added compliant StartStop test; * in cmd/jujud/agent pkg - only models with cloud credential are expected to have a credential validator worker. ## QA steps Internal change so all unit tests are expected to pass.

cloud cred worker/manifold

c5b2692

wallyworld requested changes Apr 13, 2018

View reviewed changes

review comments.

42f8034

howbazaar reviewed Apr 13, 2018

View reviewed changes

wallyworld approved these changes Apr 13, 2018

View reviewed changes

anastasiamac added 2 commits April 13, 2018 14:28

Rename error to be more descriptive.

c0c9249

Change licence to AGPL

db22ec4

howbazaar approved these changes Apr 13, 2018

View reviewed changes

anastasiamac added 2 commits April 13, 2018 14:41

export config.validate()

6aca7fa

remove chck from config and generally restructure based on reviews

d4afb17

jujubot merged commit 8bb3a88 into juju:develop Apr 13, 2018

anastasiamac deleted the valid-cred-manifold-etc branch April 13, 2018 06:51

anastasiamac mentioned this pull request Apr 13, 2018

Better tests for credential validator worker. #8594

Merged

		@@ -48,16 +49,15 @@ func internalNewCredentialValidatorAPI(backend Backend, resources facade.Resourc
		// changes to the given cloud credentials.

		@@ -0,0 +1,177 @@
		// Copyright 2018 Canonical Ltd.
		// Licensed under the LGPLv3, see LICENCE file for details.

Watch model's cloud credential for validity. #8586

Watch model's cloud credential for validity. #8586

Conversation

anastasiamac commented Apr 12, 2018

Description of change

anastasiamac commented Apr 12, 2018

wallyworld 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

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

wallyworld left a comment

Choose a reason for hiding this comment

howbazaar commented Apr 13, 2018

anastasiamac commented Apr 13, 2018

anastasiamac commented Apr 13, 2018