upcoming: [M3-7401] - AGLB Load Balancer and Configuration Refinements #9903

bnussman-akamai · 2023-11-14T18:16:12Z

Description 📝

The main goal of the PR is to get AGLB Configuration creation working with the real API. A handful of other small refinements are included

Changes 🔄

Adds hostname to AGLB landing table
Make accordion open upon successful config creation
Removed unneeded UnusedConfigurationNotice logic and components
- The API handled making the association between a config and a load balancer so we don't need to worry about this
Improves React Query invalidations
Makes the Port TextField type="number" so that a integer gets passed to the API
- This fixes Configuration creation with the real API
Adds skeleton loader and error state to Load Balancer Landing ports column
Improves loading state of Load Balancer Details
Removes hardcoded health status components
- This won't be available for alpha. I could have commented this out instead of deleting, but it needed to be made into a component anyway. A ticket will be created when we build a "beta" epic.

Preview 📷

Before	After
	Screen.Recording.2023-11-14.at.1.13.40.PM.mov

How to test 🧪

Prerequisites

You should be able to test with the dev-test-aglb account in dev

Verification steps

Verify each change listed in the Changes section
The main things to check
- You can create a Configuration using the real API
- Check for general regressions, all other changes are minor

As an Author I have considered 🤔

…ther refinement

bnussman-akamai · 2023-11-15T13:46:13Z

...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx

-      <UnusedConfigurationNotice
-        configurationId={configuration.id}
-        loadbalancerId={loadbalancerId}
-      />


This turned out to be a non-issue on the API, so we can remove it

bnussman-akamai · 2023-11-15T13:47:13Z

...s/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationForm.tsx

