ui: Add Consul API Gateway as an external source #11371

kaxcode · 2021-10-20T20:50:18Z

✨ Description:

This PR adds support of Consul API Gateway as an external source.

Support appears in the following areas:

Service List row items
Top of Service show page
Service Topology tab - metrics and Configure metrics dashboard messaging hidden
Service Instances tab - list row items
Service Intentions tab - actions button hidden
Top of Service Instance show page

📸 Demo:

⚡ Backend Changes:

BE Support in an internal repo. The BE support has been merged.

🤡 Updates to mock-api:

Updated every endpoint that had "external-source": "${fake.helpers.randomize(['kubernetes'])}".

🧪 Testing:

Added an acceptance test for hiding the metrics for a Consul API Gateway.

📝 PR Tasks:

Changelog

kaxcode · 2021-10-20T21:17:27Z

ui/packages/consul-ui/app/components/consul/intention/list/table/index.hbs

@@ -65,7 +65,7 @@ as |item index|>
        {{/if}}
        </td>
    </BlockSlot>
-{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}}
+{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}}


Temporarily hiding the actions button until we have docs and learn guides.

I don't think (at least for the moment) that intentions will ever have this meta value so you can probably remove this now.

Support for intentions will be merged today/tomorrow.

Oh wow, TIL, so now I'm trying to understand why we are hiding the actions dropdown? Should I still be allowed to click on the row (I can currently). What about the actions at the bottom of the form once I click into it? Should I be able to delete and save from there if it's a consul-api-gateway intention?

Should we be showing a badge/pill in the row to show that the intention has been added/is managed via consul-api-gateway? At the moment I just don't see an actions button but with no clue as to why I don't.

The why is mentioned in my initial comment. Please read and remember this is temporary.

There are no create/delete actions. Please refer to the mock-ups.

I'll ask the designer about the badge/pill.

Cool gotcha

Regarding the badge/pill. I spoke with the designer and they would like to leave that for a future release.

Ok so just so I follow here. For this release it sounds like we are not doing anything to do with intentions coming from consul-api-gateway:

We aren't adding a badge/pill to an intention if it has api-gateway metadata?

We temporarily hide the actions menu only here in this PR if it has api-gateway metadata, but this temporary measure will be undone once we have docs and learn guides?

Thats ^ what I understand right now, if this isn't correct or changes could you let me know.

kaxcode · 2021-10-20T21:18:22Z

ui/packages/consul-ui/app/components/popover-menu/layout.scss

@@ -4,9 +4,6 @@
 %popover-menu-trigger {
  display: block;
 }
-%popover-menu-panel {


I tested this in a couple areas to see if it broken anything. I couldn't find any areas. I think width:100% in a parent div is holding things together.

Seems legit

Before:

After:

This change makes all our menus dynamically sized to the containing text, so it essentially shrinks the widths of them all, and I don't think that's what is desired (I could be wrong?). The hardcoded width here will have been taken from the original pixel value from the original designs.

If we take the time to un-hardcode this, this should be a min width I think (we also do a weird fit-content thing somewhere that was for something similar to this, not sure this is the right tool here but thought I'd mention incase). Again, separate PR would be better, but I'm easy. (edit: but if we want to do it here, then this will need changing)

(also e2e testing/screen-component-grabbing 💯 )

I've added back min-width.

kaxcode · 2021-10-20T21:19:12Z

ui/packages/consul-ui/app/components/popover-select/index.scss

  @extend %with-logo-consul-color-icon, %as-pseudo;
 }
 %popover-select .nomad button::before {
  @extend %with-logo-nomad-color-icon, %as-pseudo;
 }
+%popover-select .vault button::before {


I noticed Vault was missing here. I was going to make it into a separate PR, but then I realized I would need to wait for it to be merged and rebase this PR.

kaxcode · 2021-10-20T21:19:50Z

ui/packages/consul-ui/app/components/topology-metrics/index.hbs

@@ -46,6 +46,7 @@
    <div class="metrics-header">
      {{@service.Service.Service}}
    </div>
+  {{#if (not-eq @service.Service.Meta.external-source 'consul-api-gateway')}}


Hiding metrics and the Configure metrics dashboard messaging

Would it be possible to change this to use ember-can so can("view dashboard for service" item=service) or similar? ty!

No, this conditional works. Using ember-can for this specific case will open this PR to other changes that will be out of scope.

Realised we might need to hide the Configure dashboard messaging in the other place that we show this button.

Q: Do you know what Kinds could have the metadata consul-api-gateway?

kaxcode · 2021-10-20T21:20:23Z

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

@@ -97,7 +97,7 @@ as |item|}}
            <h1>
              <route.Title @title={{item.Service.ID}} />
            </h1>
-            <Consul::ExternalSource @item={{item}} />
+            <Consul::ExternalSource @item={{item}} @withInfo={{true}} />


Copied a similar implementation as Kind.

evrowe · 2021-10-20T21:22:39Z

ui/packages/consul-ui/app/components/consul/intention/list/table/index.hbs

@@ -65,7 +65,7 @@ as |item index|>
        {{/if}}
        </td>
    </BlockSlot>
-{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}}
+{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}}


