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

kaxcode · 2021-10-05T14:13:14Z

✨ Description:

This PR updates the conditionals used to show the Default Allow notice, the Permissive Intentions notice, and all the collapsible notices.

Demo Link

📸 Screenshots:

Permissive Intention Notice

Default Allow Notice

⚡ Backend Changes:

No BE changes but we did verify with BE what the response should look like.

🤡 Updates to mock-api:

Removed DefaultAllow and WildcardIntention from root /v1/internal/ui/service-topology response.

🧪 Testing:

Removed the test that covered the Default Allow notice, because of limitations with mocking out API header response. Time-blocked the effort and deleted it in the end.

Updated the test that covered the Permissive Intentions notice.

📝 PR Tasks:

Backport
Changelog
Demo Link

kaxcode · 2021-10-05T16:14:16Z

ui/packages/consul-ui/app/components/collapsible-notices/index.js

+    const allNotices = [
+      ...document.querySelectorAll('div.collapsible-notices > div.notices > .notice'),
+    ];
+    this.collapsible = allNotices.length > 2;


These code changes are a more efficient solution to showing or not the collapsible button.

FYI in upcoming Topology changes we'll be adding more notices.

Curious as to why this could be more efficient? Mostly in modern frameworks the idea is to declaratively configure what you want the view to be based on the state and then the framework efficiently creates that view for you - using querySelector[All] is generally shied away from. I would say that generally if you find yourself using DOM methods like this it might be worth thinking of another way to do it (there's always an outlier but generally the reason for that outlier is clear if there is)

kaxcode · 2021-10-05T16:15:45Z

ui/packages/consul-ui/app/models/topology.js

@@ -33,14 +31,19 @@ export default class Topology extends Model {
    return undefinedDownstream;
  }

-  @computed('FilteredByACL', 'DefaultAllow', 'WildcardIntention', 'notDefinedIntention')


I removed this code because it was not efficient to show or not the collapsible banner. Soon it will be impossible to figure out all the "notice combos" that will exist.

With BE folks we verified that default allow and wildcard intentions can co-exist.

kaxcode · 2021-10-05T16:17:52Z

ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs

@@ -136,7 +98,7 @@ as |nspace dc items topology|}}
    {{#if config.data}}
      <TopologyMetrics
        @nspace={{nspace}}
-        @dc={{dc.Name}}
+        @dc={{dc}}


I had to pass the whole dc object since I need the dc.DefaultACLPolicy in topology-metrics/index.js

Cool yeah good move, thanks for the explanation.

kaxcode · 2021-10-05T16:18:32Z

ui/packages/consul-ui/mock-api/v1/internal/ui/service-topology/_

@@ -53,17 +53,11 @@ ${
    fake.seed(index);


-    // Randomly make permissive intentions
-    const defaultAllow = fake.random.boolean();


I thought about keeping this, but we verified with BE that default allow and wildcard intentions can co-exist.

johncowen · 2021-10-05T17:29:34Z

Super nice we can do this now with that new ACL header.

I have a few question based on your description before I give this a proper look:

⚡ Backend Changes:
No BE changes but we did verify with BE what the response should look like.

🤡 Updates to mock-api:
Removed DefaultAllow and WildcardIntention from root /v1/internal/ui/service-topology response.

If there are no backend changes, should we be updating the mocks? The mocks should be as close to the backend as possible. Is there a plan to make the same changes to the backend that are made here in the mocks?

Removed the test that covered the Default Allow notice, because of limitations with mocking out API header response. Time-blocked the effort and deleted it in the end.

Could you explain what you mean by the limitations here, off the top of my head I can't think of what that might be?

Lemme know and I can take a look over this

kaxcode · 2021-10-05T17:37:27Z

@johncowen

If there are no backend changes, should we be updating the mocks? The mocks should be as close to the backend as possible. Is there a plan to make the same changes to the backend that are made here in the mocks?

The mocks were wrong and did not match BE response. No changes are needed in the BE.

Could you explain what you mean by the limitations here, off the top of my head I can't think of what that might be?

I could not set X-Consul-Default-Acl-Policy = 'allow' as a header response from the v1/catalog/datacenters endpoint.

johncowen · 2021-10-06T10:21:01Z

The mocks were wrong and did not match BE response. No changes are needed in the BE.

Ah gotcha, thanks for the explanation!

I could not set X-Consul-Default-Acl-Policy = 'allow' as a header response from the v1/catalog/datacenters endpoint.

Ah, so not a limitation as such, more you're not sure how it's done? How much time did you time block this for? I can either walk you through it, show you where we do this already or pop something on the end of this myself depending on whether you can spend some more time on this or not. Lemme know how big your time block is and we can sort either way.

johncowen

I made a few inline comments also, but don't feel like you have to address them before we get a chance to talk about avoiding deleting the test here (re: mocking out the ACL header). I'll wait til you get back to me on the other (not-inline) comment I made and we can go from there.

edit: Is this the one we had a list of user stories to test for? The one we were going to make a spreadsheet for to make sure we covered them all?

johncowen · 2021-10-06T10:29:46Z

ui/packages/consul-ui/app/components/collapsible-notices/index.js

+    const allNotices = [
+      ...document.querySelectorAll('div.collapsible-notices > div.notices > .notice'),
+    ];
+    this.collapsible = allNotices.length > 2;


Curious as to why this could be more efficient? Mostly in modern frameworks the idea is to declaratively configure what you want the view to be based on the state and then the framework efficiently creates that view for you - using querySelector[All] is generally shied away from. I would say that generally if you find yourself using DOM methods like this it might be worth thinking of another way to do it (there's always an outlier but generally the reason for that outlier is clear if there is)

johncowen · 2021-10-06T10:31:41Z

ui/packages/consul-ui/app/templates/dc/services/show/topology.hbs

@@ -136,7 +98,7 @@ as |nspace dc items topology|}}
    {{#if config.data}}
      <TopologyMetrics
        @nspace={{nspace}}
-        @dc={{dc.Name}}
+        @dc={{dc}}


Cool yeah good move, thanks for the explanation.

johncowen · 2021-10-06T10:38:54Z

ui/packages/consul-ui/app/models/topology.js

+      this.Upstreams.filter(item => !item.Intention.HasExact && item.Intention.Allowed).length !==
+      0;
+
+    return downstreamWildcard || upstreamWildcard;


Just a note here, not sure if it matters or not. Did you know we have all this intention information available to us in the API call we make for the Services > Intentions tab (we also use the same call to do the intention updating on this Topology Tab). I would guess reusing that would save a bunch of backend work here, and if the user was using TProxy I don't think (not entirely sure) we'd have to make the extra topology endpoint API call at all. Anyway, no action to be made on FEs part here, but if you are talking to BE about this it might free them up a little.

kaxcode · 2021-10-06T13:47:43Z

Ah, so not a limitation as such, more you're not sure how it's done? How much time did you time block this for? I can either walk you through it, show you where we do this already or pop something on the end of this myself depending on whether you can spend some more time on this or not. Lemme know how big your time block is and we can sort either way.

@johncowen I time-blocked a whole afternoon for this one assertion. I mostly looked through the tests/steps/doubles files and the the tests/steps.js file. I did not see any examples of setting a API response header. Feel free to share any insight on how it could be done at a later time.

kaxcode · 2021-10-06T14:13:40Z

Curious as to why this could be more efficient? Mostly in modern frameworks the idea is to declaratively configure what you want the view to be based on the state and then the framework efficiently creates that view for you - using querySelector[All] is generally shied away from. I would say that generally if you find yourself using DOM methods like this it might be worth thinking of another way to do it (there's always an outlier but generally the reason for that outlier is clear if there is)

@johncowen I see what you mean. I've refactored the code I wrote in the CollapsibleNotices JS file to be in a helper. I made the changes in a separate commit.

kaxcode · 2021-10-06T15:38:44Z

Is this the one we had a list of user stories to test for? The one we were going to make a spreadsheet for to make sure we covered them all?

@johncowen sorry, I forgot to reply to this. Yes, we got on a call and discovered this was the actual issue. A spreadsheet has been made, but we still see no need for new notices atm.

johncowen · 2021-10-06T15:40:37Z

Feel free to share any insight on how it could be done at a later time.

You can use the env function in .config files also:

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

Line 5 in 2d05164

    
           latency: ${ location.search.index ? env('CONSUL_LATENCY', 60000) : env('CONSUL_LATENCY', 0) }

So instead of using randomize here use env

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

Line 7 in 2d05164

X-Consul-Default-Acl-Policy: ${fake.helpers.randomize(['allow', 'deny'])}

env here takes a default as a second argument so you could use:

X-Consul-Default-Acl-Policy: ${env('CONSUL_ACL_POLICY', fake.helpers.randomize(['allow', 'deny']))}

To make it still use a random value if the env var (the cookie/mock) isn't set. If you try this just in your browser first to make sure you can set it in your development environment, then you can add mocking it in a test environment.

During testing, you were right, you will need to add a new step like:

Given the default ACL policy is "$policy"

And then set your cookie in that step in order to mock this value out. There are a couple of ways to do that, one simpler that the other:

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

Lines 15 to 17 in 2d05164

    
           .given('a network latency of $number', function(number) { 
        
             set('CONSUL_LATENCY', number); 
        
           });

or something like this:

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

Line 11 in 2d05164

doc.cookie = `CONSUL_DATACENTER_LOCAL=${value}`;

Looking at the new stuff now 👍

kaxcode · 2021-10-06T16:50:54Z

@johncowen I've put the solution you suggested to the test in this branch ui/chore/default-allow-notices-test. I left a console.log in there so you can see the cookies make no impact on the mocked API headers response.

johncowen · 2021-10-06T17:06:22Z

Did you try both approaches I linked to? The first one works fine for me.

johncowen · 2021-10-07T11:20:04Z

Just a quick note: I'm gonna wait for that ^ (#11240) to merge here before re-reviewing. I think its all good tho and I think my remaining thoughts/comment will be solved by #11240. Good work! 👍

kaxcode · 2021-10-11T14:17:05Z

@johncowen I've rebased this PR. Is it ready to be merged?

johncowen

Not a blocking question, but whats the difference between a "wildcard intention" and a "permissive intention"?

kaxcode · 2021-10-12T13:26:50Z

Not a blocking question, but whats the difference between a "wildcard intention" and a "permissive intention"?

@johncowen they mean the same thing. The only difference is WildcardIntention is what we thought BE was going to return so we implemented it with that name. "Permissive Intentions" was used for the notice. I can make a separate PR to reconcile the names.

hc-github-team-consul-core · 2021-10-12T13:28:12Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/469698.

hc-github-team-consul-core · 2021-10-12T13:28:14Z

🍒❌ Cherry pick of commit daec73e onto release/1.10.x failed! Build Log

kaxcode added theme/ui Anything related to the UI backport/1.10 labels Oct 5, 2021

kaxcode requested a review from johncowen October 5, 2021 14:13

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

vercel bot temporarily deployed to Preview – consul-ui-staging October 5, 2021 14:14 Inactive

kaxcode added this to the 1.11.0 milestone Oct 5, 2021

kaxcode added the pr/do-not-merge PR cannot be merged in its current form. label Oct 5, 2021

kaxcode force-pushed the ui/bugfix/topology-notices branch from 16f9582 to 4042d33 Compare October 5, 2021 16:11

vercel bot temporarily deployed to Preview – consul-ui-staging October 5, 2021 16:11 Inactive

vercel bot temporarily deployed to Preview – consul October 5, 2021 16:11 Inactive

kaxcode commented Oct 5, 2021

View reviewed changes

kaxcode removed the pr/do-not-merge PR cannot be merged in its current form. label Oct 5, 2021

johncowen suggested changes Oct 6, 2021

View reviewed changes

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

vercel bot temporarily deployed to Preview – consul October 6, 2021 14:12 Inactive

kaxcode requested a review from johncowen October 6, 2021 14:14

kaxcode mentioned this pull request Oct 6, 2021

ui: Default allow notices test #11240

Merged

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

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

vercel bot temporarily deployed to Preview – consul October 11, 2021 14:02 Inactive

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

kaxcode and others added 7 commits October 11, 2021 10:03

Update topology mock data

1e89608

Fix topology notices and tests

e6393e9

Refactor conditions for collapsible banners

731f3f6

Add changelog

7d29ed4

Refactor collapsible notices with a helper

3b1c883

Update test name

037fe85

ui: Default allow notices test (#11240)

408d52c

kaxcode force-pushed the ui/bugfix/topology-notices branch from 17d81e0 to 408d52c Compare October 11, 2021 14:08

vercel bot temporarily deployed to Preview – consul October 11, 2021 14:08 Inactive

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

johncowen approved these changes Oct 12, 2021

View reviewed changes

kaxcode merged commit daec73e into main Oct 12, 2021

kaxcode deleted the ui/bugfix/topology-notices branch October 12, 2021 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kaxcode commented Oct 5, 2021 •

edited

Loading

kaxcode Oct 5, 2021 •

edited

Loading

johncowen Oct 6, 2021

kaxcode Oct 5, 2021 •

edited

Loading

kaxcode Oct 5, 2021

johncowen Oct 6, 2021

kaxcode Oct 5, 2021

johncowen commented Oct 5, 2021

kaxcode commented Oct 5, 2021 •

edited

Loading

johncowen commented Oct 6, 2021

johncowen left a comment •

edited

Loading

johncowen Oct 6, 2021

johncowen Oct 6, 2021

johncowen Oct 6, 2021

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

johncowen commented Oct 6, 2021

kaxcode commented Oct 6, 2021

johncowen commented Oct 6, 2021

johncowen commented Oct 7, 2021

kaxcode commented Oct 11, 2021

johncowen left a comment

kaxcode commented Oct 12, 2021

hc-github-team-consul-core commented Oct 12, 2021

hc-github-team-consul-core commented Oct 12, 2021

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

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

Conversation

kaxcode commented Oct 5, 2021 • edited Loading

✨ Description:

📸 Screenshots:

⚡ Backend Changes:

🤡 Updates to mock-api:

🧪 Testing:

📝 PR Tasks:

kaxcode Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

johncowen Oct 6, 2021

Choose a reason for hiding this comment

kaxcode Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

kaxcode Oct 5, 2021

Choose a reason for hiding this comment

johncowen Oct 6, 2021

Choose a reason for hiding this comment

kaxcode Oct 5, 2021

Choose a reason for hiding this comment

johncowen commented Oct 5, 2021

kaxcode commented Oct 5, 2021 • edited Loading

johncowen commented Oct 6, 2021

johncowen left a comment • edited Loading

Choose a reason for hiding this comment

johncowen Oct 6, 2021

Choose a reason for hiding this comment

johncowen Oct 6, 2021

Choose a reason for hiding this comment

johncowen Oct 6, 2021

Choose a reason for hiding this comment

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

kaxcode commented Oct 6, 2021

johncowen commented Oct 6, 2021

kaxcode commented Oct 6, 2021

johncowen commented Oct 6, 2021

johncowen commented Oct 7, 2021

kaxcode commented Oct 11, 2021

johncowen left a comment

Choose a reason for hiding this comment

kaxcode commented Oct 12, 2021

hc-github-team-consul-core commented Oct 12, 2021

hc-github-team-consul-core commented Oct 12, 2021

kaxcode commented Oct 5, 2021 •

edited

Loading

kaxcode Oct 5, 2021 •

edited

Loading

kaxcode Oct 5, 2021 •

edited

Loading

kaxcode commented Oct 5, 2021 •

edited

Loading

johncowen left a comment •

edited

Loading