Skip to content

Commit

Permalink
Merge pull request juju#10968 from anastasiamac/add-cred-UR-force
Browse files Browse the repository at this point in the history 
juju#10968

## Please provide the following details to expedite Pull Request review:

### Checklist

 - [x] Checked if it requires a [pylibjuju](https://github.com/juju/python-libjuju) change?
_No API server change, only api CLI client._
 - [ ]~~ Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR?~~
 - [ ] ~~Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed?~~
 - [x] Do comments answer the question of why design decisions were made?

----

## Description of change

Cloud facade has supported the ability to force update and force remove credentials. This PR catches up corresponding CLI.

As a drive-by, I have renamed an internal variable in credential validity to maintain clarity.
 
## QA steps

1. bootstrap with credential A
2. add model with different credential B
3. remove credential B [will fail because a model uses this credential]
4. force remove credential [success]

## Bug reference

Part 2 for https://bugs.launchpad.net/juju/+bug/1852412
  • Loading branch information
@jujubot @anastasiamac
jujubot authored and anastasiamac committed Dec 9, 2019
1 parent 85671eb commit 9cef799
Show file tree
Hide file tree
Showing 11 changed files with 162 additions and 51 deletions.
25 changes: 17 additions & 8 deletions api/cloud/cloud.go
View file
Expand Up @@ -96,18 +96,27 @@ func (c *Client) UserCredentials(user names.UserTag, cloud names.CloudTag) ([]na

// UpdateCloudsCredentials updates clouds credentials content on the controller.
// Passed in credentials are keyed on the credential tag.
func (c *Client) UpdateCloudsCredentials(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
var tagged []params.TaggedCredential
// This operation can be forced to ignore validation checks.
func (c *Client) UpdateCloudsCredentials(cloudCredentials map[string]jujucloud.Credential, force bool) ([]params.UpdateCredentialResult, error) {
return c.internalUpdateCloudsCredentials(params.UpdateCredentialArgs{Force: force}, cloudCredentials)
}

// AddCloudsCredentials adds/uploads clouds credentials content to the controller.
// Passed in credentials are keyed on the credential tag.
func (c *Client) AddCloudsCredentials(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
return c.internalUpdateCloudsCredentials(params.UpdateCredentialArgs{}, cloudCredentials)
}

func (c *Client) internalUpdateCloudsCredentials(in params.UpdateCredentialArgs, cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
for tag, credential := range cloudCredentials {
tagged = append(tagged, params.TaggedCredential{
in.Credentials = append(in.Credentials, params.TaggedCredential{
Tag: tag,
Credential: params.CloudCredential{
AuthType: string(credential.AuthType()),
Attributes: credential.Attributes(),
},
})
}
in := params.UpdateCredentialArgs{Credentials: tagged}
count := len(cloudCredentials)

countErr := func(got int) error {
Expand All @@ -127,7 +136,7 @@ func (c *Client) UpdateCloudsCredentials(cloudCredentials map[string]jujucloud.C
}
converted := make([]params.UpdateCredentialResult, count)
for i, one := range out.Results {
converted[i] = params.UpdateCredentialResult{CredentialTag: tagged[i].Tag, Error: one.Error}
converted[i] = params.UpdateCredentialResult{CredentialTag: in.Credentials[i].Tag, Error: one.Error}
}
return converted, nil
}
Expand All @@ -146,7 +155,7 @@ func (c *Client) UpdateCloudsCredentials(cloudCredentials map[string]jujucloud.C
// stored on the controller. This call validates that the new content works
// for all models that are using this credential.
func (c *Client) UpdateCredentialsCheckModels(tag names.CloudCredentialTag, credential jujucloud.Credential) ([]params.UpdateCredentialModelResult, error) {
out, err := c.UpdateCloudsCredentials(map[string]jujucloud.Credential{tag.String(): credential})
out, err := c.UpdateCloudsCredentials(map[string]jujucloud.Credential{tag.String(): credential}, false)
if err != nil {
return nil, errors.Trace(err)
}
Expand All @@ -158,7 +167,7 @@ func (c *Client) UpdateCredentialsCheckModels(tag names.CloudCredentialTag, cred
}

// RevokeCredential revokes/deletes a cloud credential.
func (c *Client) RevokeCredential(tag names.CloudCredentialTag) error {
func (c *Client) RevokeCredential(tag names.CloudCredentialTag, force bool) error {
var results params.ErrorResults

if c.facade.BestAPIVersion() < 3 {
Expand All @@ -175,7 +184,7 @@ func (c *Client) RevokeCredential(tag names.CloudCredentialTag) error {

args := params.RevokeCredentialArgs{
Credentials: []params.RevokeCredentialArg{
{Tag: tag.String()},
{Tag: tag.String(), Force: force},
},
}
if err := c.facade.FacadeCall("RevokeCredentialsCheckModels", args, &results); err != nil {
Expand Down
66 changes: 53 additions & 13 deletions api/cloud/cloud_test.go
View file
Expand Up @@ -455,7 +455,7 @@ func (s *cloudSuite) TestRevokeCredential(c *gc.C) {
c.Assert(result, gc.FitsTypeOf, &params.ErrorResults{})
c.Assert(a, jc.DeepEquals, params.RevokeCredentialArgs{
Credentials: []params.RevokeCredentialArg{
{Tag: "cloudcred-foo_bob_bar"},
{Tag: "cloudcred-foo_bob_bar", Force: true},
},
})
*result.(*params.ErrorResults) = params.ErrorResults{
Expand All @@ -472,7 +472,7 @@ func (s *cloudSuite) TestRevokeCredential(c *gc.C) {

client := cloudapi.NewClient(apiCaller)
tag := names.NewCloudCredentialTag("foo/bob/bar")
err := client.RevokeCredential(tag)
err := client.RevokeCredential(tag, true)
c.Assert(err, jc.ErrorIsNil)
c.Assert(s.called, jc.IsTrue)
}
Expand Down Expand Up @@ -505,7 +505,7 @@ func (s *cloudSuite) TestRevokeCredentialV2(c *gc.C) {

client := cloudapi.NewClient(apiCaller)
tag := names.NewCloudCredentialTag("foo/bob/bar")
err := client.RevokeCredential(tag)
err := client.RevokeCredential(tag, false)
c.Assert(err, jc.ErrorIsNil)
c.Assert(s.called, jc.IsTrue)
}
Expand Down Expand Up @@ -1087,6 +1087,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentials(c *gc.C) {
c.Check(request, gc.Equals, "UpdateCredentialsCheckModels")
c.Assert(result, gc.FitsTypeOf, &params.UpdateCredentialResults{})
c.Assert(a, jc.DeepEquals, params.UpdateCredentialArgs{
Force: true,
Credentials: []params.TaggedCredential{{
Tag: "cloudcred-foo_bob_bar0",
Credential: params.CloudCredential{
Expand All @@ -1107,7 +1108,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentials(c *gc.C) {
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.UpdateCloudsCredentials(createCredentials(1))
result, err := client.UpdateCloudsCredentials(createCredentials(1), true)
c.Assert(err, jc.ErrorIsNil)
c.Assert(result, gc.DeepEquals, []params.UpdateCredentialResult{{}})
c.Assert(s.called, jc.IsTrue)
Expand All @@ -1132,7 +1133,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsErrorV2(c *gc.C) {
BestVersion: 2,
}
client := cloudapi.NewClient(apiCaller)
errs, err := client.UpdateCloudsCredentials(createCredentials(1))
errs, err := client.UpdateCloudsCredentials(createCredentials(1), true)
c.Assert(err, jc.ErrorIsNil)
c.Assert(errs, gc.DeepEquals, []params.UpdateCredentialResult{
{CredentialTag: "cloudcred-foo_bob_bar0", Error: &params.Error{Message: "validation failure", Code: ""}},
Expand Down Expand Up @@ -1163,7 +1164,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsError(c *gc.C) {
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
errs, err := client.UpdateCloudsCredentials(createCredentials(1))
errs, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, jc.ErrorIsNil)
c.Assert(errs, gc.DeepEquals, []params.UpdateCredentialResult{
{CredentialTag: "cloudcred-foo_bob_bar0", Error: common.ServerError(errors.New("validation failure"))},
Expand All @@ -1187,7 +1188,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsCallErrorV2(c *gc.C) {
BestVersion: 2,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.UpdateCloudsCredentials(createCredentials(1))
result, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, gc.ErrorMatches, "scary but true")
c.Assert(result, gc.IsNil)
c.Assert(s.called, jc.IsTrue)
Expand All @@ -1209,7 +1210,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsCallError(c *gc.C) {
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.UpdateCloudsCredentials(createCredentials(1))
result, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, gc.ErrorMatches, "scary but true")
c.Assert(result, gc.IsNil)
c.Assert(s.called, jc.IsTrue)
Expand Down Expand Up @@ -1237,7 +1238,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsManyResultsV2(c *gc.C) {
BestVersion: 2,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.UpdateCloudsCredentials(createCredentials(1))
result, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, gc.ErrorMatches, `expected 1 result got 2 when updating credentials`)
c.Assert(result, gc.IsNil)
c.Assert(s.called, jc.IsTrue)
Expand Down Expand Up @@ -1266,7 +1267,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsManyMatchingResultsV2(c *gc.C) {
}
client := cloudapi.NewClient(apiCaller)
in := createCredentials(2)
result, err := client.UpdateCloudsCredentials(in)
result, err := client.UpdateCloudsCredentials(in, false)
c.Assert(err, jc.ErrorIsNil)
c.Assert(result, gc.HasLen, len(in))
for _, one := range result {
Expand Down Expand Up @@ -1297,7 +1298,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsManyResults(c *gc.C) {
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.UpdateCloudsCredentials(createCredentials(1))
result, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, gc.ErrorMatches, `expected 1 result got 2 when updating credentials`)
c.Assert(result, gc.IsNil)
c.Assert(s.called, jc.IsTrue)
Expand Down Expand Up @@ -1325,7 +1326,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsManyMatchingResults(c *gc.C) {
}
client := cloudapi.NewClient(apiCaller)
count := 2
result, err := client.UpdateCloudsCredentials(createCredentials(count))
result, err := client.UpdateCloudsCredentials(createCredentials(count), false)
c.Assert(err, jc.ErrorIsNil)
c.Assert(result, gc.HasLen, count)
c.Assert(s.called, jc.IsTrue)
Expand Down Expand Up @@ -1363,7 +1364,7 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsModelErrors(c *gc.C) {
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
errs, err := client.UpdateCloudsCredentials(createCredentials(1))
errs, err := client.UpdateCloudsCredentials(createCredentials(1), false)
c.Assert(err, jc.ErrorIsNil)
c.Assert(errs, gc.DeepEquals, []params.UpdateCredentialResult{
{CredentialTag: "cloudcred-foo_bob_bar",
Expand All @@ -1380,3 +1381,42 @@ func (s *cloudSuite) TestUpdateCloudsCredentialsModelErrors(c *gc.C) {
})
c.Assert(s.called, jc.IsTrue)
}

func (s *cloudSuite) TestAddCloudsCredentials(c *gc.C) {
apiCaller := basetesting.BestVersionCaller{
APICallerFunc: basetesting.APICallerFunc(
func(objType string,
version int,
id, request string,
a, result interface{},
) error {
c.Check(objType, gc.Equals, "Cloud")
c.Check(id, gc.Equals, "")
c.Check(request, gc.Equals, "UpdateCredentialsCheckModels")
c.Assert(result, gc.FitsTypeOf, &params.UpdateCredentialResults{})
c.Assert(a, jc.DeepEquals, params.UpdateCredentialArgs{
Credentials: []params.TaggedCredential{{
Tag: "cloudcred-foo_bob_bar0",
Credential: params.CloudCredential{
AuthType: "userpass",
Attributes: map[string]string{
"username": "admin",
"password": "adm1n",
},
},
}}})
*result.(*params.UpdateCredentialResults) = params.UpdateCredentialResults{
Results: []params.UpdateCredentialResult{{}},
}
s.called = true
return nil
},
),
BestVersion: 3,
}
client := cloudapi.NewClient(apiCaller)
result, err := client.AddCloudsCredentials(createCredentials(1))
c.Assert(err, jc.ErrorIsNil)
c.Assert(result, gc.DeepEquals, []params.UpdateCredentialResult{{}})
c.Assert(s.called, jc.IsTrue)
}
16 changes: 8 additions & 8 deletions apiserver/common/credentialcommon/modelcredential.go
View file
Expand Up @@ -18,7 +18,7 @@ import (
)

// ValidateExistingModelCredential checks if the cloud credential that a given model uses is valid for it.
func ValidateExistingModelCredential(backend PersistentBackend, callCtx context.ProviderCallContext, migrating bool) (params.ErrorResults, error) {
func ValidateExistingModelCredential(backend PersistentBackend, callCtx context.ProviderCallContext, checkCloudInstances bool) (params.ErrorResults, error) {
model, err := backend.Model()
if err != nil {
return params.ErrorResults{}, errors.Trace(err)
Expand All @@ -38,11 +38,11 @@ func ValidateExistingModelCredential(backend PersistentBackend, callCtx context.
return params.ErrorResults{}, errors.NotValidf("credential %q", storedCredential.Name)
}
credential := cloud.NewCredential(cloud.AuthType(storedCredential.AuthType), storedCredential.Attributes)
return ValidateNewModelCredential(backend, callCtx, credentialTag, &credential, migrating)
return ValidateNewModelCredential(backend, callCtx, credentialTag, &credential, checkCloudInstances)
}

// ValidateNewModelCredential checks if a new cloud credential could be valid for a given model.
func ValidateNewModelCredential(backend PersistentBackend, callCtx context.ProviderCallContext, credentialTag names.CloudCredentialTag, credential *cloud.Credential, migrating bool) (params.ErrorResults, error) {
func ValidateNewModelCredential(backend PersistentBackend, callCtx context.ProviderCallContext, credentialTag names.CloudCredentialTag, credential *cloud.Credential, checkCloudInstances bool) (params.ErrorResults, error) {
openParams, err := buildOpenParams(backend, credentialTag, credential)
if err != nil {
return params.ErrorResults{}, errors.Trace(err)
Expand All @@ -55,7 +55,7 @@ func ValidateNewModelCredential(backend PersistentBackend, callCtx context.Provi
case state.ModelTypeCAAS:
return checkCAASModelCredential(openParams)
case state.ModelTypeIAAS:
return checkIAASModelCredential(openParams, backend, callCtx, migrating)
return checkIAASModelCredential(openParams, backend, callCtx, checkCloudInstances)
default:
return params.ErrorResults{}, errors.NotSupportedf("model type %q", model.Type())
}
Expand All @@ -74,7 +74,7 @@ func checkCAASModelCredential(brokerParams environs.OpenParams) (params.ErrorRes
return params.ErrorResults{}, nil
}

func checkIAASModelCredential(openParams environs.OpenParams, backend PersistentBackend, callCtx context.ProviderCallContext, migrating bool) (params.ErrorResults, error) {
func checkIAASModelCredential(openParams environs.OpenParams, backend PersistentBackend, callCtx context.ProviderCallContext, checkCloudInstances bool) (params.ErrorResults, error) {
env, err := newEnv(openParams)
if err != nil {
return params.ErrorResults{}, errors.Trace(err)
Expand All @@ -83,13 +83,13 @@ func checkIAASModelCredential(openParams environs.OpenParams, backend Persistent
// In the future, this check may be extended to other cloud resources,
// entities and operation-level authorisations such as interfaces,
// ability to CRUD storage, etc.
return checkMachineInstances(backend, env, callCtx, migrating)
return checkMachineInstances(backend, env, callCtx, checkCloudInstances)
}

// checkMachineInstances compares model machines from state with
// the ones reported by the provider using supplied credential.
// This only makes sense for non-k8s providers.
func checkMachineInstances(backend PersistentBackend, provider CloudProvider, callCtx context.ProviderCallContext, migrating bool) (params.ErrorResults, error) {
func checkMachineInstances(backend PersistentBackend, provider CloudProvider, callCtx context.ProviderCallContext, checkCloudInstances bool) (params.ErrorResults, error) {
fail := func(original error) (params.ErrorResults, error) {
return params.ErrorResults{}, original
}
Expand Down Expand Up @@ -149,7 +149,7 @@ func checkMachineInstances(backend PersistentBackend, provider CloudProvider, ca
for _, instance := range instances {
id := string(instance.Id())
instanceIds.Add(id)
if migrating {
if checkCloudInstances {
if _, found := machinesByInstance[id]; !found {
results = append(results, serverError(errors.Errorf("no machine with instance %q", id)))
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/juju/cloud/addcredential.go
View file
Expand Up @@ -642,7 +642,7 @@ func (c *addCredentialCommand) addRemoteCredentials(ctxt *cmd.Context, all map[s
return err
}
defer client.Close()
results, err := client.UpdateCloudsCredentials(verified)
results, err := client.AddCloudsCredentials(verified)
if err != nil {
logger.Errorf("%v", err)
ctxt.Warningf("Could not upload credentials to controller %q", c.ControllerName)
Expand Down
2 changes: 1 addition & 1 deletion cmd/juju/cloud/addcredential_test.go
View file
Expand Up @@ -851,7 +851,7 @@ credentials:
sourceFile := s.createTestCredentialFile(c, expectedContents)

called := false
s.api.updateCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
s.api.addCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
c.Assert(cloudCredentials, gc.HasLen, 1)
called = true
expectedTag := names.NewCloudCredentialTag(fmt.Sprintf("%v/admin@local/blah", cloudName)).String()
Expand Down
2 changes: 1 addition & 1 deletion cmd/juju/cloud/detectcredentials.go
View file
Expand Up @@ -491,7 +491,7 @@ func (c *detectCredentialsCommand) addRemoteCredentials(ctxt *cmd.Context, cloud
if len(verified) == 0 {
return erred
}
result, err := client.UpdateCloudsCredentials(verified)
result, err := client.AddCloudsCredentials(verified)
if err != nil {
logger.Errorf("%v", err)
ctxt.Warningf("Could not upload credentials to controller %q", c.ControllerName)
Expand Down
4 changes: 2 additions & 2 deletions cmd/juju/cloud/detectcredentials_test.go
View file
Expand Up @@ -372,7 +372,7 @@ func (s *detectCredentialsSuite) TestRemoteLoad(c *gc.C) {
s.setupStore(c)
cloudName := "test-cloud"
called := false
s.api.updateCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
s.api.addCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
c.Assert(cloudCredentials, gc.HasLen, 1)
called = true
expectedTag := names.NewCloudCredentialTag(fmt.Sprintf("%v/admin@local/blah", cloudName)).String()
Expand Down Expand Up @@ -446,7 +446,7 @@ func (s *detectCredentialsSuite) assertAutoloadCredentials(c *gc.C, expectedStde
s.setupStore(c)
cloudName := "test-cloud"
called := false
s.api.updateCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
s.api.addCloudsCredentials = func(cloudCredentials map[string]jujucloud.Credential) ([]params.UpdateCredentialResult, error) {
c.Assert(cloudCredentials, gc.HasLen, 1)
called = true
expectedTag := names.NewCloudCredentialTag(fmt.Sprintf("%v/admin@local/blah", cloudName)).String()
Expand Down

0 comments on commit 9cef799

Please sign in to comment.