Is our preference to always keep this kind of logic in the template or is there a point at which we prefer to move that logic into a computed property or method on the component?

(This is readable enough for now but it feels like it might be pushing the boundaries of how much logic to put into the template, if that's a thing we limit)

Temporarily hiding the actions button until we have docs and learn guides.

@kaxcode In that case, would the and logic introduced here be removed later once the docs and learn guides are available?

Yes, this is temporary. If needed this could be refactored.

evrowe · 2021-10-20T21:23:34Z

ui/packages/consul-ui/translations/components/consul/en-us.yaml

+external-source:
+  consul-api-gateway: Consul API Gateway
+  vault: Vault
+  kubernetes: Kubernetes
+  terraform: Terraform
+  nomad: Nomad
+  consul: Consul
+  aws: AWS


oh, I actually just deleted it because I remember I could use common.brand 😂

evrowe · 2021-10-20T21:26:41Z

ui/packages/consul-ui/app/components/consul/external-source/index.hbs

+      {{else}}
+        Registered via {{t (concat "components.consul.external-source." externalSource)}}
+      {{/if}}


Glad to see the previous logic for AWS resolved by using localization, feels like a good solution that should scale as the list grows

evrowe · 2021-10-20T21:28:50Z

ui/packages/consul-ui/app/components/consul/external-source/index.hbs

+        <dl class="tooltip-panel">
+          <dt>
+            <span
+              data-test-external-source={{externalSource}}
+              class="consul-external-source {{externalSource}}"
+              ...attributes
+            >
+              Registered via {{t (concat "components.consul.external-source." externalSource)}}
+            </span>
+          </dt>
+          <dd>
+            <MenuPanel @position="left" @menu={{false}}>
+              <BlockSlot @name="header">
+                API Gateways manage north-south traffic from external services to services in the Datacenter. For more information, read our documentation.
+              </BlockSlot>
+              <BlockSlot @name="menu">
+                <li role="separator">
+                  About {{t (concat "components.consul.external-source." externalSource)}}
+                </li>
+                <li role="none" class="learn-link">
+                  <a tabindex="-1" role="menuitem" href={{concat (env 'CONSUL_DOCS_LEARN_URL')}} rel="noopener noreferrer" target="_blank">
+                    Learn guides
+                  </a>
+                </li>
+              </BlockSlot>
+            </MenuPanel>
+          </dd>
+        </dl>


Not necessary right now but if this is a pattern we see a lot of I wonder if we could make a cleanup task for the future to"componentize" this behavior (and possibly have that yield some sub-components) to help cut down on the need for manual markup?

This implementation is also done in Consul::Kind. Making a change to how we use MenuPanel could scope creep a bit. Definitely, something to change in a separate PR.

johncowen

Nice work! I left some inline feedback here, not taken it for a spin yet but let me know what you think about the withInfo thing in the meantime.

johncowen · 2021-10-22T16:15:13Z

ui/packages/consul-ui/app/components/consul/intention/list/table/index.hbs

@@ -65,7 +65,7 @@ as |item index|>
        {{/if}}
        </td>
    </BlockSlot>
-{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}}
+{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}}


I don't think (at least for the moment) that intentions will ever have this meta value so you can probably remove this now.

johncowen · 2021-10-22T16:16:07Z

ui/packages/consul-ui/mock-api/v1/connect/_

