-
Notifications
You must be signed in to change notification settings - Fork 503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Watch model's cloud credential for validity. #8586
Changes from 1 commit
c5b2692
42f8034
c0c9249
db22ec4
6aca7fa
d4afb17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
} | ||
|
||
type CredentialValidatorAPI struct { | ||
|
@@ -48,16 +49,15 @@ func internalNewCredentialValidatorAPI(backend Backend, resources facade.Resourc | |
// changes to the given cloud credentials. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment is plural but api call is single. |
||
// The order of returned watchers is important and corresponds directly to the | ||
// order of supplied cloud credentials collection. | ||
func (api *CredentialValidatorAPI) WatchCredential(tag string) (params.NotifyWatchResult, error) { | ||
func (api *CredentialValidatorAPI) WatchCredential(tag params.Entity) (params.NotifyWatchResult, error) { | ||
fail := func(failure error) (params.NotifyWatchResult, error) { | ||
return params.NotifyWatchResult{}, common.ServerError(failure) | ||
} | ||
|
||
credentialTag, err := names.ParseCloudCredentialTag(tag) | ||
credentialTag, err := names.ParseCloudCredentialTag(tag.Tag) | ||
if err != nil { | ||
return fail(err) | ||
} | ||
|
||
// Is credential used by the model that has created this backend? | ||
isUsed, err := api.backend.ModelUsesCredential(credentialTag) | ||
if err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we care about the ConfigAttrs? Are the defaults not good enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
ConfigAttrs: coretesting.Attrs{ | ||
"max-status-history-age": "2h", | ||
"max-status-history-size": "4M", | ||
"max-action-results-age": "2h", | ||
"max-action-results-size": "4M", | ||
}, | ||
CloudCredential: names.NewCloudCredentialTag("dummy/admin/cred"), | ||
}) | ||
defer func() { | ||
err := st.Close() | ||
c.Check(err, jc.ErrorIsNil) | ||
}() | ||
|
||
uuid := st.ModelUUID() | ||
|
||
tracker := agenttest.NewEngineTracker() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
// Copyright 2018 Canonical Ltd. | ||
// Licensed under the AGPLv3, see LICENCE file for details. | ||
|
||
package credentialvalidator | ||
|
||
import ( | ||
"github.com/juju/errors" | ||
worker "gopkg.in/juju/worker.v1" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete alias. someone introduced this a while back, not sure why |
||
|
||
"github.com/juju/juju/api/base" | ||
"github.com/juju/juju/cmd/jujud/agent/engine" | ||
"github.com/juju/juju/worker/dependency" | ||
) | ||
|
||
// ManifoldConfig holds the dependencies and configuration for a | ||
// Worker manifold. | ||
type ManifoldConfig struct { | ||
APICallerName string | ||
Check Predicate | ||
|
||
NewFacade func(base.APICaller) (Facade, error) | ||
NewWorker func(Config) (worker.Worker, error) | ||
} | ||
|
||
// validate is called by start to check for bad configuration. | ||
func (config ManifoldConfig) validate() error { | ||
if config.APICallerName == "" { | ||
return errors.NotValidf("empty APICallerName") | ||
} | ||
if config.Check == nil { | ||
return errors.NotValidf("nil Check") | ||
} | ||
if config.NewFacade == nil { | ||
return errors.NotValidf("nil NewFacade") | ||
} | ||
if config.NewWorker == nil { | ||
return errors.NotValidf("nil NewWorker") | ||
} | ||
return nil | ||
} | ||
|
||
// start is a StartFunc for a Worker manifold. | ||
func (config ManifoldConfig) start(context dependency.Context) (worker.Worker, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I quite like the idea of breaking start out as a config method. seems cleaner. |
||
if err := config.validate(); err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
var apiCaller base.APICaller | ||
if err := context.Get(config.APICallerName, &apiCaller); err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
facade, err := config.NewFacade(apiCaller) | ||
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
worker, err := config.NewWorker(Config{ | ||
Facade: facade, | ||
Check: config.Check, | ||
}) | ||
if err != nil { | ||
return nil, errors.Trace(err) | ||
} | ||
return worker, nil | ||
} | ||
|
||
// Manifold packages a Worker for use in a dependency.Engine. | ||
func Manifold(config ManifoldConfig) dependency.Manifold { | ||
return dependency.Manifold{ | ||
Inputs: []string{config.APICallerName}, | ||
Start: config.start, | ||
Output: engine.FlagOutput, | ||
Filter: filterErrors, | ||
} | ||
} | ||
|
||
func filterErrors(err error) error { | ||
cause := errors.Cause(err) | ||
if cause == ErrChanged || cause == ErrModelCredentialChanged { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the situation where the credentails become invalid? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ErrChanged will be thrown specifically when credential is invalid. This is defined in the worker :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll rename the error since when we throw ErrChanged, we are specifically reacting to 'invalid credential' only. |
||
return dependency.ErrBounce | ||
} else if cause == ErrModelDoesNotNeedCredential { | ||
return dependency.ErrUninstall | ||
} | ||
return err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete me