From be694366a74a877aaf617a03ba96a797c8c1c4f3 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 0000000000..70bde0bff2 --- /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 9828e12fa8..ba4ee349cc 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 0000000000..7ef4000c5d --- /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 ad03bf9404..b8241d37a4 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 b278b4ff21..b91c2c31e5 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 1fe23e65d9..49e1511534 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 e04eb9bdf5..8e6e9c1a27 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 0000000000..29bec89f25 --- /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 0000000000..3231912b98 --- /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 68138bbf81..905480bee7 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) {