Merge pull request juju#16692 from hmlanigan/remove-api-server-restri…

…ctions

juju#16692

This change enables support of the terraform provider for juju with a juju 2.9.latest controller once it moves to the juju 3.3 branch. This is part of a move to ensure that juju clients support their current major version and the last patch of the last minor of the prior major version. 

This change also makes the upgrade process from 2.9 to 3.x easier as only 1 client will now be necessary.

Functionality supported by a juju client is determined by which facade versions both the client and server support. Older clients may not support the latest features in each major release. 

There is no expectation that a juju 3.x client can bootstrap a juju 2.9 controller etc. 

Once the client stopped rejecting clients of the next major version, client negotiation is done via facade version. But there's a problem as the params structures used as arguments in a specific set of facade methods are different between 2.9 and 3.x without a facade version nor structure name change. The series, os and channel elements of origin structures were removed. This impacts: deploy, refresh and add-machine. 

There were 2 choices of how to approach ensuring a 2.9 controller could properly deploy, refresh and add a machine. 

1. "Fix" the args to each facade call before they were used to add the series data based on the base supplied in the same structure.
2. Update the workflows for deploy, refresh and adding a machine to prefer a base if provided.

Given that a 2.9 controller also requires compatibility with a 2.8 client and many changes were made related to series/base in 3.x, I choose option 1 as being less intrusive. Also less potentially for handling series/base differently from the 3.x code.

