Merge pull request juju#17309 from babakks/css-8360/apply-client-in-s…

…imple-connector

juju#17309

**(Note that this PR is relevant to JIMM controllers which support login with client credentials. So whenever we say *controller*, it's a *JIMM controller*.)**

When Juju Terraform Provider uses client credentials (as a service account) to communicate with the API using a `SimpleConnector`, it leaves `SimpleConfig.Username` empty and assigns the client credentials to `SimpleConfig.ClientID` and `SimpleConfig.ClientSecret`.

The problem is the `NewSimple` function does not take this into account and panics with the following message whenever the `Username` is empty:

```
invalid user tag ""
```

This PR adds a simple check to prevent the panic.

## Checklist

<!-- If an item is not applicable, use `~strikethrough~`. -->

- [x] Code style: imports ordered, good names, simple structure, etc
- [x] Comments saying why design decisions were made
- [ ] ~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

QA-ing this change is a bit complicated because of two reasons:

1. The `NewSimple` function is meant to be used by users of the `juju/juju` module, and hence there's no internal usage in Juju (so we can verify the changes without spinning up other things). To invoke the method, we can use Juju Terraform Provider. Note that, for this purpose, we need to build the provider with the new Juju changes.
2. We'd need a controller that provides the newly introduced login method, `LoginWithClientCredentials`. If the controller The only controller that supports this method at the moment, is JIMM. Therefore, we need to spin up a JIMM controller. If we try with a non-JIMM controller, instead of a panic we get this error, which is a bit misleading but converts the unsupported state:
 ```
 this version of Juju does not support login from old clients (not supported) (not supported)
 ```

We can, of course, help out with QA-ing this to make sure it works as expected.

Anyway, the whole QA process should look like this:

0. Build the Juju Terraform Provider with the changes in Juju.
1. Spin up a JIMM controller.
2. Create a Terraform plan from this template:
 ```tf
 terraform {
 required_providers {
 juju = {
 source = "registry.terraform.io/juju/juju" # (uses local provider repository)
 version = "=0.12.0"
 }
 }
 }

 provider "juju" {
 controller_addresses = "jimm.localhost:443"

 client_id = "some-client-id" # Value not important
 client_secret = "some-secret" # Value not important

 ca_certificate = <<EOT
 -----BEGIN CERTIFICATE-----
 JIMM TLS termination CERT
 -----END CERTIFICATE-----
 EOT

 }

 resource "juju_model" "qa" {
 name = "qa"

 cloud {
 name = "localhost"
 }
 }

 resource "juju_application" "qa" {
 name = "qa"

 model = juju_model.qa.name

 charm {
 name = "juju-qa-test"
 }

 units = 1
 }
 ```
3. Run `terraform init` and `terraform apply`.
4. No Panic (i.e., `invalid user tag ""`) should happen.

## Documentation changes

<!-- How it affects user workflow (CLI or API). -->

## Links

<!-- Link to all relevant specification, documentation, bug, issue or JIRA card. -->

**Launchpad bug:** https://bugs.launchpad.net/juju/+bug/

**Jira card:** JUJU-

api/connector/package_test.go

@@ -0,0 +1,14 @@
+// Copyright 2020 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package connector
+
+import (
+	"testing"
+
+	gc "gopkg.in/check.v1"
+)
+
+func Test(t *testing.T) {
+	gc.TestingT(t)
+}

api/connector/simpleconnector.go

@@ -4,6 +4,7 @@
 package connector
 
 import (
+	"github.com/juju/errors"
 	"github.com/juju/names/v5"
 	"gopkg.in/macaroon.v2"
 
@@ -47,15 +48,28 @@ var _ Connector = (*SimpleConnector)(nil)
 // connect according to the specified options.  If some options are invalid an
 // error is returned.
 func NewSimple(opts SimpleConfig, dialOptions ...api.DialOption) (*SimpleConnector, error) {
+	if opts.Username == "" && opts.ClientID == "" {
+		return nil, errors.New("one of Username or ClientID must be set")
+	} else if opts.Username != "" && opts.ClientID != "" {
+		return nil, errors.New("only one of Username or ClientID should be set")
+	}
+
 	info := api.Info{
 		Addrs:    opts.ControllerAddresses,
 		CACert:   opts.CACert,
 		ModelTag: names.NewModelTag(opts.ModelUUID),
+	}
 
-		Tag:       names.NewUserTag(opts.Username),
-		Password:  opts.Password,
-		Macaroons: opts.Macaroons,
+	// When the client intends to login via client credentials (like a service
+	// account) they leave `opts.Username` empty and assign the client
+	// credentials to `opts.ClientID` and `opts.ClientSecret`. In such cases,
+	// we shouldn't assign `info.Tag` with a user tag.
+	if opts.Username != "" {
+		info.Tag = names.NewUserTag(opts.Username)
+		info.Password = opts.Password
+		info.Macaroons = opts.Macaroons
 	}
+
 	if err := info.Validate(); err != nil {
 		return nil, err
 	}

api/connector/simpleconnector_test.go

@@ -0,0 +1,102 @@
+// Copyright 2024 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package connector
+
+import (
+	"github.com/juju/names/v5"
+	gc "gopkg.in/check.v1"
+
+	"github.com/juju/juju/api"
+	"github.com/juju/juju/testing"
+)
+
+type simpleConnectorSuite struct {
+	testing.BaseSuite
+}
+
+var _ = gc.Suite(&simpleConnectorSuite{})
+
+func (s *simpleConnectorSuite) TestNewSimpleRespectsClientCredentials(c *gc.C) {
+	tests := []struct {
+		name                    string
+		opts                    SimpleConfig
+		expectedError           string
+		expectedAPIInfo         api.Info
+		expectedDefaultDialOpts func() api.DialOpts
+	}{
+		{
+			name: "with username/password",
+			opts: SimpleConfig{
+				ControllerAddresses: []string{"some.host:9999"},
+				ModelUUID:           "some-uuid",
+				Username:            "some-username",
+				Password:            "some-password",
+			},
+			expectedAPIInfo: api.Info{
+				Addrs:    []string{"some.host:9999"},
+				ModelTag: names.NewModelTag("some-uuid"),
+				Tag:      names.NewUserTag("some-username"),
+				Password: "some-password",
+			},
+		},
+		{
+			name: "with client credentials",
+			opts: SimpleConfig{
+				ControllerAddresses: []string{"some.host:9999"},
+				ModelUUID:           "some-uuid",
+				ClientID:            "some-client-id",
+				ClientSecret:        "some-client-secret",
+			},
+			expectedAPIInfo: api.Info{
+				Addrs:    []string{"some.host:9999"},
+				ModelTag: names.NewModelTag("some-uuid"),
+			},
+			expectedDefaultDialOpts: func() api.DialOpts {
+				expected := api.DefaultDialOpts()
+				expected.LoginProvider = api.NewClientCredentialsLoginProvider("some-client-id", "some-client-secret")
+				return expected
+			},
+		},
+		{
+			name: "with both username and client ID",
+			opts: SimpleConfig{
+				ControllerAddresses: []string{"some.host:9999"},
+				ModelUUID:           "some-uuid",
+				Username:            "some-username",
+				Password:            "some-password",
+				ClientID:            "some-client-id",
+				ClientSecret:        "some-client-secret",
+			},
+			expectedError: "only one of Username or ClientID should be set",
+		},
+		{
+			name: "with neither username nor client ID",
+			opts: SimpleConfig{
+				ControllerAddresses: []string{"some.host:9999"},
+				ModelUUID:           "some-uuid",
+			},
+			expectedError: "one of Username or ClientID must be set",
+		},
+	}
+
+	for _, test := range tests {
+		c.Logf("running test %s", test.name)
+
+		connector, err := NewSimple(test.opts)
+
+		if test.expectedError != "" {
+			c.Assert(err, gc.ErrorMatches, test.expectedError)
+			c.Assert(connector, gc.IsNil)
+		} else {
+			c.Assert(err, gc.IsNil)
+			c.Assert(connector.info, gc.DeepEquals, test.expectedAPIInfo)
+
+			expectedDefaultDialOpts := api.DefaultDialOpts()
+			if test.expectedDefaultDialOpts != nil {
+				expectedDefaultDialOpts = test.expectedDefaultDialOpts()
+			}
+			c.Assert(connector.defaultDialOpts, gc.DeepEquals, expectedDefaultDialOpts)
+		}
+	}
+}