ui: Default allow notices test #11240

kaxcode · 2021-10-06T21:13:24Z

Small PR to have a test for Default Allow notices.

kaxcode · 2021-10-06T21:14:33Z

@johncowen I made this into a separate PR because the convo was getting too long for PR #11216

kaxcode · 2021-10-06T21:15:10Z

ui/packages/consul-ui/mock-api/v1/catalog/.config

@@ -4,4 +4,4 @@
    "*":
      headers:
        response:
-          X-Consul-Default-Acl-Policy: ${fake.helpers.randomize(['allow', 'deny'])}
+          x-consul-default-acl-policy: ${env('CONSUL_ACL_POLICY', fake.helpers.randomize(['allow', 'deny']))}


I think the serializer is lowercasing this or something. In the end it's what made dev and the tests worked.

The mocks should mock the real backend exactly (as much as we possibly can). So this should be reverted, and then we should look into why the tests fail (if thats what happens).

Off the top of my head I know that within the code we lowercase all header keynames to cope with the behaviour of different browsers (apparently some browsers/XHRs impl. lowercase the headers, although a lot of time has passed since this was done so maybe thats no longer the case 🤷 )

If something here is breaking when its not lowercased, then I would assume it would also break when using a real Consul backend.

This change does not break the UI when it's running against Consul binary. As I stated in #11216 I've spent more than enough time getting this test to work. I will not be reverting this change or doing any further research on why the header needs to be lowercased.

If you don't want this mock-api change to go through, I'll close the PR and this test can be added at a later time. Please let me know. @johncowen

kaxcode · 2021-10-06T21:15:53Z

ui/packages/consul-ui/tests/acceptance/dc/services/show/topology/tproxy.feature

+          Intention:
+            Allowed: false
+    ---
+    And the default ACL policy is "allow"


The set solution for changing cookies works but the action has to be done before visiting page.

Yup, thats expected you need to set up your tests case before testing.

Arrange. Act. Assert.

This whole thing (setting up the default ACL policy) was about Arranging and so needs to happen before Acting (visiting the page) and Asserting.

Related: The catalog/datacenters HTTP API call where we grab the header is pretty much the first HTTP call the app makes, so you absolutely need to Arrange the mock before you visit anything.

@johncowen there's no need for you to explain how to set up tests. One, I didn't ask you. Two, I clearly see the mistake I made. I'm the one who pointed out what I did wrong. Please don't assume I don't know things just because I made a mistake.

kaxcode · 2021-10-06T21:16:45Z

ui/packages/consul-ui/tests/steps/doubles/model.js

