From cfbd1bb84e20c80ed5b3df079fb7f159e56ea39d Mon Sep 17 00:00:00 2001 From: John Cowen Date: Wed, 22 Sep 2021 10:21:20 +0100 Subject: [PATCH] ui: [BUGFIX] Re-enable namespace menus whilst editing intentions (#11095) This PR supersedes #10706 and fixes #10686 whilst making sure that saving intentions continues to work. The original fix in #10706 ignored the change action configured for the change event on the menus, meaning that the selected source/destination namespace could not be set by the user when editing/creating intentions. This, coupled with the fact that using the later intention exact endpoint for API requests endpoint means that you could not use wildcard namespaces for saving intentions. All in all this meant that intentions could no longer be saved using the UI (whilst using ENT) This PR reverts #10706 to fix the intention saving issue, and adds a fix for the original visual issue of nspaces doubling up in the menu once clicked. This meant repeating the existing functionality for nspaces aswell as services. It did seem strange to me that the original issue was only apparent for the nspace menus and not the service menus which should all function exactly the same way. There is potentially more to come here partly related to what the exact functionality should be, but I'm working with other folks to figure out what the best way forwards is longer term. In the meantime this brings us back to the original functionality with the visual issue fixed. Squashed commits: * Revert "ui: Fix dropdown option duplications (#10706)" This reverts commit eb5512fb74781ea49be743e2f0f16b3f1863ef61. * ui: Ensure additional nspaces are added to the unique list of nspaces * Add some acceptance tests --- .changelog/11095.txt | 3 ++ .../consul/intention/form/fieldsets/index.hbs | 4 +-- .../components/consul/intention/form/index.js | 31 ++++++++-------- .../acceptance/dc/intentions/create.feature | 29 ++++++++++----- .../dc/intentions/filtered-select.feature | 36 +++++++++++++++++++ .../consul-ui/tests/steps/assertions/dom.js | 8 ++--- 6 files changed, 83 insertions(+), 28 deletions(-) create mode 100644 .changelog/11095.txt diff --git a/.changelog/11095.txt b/.changelog/11095.txt new file mode 100644 index 0000000000..d2bb7f0bde --- /dev/null +++ b/.changelog/11095.txt @@ -0,0 +1,3 @@ +```release-note:bug +ui: **(Enterprise Only)** Fix saving intentions with namespaced source/destination +``` diff --git a/ui/packages/consul-ui/app/components/consul/intention/form/fieldsets/index.hbs b/ui/packages/consul-ui/app/components/consul/intention/form/fieldsets/index.hbs index d08a786c7d..e19be3d500 100644 --- a/ui/packages/consul-ui/app/components/consul/intention/form/fieldsets/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/intention/form/fieldsets/index.hbs @@ -39,7 +39,7 @@ @buildSuggestion={{action "createNewLabel" "Use a Consul Namespace called '{{term}}'"}} @showCreateWhen={{action "isUnique" nspaces}} @onCreate={{action onchange "SourceNS"}} - @onChange={{fn (mut this.SourceNS)}} as |nspace|> + @onChange={{action onchange "SourceNS"}} as |nspace|> {{#if (eq nspace.Name '*') }} * (All Namespaces) {{else}} @@ -88,7 +88,7 @@ @buildSuggestion={{action "createNewLabel" "Use a future Consul Namespace called '{{term}}'"}} @showCreateWhen={{action "isUnique" nspaces}} @onCreate={{action onchange "DestinationNS"}} - @onChange={{fn (mut this.DestinationNS)}} as |nspace|> + @onChange={{action onchange "DestinationNS"}} as |nspace|> {{#if (eq nspace.Name '*') }} * (All Namespaces) {{else}} diff --git a/ui/packages/consul-ui/app/components/consul/intention/form/index.js b/ui/packages/consul-ui/app/components/consul/intention/form/index.js index 4f7529009c..1f39e9dbc5 100644 --- a/ui/packages/consul-ui/app/components/consul/intention/form/index.js +++ b/ui/packages/consul-ui/app/components/consul/intention/form/index.js @@ -119,7 +119,7 @@ export default class ConsulIntentionForm extends Component { change(e, form, item) { const target = e.target; - let name, selected, match; + let name, selected; switch (target.name) { case 'SourceName': case 'DestinationName': @@ -140,21 +140,24 @@ export default class ConsulIntentionForm extends Component { // basically the difference between // `item.DestinationName` and just `DestinationName` // see if the name is already in the list - match = this.services.filterBy('Name', name); - if (match.length === 0) { - // if its not make a new 'fake' Service that doesn't exist yet - // and add it to the possible services to make an intention between - selected = { Name: name }; - switch (target.name) { - case 'SourceName': - case 'DestinationName': + + // if its not make a new 'fake' Service that doesn't exist yet + // and add it to the possible services to make an intention between + switch (target.name) { + case 'SourceName': + case 'DestinationName': + if (this.services.filterBy('Name', name).length === 0) { + selected = { Name: name }; this.services = [selected].concat(this.services.toArray()); - break; - case 'SourceNS': - case 'DestinationNS': + } + break; + case 'SourceNS': + case 'DestinationNS': + if (this.nspaces.filterBy('Name', name).length === 0) { + selected = { Name: name }; this.nspaces = [selected].concat(this.nspaces.toArray()); - break; - } + } + break; } this[target.name] = selected; break; diff --git a/ui/packages/consul-ui/tests/acceptance/dc/intentions/create.feature b/ui/packages/consul-ui/tests/acceptance/dc/intentions/create.feature index ded06d0f52..38627f187d 100644 --- a/ui/packages/consul-ui/tests/acceptance/dc/intentions/create.feature +++ b/ui/packages/consul-ui/tests/acceptance/dc/intentions/create.feature @@ -15,6 +15,10 @@ Feature: dc / intentions / create: Intention Create - Name: cache Kind: ~ --- + And 1 nspace model from yaml + --- + - Name: nspace-0 + --- When I visit the intention page for yaml --- dc: datacenter @@ -31,17 +35,26 @@ Feature: dc / intentions / create: Intention Create And I type "db" into ".ember-power-select-search-input" And I click ".ember-power-select-option:first-child" Then I see the text "db" in "[data-test-destination-element] .ember-power-select-selected-item" + # Set source nspace + And I click "[data-test-source-nspace] .ember-power-select-trigger" + And I click ".ember-power-select-option:nth-child(2)" + Then I see the text "nspace-0" in "[data-test-source-nspace] .ember-power-select-selected-item" + # Set destination nspace + And I click "[data-test-destination-nspace] .ember-power-select-trigger" + And I click ".ember-power-select-option:nth-child(2)" + Then I see the text "nspace-0" in "[data-test-destination-nspace] .ember-power-select-selected-item" # Specifically set deny And I click ".value-deny" And I submit - # TODO: When namespace is empty we expect * - # Then a PUT request was made to "/v1/connect/intentions/exact?source=@namespace%2Fweb&destination=@namespace%2Fdb&dc=datacenter" from yaml - # --- - # body: - # SourceName: web - # DestinationName: db - # Action: deny - # --- + Then a PUT request was made to "/v1/connect/intentions/exact?source=nspace-0%2Fweb&destination=nspace-0%2Fdb&dc=datacenter" from yaml + --- + body: + SourceName: web + DestinationName: db + SourceNS: nspace-0 + DestinationNS: nspace-0 + Action: deny + --- Then the url should be /datacenter/intentions And the title should be "Intentions - Consul" And "[data-notification]" has the "notification-update" class diff --git a/ui/packages/consul-ui/tests/acceptance/dc/intentions/filtered-select.feature b/ui/packages/consul-ui/tests/acceptance/dc/intentions/filtered-select.feature index 478b504781..316c92b891 100644 --- a/ui/packages/consul-ui/tests/acceptance/dc/intentions/filtered-select.feature +++ b/ui/packages/consul-ui/tests/acceptance/dc/intentions/filtered-select.feature @@ -25,6 +25,42 @@ Feature: dc / intentions / filtered-select: Intention Service Select Dropdowns Then I see the text "* (All Services)" in ".ember-power-select-option:nth-last-child(3)" Then I see the text "service-0" in ".ember-power-select-option:nth-last-child(2)" Then I see the text "service-1" in ".ember-power-select-option:last-child" + Where: + --------------- + | Name | + | source | + | destination | + --------------- + @onlyNamespaceable + Scenario: Opening and closing the nspace [Name] dropdown doesn't double up items + Given 1 datacenter model with the value "datacenter" + And 2 nspace models from yaml + --- + - Name: nspace-0 + - Name: nspace-1 + --- + When I visit the intention page for yaml + --- + dc: datacenter + --- + Then the url should be /datacenter/intentions/create + Given a network latency of 60000 + And I click "[data-test-[Name]-nspace] .ember-power-select-trigger" + Then I see the text "* (All Namespaces)" in ".ember-power-select-option:nth-last-child(3)" + Then I see the text "nspace-0" in ".ember-power-select-option:nth-last-child(2)" + Then I see the text "nspace-1" in ".ember-power-select-option:last-child" + And I click ".ember-power-select-option:last-child" + And I click "[data-test-[Name]-nspace] .ember-power-select-trigger" + And I click ".ember-power-select-option:last-child" + And I click "[data-test-[Name]-nspace] .ember-power-select-trigger" + And I click ".ember-power-select-option:last-child" + And I click "[data-test-[Name]-nspace] .ember-power-select-trigger" + Then I don't see the text "nspace-1" in ".ember-power-select-option:nth-last-child(6)" + Then I don't see the text "nspace-1" in ".ember-power-select-option:nth-last-child(5)" + Then I don't see the text "nspace-1" in ".ember-power-select-option:nth-last-child(4)" + Then I see the text "* (All Namespaces)" in ".ember-power-select-option:nth-last-child(3)" + Then I see the text "nspace-0" in ".ember-power-select-option:nth-last-child(2)" + Then I see the text "nspace-1" in ".ember-power-select-option:last-child" Where: --------------- | Name | diff --git a/ui/packages/consul-ui/tests/steps/assertions/dom.js b/ui/packages/consul-ui/tests/steps/assertions/dom.js index 617df0fb6b..425add01dd 100644 --- a/ui/packages/consul-ui/tests/steps/assertions/dom.js +++ b/ui/packages/consul-ui/tests/steps/assertions/dom.js @@ -14,11 +14,11 @@ export default function(scenario, assert, pauseUntil, find, currentURL, clipboar return retry(); }, `Expected to see "${text}" in "${selector}"`); }) - .then(['I see the text "$text" in "$selector"'], function(text, selector) { - const textContent = find(selector).textContent; - assert.ok( + .then([`I${dont} see the text "$text" in "$selector"`], function(negative, text, selector) { + const textContent = (find(selector) || { textContent: '' }).textContent; + assert[negative ? 'notOk' : 'ok']( textContent.indexOf(text) !== -1, - `Expected to see "${text}" in "${selector}", was "${textContent}"` + `Expected${negative ? ' not' : ''} to see "${text}" in "${selector}", was "${textContent}"` ); }) .then(['I copied "$text"'], function(text) {