Merge pull request juju#17168 from jack-w-shaw/3.5-into-main

juju#17168

Forward merge:
- juju#17050
- juju#17051
- juju#17035
- juju#17066
- juju#17071
- juju#17073
- juju#17074
- juju#17077
- juju#17078
- juju#17083
- juju#17085
- juju#17087
- juju#17088
- juju#17090
- juju#17094
- juju#17095
- juju#17096
- juju#17092
- juju#17101
- juju#17109


Conflicts:
- api/controller/crossmodelrelations/crossmodelrelations.go
- apiserver/facades/client/keymanager/keymanager.go
- apiserver/facades/client/modelupgrader/upgrader.go
- apiserver/facades/client/modelupgrader/upgrader_test.go
- cmd/modelcmd/base.go
- go.mod
- go.sum
- internal/worker/remoterelations/remoteapplicationworker.go
- internal/worker/uniter/resolver.go
- state/migration_export.go

Conflicts were mostly resolved trivially, as they were a result of contexts being added to all faacdes.

Keymanager was slightly more difficult. There are 4 unit test failures in `github.com/juju/juju/apiserver/facades/client/keymanager` which I have not been able to resolve. @Aflynn50 could you take a look?

api/controller/crossmodelrelations/crossmodelrelations.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/juju/juju/api/base"
 	apiwatcher "github.com/juju/juju/api/watcher"
-	apiservererrors "github.com/juju/juju/apiserver/errors"
 	"github.com/juju/juju/core/watcher"
 	"github.com/juju/juju/rpc/params"
 )
@@ -498,7 +497,7 @@ func (c *Client) WatchConsumedSecretsChanges(ctx context.Context, applicationTok
 		// Reset the results struct before each api call.
 		results = params.SecretRevisionWatchResults{}
 		if err := c.facade.FacadeCall(context.TODO(), "WatchConsumedSecretsChanges", args, &results); err != nil {
-			return errors.Trace(err)
+			return params.TranslateWellKnownError(err)
 		}
 		if len(results.Results) != 1 {
 			return errors.Errorf("expected 1 result, got %d", len(results.Results))
@@ -528,7 +527,7 @@ func (c *Client) WatchConsumedSecretsChanges(ctx context.Context, applicationTok
 		result = results.Results[0]
 	}
 	if result.Error != nil {
-		return nil, apiservererrors.RestoreError(result.Error)
+		return nil, params.TranslateWellKnownError(result.Error)
 	}
 
 	w := apiwatcher.NewSecretsRevisionWatcher(c.facade.RawAPICaller(), result)

api/controller/crossmodelrelations/crossmodelrelations_test.go

@@ -722,9 +722,9 @@ func (s *CrossModelRelationsSuite) TestWatchConsumedSecretsChanges(c *gc.C) {
 	mac, err := jujutesting.NewMacaroon("id")
 	c.Assert(err, jc.ErrorIsNil)
 	var callCount int
-	apiCaller := testing.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
+	apiCaller := testing.BestVersionCaller{APICallerFunc: testing.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
 		c.Check(objType, gc.Equals, "CrossModelRelations")
-		c.Check(version, gc.Equals, 0)
+		c.Check(version, gc.Equals, 3)
 		c.Check(id, gc.Equals, "")
 		c.Check(arg, jc.DeepEquals, params.WatchRemoteSecretChangesArgs{Args: []params.WatchRemoteSecretChangesArg{{
 			ApplicationToken: appToken, RelationToken: relToken, Macaroons: macaroon.Slice{mac},
@@ -739,7 +739,7 @@ func (s *CrossModelRelationsSuite) TestWatchConsumedSecretsChanges(c *gc.C) {
 		}
 		callCount++
 		return nil
-	})
+	}), BestVersion: 3}
 	client := crossmodelrelations.NewClientWithCache(apiCaller, s.cache)
 	_, err = client.WatchConsumedSecretsChanges(context.Background(), appToken, relToken, mac)
 	c.Check(err, gc.ErrorMatches, "FAIL")
@@ -758,7 +758,7 @@ func (s *CrossModelRelationsSuite) TestWatchConsumedSecretsChangesDischargeRequi
 		callCount    int
 		dischargeMac macaroon.Slice
 	)
-	apiCaller := testing.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
+	apiCaller := testing.BestVersionCaller{APICallerFunc: testing.APICallerFunc(func(objType string, version int, id, request string, arg, result interface{}) error {
 		var resultErr *params.Error
 		switch callCount {
 		case 2, 3: //Watcher Next, Stop
@@ -781,7 +781,7 @@ func (s *CrossModelRelationsSuite) TestWatchConsumedSecretsChangesDischargeRequi
 		s.fillResponse(c, result, resp)
 		callCount++
 		return nil
-	})
+	}), BestVersion: 3}
 	acquirer := &mockDischargeAcquirer{}
 	callerWithBakery := testing.APICallerWithBakery(apiCaller, acquirer)
 	client := crossmodelrelations.NewClientWithCache(callerWithBakery, s.cache)

apiserver/facades/client/application/deployrepository.go

@@ -410,8 +410,9 @@ func (v *deployFromRepositoryValidator) validate(ctx context.Context, arg params
 		bindings, err := v.newStateBindings(v.state, arg.EndpointBindings)
 		if err != nil {
 			errs = append(errs, err)
+		} else {
+			dt.endpoints = bindings.Map()
 		}
-		dt.endpoints = bindings.Map()
 	}
 	// resolve and validate resources
 	resources, pendingResourceUploads, resolveResErr := v.resolveResources(ctx, dt.charmURL, dt.origin, dt.resources, resolvedCharm.Meta().Resources)

apiserver/facades/client/application/deployrepository_test.go

@@ -243,6 +243,55 @@ func (s *validatorSuite) TestValidateEndpointBindingSuccess(c *gc.C) {
 	})
 }
 
