/juju

apiserver: organise facades #7606

Merged
merged 1 commit into from Jul 11, 2017

Conversation

Reviewers

@jameinel jameinel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@axw @jameinel @jujubot
@axw
Member

axw commented Jul 10, 2017

Description of change

Organise facades into subdirectories:

  • facades/agent
  • facades/client
  • facades/controller

There are a few facades that do not have
an obvious home:

  • storageprovisioner is run by the controllers
    (for model-level storage), and on machine
    agents (for machine-level storage).
  • imagemetadata is, unfortunately, serving two
    purposes: it serves the client, for listing,
    adding and removing image metadata; and it
    serves the image metadata worker. Because the
    image metadata client code is behind a feature
    flag, we could probably break API compatibility,
    and make it worker-only. I've left it alone for
    now, and put it in the facades/client package.
  • migrationtarget is for controllers to talk to
    each other, but it accepts only client
    authentication. Nevertheless, I've put it in the
    facades/controller package.

And then there were some bugs in facade auth:

  • logfwd was using AuthMachineAgent, and should
    have used AuthController. It has been updated
    to use the latter.
  • payloads had no auth at all; it has been updated
    to use AuthClient.
  • metricsmanager and undertaker: redundant calls
    to AuthMachineAgent, when AuthController would
    do. Simplified.
  • charmrevisionupdater: was allowing non-controller
    agents, now does not.
  • keymanager and highavailability: both were allowing
    controller unnecessarily; dropped.

QA steps

Run tests. Bootstrap.

Documentation changes

None.

Bug reference

None.

axw added a commit to axw/juju that referenced this pull request Jul 10, 2017

@axw
apiserver/...: fix several auth issues 
Backport of auth issues found/fixed in
juju#7606.

- logfwd was using AuthMachineAgent, and should
  have used AuthController. It has been updated
  to use the latter.
- payloads had no auth at all; it has been updated
  to use AuthClient.
- metricsmanager and undertaker: redundant calls
  to AuthMachineAgent, when AuthController would
  do. Simplified.
- charmrevisionupdater: was allowing non-controller
  agents, now does not.
- keymanager and highavailability: both were allowing
  controller unnecessarily; dropped.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
e3f21ac

axw added a commit to axw/juju that referenced this pull request Jul 10, 2017

@axw
apiserver/...: fix several auth issues 
Backport of auth issues found/fixed in
juju#7606.

- logfwd was using AuthMachineAgent, and should
  have used AuthController. It has been updated
  to use the latter.
- payloads had no auth at all; it has been updated
  to use AuthClient.
- metricsmanager and undertaker: redundant calls
  to AuthMachineAgent, when AuthController would
  do. Simplified.
- charmrevisionupdater: was allowing non-controller
  agents, now does not.
- keymanager and highavailability: both were allowing
  controller unnecessarily; dropped.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
aadf356

@axw axw referenced this pull request Jul 10, 2017

Merged

apiserver/...: fix several auth issues #7607

axw added a commit to axw/juju that referenced this pull request Jul 10, 2017

@axw
apiserver/...: fix several auth issues 
Backport of auth issues found/fixed in
juju#7606.

- logfwd was using AuthMachineAgent, and should
  have used AuthController. It has been updated
  to use the latter.
- payloads had no auth at all; it has been updated
  to use AuthClient.
- metricsmanager and undertaker: redundant calls
  to AuthMachineAgent, when AuthController would
  do. Simplified.
- charmrevisionupdater: was allowing non-controller
  agents, now does not.
- keymanager and highavailability: both were allowing
  controller unnecessarily; dropped.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
3962889

jujubot added a commit that referenced this pull request Jul 10, 2017

@jujubot
Merge pull request #7607 from axw/apiserver-facade-auth-fixes 
apiserver/...: fix several auth issues

## Description of change

Backport of auth issues found/fixed in
#7606.

- logfwd was using AuthMachineAgent, and should
  have used AuthController. It has been updated
  to use the latter.
- payloads had no auth at all; it has been updated
  to use AuthClient.
- metricsmanager and undertaker: redundant calls
  to AuthMachineAgent, when AuthController would
  do. Simplified.
- charmrevisionupdater: was allowing non-controller
  agents, now does not.
- keymanager and highavailability: both were allowing
  controller unnecessarily; dropped.

## QA steps

Run tests. Bootstrap.

## Documentation changes

None.

## Bug reference

None.
269266b
@jameinel
Owner

jameinel commented Jul 11, 2017

Offhand it feels like storageprovisioner can be made an 'agent' and we should fix 'imagemetadata' because it should not be serving 2 masters. If we need to pull some of the code into 'common', that would be ok, but we should have a separate facade for clients than for agents. Not that you have to do that for this patch, but its a clear mistake to combine them.
@axw
Member

axw commented Jul 11, 2017

we should fix 'imagemetadata' because it should not be serving 2 masters. If we need to pull some of the code into 'common', that would be ok, but we should have a separate facade for clients than for agents. Not that you have to do that for this patch, but its a clear mistake to combine them.

Agreed. I have filed a bug (https://bugs.launchpad.net/juju/+bug/1703582) and added a card.
@axw
apiserver: organise facades 
Organise facades into subdirectories:
 - facades/agent
 - facades/client
 - facades/controller

There are a few facades that do not have
an obvious home:

 - storageprovisioner is run by the controllers
   (for model-level storage), and on machine
   agents (for machine-level storage).
 - imagemetadata is, unfortunately, serving two
   purposes: it serves the client, for listing,
   adding and removing image metadata; and it
   serves the image metadata worker. Because the
   image metadata client code is behind a feature
   flag, we could probably break API compatibility,
   and make it worker-only. I've left it alone for
   now, and put it in the facades/client package.
 - migrationtarget is for controllers to talk to
   each other, but it accepts only client
   authentication. Nevertheless, I've put it in the
   facades/controller package.

And then there were some bugs in facade auth:

 - logfwd was using AuthMachineAgent, and should
   have used AuthController. It has been updated
   to use the latter.
 - payloads had no auth at all; it has been updated
   to use AuthClient.
 - metricsmanager and undertaker: redundant calls
   to AuthMachineAgent, when AuthController would
   do. Simplified.
 - charmrevisionupdater: was allowing non-controller
   agents, now does not.
 - keymanager and highavailability: both were allowing
   controller unnecessarily; dropped.
This commit was signed with a verified signature.
@axw axw Andrew Wilkins
GPG key ID: 55314F7628952B05 Learn about signing commits
6e125a7
@axw
Member

axw commented Jul 11, 2017

$$merge$$
@jujubot
Contributor

jujubot commented Jul 11, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 551f83c into juju:develop Jul 11, 2017
1 check failed

1 check failed

github-check-merge-juju Ran tests against PR. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment