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) {