ui: Ensure service instance data does not get re-written on blocking refresh #11903

johncowen · 2021-12-22T14:31:46Z

This PR is quite in-depth and there are a couple of things to understand here.

Routlet/Outlets/Routes - There is an RFC detailing this, ~~I'll see if I can find the title here so folks can take a look if needed~~ CSL-164: Sharing data across Routes and Components in the Consul UI. (Also CSL-092 A componentized approach to data, whilst not entirely necessary may also help here)
(Previous to this PR) Our approach to creating special munged together ProxyServiceInstances, which where a combination of the response from 2 endpoints - How we did this was the root of the issue here.

I've added quite a few inline comments and the link to the issue explains what the user facing problem/consequence was. But let me know if I can add any more color/help here. Also commit by commit review might help folks here.

This one is targeting our next 1.11.x point release, and needs review/approval/merge for then.

johncowen

Notes:

johncowen · 2021-12-22T14:38:31Z

ui/packages/consul-ui/app/components/outlet/index.js

-        this.route._model = this.args.model;
+        if(typeof this.route !== 'undefined') {
+          this.route._model = value;
+        }


value is the same as this.args.model here

johncowen · 2021-12-22T14:39:12Z

ui/packages/consul-ui/app/components/route/index.hbs

@@ -1,7 +1,7 @@
 {{did-insert this.connect}}
 {{will-destroy this.disconnect}}
 {{yield (hash
-  model=(or this.model this._model)
+  model=this.model


The or stuff was moved into the JS (you can see it in the next file down) and it now ever so slightly different.

johncowen · 2021-12-22T14:41:31Z

ui/packages/consul-ui/app/components/route/index.js

-      if(Object.keys(model).length === 0) {
-        return null;
-      }
-      return model;


All of this split/join fiddling has now been replaced with outletFor(routeName), and when you have access to the outlet, then you can also grab the name directly from the outlet and then use the routlet service to get the outlet's model.

johncowen · 2021-12-22T14:52:40Z

ui/packages/consul-ui/app/filter/predicates/service-instance.js

@@ -5,7 +5,7 @@ export default {
    passing: (item, value) => item.Status === value,
    warning: (item, value) => item.Status === value,
    critical: (item, value) => item.Status === value,
-    empty: (item, value) => item.MeshChecks.length === 0,
+    empty: (item, value) => item.ServiceChecks.length === 0,


Whilst this ideally should be using MeshChecks (i.e. check both the service instances health checks and the proxy that is in front of the service instances healthchecks). I noticed that nothing else here uses MeshChecks. Even more importantly the visible healthchecks for the service instances list also do not take into account the healthchecks for the proxy (the MeshChecks) and they should.

All in all, this should all be revisited, but not in this PR (and theres an issue that touches on this #11875).

I made this small change here as I wanted to be rid of MeshChecks in this PR. It may or may not come back when I tackle the more in depth issue here in a separate PR)

Just a bit of extra "thinking out loud" incase, when I was looking at this entire "What do these filter actually mean?" I think this needs some discussion amongst the UI team:

In this listing view we have Service Checks and Node Checks. Do we actually want to be using the NodeChecks in the filter? In other places in the UI we've rejigged things so that NodeChecks aren't as important when viewing Services/ServiceInstances.

We should make the ServiceInstance listing actually show the health of the Service plus Proxy (MeshChecks), before using Service plus Proxy (MeshChecks) to filter the list.

Hopefully the screengrab below helps with this explanation:

johncowen · 2021-12-22T14:53:30Z

ui/packages/consul-ui/app/helpers/merge-checks.js

+    checks,
+    exposed
+  );
+});


Allow the already existing mergeChecks to be used in a template.

johncowen · 2021-12-22T15:13:28Z

ui/packages/consul-ui/app/templates/dc/services/instance.hbs

@@ -129,6 +135,8 @@ as |item|}}
            </h1>
            <Consul::ExternalSource @item={{item}} @withInfo={{true}} />
            <Consul::Kind @item={{item}} @withInfo={{true}} />
+            {{! TODO: Looks like we can get this straight from item.Proxy.Mode }}
+            {{! the less we need `proxy` the better }}
            {{#if (eq proxy.ServiceProxy.Mode 'transparent')}}
              <Consul::TransparentProxy />
            {{/if}}


I noticed here that we also have access to Mode on the item (the service instance) as well as the proxy. The less we use proxy the better. Actually, glad I self reviewed this I need to look at this a little more 🤔 brb

After I looked more at this I realized that I needed to also swap our proxy for meta here and in a few other places. I've added another 4 commits on the end here to cover this (from Make TProxy... cc698d3 to Document CONSUL_... 38ba3c0)

johncowen · 2021-12-22T15:13:40Z

ui/packages/consul-ui/app/templates/dc/services/instance.hbs

+                                          (hash label="Tags & Meta" href=(href-to "dc.services.instance.metadata") selected=(is-href "dc.services.instance.metadata"))
+                  )
+              }}
+            />


Just formatting

johncowen · 2021-12-22T15:14:55Z

ui/packages/consul-ui/app/templates/dc/services/instance/healthchecks.hbs

@@ -27,7 +27,7 @@ as |route|>
      )
    )

-    route.model.item.MeshChecks
+    (merge-checks (array route.model.item.Checks route.model.proxy.Checks) route.model.proxy.ServiceProxy.Expose.Checks)