## Checklist

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [x] Go unit tests, with comments saying what you're testing
- [ ] ~[Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~
- [ ] ~[doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~

## QA steps
Note for QA steps:

- `juju` refers to the compiled code under test; `juju_33` is the juju snap with channel 3.3/stable.

```bash
#
# Bootstrap a juju 3.3 controller for later upgrade and migration of a 2.9 model.
# 
$ juju_33 bootstrap localhost destination

# 
# Bootstrap a controller to test the changes
# 
$ juju bootstrap localhost testing

# 
# Run various commands requiring series and base, including charms with and without 
# 
$ juju_33 add-mode moveme
$ juju_33 deploy juju-qa-test --revision 23 --channel edge
$ juju_33 deploy ./tests/suites/deploy/charms/lxd-profile-alt
$ juju_33 deploy postgresql
$ juju_33 add-machine --base ubuntu@20.04 
$ juju_33 refresh lxd-profile-alt --path ./tests/suites/deploy/charms/lxd-profile-alt

# Verify the juju-qa-test resource was correctly downloaded, check the application status for an output change.
$ juju_33 config juju-qa-test foo-file=true

# Update the base of an application for the next unit added.
$ juju_33 set-application-base juju-qa-test ubuntu@20.04

# Migrate to the 3.3. controller & upgrade.
$ juju_33 migrate moveme destination
$ juju_33 switch destination
$ juju_33 upgrade-model

# Ensure base change successful, validate the new unit is using ubuntu@20.04
$ juju_33 add-unit juju-qa-test

# Validate migrations back still fail:
$ juju_33 migrate five terraform
ERROR target prechecks failed: model has higher version than target controller (3.3.0 > 2.9.48.6)
$ juju migrate five terraform
ERROR target prechecks failed: model has higher version than target controller (3.3.0 > 2.9.48.6)
```


## Links
JUJU-4922

api/apiclient.go

@@ -1227,35 +1227,12 @@ func isX509Error(err error) bool {
 // object id, and the specific RPC method. It marshalls the Arguments, and will
 // unmarshall the result into the response object that is supplied.
 func (s *state) APICall(facade string, vers int, id, method string, args, response interface{}) error {
-	err := s.client.Call(rpc.Request{
+	return s.client.Call(rpc.Request{
 		Type:    facade,
 		Version: vers,
 		Id:      id,
 		Action:  method,
 	}, args, response)
-	if err == nil {
-		return nil
-	}
-	code := params.ErrCode(err)
-	if code != params.CodeIncompatibleClient {
-		return errors.Trace(err)
-	}
-	// Default to major version 2 for older servers.
-	serverMajorVersion := 2
-	err = errors.Cause(err)
-	apiErr, ok := err.(*rpc.RequestError)
-	if ok {
-		if serverVersion, ok := apiErr.Info["server-version"]; ok {
-			serverVers, err := version.Parse(fmt.Sprintf("%v", serverVersion))
-			if err == nil {
-				serverMajorVersion = serverVers.Major
-			}
-		}
-	}
-	logger.Debugf("%v.%v API call not supported", facade, method)
-	return errors.NewNotSupported(nil, fmt.Sprintf(
-		"juju client with version %d.%d used with a controller having major version %d not supported\nre-install your juju client to match the version running on the controller",
-		jujuversion.Current.Major, jujuversion.Current.Minor, serverMajorVersion))
 }
 
 func (s *state) Close() error {

api/apiclient_test.go

@@ -1187,24 +1187,6 @@ func (s *apiclientSuite) TestLoginCapturesCLIArgs(c *gc.C) {
 	c.Assert(request.CLIArgs, gc.Equals, `this is "the test" command`)
 }
 
-func (s *apiclientSuite) TestLoginIncompatibleClient(c *gc.C) {
-	clock := &fakeClock{}
-	conn := api.NewTestingState(api.TestingStateParams{
-		RPCConnection: newRPCConnection(&rpc.RequestError{
-			Code: "incompatible client",
-			Info: map[string]interface{}{"server-version": "99.0.0"},
-		}),
-		Clock: clock,
-	})
-
-	err := conn.APICall("facade", 1, "id", "method", nil, nil)
-	c.Check(clock.waits, gc.HasLen, 0)
-	c.Assert(err, gc.ErrorMatches, fmt.Sprintf(
-		"juju client with version %d.%d used with a controller having major version %d not supported\\n.*",
-		jujuversion.Current.Major, jujuversion.Current.Minor, 99,
-	))
-}
-
 type clientDNSNameSuite struct {
 	jjtesting.JujuConnSuite
 }

apiserver/admin.go

@@ -13,7 +13,6 @@ import (
 	"github.com/juju/errors"
 	"github.com/juju/names/v4"
 	"github.com/juju/rpcreflect"
-	"github.com/juju/version/v2"
 
 	"github.com/juju/juju/api"
 	"github.com/juju/juju/apiserver/common"
@@ -136,19 +135,11 @@ func (a *admin) login(ctx context.Context, req params.LoginRequest, loginVersion
 		return fail, errors.Trace(err)
 	}
 
-	// Default client version to 2 since older 2.x clients
-	// don't send this field.
-	loginClientVersion := version.Number{Major: 2}
-	if clientVersion, err := version.Parse(req.ClientVersion); err == nil {
-		loginClientVersion = clientVersion
-	}
-
 	apiRoot, err = restrictAPIRoot(
 		a.srv,
 		apiRoot,
 		a.root.model,
 		*authResult,
-		loginClientVersion,
 	)
 	if err != nil {
 		return fail, errors.Trace(err)

apiserver/errors/errors.go

@@ -150,7 +150,6 @@ func ServerError(err error) *params.Error {
 
 	var (
 		dischargeRequiredError       *DischargeRequiredError
-		incompatibleClientError      *params.IncompatibleClientError
 		notLeaderError               *NotLeaderError
 		redirectError                *RedirectError
 		upgradeSeriesValidationError *UpgradeSeriesValidationError
@@ -242,9 +241,6 @@ func ServerError(err error) *params.Error {
 		code = params.CodeNotYetAvailable
 	case errors.Is(err, ErrTryAgain):
 		code = params.CodeTryAgain
-	case errors.As(err, &incompatibleClientError):
-		code = params.CodeIncompatibleClient
-		info = incompatibleClientError.AsMap()
 	case errors.As(err, &notLeaderError):
 		code = params.CodeNotLeader
 		info = notLeaderError.AsMap()

apiserver/errors/errors_test.go

@@ -23,7 +23,6 @@ import (
 	"github.com/juju/juju/rpc/params"
 	stateerrors "github.com/juju/juju/state/errors"
 	"github.com/juju/juju/testing"
-	jujuversion "github.com/juju/juju/version"
 )
 
 type errorsSuite struct {
@@ -239,25 +238,6 @@ var errorTransformTests = []struct {
 	code:       params.CodeNotYetAvailable,
 	status:     http.StatusConflict,
 	helperFunc: params.IsCodeNotYetAvailable,
-}, {
-	err: &params.IncompatibleClientError{
-		ServerVersion: jujuversion.Current,
-	},
-	code:   params.CodeIncompatibleClient,
-	status: http.StatusInternalServerError,
-	helperFunc: func(err error) bool {
-		err1, ok := err.(*params.Error)
-		err2 := &params.IncompatibleClientError{
-			ServerVersion: jujuversion.Current,
-		}
-		if !ok || err1.Info == nil || !reflect.DeepEqual(err1.Info, err2.AsMap()) {
-			return false
-		}
-		return true
-	},
-	targetTester: func(e error) bool {
-		return errors.HasType[*params.IncompatibleClientError](e)
-	},
 }, {
 	err:    apiservererrors.NewNotLeaderError("1.1.1.1", "1"),
 	code:   params.CodeNotLeader,
@@ -373,8 +353,7 @@ func (s *errorsSuite) TestErrorTransform(c *gc.C) {
 			params.CodeMachineHasAttachedStorage,
 			params.CodeDischargeRequired,
 			params.CodeModelNotFound,
-			params.CodeRedirect,
-			params.CodeIncompatibleClient:
+			params.CodeRedirect:
 			continue
 		case params.CodeOperationBlocked:
 			// ServerError doesn't actually have a case for this code.

apiserver/export_test.go

@@ -9,7 +9,6 @@ import (
 	"github.com/juju/clock"
 	"github.com/juju/names/v4"
 	jc "github.com/juju/testing/checkers"
-	"github.com/juju/version/v2"
 	gc "gopkg.in/check.v1"
 
 	"github.com/juju/juju/apiserver/common"
@@ -146,20 +145,6 @@ func TestingRestrictedRoot(check func(string, string) error) rpc.Root {
 	return restrictRoot(r, check)
 }
 
-// TestingAboutToRestoreRoot returns a limited root which allows
-// methods as per when a restore is about to happen.
-func TestingAboutToRestoreRoot() rpc.Root {
-	r := TestingAPIRoot(AllFacades())
-	return restrictRoot(r, aboutToRestoreMethodsOnly)
-}
-
-// TestingUpgradeOrMigrationOnlyRoot returns a restricted srvRoot
-// as if called from a newer client.
-func TestingUpgradeOrMigrationOnlyRoot(userLogin bool, clientVersion version.Number) rpc.Root {
-	r := TestingAPIRoot(AllFacades())
-	return restrictRoot(r, checkClientVersion(userLogin, clientVersion))
-}
-
 // PatchGetMigrationBackend overrides the getMigrationBackend function
 // to support testing.
 func PatchGetMigrationBackend(p Patcher, ctrlSt controllerBackend, st migrationBackend) {

apiserver/facades/client/application/application.go

@@ -417,16 +417,19 @@ func (api *APIBase) Deploy(args params.ApplicationsDeploy) (params.ErrorResults,
 	}
 
 	for i, arg := range args.Applications {
-		err := deployApplication(
-			api.backend,
-			api.model,
-			api.stateCharm,
-			arg,
-			api.deployApplicationFunc,
-			api.storagePoolManager,
-			api.registry,
-			api.caasBroker,
-		)
+		var err error
+		if arg, err = compatibilityApplicationDeployArgs(arg); err == nil {
+			err = deployApplication(
+				api.backend,
+				api.model,
+				api.stateCharm,
+				arg,
+				api.deployApplicationFunc,
+				api.storagePoolManager,
+				api.registry,
+				api.caasBroker,
+			)
+		}
 		result.Results[i].Error = apiservererrors.ServerError(err)
 
 		if err != nil && len(arg.Resources) != 0 {
@@ -447,6 +450,32 @@ func (api *APIBase) Deploy(args params.ApplicationsDeploy) (params.ErrorResults,
 	return result, nil
 }
 
+// compatibilityApplicationDeployArgs ensures that Deploy calls from
+// a juju 3.x client will work against a juju 2.9.x controller. In
+// juju 3.x, params.ApplicationsDeploy was changed to remove series
+// however, the facade version was not changed, nor was the name of
+// the params.ApplicationsDeploy changed. Thus it appears you can use
+// a juju 3.x client to deploy from a juju 2.9 controller, however it
+// fails because the series was not found. Make those corrections here.
+func compatibilityApplicationDeployArgs(arg params.ApplicationDeploy) (params.ApplicationDeploy, error) {
+	origin := arg.CharmOrigin
+	if origin.Series != "" {
+		return arg, nil
+	}
+	originSeries, err := series.GetSeriesFromChannel(origin.Base.Name, origin.Base.Channel)
+	if err != nil {
+		return arg, err
+	}
+	curl, err := charm.ParseURL(arg.CharmURL)
+	if err != nil {
+		return arg, err
+	}
+	arg.CharmURL = curl.WithSeries(originSeries).String()
+	origin.Series = originSeries
+	arg.Series = originSeries
+	return arg, nil
+}
+
 func applicationConfigSchema(modelType state.ModelType) (environschema.Fields, schema.Defaults, error) {
 	if modelType != state.ModelTypeCAAS {
 		return trustFields, trustDefaults, nil