@@ -96,7 +96,7 @@ ${fake.helpers.randomize([
            "Precedence": ${i + 1},
 ${ fake.random.number({min: 1, max: 10}) > 2 ? `
            "Meta": {
-              "external-source": "${fake.helpers.randomize(['kubernetes'])}"
+              "external-source": "${fake.helpers.randomize(['kubernetes', 'consul-api-gateway'])}"


As above I don't think API Gateway will add this meta, below in the other intention endpoints the same comment applies.

Support for intentions will be merged today/tomorrow.

Oh wow, TIL

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

johncowen · 2021-10-22T16:17:34Z

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

+      dc: datacenter
+      service: web
+    ---
+    And I don't see the "[data-test-sparkline]" element


I like it, test all reads nicely!

johncowen · 2021-11-01T12:01:56Z

ui/packages/consul-ui/app/components/consul/external-source/index.hbs

+        Registered via {{t (concat "common.brand." externalSource)}}
+      {{/if}}
+    </span>
+  {{/if}}


I see what you were thinking here copying the the @withInfo attribute from Consul::Kind. I think that now we have this kind of thing in multiple places it might be worthwhile changing things slightly with the multiplicity in mind.

One thing that I'm not 100% sure of here is having the decision on whether to show the info in two places. One place is here in the component (does external-source === 'consul-api-gateway'?), and the other place is where ever you use the component with the @withInfo argument (which needs to be true). Both need to be true to show the info and the info is very specific to external-source being consul-api-gateway. I think I follow why you need 2 'knobs' for this, but it would be good to chat through which way to do it. An additional side-note here is that if we are going to use i18n here we should use it 100% for every piece of copy, not just the odd word.

Considering all of the above, I think it would be good to think about this a little more and possibly do it differently using named slots (possibly similar thinking to evan's comment). So I'd suggest splitting this bit off into a separate PR so we can get the initial functionality merged in separately and discuss this withInfo stuff in a bit more detail. Lemme know what you think.

So I'd suggest splitting this bit off into a separate PR so we can get the initial functionality merged in separately and discuss this withInfo stuff in a bit more detail. Lemme know what you think.

Would it be alright to merge things more or less as-is and make a point to take a pass at withInfo/named slots as a separate issue, knowing that we'd need to come back and update this later? The upshot is that allows the implementation discussion around how to standardize and/or make it easier to use consistent patterns to not be under time pressure, and I'd prefer not to see this blocked on that discussion unless the conversation around what to change and implementation of said changes can be accomplished within a day or two.

I would guess that it could be accomplished within a day or two, but happy to merge as is if need be. It also fits in with your comment here on componentizing this a little more. My main worry would be for it to get left and then spread with the next copypaste and then it suddenly becomes a pattern by accident.

We could put this in a separate PR, merge this one pretty much immediately (yay!) and then work on this almost separate feature over the next day or so, I can't see it missing a release for spending a tiny bit more time on it. I think when I originally wrote this up in as a task we split it potentially into two tasks anyway.

I'm easy either way.

I think maybe at the very least, if we are going to use i18n we shouldn't just i18n separate words and hardcode the rest.

Oh actually, just noticed, we this extra functionality is pretty much just to provide links to docs for more info, and those links don't exist yet, so we're gonna have to come back and amend this anyway. Feels like we could just do that in one hit rather than risk this going out in a release with empty links?

Anyway, as I said I'm easy either way but obvs would prefer to split this off if it was me.

I think that if we as a group agree to circle back and address the standardization/componentizing issue as a fast follow, that's a strong enough commitment to make me comfortable with merging this as-is.

My main worry would be for it to get left and then spread with the next copypaste and then it suddenly becomes a pattern by accident.

I am happy to prioritize addressing this specific issue so that it doesn't fall into being a pattern. And even if it did, we can change it/reverse it.

I can't see it missing a release for spending a tiny bit more time on it

I'm not concerned about it missing the release; I'd like us to get into the practice of reducing the turnaround time on reviews/approvals/merges for PRs. I see this particular issue as one that isn't crucial to fix for this PR but that is important that we address separately and quickly, and we can add changes to other parts of the code that could benefit from an update as part of that work as well.

Feels like we could just do that in one hit rather than risk this going out in a release with empty links?

Is that something we've had an issue with in the past? As long as this isn't being backported to anything earlier than 1.11 I don't expect that would be a problem as docs would be required for the release to go out

We could put this in a separate PR, merge this one pretty much immediately (yay!) and then work on this almost separate feature over the next day or so

Cool yeah I think we want the same outcome here, just different ways of going about it 😆

Do we want to address the i18n stuff here or later? Again, I'm easy either way.

Is that something we've had an issue with in the past?

Kind of yeah, just links not being ready and we either leave them or put placeholders in, and then they get left as they are forgotten about, and we end up with links going to 404s or the wrong page (🤔 another thing proper e2e testing could help with)

All in all, nothing here that is a specific blocker for merging as is, as long as we are coming back to anything that isn't addressed here.

I think this looks good to merge. We have some follow-up items we've identified that can be addressed separately once this lands and that we can get done in time for the release.

Do we want to address the i18n stuff here or later?

I agree that if we are going to be using i18n/localization in the app that we need to be using it more thoroughly, however I'd like there to be a more formal discussion and decision process (as part of general team working agreements) around this before we start requiring for every part of a PR. Let's leave as-is for now but it's on my list of things for us to discuss as a group in the near future.

Sounds good

johncowen

Cool, I think this is good to go. I left a couple more comments/questions, but the only one that I think might be actionable in this PR is the one re: the other Configure dashboard button. Not sure if you want to do that here or in another PR, but I'll leave an approval here anyway so its up to you.

johncowen · 2021-11-09T12:20:13Z

ui/packages/consul-ui/app/components/consul/intention/list/table/index.hbs

@@ -65,7 +65,7 @@ as |item index|>
        {{/if}}
        </td>
    </BlockSlot>
-{{#if (or (can "write intention" item=item) (can "view CRD intention" item=item))}}
+{{#if (and (or (can "write intention" item=item) (can "view CRD intention" item=item)) (not-eq item.Meta.external-source 'consul-api-gateway'))}}


Ok so just so I follow here. For this release it sounds like we are not doing anything to do with intentions coming from consul-api-gateway:

We aren't adding a badge/pill to an intention if it has api-gateway metadata?

We temporarily hide the actions menu only here in this PR if it has api-gateway metadata, but this temporary measure will be undone once we have docs and learn guides?

Thats ^ what I understand right now, if this isn't correct or changes could you let me know.

johncowen · 2021-11-09T13:07:19Z

ui/packages/consul-ui/app/components/topology-metrics/index.hbs

@@ -46,6 +46,7 @@
    <div class="metrics-header">
      {{@service.Service.Service}}
    </div>
+  {{#if (not-eq @service.Service.Meta.external-source 'consul-api-gateway')}}


Realised we might need to hide the Configure dashboard messaging in the other place that we show this button.

Q: Do you know what Kinds could have the metadata consul-api-gateway?

hc-github-team-consul-core · 2021-11-10T21:55:16Z

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

kaxcode added the archived/webui This was used for v1/ui label Oct 20, 2021

kaxcode added this to the 1.11.0 milestone Oct 20, 2021

kaxcode requested a review from johncowen October 20, 2021 20:50

github-actions bot added the theme/ui Anything related to the UI label Oct 20, 2021

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

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

kaxcode commented Oct 20, 2021

View reviewed changes

evrowe reviewed Oct 20, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – consul October 20, 2021 21:24 Inactive

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

kaxcode force-pushed the ui/feature/add-consul-api-gateway-as-external-source branch from 8383bdc to 6f4c398 Compare October 20, 2021 21:25

vercel bot temporarily deployed to Preview – consul October 20, 2021 21:25 Inactive

evrowe reviewed Oct 20, 2021

View reviewed changes

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

evrowe reviewed Oct 20, 2021

View reviewed changes

johncowen reviewed Oct 22, 2021

View reviewed changes

johncowen removed the archived/webui This was used for v1/ui label Oct 22, 2021

johncowen reviewed Nov 1, 2021

View reviewed changes

vercel bot temporarily deployed to Preview – consul November 2, 2021 14:53 Inactive

vercel bot deployed to Preview – consul-ui-staging November 2, 2021 14:53 View deployment

kaxcode requested a review from johncowen November 2, 2021 14:54

kaxcode added 4 commits November 3, 2021 15:56

Add consul-api-gateway to mock-data

c51a553

Hides the actions button from Intentions with consul-api-gateway

ce0bf7a

Refactors ExternalSource to support consul-api-gateway with Info

47a07e8

Hide metrics graph and stats for Consul API Gateway main service

3c8680e

kaxcode added 5 commits November 3, 2021 15:56

Adds tests for hiding metrics

8a2247f

Adds Consul API Gateway to Source dropdown

7290c01

Add changelog

088e89b

Remove duplicated translations

e7eb38a

Fix

9beacaa

kaxcode force-pushed the ui/feature/add-consul-api-gateway-as-external-source branch from ede3c5c to 9beacaa Compare November 3, 2021 20:10

vercel bot deployed to Preview – consul-ui-staging November 3, 2021 20:10 View deployment

vercel bot temporarily deployed to Preview – consul November 3, 2021 20:10 Inactive

johncowen approved these changes Nov 9, 2021

View reviewed changes

kaxcode merged commit 37de276 into main Nov 10, 2021

kaxcode deleted the ui/feature/add-consul-api-gateway-as-external-source branch November 10, 2021 21:54

ui: Add Consul API Gateway as an external source #11371

ui: Add Consul API Gateway as an external source #11371

Conversation

kaxcode commented Oct 20, 2021 • edited Loading

✨ Description:

📸 Demo:

⚡ Backend Changes:

🤡 Updates to mock-api:

🧪 Testing:

📝 PR Tasks:

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

johncowen Nov 3, 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

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

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

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

hc-github-team-consul-core commented Nov 10, 2021

kaxcode commented Oct 20, 2021 •

edited

Loading

johncowen Nov 3, 2021 •

edited

Loading