So where we merge all the checks (those from the ServiceInstance and the Proxy) together before iterating through them. This uses the exact same code as previously, only we do it in the template instead of in the model which then avoids all manner of problems.

johncowen · 2021-12-22T15:17:36Z

ui/packages/consul-ui/mock-api/v1/health/service/backend-sidecar-proxy

+      }
+    ]
+  }
+]


These are all real responses from a working Consul to give us a more realistic UI view, it also made it easier to reproduce and manually debug/test what was happening. We generally just use faker.js to give us test data, but we've done this sort of 'non-random fixtures' for the discovery-chain work to.

The consequences of this are that if you view the ServiceInstance page specifically for a service called backend only (http://localhost:4200/ui/dc1/services/backend/instances) - then you will get this exact data, not the randomized stuff.

johncowen · 2021-12-22T15:20:12Z

ui/packages/consul-ui/tests/integration/services/routlet-test.js

+  actual = routlet.outletFor('application');
+  expected = undefined;
+  assert.equal(actual, expected);
+});


Started out with some basic tests for outletFor, this is likely to be added to moving forwards, and potentially the interface may change a little (I saw that instead of passing (name, route/outlet) we could just pass (route/outlet) and get the name straight off the route/outlet name property. I'd rather do that in a refactor PR than a bugfix one though - so that may happen at some point in the future. For now this is just a bit of reassurance testing so I know it does what I need correctly.

…` etc

johncowen · 2021-12-22T16:24:48Z

ui/packages/consul-ui/app/templates/dc/services/instance/addresses.hbs

      <TabularCollection
        data-test-addresses
        class="consul-tagged-addresses"
-        @items={{entries item.Service.TaggedAddresses}} as |taggedAddress index|
+        @items={{items}} as |taggedAddress index|


I needed to check this file to see if we needed to move to use meta here, so I fixed up some formatting while I was here

natmegs

Pair reviewed

hc-github-team-consul-core · 2022-01-07T19:17:11Z

🍒 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/543358.

hc-github-team-consul-core · 2022-01-07T19:17:14Z

🍒✅ Cherry pick of commit 6c240fb onto release/1.11.x succeeded!

…refresh (#11903) * Add some less fake API data * Rename the models class so as to not be confused with JS Proxies * Rearrange routlets slightly and add some initial outletFor tests * Move away from a MeshChecks computed property and just use a helper * Just use ServiceChecks for healthiness filtering for the moment * Make TProxy cookie configurable * Amend exposed paths and upstreams so they know about meta AND proxy * Slight bit of TaggedAddresses refactor while I was checking for `meta` etc * Document CONSUL_TPROXY_ENABLE

John Cowen added 5 commits December 22, 2021 14:18

Add some less fake API data

eb4961f

Rename the models class so as to not be confused with JS Proxies

a89a0f3

Rearrange routlets slightly and add some initial outletFor tests

cecd51b

Move away from a MeshChecks computed property and just use a helper

78e10ce

Just use ServiceChecks for healthiness filtering for the moment

7737301

johncowen added type/bug Feature does not function as expected theme/ui Anything related to the UI backport/1.11 labels Dec 22, 2021

johncowen mentioned this pull request Dec 22, 2021

Sidecar proxy checks not showing up in UI until after a refresh #11873

Closed

johncowen commented Dec 22, 2021

View reviewed changes

johncowen marked this pull request as ready for review December 22, 2021 15:25

johncowen requested review from natmegs, amyrlam, a user and jgwhite December 22, 2021 15:25

John Cowen added 4 commits December 22, 2021 16:17

Make TProxy cookie configurable

cc698d3

Amend exposed paths and upstreams so they know about meta AND proxy

c9973b0

Slight bit of TaggedAddresses refactor while I was checking for `meta…

2efd8a9

…` etc

Document CONSUL_TPROXY_ENABLE

38ba3c0

vercel bot temporarily deployed to Preview – consul December 22, 2021 16:20 Inactive

vercel bot deployed to Preview – consul-ui-staging December 22, 2021 16:20 View deployment

johncowen commented Dec 22, 2021

View reviewed changes

natmegs approved these changes Jan 7, 2022

View reviewed changes

Changelog

ce8ef88

vercel bot temporarily deployed to Preview – consul January 7, 2022 19:07 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging January 7, 2022 19:07 Inactive

johncowen merged commit 6c240fb into main Jan 7, 2022

johncowen deleted the ui/bugfix/proxy-healthchecks branch January 7, 2022 19:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Ensure service instance data does not get re-written on blocking refresh #11903

ui: Ensure service instance data does not get re-written on blocking refresh #11903

johncowen commented Dec 22, 2021 •

edited

Loading

johncowen left a comment

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

johncowen Dec 22, 2021

natmegs left a comment

hc-github-team-consul-core commented Jan 7, 2022

hc-github-team-consul-core commented Jan 7, 2022

ui: Ensure service instance data does not get re-written on blocking refresh #11903

ui: Ensure service instance data does not get re-written on blocking refresh #11903

Conversation

johncowen commented Dec 22, 2021 • edited Loading

johncowen left a 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

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

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

natmegs left a comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Jan 7, 2022

hc-github-team-consul-core commented Jan 7, 2022

johncowen commented Dec 22, 2021 •

edited

Loading