+func (s *validatorSuite) TestValidateEndpointBindingFail(c *gc.C) {
+	defer s.setupMocks(c).Finish()
+	s.expectSimpleValidate()
+	// resolveCharm
+	curl := charm.MustParseURL("testcharm")
+	resultURL := charm.MustParseURL("ch:amd64/jammy/testcharm-4")
+	origin := corecharm.Origin{
+		Source:   "charm-hub",
+		Channel:  &charm.Channel{Risk: "stable"},
+		Platform: corecharm.Platform{Architecture: "amd64"},
+	}
+	resolvedOrigin := corecharm.Origin{
+		Source:   "charm-hub",
+		Type:     "charm",
+		Channel:  &charm.Channel{Track: "default", Risk: "stable"},
+		Platform: corecharm.Platform{Architecture: "amd64", OS: "ubuntu", Channel: "22.04"},
+		Revision: intptr(4),
+	}
+	// getCharm
+	charmID := corecharm.CharmID{URL: curl, Origin: origin}
+	resolvedData := getResolvedData(resultURL, resolvedOrigin)
+	s.repo.EXPECT().ResolveForDeploy(gomock.Any(), charmID).Return(resolvedData, nil)
+	s.repo.EXPECT().ResolveResources(gomock.Any(), nil, corecharm.CharmID{URL: resultURL, Origin: resolvedOrigin}).Return(nil, nil)
+	s.model.EXPECT().UUID().Return("")
+
+	// state bindings
+	endpointMap := map[string]string{"to": "from"}
+	s.state.EXPECT().ModelConstraints().Return(constraints.Value{Arch: strptr("arm64")}, nil)
+	s.state.EXPECT().Charm(gomock.Any()).Return(nil, errors.NotFoundf("charm"))
+
+	s.repoFactory.EXPECT().GetCharmRepository(gomock.Any(), gomock.Any()).Return(s.repo, nil).AnyTimes()
+	v := &deployFromRepositoryValidator{
+		model:       s.model,
+		state:       s.state,
+		repoFactory: s.repoFactory,
+		newStateBindings: func(st any, givenMap map[string]string) (Bindings, error) {
+			return nil, errors.NotFoundf("space")
+		},
+	}
+
+	arg := params.DeployFromRepositoryArg{
+		CharmName:        "testcharm",
+		EndpointBindings: endpointMap,
+	}
+	_, errs := v.validate(context.Background(), arg)
+	c.Assert(errs, gc.HasLen, 1)
+	c.Assert(errs[0], jc.ErrorIs, errors.NotFound)
+}
+
 func (s *validatorSuite) expectSimpleValidate() {
 	// createOrigin
 	s.state.EXPECT().ModelConstraints().Return(constraints.Value{}, nil)

apiserver/facades/client/keymanager/keymanager.go

@@ -144,6 +144,7 @@ func (api *KeyManagerAPI) currentKeyDataForAdd(ctx context.Context) (keys []stri
 		fingerprint, _, err := ssh.KeyFingerprint(key)
 		if err != nil {
 			api.logger.Warningf("ignoring invalid ssh key %q: %v", key, err)
+			continue
 		}
 		fingerprints.Add(fingerprint)
 	}
@@ -293,33 +294,47 @@ func (api *KeyManagerAPI) ImportKeys(ctx context.Context, arg params.ModifyUserS
 	return params.ErrorResults{Results: results}, nil
 }
 
+type keyDataForDelete struct {
+	allKeys       []string
+	byFingerprint map[string]string
+	byComment     map[string]string
+	invalidKeys   map[string]string
+}
+
 // currentKeyDataForDelete gathers data used when deleting ssh keys.
-func (api *KeyManagerAPI) currentKeyDataForDelete(ctx context.Context) (
-	currentKeys []string, byFingerprint map[string]string, byComment map[string]string, err error) {
+func (api *KeyManagerAPI) currentKeyDataForDelete(ctx context.Context) (keyDataForDelete, error) {
 
 	cfg, err := api.model.ModelConfig(ctx)
 	if err != nil {
-		return nil, nil, nil, fmt.Errorf("reading current key data: %v", err)
+		return keyDataForDelete{}, fmt.Errorf("reading current key data: %v", err)
 	}
 	// For now, authorised keys are global, common to all users.
-	currentKeys = ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
+	currentKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
 
 	// Make two maps that index keys by fingerprint and by comment for fast
 	// lookup of keys to delete which may be given as either.
-	byFingerprint = make(map[string]string)
-	byComment = make(map[string]string)
+	byFingerprint := make(map[string]string)
+	byComment := make(map[string]string)
+	invalidKeys := make(map[string]string)
 	for _, key := range currentKeys {
 		fingerprint, comment, err := ssh.KeyFingerprint(key)
 		if err != nil {
-			api.logger.Debugf("keeping unrecognised existing ssh key %q: %v", key, err)
+			api.logger.Debugf("invalid existing ssh key %q: %v", key, err)
+			invalidKeys[key] = key
 			continue
 		}
 		byFingerprint[fingerprint] = key
 		if comment != "" {
 			byComment[comment] = key
 		}
 	}
-	return currentKeys, byFingerprint, byComment, nil
+	data := keyDataForDelete{
+		allKeys:       currentKeys,
+		byFingerprint: byFingerprint,
+		byComment:     byComment,
+		invalidKeys:   invalidKeys,
+	}
+	return data, nil
 }
 
 // DeleteKeys deletes the authorised ssh keys for the specified user.
@@ -334,7 +349,7 @@ func (api *KeyManagerAPI) DeleteKeys(ctx context.Context, arg params.ModifyUserS
 		return params.ErrorResults{}, nil
 	}
 
-	allKeys, byFingerprint, byComment, err := api.currentKeyDataForDelete(ctx)
+	keyData, err := api.currentKeyDataForDelete(ctx)
 	if err != nil {
 		return params.ErrorResults{}, apiservererrors.ServerError(fmt.Errorf("reading current key data: %v", err))
 	}
@@ -344,27 +359,33 @@ func (api *KeyManagerAPI) DeleteKeys(ctx context.Context, arg params.ModifyUserS
 
 	results := transform.Slice(arg.Keys, func(keyId string) params.ErrorResult {
 		// Is given keyId a fingerprint?
-		key, ok := byFingerprint[keyId]
+		key, ok := keyData.byFingerprint[keyId]
 		if ok {
 			keysToDelete.Add(key)
 			return params.ErrorResult{}
 		}
 		// Not a fingerprint, is it a comment?
-		key, ok = byComment[keyId]
+		key, ok = keyData.byComment[keyId]
 		if ok {
 			if jujuKeyCommentIdentifiers.Contains(keyId) {
 				return params.ErrorResult{Error: apiservererrors.ServerError(fmt.Errorf("may not delete internal key: %s", keyId))}
 			}
 			keysToDelete.Add(key)
 			return params.ErrorResult{}
 		}
-		return params.ErrorResult{Error: apiservererrors.ServerError(fmt.Errorf("invalid ssh key: %s", keyId))}
+		// Allow invalid keys to be deleted by writing out key verbatim.
+		key, ok = keyData.invalidKeys[keyId]
+		if ok {
+			keysToDelete.Add(key)
+			return params.ErrorResult{}
+		}
+		return params.ErrorResult{Error: apiservererrors.ServerError(fmt.Errorf("key not found: %s", keyId))}
 	})
 
 	var keysToWrite []string
 
 	// Add back only the keys that are not deleted, preserving the order.
-	for _, key := range allKeys {
+	for _, key := range keyData.allKeys {
 		if !keysToDelete.Contains(key) {
 			keysToWrite = append(keysToWrite, key)
 		}

apiserver/facades/client/keymanager/keymanager_test.go

@@ -119,6 +119,7 @@ func (s *keyManagerSuite) assertAddKeys(c *gc.C) {
 	s.setAuthorizedKeys(c, key1, key2, "bad key")
 
 	newKey := sshtesting.ValidKeyThree.Key + " newuser@host"
+	newLineKey := sshtesting.ValidKeyFour.Key + " line1\nline2"
 
 	newAttrs := map[string]interface{}{
 		config.AuthorizedKeysKey: strings.Join([]string{key1, key2, "bad key", newKey}, "\n"),
@@ -127,7 +128,7 @@ func (s *keyManagerSuite) assertAddKeys(c *gc.C) {
 
 	args := params.ModifyUserSSHKeys{
 		User: names.NewUserTag("admin").Name(),
-		Keys: []string{key2, newKey, newKey, "invalid-key"},
+		Keys: []string{key2, newKey, newKey, "invalid-key", newLineKey},
 	}
 	results, err := s.api.AddKeys(context.Background(), args)
 	c.Assert(err, jc.ErrorIsNil)
@@ -138,6 +139,7 @@ func (s *keyManagerSuite) assertAddKeys(c *gc.C) {
 			{Error: nil},
 			{Error: apiservertesting.ServerError(fmt.Sprintf("duplicate ssh key: %s", newKey))},
 			{Error: apiservertesting.ServerError("invalid ssh key: invalid-key")},
+			{Error: apiservertesting.ServerError(fmt.Sprintf("invalid ssh key: %s", newLineKey))},
 		},
 	})
 }
@@ -207,24 +209,25 @@ func (s *keyManagerSuite) TestAddJujuSystemKey(c *gc.C) {
 func (s *keyManagerSuite) assertDeleteKeys(c *gc.C) {
 	key1 := sshtesting.ValidKeyOne.Key + " user@host"
 	key2 := sshtesting.ValidKeyTwo.Key
-	s.setAuthorizedKeys(c, key1, key2, "bad key")
+	s.setAuthorizedKeys(c, key1, key2, "bad key 1", "bad key 2")
 
 	newAttrs := map[string]interface{}{
-		config.AuthorizedKeysKey: strings.Join([]string{key1, "bad key"}, "\n"),
+		config.AuthorizedKeysKey: strings.Join([]string{key1, "bad key 1"}, "\n"),
 	}
 	s.model.EXPECT().UpdateModelConfig(gomock.Any(), newAttrs, nil)
 
 	args := params.ModifyUserSSHKeys{
 		User: names.NewUserTag("admin").String(),
-		Keys: []string{sshtesting.ValidKeyTwo.Fingerprint, sshtesting.ValidKeyThree.Fingerprint, "invalid-key"},
+		Keys: []string{sshtesting.ValidKeyTwo.Fingerprint, sshtesting.ValidKeyThree.Fingerprint, "invalid-key", "bad key 2"},
 	}
 	results, err := s.api.DeleteKeys(context.Background(), args)
 	c.Assert(err, jc.ErrorIsNil)
 	c.Assert(results, gc.DeepEquals, params.ErrorResults{
 		Results: []params.ErrorResult{
 			{Error: nil},
-			{Error: apiservertesting.ServerError("invalid ssh key: " + sshtesting.ValidKeyThree.Fingerprint)},
-			{Error: apiservertesting.ServerError("invalid ssh key: invalid-key")},
+			{Error: apiservertesting.ServerError("key not found: " + sshtesting.ValidKeyThree.Fingerprint)},
+			{Error: apiservertesting.ServerError("key not found: invalid-key")},
+			{Error: nil},
 		},
 	})
 }

apiserver/facades/client/modelupgrader/upgrader.go

@@ -166,15 +166,10 @@ func (m *ModelUpgraderAPI) UpgradeModel(ctx stdcontext.Context, arg params.Upgra
 		return result, errors.Trace(err)
 	}
 
-	m.logger.Debugf("deciding target version for model upgrade, from %q to %q for stream %q", currentVersion, targetVersion, arg.AgentStream)
-	args := common.FindAgentsParams{
-		AgentStream:   arg.AgentStream,
-		ControllerCfg: controllerCfg,
-		ModelType:     model.Type(),
-	}
 	// For non controller models, we use the exact controller
 	// model version to upgrade to, unless an explicit target
 	// has been specified.
+	useControllerVersion := false
 	if !model.IsControllerModel() {
 		ctrlModel, err := m.statePool.ControllerModel()
 		if err != nil {
@@ -184,26 +179,35 @@ func (m *ModelUpgraderAPI) UpgradeModel(ctx stdcontext.Context, arg params.Upgra
 		if err != nil {
 			return result, errors.Trace(err)
 		}
-		if targetVersion == version.Zero {
+		if targetVersion == version.Zero || targetVersion.Compare(vers) == 0 {
 			targetVersion = vers
-		} else if vers.Compare(targetVersion.ToPatch()) > 0 {
-			return result, errors.Errorf("cannot upgrade to a version %q greater than that of the controller %q", args.Number, vers)
+			useControllerVersion = true
+		} else if vers.Compare(targetVersion.ToPatch()) < 0 {
+			return result, errors.Errorf("cannot upgrade to a version %q greater than that of the controller %q", targetVersion, vers)
 		}
 	}
-	if targetVersion == version.Zero {
-		args.MajorVersion = currentVersion.Major
-		args.MinorVersion = currentVersion.Minor
-	} else {
-		args.Number = targetVersion
-	}
-	targetVersion, err = m.decideVersion(ctx, currentVersion, args)
-	if errors.Is(errors.Cause(err), errors.NotFound) || errors.Is(errors.Cause(err), errors.AlreadyExists) {
-		result.Error = apiservererrors.ServerError(err)
-		return result, nil
-	}
+	if !useControllerVersion {
+		m.logger.Debugf("deciding target version for model upgrade, from %q to %q for stream %q", currentVersion, targetVersion, arg.AgentStream)
+		args := common.FindAgentsParams{
+			AgentStream:   arg.AgentStream,
+			ControllerCfg: controllerCfg,
+			ModelType:     model.Type(),
+		}
+		if targetVersion == version.Zero {
+			args.MajorVersion = currentVersion.Major
+			args.MinorVersion = currentVersion.Minor
+		} else {
+			args.Number = targetVersion
+		}
+		targetVersion, err = m.decideVersion(ctx, currentVersion, args)
+		if errors.Is(errors.Cause(err), errors.NotFound) || errors.Is(errors.Cause(err), errors.AlreadyExists) {
+			result.Error = apiservererrors.ServerError(err)
+			return result, nil
+		}
 
-	if err != nil {
-		return result, errors.Trace(err)
+		if err != nil {
+			return result, errors.Trace(err)
+		}
 	}
 
 	// Before changing the agent version to trigger an upgrade or downgrade,