Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dev] Include exposed endpoints in diff bundle output #12025

Conversation

achilleasa
Copy link
Contributor

@achilleasa achilleasa commented Sep 18, 2020

Description of change

This PR ensures that the juju diff-bundle command reports exposed endpoint differences in its output.

The diff-bundle logic fetches an overview of the model from the controller via the FullStatus method of the Client facade. Its response payload (ApplicationStatus) has been augmented with an extra, optional field to report the set of expose settings (if any) for the application endpoints.

QA steps

$ juju bootstrap lxd test --no-gui

$ juju deploy apache2
$ juju expose apache2 --endpoints website --to-spaces alpha --to-cidrs 10.0.0.0/24
$ juju show-application apache2
apache2:
  charm: apache2
  series: bionic
  channel: stable
  principal: true
  exposed: true
  exposed-endpoints: <----- you should see this block as part of the output
    website:
      expose-to-spaces:
      - alpha
      expose-to-cidrs:
      - 10.0.0.0/24
  remote: false
  endpoint-bindings:
    "": alpha
    apache-website: alpha
    balancer: alpha
    local-monitors: alpha
    logging: alpha
    logs: alpha
    nrpe-external-master: alpha
    reverseproxy: alpha
    vhost-config: alpha
    website: alpha
    website-cache: alpha

# Check 1: 2.8-compatible bundle without expose flag
$ cat bundle1.yaml
applications:
  apache2:
    charm: cs:apache2-35
    series: bionic
    num_units: 1
    to:
    - "0"
machines:
  "0":
    series: bionic

$ juju diff-bundle bundle1.yaml
applications:
  apache2:
    expose:
      bundle: false
      model: true
    exposed_endpoints:
      website:
        bundle: null
        model:
          expose_to_spaces:
          - alpha
          expose_to_cidrs:
          - 10.0.0.0/24

# Check 2: 2.8-compatible bundle WITH expose flag (implicit expose-all-to-0.0.0.0/0)
$ cat bundle2.yaml
applications:
  apache2:
    charm: cs:apache2-35
    series: bionic
    expose: true
    num_units: 1
    to:
    - "0"
machines:
  "0":
    series: bionic

$ juju diff-bundle bundle2.yaml
applications:
  apache2:
    exposed_endpoints:
      "":
        bundle:
          expose_to_cidrs:
          - 0.0.0.0/0
          - ::/0
        model: null
      website:
        bundle: null
        model:
          expose_to_spaces:
          - alpha
          expose_to_cidrs:
          - 10.0.0.0/24

# Check 3: 2.9-compatible bundle with endpoint-specific expose settings
$ cat bundle3.yaml
applications:
  apache2:
    charm: cs:apache2-35
    series: bionic
    num_units: 1
    to:
    - "0"
machines:
  "0":
    series: bionic
--- # overlay
applications:
  apache2:
    exposed-endpoints:
      "":
        expose-to-cidrs:
        - 0.0.0.0/0
      website:
        expose-to-cidrs:
        - 192.168.0.0/24

$  juju diff-bundle bundle3.yaml
applications:
  apache2:
    exposed_endpoints:
      "":
        bundle:
          expose_to_cidrs:
          - 0.0.0.0/0
        model: null
      website:
        bundle:
          expose_to_cidrs:
          - 192.168.0.0/24
        model:
          expose_to_spaces:
          - alpha
          expose_to_cidrs:
          - 10.0.0.0/24

@@ -175,7 +175,8 @@ func AllFacades() *facade.Registry {
reg("Charms", 3, charms.NewFacadeV3)
reg("Cleaner", 2, cleaner.NewCleanerAPI)
reg("Client", 1, client.NewFacadeV1)
reg("Client", 2, client.NewFacade)
reg("Client", 2, client.NewFacadeV2)
reg("Client", 3, client.NewFacade)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern where the base is the latest version feels really odd!

@achilleasa achilleasa force-pushed the dev-include-exposed-endpoints-in-diff-bundle-output branch from a2e4227 to 459fe3b Compare September 22, 2020 18:42
@achilleasa
Copy link
Contributor Author

@SimonRichardson I have rebased this PR to include the IPV6 changes that landed on develop and updated the QA steps accordingly.

Copy link
Member

@SimonRichardson SimonRichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the rules around Client facades, the code looks fine though and Q&A went well, so until I get clarification about it, I'm just going to leave a comment for now.

This test was supposed to land together with commit d1b5f22 but was
unfortunately skipped.
The new version of bundlechanges includes exposed endpoints when diffing
bundles and models.
The new field is optional and not used by older controllers so we can
avoid bumping the Client facade version.
@achilleasa achilleasa force-pushed the dev-include-exposed-endpoints-in-diff-bundle-output branch from 459fe3b to 8e05264 Compare September 28, 2020 15:27
@achilleasa
Copy link
Contributor Author

The PR has been updated to revert the versioning changes and instead just add an extra field (with omitempty) to the existing payload.

@achilleasa
Copy link
Contributor Author

$$merge$$

@jujubot jujubot merged commit 4b58956 into juju:develop Sep 28, 2020
@achilleasa achilleasa deleted the dev-include-exposed-endpoints-in-diff-bundle-output branch September 28, 2020 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants