Skip to content

Commit

Permalink
Merge pull request juju#17897 from barrettj12/rm-findtools
Browse files Browse the repository at this point in the history 
juju#17897

This PR adds a version 8 of the Client facade, which removes the `FindTools` method. This method has not been used by the client since 2.9. In 4.0, versions 6 and 7 of the Client facade will be removed, so only v8 will be supported.

A lot of unused code was removed, including unused interfaces (Pool) and fields on the client.

## 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
- [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~
- [x] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages

## QA steps

Client facade methods should still work as expected:
- `WatchAll`: used by `wait-for` and bundle deploys
- `StatusHistory`: used by `show-status-log`
- `FullStatus`: used by `status`

Bootstrap Juju and add a new model:
```
juju bootstrap aws aws
juju add-model m
```

Deploy a bundle (any bundle) and check the deployment works as expected:
```
juju deploy landscape-dense
```

Check that `status` works:
```
juju status
juju status --format json
```

Wait on one of the applications to check that `wait-for` works:
```
juju wait-for application haproxy
```

Test `show-status-log` on one of the units:
```
juju show-status-log haproxy/0
```

## Links

**Jira card:** JUJU-6496
  • Loading branch information
@jujubot
jujubot authored Aug 15, 2024
2 parents 55ca71e + d938fc0 commit 805db90
Show file tree
Hide file tree
Showing 11 changed files with 177 additions and 318 deletions.
2 changes: 1 addition & 1 deletion api/facadeversions.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ var facadeVersions = facades.FacadeVersions{
"CharmRevisionUpdater": {2},
"Charms": {5, 6, 7},
"Cleaner": {2},
"Client": {6, 7},
"Client": {6, 7, 8},
"Cloud": {7},
"Controller": {11, 12},
"CredentialManager": {1},
Expand Down
2 changes: 1 addition & 1 deletion api/state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ func (s *stateSuite) TestLoginMacaroonInvalidId(c *gc.C) {
}

func (s *stateSuite) TestBestFacadeVersion(c *gc.C) {
c.Check(s.APIState.BestFacadeVersion("Client"), gc.Equals, 7)
c.Check(s.APIState.BestFacadeVersion("Client"), gc.Equals, 8)
}

func (s *stateSuite) TestAPIHostPortsMovesConnectedValueFirst(c *gc.C) {
Expand Down
22 changes: 0 additions & 22 deletions apiserver/facades/client/client/backend.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,6 @@ type Model interface {
Status() (status.StatusInfo, error)
}

// Pool contains the StatePool functionality used in this package.
type Pool interface {
GetModel(string) (*state.Model, func(), error)
SystemState() (*state.State, error)
}

// Application represents a state.Application.
type Application interface {
StatusHistory(status.StatusHistoryFilter) ([]status.StatusInfo, error)
Expand Down Expand Up @@ -167,22 +161,6 @@ func (s *stateShim) AllApplicationOffers() ([]*crossmodel.ApplicationOffer, erro
return offers.AllApplicationOffers()
}

type poolShim struct {
pool *state.StatePool
}

func (p *poolShim) SystemState() (*state.State, error) {
return p.pool.SystemState()
}

func (p *poolShim) GetModel(uuid string) (*state.Model, func(), error) {
model, ph, err := p.pool.GetModel(uuid)
if err != nil {
return nil, nil, errors.Trace(err)
}
return model, func() { ph.Release() }, nil
}

func (s stateShim) ModelConfig() (*config.Config, error) {
model, err := s.State.Model()
if err != nil {
Expand Down
92 changes: 37 additions & 55 deletions apiserver/facades/client/client/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/juju/juju/core/permission"
"github.com/juju/juju/docker"
"github.com/juju/juju/docker/registry"
"github.com/juju/juju/environs/context"
envtools "github.com/juju/juju/environs/tools"
"github.com/juju/juju/feature"
"github.com/juju/juju/rpc/params"
Expand All @@ -35,45 +34,41 @@ import (

var logger = loggo.GetLogger("juju.apiserver.client")

type API struct {
// Client serves client-specific API methods.
type Client struct {
stateAccessor Backend
pool Pool
storageAccessor StorageInterface
auth facade.Authorizer
resources facade.Resources
presence facade.Presence

multiwatcherFactory multiwatcher.Factory

toolsFinder common.ToolsFinder
leadershipReader leadership.Reader
modelCache *cache.Model
leadershipReader leadership.Reader
modelCache *cache.Model
}

// TODO(wallyworld) - remove this method
// state returns a state.State instance for this API.
// Until all code is refactored to use interfaces, we
// need this helper to keep older code happy.
func (api *API) state() *state.State {
return api.stateAccessor.(*stateShim).State
func (c *Client) state() *state.State {
return c.stateAccessor.(*stateShim).State
}

// Client serves client-specific API methods.
type Client struct {
api *API
newEnviron common.NewEnvironFunc
check *common.BlockChecker
callContext context.ProviderCallContext
// ClientV7 serves the (v7) client-specific API methods.
type ClientV7 struct {
*Client
registryAPIFunc func(repoDetails docker.ImageRepoDetails) (registry.Registry, error)
toolsFinder common.ToolsFinder
}

// ClientV6 serves the (v6) client-specific API methods.
type ClientV6 struct {
*Client
*ClientV7
}

func (c *Client) checkCanRead() error {
err := c.api.auth.HasPermission(permission.SuperuserAccess, c.api.stateAccessor.ControllerTag())
err := c.auth.HasPermission(permission.SuperuserAccess, c.stateAccessor.ControllerTag())
if err != nil && !errors.Is(err, authentication.ErrorEntityMissingPermission) {
return errors.Trace(err)
}
Expand All @@ -82,11 +77,11 @@ func (c *Client) checkCanRead() error {
return nil
}

return c.api.auth.HasPermission(permission.ReadAccess, c.api.stateAccessor.ModelTag())
return c.auth.HasPermission(permission.ReadAccess, c.stateAccessor.ModelTag())
}

func (c *Client) checkCanWrite() error {
err := c.api.auth.HasPermission(permission.SuperuserAccess, c.api.stateAccessor.ControllerTag())
err := c.auth.HasPermission(permission.SuperuserAccess, c.stateAccessor.ControllerTag())
if err != nil && !errors.Is(err, authentication.ErrorEntityMissingPermission) {
return errors.Trace(err)
}
Expand All @@ -95,11 +90,11 @@ func (c *Client) checkCanWrite() error {
return nil
}

return c.api.auth.HasPermission(permission.WriteAccess, c.api.stateAccessor.ModelTag())
return c.auth.HasPermission(permission.WriteAccess, c.stateAccessor.ModelTag())
}

func (c *Client) checkIsAdmin() error {
err := c.api.auth.HasPermission(permission.SuperuserAccess, c.api.stateAccessor.ControllerTag())
err := c.auth.HasPermission(permission.SuperuserAccess, c.stateAccessor.ControllerTag())
if err != nil && !errors.Is(err, authentication.ErrorEntityMissingPermission) {
return errors.Trace(err)
}
Expand All @@ -108,13 +103,13 @@ func (c *Client) checkIsAdmin() error {
return nil
}

return c.api.auth.HasPermission(permission.AdminAccess, c.api.stateAccessor.ModelTag())
return c.auth.HasPermission(permission.AdminAccess, c.stateAccessor.ModelTag())
}

// NewFacade creates a Client facade to handle API requests.
// NewFacadeV7 creates a ClientV7 facade to handle API requests.
// Changes:
// - FindTools deals with CAAS models now;
func NewFacade(ctx facade.Context) (*Client, error) {
func NewFacadeV7(ctx facade.Context) (*ClientV7, error) {
st := ctx.State()
resources := ctx.Resources()
authorizer := ctx.Auth()
Expand All @@ -136,7 +131,6 @@ func NewFacade(ctx facade.Context) (*Client, error) {
}
urlGetter := common.NewToolsURLGetter(modelUUID, systemState)
toolsFinder := common.NewToolsFinder(configGetter, st, urlGetter, newEnviron)
blockChecker := common.NewBlockChecker(st)
leadershipReader, err := ctx.LeadershipReader(modelUUID)
if err != nil {
return nil, errors.Trace(err)
Expand All @@ -152,81 +146,69 @@ func NewFacade(ctx facade.Context) (*Client, error) {
return nil, errors.Trace(err)
}

return NewClient(
return NewClientV7(
&stateShim{st, model, nil},
&poolShim{ctx.StatePool()},
storageAccessor,
resources,
authorizer,
presence,
toolsFinder,
newEnviron,
blockChecker,
context.CallContext(st),
leadershipReader,
modelCache,
factory,
registry.New,
)
}

// NewClient creates a new instance of the Client Facade.
func NewClient(
// NewClientV7 creates a new instance of the ClientV7 Facade.
func NewClientV7(
backend Backend,
pool Pool,
storageAccessor StorageInterface,
resources facade.Resources,
authorizer facade.Authorizer,
presence facade.Presence,
toolsFinder common.ToolsFinder,
newEnviron common.NewEnvironFunc,
blockChecker *common.BlockChecker,
callCtx context.ProviderCallContext,
leadershipReader leadership.Reader,
modelCache *cache.Model,
factory multiwatcher.Factory,
registryAPIFunc func(docker.ImageRepoDetails) (registry.Registry, error),
) (*Client, error) {
) (*ClientV7, error) {
if !authorizer.AuthClient() {
return nil, apiservererrors.ErrPerm
}
client := &Client{
api: &API{

return &ClientV7{
Client: &Client{
stateAccessor: backend,
pool: pool,
storageAccessor: storageAccessor,
auth: authorizer,
resources: resources,
presence: presence,
toolsFinder: toolsFinder,
leadershipReader: leadershipReader,
modelCache: modelCache,
multiwatcherFactory: factory,
},
newEnviron: newEnviron,
check: blockChecker,
callContext: callCtx,
registryAPIFunc: registryAPIFunc,
}
return client, nil
toolsFinder: toolsFinder,
}, nil
}

// WatchAll initiates a watcher for entities in the connected model.
func (c *Client) WatchAll() (params.AllWatcherId, error) {
if err := c.checkCanRead(); err != nil {
return params.AllWatcherId{}, err
}
isAdmin, err := common.HasModelAdmin(c.api.auth, c.api.stateAccessor.ControllerTag(), names.NewModelTag(c.api.state().ModelUUID()))
isAdmin, err := common.HasModelAdmin(c.auth, c.stateAccessor.ControllerTag(), names.NewModelTag(c.state().ModelUUID()))
if err != nil {
return params.AllWatcherId{}, errors.Trace(err)
}
modelUUID := c.api.stateAccessor.ModelUUID()
w := c.api.multiwatcherFactory.WatchModel(modelUUID)
modelUUID := c.stateAccessor.ModelUUID()
w := c.multiwatcherFactory.WatchModel(modelUUID)
if !isAdmin {
w = &stripApplicationOffers{w}
}
return params.AllWatcherId{
AllWatcherId: c.api.resources.Register(w),
AllWatcherId: c.resources.Register(w),
}, nil
}

Expand Down Expand Up @@ -258,16 +240,16 @@ func (s *stripApplicationOffers) Next() ([]multiwatcher.Delta, error) {

// FindTools returns a List containing all tools matching the given parameters.
// TODO(juju 3.1) - remove, used by 2.9 client only
func (c *Client) FindTools(args params.FindToolsParams) (params.FindToolsResult, error) {
func (c *ClientV7) FindTools(args params.FindToolsParams) (params.FindToolsResult, error) {
if err := c.checkCanWrite(); err != nil {
return params.FindToolsResult{}, err
}
model, err := c.api.stateAccessor.Model()
model, err := c.stateAccessor.Model()
if err != nil {
return params.FindToolsResult{}, errors.Trace(err)
}

list, err := c.api.toolsFinder.FindAgents(common.FindAgentsParams{
list, err := c.toolsFinder.FindAgents(common.FindAgentsParams{
Number: args.Number,
MajorVersion: args.MajorVersion,
Arch: args.Arch,
Expand Down Expand Up @@ -302,9 +284,9 @@ func (c *Client) FindTools(args params.FindToolsParams) (params.FindToolsResult,
return c.toolVersionsForCAAS(args, streamsVersions, currentVersion)
}

func (c *Client) toolVersionsForCAAS(args params.FindToolsParams, streamsVersions set.Strings, current version.Number) (params.FindToolsResult, error) {
func (c *ClientV7) toolVersionsForCAAS(args params.FindToolsParams, streamsVersions set.Strings, current version.Number) (params.FindToolsResult, error) {
result := params.FindToolsResult{}
controllerCfg, err := c.api.stateAccessor.ControllerConfig()
controllerCfg, err := c.stateAccessor.ControllerConfig()
if err != nil {
return result, errors.Trace(err)
}
Expand Down
48 changes: 30 additions & 18 deletions apiserver/facades/client/client/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,6 @@ func (s *serverSuite) authClientForState(c *gc.C, st *state.State, auth facade.A
s.newEnviron = func() (environs.BootstrapEnviron, error) {
return environs.GetEnviron(stateenvirons.EnvironConfigGetter{Model: m}, environs.New)
}
client.SetNewEnviron(apiserverClient, func() (environs.BootstrapEnviron, error) {
return s.newEnviron()
})
return apiserverClient
}

Expand Down Expand Up @@ -463,11 +460,16 @@ func (s *findToolsSuite) TestFindToolsIAAS(c *gc.C) {
model.EXPECT().Type().Return(state.ModelTypeIAAS),
)

api, err := client.NewClient(
backend, nil,
nil, nil,
authorizer, nil, toolsFinder,
nil, nil, nil, nil, nil, nil,
api, err := client.NewClientV7(
backend,
nil,
nil,
authorizer,
nil,
toolsFinder,
nil,
nil,
nil,
func(docker.ImageRepoDetails) (registry.Registry, error) {
return registryProvider, nil
},
Expand Down Expand Up @@ -553,11 +555,16 @@ func (s *findToolsSuite) assertFindToolsCAASReleased(c *gc.C, wantArch, expectAr
registryProvider.EXPECT().Close().Return(nil),
)

api, err := client.NewClient(
backend, nil,
nil, nil,
authorizer, nil, toolsFinder,
nil, nil, nil, nil, nil, nil,
api, err := client.NewClientV7(
backend,
nil,
nil,
authorizer,
nil,
toolsFinder,
nil,
nil,
nil,
func(repo docker.ImageRepoDetails) (registry.Registry, error) {
c.Assert(repo, gc.DeepEquals, docker.ImageRepoDetails{
Repository: "test-account",
Expand Down Expand Up @@ -637,11 +644,16 @@ func (s *findToolsSuite) TestFindToolsCAASNonReleased(c *gc.C) {
registryProvider.EXPECT().Close().Return(nil),
)

api, err := client.NewClient(
backend, nil,
nil, nil,
authorizer, nil, toolsFinder,
nil, nil, nil, nil, nil, nil,
api, err := client.NewClientV7(
backend,
nil,
nil,
authorizer,
nil,
toolsFinder,
nil,
nil,
nil,
func(repo docker.ImageRepoDetails) (registry.Registry, error) {
c.Assert(repo, gc.DeepEquals, docker.ImageRepoDetails{
Repository: "test-account",
Expand Down
8 changes: 8 additions & 0 deletions apiserver/facades/client/client/doc.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Copyright 2024 Canonical Ltd.
// Licensed under the AGPLv3, see LICENCE file for details.

// Package client defines the Client facade, which is responsible for providing
// status API methods to the Juju client. Older versions of the client facade
// also provided the FindTools and WatchAll methods, but these are going away
// in Juju 4.
package client
Loading

0 comments on commit 805db90

Please sign in to comment.