ui: Topology intention saving improvements (#9513)

* ui: Keep track of existing intentions and use those to save changes Previously we risked overwriting existing data in an intention if we tried to save an intention without having loaded it first, for example Description and Metadata would have been overwritten. This change loads in all the intentions for an origin service so we can pick off the one we need to save and change to ensure that we don't overwrite any existing data.
2021-01-19 15:40:39 +00:00 · 2021-01-19 15:40:39 +00:00 · 3372e6d14c
parent 88ab4cd159
commit 3372e6d14c
10 changed files with 200 additions and 59 deletions
--- a/.changelog/9513.txt
+++ b/.changelog/9513.txt
@ -0,0 +1,3 @@
+```release-note:bug
+ui: Fixes an issue where intention description or metadata could be overwritten if saved from the topology view.
+```
--- a/ui/packages/consul-ui/app/components/topology-metrics/index.hbs
+++ b/ui/packages/consul-ui/app/components/topology-metrics/index.hbs
@ -3,7 +3,7 @@
 <div
  {{did-insert (action this.calculate)}}
  {{did-update (action this.calculate) @topology.Upstreams @topology.Downstreams}}
-  class="topology-container"
+  class="topology-container consul-topology-metrics"
 >
 {{#if (gt @topology.Downstreams.length 0)}}
  <div id="downstream-container">
--- a/ui/packages/consul-ui/app/components/topology-metrics/notifications/index.hbs
+++ b/ui/packages/consul-ui/app/components/topology-metrics/notifications/index.hbs
@ -0,0 +1,19 @@
+{{#if (eq @type 'create')}}
+  {{#if (eq @status 'success') }}
+    Your intention has been added.
+  {{else}}
+    There was an error adding your intention.
+  {{/if}}
+{{else if (eq @type 'update') }}
+  {{#if (eq @status 'success') }}
+    Your intention has been saved.
+  {{else}}
+    There was an error saving your intention.
+  {{/if}}
+{{/if}}
+{{#let @error.errors.firstObject as |error|}}
+  {{#if error.detail }}
+    <br />{{concat '(' (if error.status (concat error.status ': ')) error.detail ')'}}
+  {{/if}}
+{{/let}}
+
--- a/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs
+++ b/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs
@ -16,16 +16,25 @@
    </:header>
    <:body>
      <p>
+        {{#if @item.Intention.HasExact}}
+          Change the action of this intention to allow.
+        {{else}}
          Add an intention that allows these two services to connect.
+        {{/if}}
      </p>
    </:body>
    <:actions as |Actions|>
      <Actions.Action class="action">
        <button
          {{on "click" @oncreate}}
+          data-test-confirm
          type="button"
        >
+          {{#if @item.Intention.HasExact}}
+            Allow
+          {{else}}
            Create
+          {{/if}}
        </button>
      </Actions.Action>
      <Actions.Action>
@ -86,10 +95,11 @@
      )
      returns=(set this 'popoverController')
    }}
-    type="button"
    {{on 'click' (fn (optional this.popoverController.show))}}
+    type="button"
    style={{{concat 'top:' @position.y 'px;left:' @position.x 'px;'}}}
    aria-label={{if (eq @type 'deny') 'Add intention' 'View intention'}}
+    data-test-action
  >
  </button>
 </div>
--- a/ui/packages/consul-ui/app/routes/dc/services/show/topology.js
+++ b/ui/packages/consul-ui/app/routes/dc/services/show/topology.js
@ -1,26 +1,62 @@
 import Route from '@ember/routing/route';
 import { inject as service } from '@ember/service';
-import { get, action } from '@ember/object';
+import { set, get, action } from '@ember/object';

 export default class TopologyRoute extends Route {
  @service('ui-config') config;
  @service('data-source/service') data;
  @service('repository/intention') repo;
+  @service('feedback') feedback;

  @action
  async createIntention(source, destination) {
-    const model = this.repo.create({
+    // begin with a create action as it makes more sense if the we can't even
+    // get a list of intentions
+    let notification = this.feedback.notification('create');
+    try {
+      // intentions will be a proxy object
+      let intentions = await this.intentions;
+      let intention = intentions.find(item => {
+        return (
+          item.Datacenter === source.Datacenter &&
+          item.SourceName === source.Name &&
+          item.SourceNS === source.Namespace &&
+          item.DestinationName === destination.Name &&
+          item.DestinationNS === destination.Namespace
+        );
+      });
+      if (typeof intention === 'undefined') {
+        intention = this.repo.create({
          Datacenter: source.Datacenter,
          SourceName: source.Name,
          SourceNS: source.Namespace || 'default',
          DestinationName: destination.Name,
          DestinationNS: destination.Namespace || 'default',
-      Action: 'allow',
        });
-    await this.repo.persist(model);
+      } else {
+        // we found an intention in the find higher up, so we are updating
+        notification = this.feedback.notification('update');
+      }
+      set(intention, 'Action', 'allow');
+      await this.repo.persist(intention);
+      notification.success(intention);
+    } catch (e) {
+      notification.error(e);
+    }
    this.refresh();
  }

+  afterModel(model, transition) {
+    this.intentions = this.data.source(
+      uri => uri`/${model.nspace}/${model.dc.Name}/intentions/for-service/${model.slug}`
+    );
+  }
+
+  async deactivate(transition) {
+    const intentions = await this.intentions;
+    intentions.destroy();
+  }
+
  async model() {
    const parent = this.routeName
      .split('.')
--- a/ui/packages/consul-ui/app/services/feedback.js
+++ b/ui/packages/consul-ui/app/services/feedback.js
@ -13,27 +13,26 @@ const notificationDefaults = function() {
  };
 };
 export default class FeedbackService extends Service {
-  @service('flashMessages')
-  notify;
+  @service('flashMessages') notify;
+  @service('logger') logger;

-  @service('logger')
-  logger;
+  notification(action) {
+    return {
+      success: item => this.success(item, action),
+      error: e => this.error(e, action),
+    };
+  }

-  execute(handle, action, status = defaultStatus, controller) {
+  success(item, action, status = defaultStatus) {
    const getAction = callableType(action);
    const getStatus = callableType(status);
-    const notify = this.notify;
-    return (
-      handle()
-        //TODO: pass this through to getAction..
-        .then(item => {
    // returning exactly `false` for a feedback action means even though
    // its successful, please skip this notification and don't display it
    if (item !== false) {
-            notify.clearMessages();
+      this.notify.clearMessages();
      // TODO right now the majority of `item` is a Transition
      // but you can resolve an object
-            notify.add({
+      this.notify.add({
        ...notificationDefaults(),
        type: getStatus(TYPE_SUCCESS),
        // here..
@ -41,26 +40,37 @@ export default class FeedbackService extends Service {
        item: item,
      });
    }
-        })
-        .catch(e => {
-          notify.clearMessages();
+  }
+
+  error(e, action, status = defaultStatus) {
+    const getAction = callableType(action);
+    const getStatus = callableType(status);
+    this.notify.clearMessages();
    this.logger.execute(e);
    if (e.name === 'TransitionAborted') {
-            notify.add({
+      this.notify.add({
        ...notificationDefaults(),
        type: getStatus(TYPE_SUCCESS),
        // and here
        action: getAction(),
      });
    } else {
-            notify.add({
+      this.notify.add({
        ...notificationDefaults(),
        type: getStatus(TYPE_ERROR, e),
        action: getAction(),
        error: e,
      });
    }
-        })
-    );
+  }
+
+  async execute(handle, action, status) {
+    let result;
+    try {
+      result = await handle();
+      this.success(result, action, status);
+    } catch (e) {
+      this.error(e, action, status);
+    }
  }
 }
--- a/ui/packages/consul-ui/app/templates/dc/services/show.hbs
+++ b/ui/packages/consul-ui/app/templates/dc/services/show.hbs
@ -34,6 +34,13 @@

    <BlockSlot @name="loaded">
      <AppView>
+        <BlockSlot @name="notification" as |status type item error|>
+          <TopologyMetrics::Notifications
+            @type={{type}}
+            @status={{status}}
+            @error={{error}}
+          />
+        </BlockSlot>
        <BlockSlot @name="breadcrumbs">
          <ol>
              <li><a data-test-back href={{href-to 'dc.services'}}>All Services</a></li>
--- a/ui/packages/consul-ui/tests/acceptance/dc/services/show/topology.feature
+++ b/ui/packages/consul-ui/tests/acceptance/dc/services/show/topology.feature
@ -0,0 +1,43 @@
+@setupApplicationTest
+Feature: dc / services / show / topology: Intention Create
+  Background:
+    Given 1 datacenter model with the value "datacenter"
+    And 1 intention model from yaml
+    ---
+      SourceNS: default
+      SourceName: web
+      DestinationNS: default
+      DestinationName: db
+      ID: intention-id
+    ---
+    And 1 node model
+    And 1 service model from yaml
+    ---
+    - Service:
+        Name: web
+        Kind: ~
+    ---
+    And 1 topology model from yaml
+    ---
+      Downstreams: []
+      Upstreams:
+        - Name: db
+          Namespace: default
+          Datacenter: datacenter
+          Intention: {}
+    ---
+    When I visit the service page for yaml
+    ---
+      dc: datacenter
+      service: web
+    ---
+  Scenario: Allow a connection between service and upstream by saving an intention
+    When I click ".consul-topology-metrics [data-test-action]"
+    And I click ".consul-topology-metrics [data-test-confirm]"
+    And "[data-notification]" has the "success" class
+  Scenario: There was an error saving the intention
+    Given the url "/v1/connect/intentions/exact?source=default%2Fweb&destination=default%2Fdb&dc=datacenter" responds with a 500 status
+    When I click ".consul-topology-metrics [data-test-action]"
+    And I click ".consul-topology-metrics [data-test-confirm]"
+    And "[data-notification]" has the "error" class
+
--- a/ui/packages/consul-ui/tests/acceptance/steps/dc/services/show/topology-steps.js
+++ b/ui/packages/consul-ui/tests/acceptance/steps/dc/services/show/topology-steps.js
@ -0,0 +1,10 @@
+import steps from '../../../steps';
+
+// step definitions that are shared between features should be moved to the
+// tests/acceptance/steps/steps.js file
+
+export default function(assert) {
+  return steps(assert).then('I should find a file', function() {
+    assert.ok(true, this.step);
+  });
+}
--- a/ui/packages/consul-ui/tests/helpers/type-to-url.js
+++ b/ui/packages/consul-ui/tests/helpers/type-to-url.js
@ -38,6 +38,9 @@ export default function(type) {
    case 'nspace':
      requests = ['/v1/namespaces', '/v1/namespace/'];
      break;
+    case 'topology':
+      requests = ['/v1/internal/ui/service-topology'];
+      break;
  }
  // TODO: An instance of URL should come in here (instead of 2 args)
  return function(url, method) {