@@ -38,6 +38,9 @@ export default function(scenario, create, set, win = window, doc = document) {
    .given(['ACLs are disabled'], function() {
      doc.cookie = `CONSUL_ACLS_ENABLE=0`;
    })
+    .given(['the default ACL policy is "$policy"'], function(policy) {
+      set('CONSUL_ACL_POLICY', policy);


As mentioned, this solution works but it has to be in a specific place. Glad you tried it out because I got to take another look.

Awesome, glad it works. See my comment above re: Arrange. Act. Assert.

johncowen

I think this is all super good. The only thing we definitely need to get to the bottom of is why the header in the mocks here needs to be lowercased. We need to make sure things work when its normal header cased (whatever that one is called snake/camel/capital/sentence/kebab/blah - basically not lower).

johncowen · 2021-10-07T11:07:02Z

ui/packages/consul-ui/mock-api/v1/catalog/.config

@@ -4,4 +4,4 @@
    "*":
      headers:
        response:
-          X-Consul-Default-Acl-Policy: ${fake.helpers.randomize(['allow', 'deny'])}
+          x-consul-default-acl-policy: ${env('CONSUL_ACL_POLICY', fake.helpers.randomize(['allow', 'deny']))}


The mocks should mock the real backend exactly (as much as we possibly can). So this should be reverted, and then we should look into why the tests fail (if thats what happens).

Off the top of my head I know that within the code we lowercase all header keynames to cope with the behaviour of different browsers (apparently some browsers/XHRs impl. lowercase the headers, although a lot of time has passed since this was done so maybe thats no longer the case 🤷 )

If something here is breaking when its not lowercased, then I would assume it would also break when using a real Consul backend.

johncowen · 2021-10-07T11:10:35Z

ui/packages/consul-ui/tests/acceptance/dc/services/show/topology/tproxy.feature

+          Intention:
+            Allowed: false
+    ---
+    And the default ACL policy is "allow"


Yup, thats expected you need to set up your tests case before testing.

Arrange. Act. Assert.

This whole thing (setting up the default ACL policy) was about Arranging and so needs to happen before Acting (visiting the page) and Asserting.

Related: The catalog/datacenters HTTP API call where we grab the header is pretty much the first HTTP call the app makes, so you absolutely need to Arrange the mock before you visit anything.

johncowen · 2021-10-07T11:11:01Z

ui/packages/consul-ui/tests/acceptance/dc/services/show/topology/tproxy.feature

+      service: web
+    ---
+    Then the url should be /datacenter/services/web/topology
+    And I see the tabs.topologyTab.defaultAllowNotice object
  Scenario: WildcardIntetions and FilteredByACLs are set to true


Suggested change

Scenario: WildcardIntetions and FilteredByACLs are set to true

Scenario: WildcardIntentions and FilteredByACLs are set to true

Actually, no biggie this, but this is no longer the scenario right? These things were never set to true in Consul itself, only in our mocks? So might want to change this sentence. As I said no biggie, its almost just a comment more than anything, but its sometimes useful for folks understanding what we are testing here.

I'll make this change in #11216

johncowen · 2021-10-07T11:14:05Z

ui/packages/consul-ui/tests/steps/doubles/model.js

@@ -38,6 +38,9 @@ export default function(scenario, create, set, win = window, doc = document) {
    .given(['ACLs are disabled'], function() {
      doc.cookie = `CONSUL_ACLS_ENABLE=0`;
    })
+    .given(['the default ACL policy is "$policy"'], function(policy) {
+      set('CONSUL_ACL_POLICY', policy);


Awesome, glad it works. See my comment above re: Arrange. Act. Assert.

kaxcode · 2021-10-07T14:16:35Z

@johncowen At this time, I'm not making any further changes or research to make this test pass. If you don't feel it can be merged because of the mock-data then I will close it.

Please let me know. I would like to unblock #11216.

…#11216) * ui: Default allow notices test (#11240)

kaxcode added theme/ui Anything related to the UI pr/no-changelog PR does not need a corresponding .changelog entry labels Oct 6, 2021

kaxcode requested a review from johncowen October 6, 2021 21:13

kaxcode commented Oct 6, 2021

View reviewed changes

johncowen suggested changes Oct 7, 2021

View reviewed changes

johncowen mentioned this pull request Oct 7, 2021

ui: Topology - Fix up Default Allow and Permissive Intentions notices #11216

Merged

3 tasks

kaxcode added 4 commits October 7, 2021 10:22

WIP

e23fb77

Fixes

3da3d05

WIP

2004392

Fixes

3cb57a7

kaxcode force-pushed the ui/chore/default-allow-notices-test branch from b5a1097 to 3cb57a7 Compare October 7, 2021 14:28

vercel bot temporarily deployed to Preview – consul October 7, 2021 14:29 Inactive

vercel bot deployed to Preview – consul-ui-staging October 7, 2021 14:29 View deployment

kaxcode requested a review from johncowen October 7, 2021 14:29

johncowen approved these changes Oct 11, 2021

View reviewed changes

kaxcode merged this pull request into ui/bugfix/topology-notices Oct 11, 2021

kaxcode deleted the ui/chore/default-allow-notices-test branch October 11, 2021 14:02

kaxcode added a commit that referenced this pull request Oct 11, 2021

ui: Default allow notices test (#11240)

408d52c

kaxcode added a commit that referenced this pull request Oct 12, 2021

ui: Topology - Fix up Default Allow and Permissive Intentions notices (…

daec73e

…#11216) * ui: Default allow notices test (#11240)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Default allow notices test #11240

ui: Default allow notices test #11240

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

kaxcode Oct 6, 2021

johncowen Oct 7, 2021

kaxcode Oct 7, 2021

kaxcode Oct 6, 2021 •

edited

Loading

johncowen Oct 7, 2021 •

edited

Loading

kaxcode Oct 7, 2021

kaxcode Oct 6, 2021

johncowen Oct 7, 2021

johncowen left a comment

johncowen Oct 7, 2021

johncowen Oct 7, 2021 •

edited

Loading

johncowen Oct 7, 2021

johncowen Oct 7, 2021

kaxcode Oct 7, 2021

johncowen Oct 7, 2021

kaxcode commented Oct 7, 2021

	Scenario: WildcardIntetions and FilteredByACLs are set to true
	Scenario: WildcardIntentions and FilteredByACLs are set to true

ui: Default allow notices test #11240

ui: Default allow notices test #11240

Conversation

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxcode Oct 6, 2021 • edited Loading

Choose a reason for hiding this comment

johncowen Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johncowen Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaxcode commented Oct 7, 2021

kaxcode Oct 6, 2021 •

edited

Loading

johncowen Oct 7, 2021 •

edited

Loading

johncowen Oct 7, 2021 •

edited

Loading