From 3372e6d14c26ccf0546244469aabdff64b27a5d4 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Tue, 19 Jan 2021 15:40:39 +0000 Subject: [PATCH] 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. --- .changelog/9513.txt | 3 + .../app/components/topology-metrics/index.hbs | 2 +- .../topology-metrics/notifications/index.hbs | 19 ++++ .../topology-metrics/popover/index.hbs | 16 ++- .../app/routes/dc/services/show/topology.js | 56 ++++++++-- .../consul-ui/app/services/feedback.js | 100 ++++++++++-------- .../app/templates/dc/services/show.hbs | 7 ++ .../dc/services/show/topology.feature | 43 ++++++++ .../steps/dc/services/show/topology-steps.js | 10 ++ .../consul-ui/tests/helpers/type-to-url.js | 3 + 10 files changed, 200 insertions(+), 59 deletions(-) create mode 100644 .changelog/9513.txt create mode 100644 ui/packages/consul-ui/app/components/topology-metrics/notifications/index.hbs create mode 100644 ui/packages/consul-ui/tests/acceptance/dc/services/show/topology.feature create mode 100644 ui/packages/consul-ui/tests/acceptance/steps/dc/services/show/topology-steps.js diff --git a/.changelog/9513.txt b/.changelog/9513.txt new file mode 100644 index 000000000..70bde0bff --- /dev/null +++ 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. +``` diff --git a/ui/packages/consul-ui/app/components/topology-metrics/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/index.hbs index 9828e12fa..ba4ee349c 100644 --- 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 @@
{{#if (gt @topology.Downstreams.length 0)}}
diff --git a/ui/packages/consul-ui/app/components/topology-metrics/notifications/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/notifications/index.hbs new file mode 100644 index 000000000..7ef4000c5 --- /dev/null +++ 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 }} +
{{concat '(' (if error.status (concat error.status ': ')) error.detail ')'}} + {{/if}} +{{/let}} + diff --git a/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs index ad03bf940..b8241d37a 100644 --- 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 @@ <:body>

- Add an intention that allows these two services to connect. + {{#if @item.Intention.HasExact}} + Change the action of this intention to allow. + {{else}} + Add an intention that allows these two services to connect. + {{/if}}

<:actions as |Actions|> @@ -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 >
diff --git a/ui/packages/consul-ui/app/routes/dc/services/show/topology.js b/ui/packages/consul-ui/app/routes/dc/services/show/topology.js index b278b4ff2..b91c2c31e 100644 --- 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({ - Datacenter: source.Datacenter, - SourceName: source.Name, - SourceNS: source.Namespace || 'default', - DestinationName: destination.Name, - DestinationNS: destination.Namespace || 'default', - Action: 'allow', - }); - await this.repo.persist(model); + // 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', + }); + } 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('.') diff --git a/ui/packages/consul-ui/app/services/feedback.js b/ui/packages/consul-ui/app/services/feedback.js index 1fe23e65d..49e151153 100644 --- a/ui/packages/consul-ui/app/services/feedback.js +++ b/ui/packages/consul-ui/app/services/feedback.js @@ -13,54 +13,64 @@ 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(); - // TODO right now the majority of `item` is a Transition - // but you can resolve an object - notify.add({ - ...notificationDefaults(), - type: getStatus(TYPE_SUCCESS), - // here.. - action: getAction(), - item: item, - }); - } - }) - .catch(e => { - notify.clearMessages(); - this.logger.execute(e); - if (e.name === 'TransitionAborted') { - notify.add({ - ...notificationDefaults(), - type: getStatus(TYPE_SUCCESS), - // and here - action: getAction(), - }); - } else { - notify.add({ - ...notificationDefaults(), - type: getStatus(TYPE_ERROR, e), - action: getAction(), - error: e, - }); - } - }) - ); + // returning exactly `false` for a feedback action means even though + // its successful, please skip this notification and don't display it + if (item !== false) { + this.notify.clearMessages(); + // TODO right now the majority of `item` is a Transition + // but you can resolve an object + this.notify.add({ + ...notificationDefaults(), + type: getStatus(TYPE_SUCCESS), + // here.. + action: getAction(), + item: item, + }); + } + } + + error(e, action, status = defaultStatus) { + const getAction = callableType(action); + const getStatus = callableType(status); + this.notify.clearMessages(); + this.logger.execute(e); + if (e.name === 'TransitionAborted') { + this.notify.add({ + ...notificationDefaults(), + type: getStatus(TYPE_SUCCESS), + // and here + action: getAction(), + }); + } else { + 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); + } } } diff --git a/ui/packages/consul-ui/app/templates/dc/services/show.hbs b/ui/packages/consul-ui/app/templates/dc/services/show.hbs index e04eb9bdf..8e6e9c1a2 100644 --- 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 @@ + + +
  1. All Services
    2. diff --git a/ui/packages/consul-ui/tests/acceptance/dc/services/show/topology.feature b/ui/packages/consul-ui/tests/acceptance/dc/services/show/topology.feature new file mode 100644 index 000000000..29bec89f2 --- /dev/null +++ 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 + diff --git 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 new file mode 100644 index 000000000..3231912b9 --- /dev/null +++ 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); + }); +} diff --git a/ui/packages/consul-ui/tests/helpers/type-to-url.js b/ui/packages/consul-ui/tests/helpers/type-to-url.js index 68138bbf8..905480bee 100644 --- 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) {