@@ -196,6 +175,7 @@ export const ConfigurationForm = (props: CreateProps | EditProps) => {
            labelTooltipText="TODO: AGLB"
            name="port"
            onChange={formik.handleChange}
+            type="number"


This fixes the formik payload so that port is a number instead of a string.

mjac0bs

I had a handful of questions when looking at this, mostly because I wasn't sure the exact status of what we are expecting to work and what we're still anticipating errors for. 😅

Verified:
⚠️ Adds hostname to AGLB landing table - it displays, but see notes below
❌ Make accordion open upon successful config creation
✅ Removed unneeded UnusedConfigurationNotice logic and components
✅ Makes the Port TextField type="number" so that a integer gets passed to the API
✅ Adds skeleton loader and error state to Load Balancer Landing ports column

Questions:

Should we be expecting a blank hostname in the loadbalancer summary right now? (API is not returning this yet, but plans to?)
- Also, I'm seeing a console error <div> cannot appear as a descendant of <p> that seems like it traces back to the IPAddress component for hostname
Should we be expecting an Oh Snap crash when trying to reach the Certificates page? (or trying to Add a Certificate in the configuration flow)
Should we be expecting an Oh Snap crash when trying to Add a Rule to a Route?
Should we be expecting to see the route added to the table in the configuration flow when we select Add Route > Create New Route? It's only visible in the table when I Add an Existing Route.
Do we need to delete existing configurations to be able to create new ones? When I tried to create a new config, I received the error shown below, so I couldn't confirm successful config creation.

Screen.Recording.2023-11-15.at.7.40.01.AM.mov

bnussman-akamai · 2023-11-15T16:19:56Z

Should we be expecting a blank hostname in the loadbalancer summary right now?

Yes, it was not populated in the db

Should we be expecting an Oh Snap crash when trying to reach the Certificates page?

Yes, the API is in progress of fixing the API shape which should resolve this.

Should we be expecting an Oh Snap crash when trying to Add a Rule to a Route?

Not anymore. The API just pushed a fix for the shape of the Service Targets response, which should fix this crash.

Should we be expecting to see the route added to the table in the configuration flow when we select Add Route > Create New Route? It's only visible in the table when I Add an Existing Route.

Looking into this 👀

Edit: It looks like the API is not behaving as expected. I will monitor and report this to Anthony

Do we need to delete existing configurations to be able to create new ones? When I tried to create a new config, I received the error shown below, so I couldn't confirm successful config creation.

You are probably getting that error because the name of the configuration is not unique or something is going wrong on the backend.

Edit: It looks like HTTPS configurations aren't createable. I'll work on this with Anthony. HTTP and TCP should work

bnussman-akamai · 2023-11-15T16:36:56Z

packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerDetail.tsx

@@ -108,7 +118,7 @@ export const LoadBalancerDetail = () => {
        docsLabel="Docs"
        docsLink="" // TODO: AGLB - Add docs link
      />
-      <Tabs index={tabIndex === -1 ? 0 : tabIndex}>
+      <Tabs index={tabIndex === -1 ? 0 : tabIndex} onChange={() => null}>


We don't need an onChange because the Tabs are Links, which cause the browser to redirect and we derive tabIndex from the URL. I had to do this because ReachUI console.errors when there is no onChange

mjac0bs

I'm back for another round! Thanks for updating the PR description with the notes on the health status removal. And for fixing the console errors!

Should we be expecting an Oh Snap crash when trying to reach the Certificates page?

Yes, the API is in progress of fixing the API shape which should resolve this.

✅ There is no longer a crash when visiting the certificates page.

⚠️ Is this work still in progress? In case it isn't and should be working now, this is what I'm seeing for both the Service Target and TLS Certificates:

Should we be expecting an Oh Snap crash when trying to Add a Rule to a Route?

Not anymore. The API just pushed a fix for the shape of the Service Targets response, which should fix this crash.

✅ There is no longer a crash when adding a rule to a route.
❌ I can add a rule to a route. (I see the following error message.)

Screen.Recording.2023-11-16.at.2.52.51.PM.mov

Should we be expecting to see the route added to the table in the configuration flow when we select Add Route > Create New Route? It's only visible in the table when I Add an Existing Route.

Looking into this 👀

Edit: It looks like the API is not behaving as expected. I will monitor and report this to Anthony

✅ I can see the route added to the table in the configuration flow when we select Add Route > Create New Route.
⚠️ Is it a known issue that adding an existing route to a configuration results in adding ALL existing routes? And then, in order to delete one route, all routes must be deleted? (See video.)

Screen.Recording.2023-11-16.at.2.50.03.PM.mov

Do we need to delete existing configurations to be able to create new ones? When I tried to create a new config, I received the error shown below, so I couldn't confirm successful config creation.

You are probably getting that error because the name of the configuration is not unique or something is going wrong on the backend.

Edit: It looks like HTTPS configurations aren't createable. I'll work on this with Anthony. HTTP and TCP should work

✅ I can create a new configuration now of any protocol type.

Found a 🐛 in the Add Rule drawer that I thought I was just imagining in weeks prior, but no, I can repro it.

When a user fills out some form fields and then goes to click on "Add a Service Target", they have to click the link twice. The first time has no effect. If the user clicks "Add a Service Target" before interacting other parts of the form, one link click works as expected.

Screen.Recording.2023-11-16.at.2.24.31.PM.mov

bnussman-akamai · 2023-11-16T23:24:26Z

Is it a known issue that adding an existing route to a configuration results in adding ALL existing routes?

Yes. This is a known issue caused by the API not properly filtering.

The rule creation issue is a known bug I need to ask the API for help on. The certificates filtering issue has a ticket created, waiting for API work.

Looking into the Add a Service Target button issue. This one is savage! No idea what's going on. Maybe it can be out of scope 😂

Edit: No findings so far. Still looking to merge this PR asap. More refinements will come in follow up PRs!

mjac0bs

Thanks for addressing all those points. What's expected to work in the PR description does work, so approving this! 🚢

As discussed offline, the issue with the Add a Service Target button has to do with the LinkButton component (which isn't entirely deprecated but mostly is?). We've switched to StyledLinkButton, but with a type=submit, we'd be submitting the form at that point and so this will probably need some reworking, to be addressed in a follow up ticket.

cpathipa

Over all LGTM! verified on creating Configurations.

You can create a Configuration using the real API
Check for general regressions, all other changes are minor

packages/validation/src/loadbalancers.schema.ts

bnussman added 3 commits November 14, 2023 12:13

initial refinements

e04fae8

add loading and error state to details

8a25b02

add hostname to table

29ac223

bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Nov 14, 2023

bnussman-akamai self-assigned this Nov 14, 2023

add some very basic testing

000e2b8

bnussman-akamai marked this pull request as ready for review November 14, 2023 19:04

bnussman-akamai requested a review from a team as a code owner November 14, 2023 19:04

bnussman-akamai requested review from hana-akamai, coliu-akamai, cpathipa and mjac0bs and removed request for a team November 14, 2023 19:04

bnussman added 2 commits November 14, 2023 14:19

Added changeset: Fix Configuration creation by fixing port type and o…

372ed16

…ther refinement

update e2e test

7c29575

bnussman-akamai commented Nov 15, 2023

View reviewed changes

Merge branch 'develop' into aglb-configuration-refinements

b5f17a0

bnussman-akamai added the Work in Progress label Nov 15, 2023

mjac0bs reviewed Nov 15, 2023

View reviewed changes

remove impossible features for alpha

e542a1a

resolve two console errors

614dbd9

bnussman-akamai commented Nov 15, 2023

View reviewed changes

bnussman-akamai removed the Work in Progress label Nov 15, 2023

fix function name

61508f6

bnussman-akamai requested a review from mjac0bs November 15, 2023 17:12

bnussman added 4 commits November 15, 2023 12:16

improve loading state of ports on summary page

8fca31b

update validation to match API

0042f79

Merge branch 'develop' into aglb-configuration-refinements

9337378

fix incorrect query key

6b9e068

mjac0bs reviewed Nov 16, 2023

View reviewed changes

Merge branch 'develop' into aglb-configuration-refinements

ae9a3e0

mjac0bs approved these changes Nov 17, 2023

View reviewed changes

mjac0bs added Approved Multiple approvals and ready to merge! Add'tl Approval Needed Waiting on another approval! and removed Approved Multiple approvals and ready to merge! labels Nov 17, 2023

bnussman added 2 commits November 17, 2023 11:11

fix LinkButton flaw (fixes Add a Service Target button behavior)

12d45a8

fix broken unit test

375272b

cpathipa approved these changes Nov 17, 2023

View reviewed changes

mjac0bs reviewed Nov 17, 2023

View reviewed changes

packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved

add changeset

3978ac5

bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 17, 2023

bnussman-akamai merged commit 1e0ea08 into linode:develop Nov 17, 2023
13 checks passed

mjac0bs mentioned this pull request Dec 11, 2023

Release v1.108.0 - staging → master #9986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-7401] - AGLB Load Balancer and Configuration Refinements #9903

upcoming: [M3-7401] - AGLB Load Balancer and Configuration Refinements #9903

bnussman-akamai commented Nov 14, 2023 •

edited

Loading

bnussman-akamai Nov 15, 2023

bnussman-akamai Nov 15, 2023

mjac0bs left a comment

bnussman-akamai commented Nov 15, 2023 •

edited

Loading

bnussman-akamai Nov 15, 2023

mjac0bs left a comment •

edited

Loading

bnussman-akamai commented Nov 16, 2023 •

edited

Loading

mjac0bs left a comment

cpathipa left a comment

upcoming: [M3-7401] - AGLB Load Balancer and Configuration Refinements #9903

upcoming: [M3-7401] - AGLB Load Balancer and Configuration Refinements #9903

Conversation

bnussman-akamai commented Nov 14, 2023 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

bnussman-akamai Nov 15, 2023

Choose a reason for hiding this comment

bnussman-akamai Nov 15, 2023

Choose a reason for hiding this comment

mjac0bs left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Nov 15, 2023 • edited Loading

bnussman-akamai Nov 15, 2023

Choose a reason for hiding this comment

mjac0bs left a comment • edited Loading

Choose a reason for hiding this comment

bnussman-akamai commented Nov 16, 2023 • edited Loading

mjac0bs left a comment

Choose a reason for hiding this comment

cpathipa left a comment

Choose a reason for hiding this comment

bnussman-akamai commented Nov 14, 2023 •

edited

Loading

bnussman-akamai commented Nov 15, 2023 •

edited

Loading

mjac0bs left a comment •

edited

Loading

bnussman-akamai commented Nov 16, 2023 •

edited

Loading