Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [M3-7521 & M3-7556 & M3-7581] - Update AGLB Service Target Validation & Fix Route Rules being cleared #10016

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Dec 21, 2023

Description 📝

Makes three major changes

  • M3-7521
    • Make our client side validation for Service Targets match the API's validation
  • M3-7556
    • Fixes Route rules being cleared
  • M3-7581
    • Removes ability to toggle Service Target health checks based on discussion with Mark and Daniel
  • Other
    • Removes host option from a Rule match_type

Changes 🔄

  • Fixes route rules bring cleared when trying to update a route
  • Make Service Target forms hide errors unless the field is touched
  • Updates Service Target validation to match
healthcheck.host is a mandatory parameter when using http protocol for service-target health-check.

Both healthcheck.host and healthcheck.path are not required when TCP health-check is used.

Preview 📷

Before After
Screenshot 2023-12-21 at 10 11 52 AM Screenshot 2023-12-21 at 10 28 16 AM

How to test 🧪

Prerequisites

  • Login to dev-test-aglb in the dev environment

Verification steps

Service Target Validation

  • Go to http://localhost:3000/loadbalancers/21/service-targets
  • Verify you can create a Service Target and that the validation explained above holds true
    • ⚠️ There is an API bug causing Service Targets with a TCP Health Check Protocol to not be creatable
  • Verify can you edit a service target and that the validation explained above holds true

Rules getting cleared bug

As an Author I have considered 🤔

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@bnussman-akamai bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Dec 21, 2023
@bnussman-akamai bnussman-akamai self-assigned this Dec 21, 2023
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner December 21, 2023 15:14
@bnussman-akamai bnussman-akamai requested review from coliu-akamai and tyler-akamai and removed request for a team December 21, 2023 15:14
Copy link

github-actions bot commented Dec 21, 2023

Coverage Report:
Base Coverage: 79.85%
Current Coverage: 79.85%

@bnussman-akamai bnussman-akamai requested a review from a team as a code owner December 21, 2023 18:18
@bnussman-akamai bnussman-akamai requested review from cliu-akamai and removed request for a team December 21, 2023 18:18
@bnussman-akamai bnussman-akamai requested review from cpathipa and mjac0bs and removed request for coliu-akamai and tyler-akamai January 2, 2024 17:40
@jdamore-linode
Copy link
Contributor

I'm seeing a couple issues but one may be related to that API bug involving TCP host checks you mentioned in your description, and the other may have already existed:

  • When creating/editing a service target and checking "TCP" under "Health Checks", nothing happens when clicking "Create Service Target". I believe this is a validation issue because no API request is being made, and if you then check the "HTTP" radio, you'll see the host field validation error is present.

    • Similarly, if you then enter a hostname, switch back to "TCP", and then submit the form, the service target gets created.
    • If you then edit the new service target, you'll see that "TCP" is correctly selected for the health check, but the hostname does persist if you click the "HTTPS" radio button -- kind of unexpected behavior from a UX standpoint (and probably caused/handled by the API rather than us)
  • If you delete a service target, and then click "Create Service Target", the drawer title/submit button are displayed as if you're editing the service target that was just deleted (i.e. the title is "Edit ", the submit button is "Save Service Target"). I know this one may not be caused by this PR but just wanted to point it out

@bnussman-akamai
Copy link
Member Author

@jdamore-linode

When creating/editing a service target and checking "TCP" under "Health Checks", nothing happens when clicking "Create Service Target"

This is caused by an API bug and will be resolved by API PR #5930! An API request should occur. An API error is not being surfaced in the UI because the API error field is not correct.

If you delete a service target, and then click "Create Service Target", the drawer title/submit button are displayed as if you're editing

Fixed in 561d8f8

I will re-assess the other things you mentioned when we get this PR and the API PR merged!

@jdamore-linode
Copy link
Contributor

I will re-assess the other things you mentioned when we get this PR and the API PR merged!

Thanks Banks!

An API request should occur. An API error is not being surfaced in the UI because the API error field is not correct.

I may be misunderstanding, but just to clarify, when I attempted to create a TCP host check, Cloud did not make an API request -- it wasn't that the API responded with an error that wasn't surfaced, there was no request/response at all.

Screen.Recording.2024-01-02.at.4.42.25.PM.mp4

On second look, it appears each submit click triggered a console error:

Screenshot 2024-01-02 at 4 43 50 PM Screenshot 2024-01-02 at 4 44 07 PM

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple things I found that feel probably outside the scope of this PR but open to however (whenever) you'd like to address:

  1. Host doesn't look like it should be an option here any more. When I tried to create a rule using that option, I received the following error from API.
    Screenshot 2024-01-02 at 1 38 20 PM
    Screenshot 2024-01-02 at 1 39 01 PM
  2. When I tried to create a route routing to the same service target multiple times, I saw some weird behavior. The request seemed to fail and I saw the API error notice in the drawer, but the rule still appeared in the table.
    Req and response:
    Screenshot 2024-01-02 at 1 35 54 PM
    Screenshot 2024-01-02 at 1 36 03 PM
    Viewing and editing my new rule that supposedly errored, but is visible with just one service target routed at 50%:
    Screenshot 2024-01-02 at 1 36 15 PM

M3-xxxx
Removes ability to toggle Service Target health checks based on discussion with Mark and Daniel

I use x's as placeholders too, ha. Did a ticket for this get created? Can we update the PR title and description for completeness?

Confirmed:

  • I can add/edit a service target and see the health check host is a required field for HTTP protocol; validation reflects this.
  • I confirmed the fix to the behavior Joe mentioned - after deleting a service target, the drawer is no longer in edit mode of the deleted service target; it's in add mode.
  • Route rules are no longer getting cleared when updating a route.
  • Health checks for service targets are now mandatory; the toggle has been removed.
Develop This Branch
image Screenshot 2024-01-02 at 2 01 44 PM

@bnussman-akamai
Copy link
Member Author

@jdamore-linode Should be "fixed" now. Meaning you should see the following:

Screen.Recording.2024-01-02.at.4.52.44.PM.mov

@bnussman-akamai bnussman-akamai changed the title fix: [M3-7521 & M3-7556] - Update AGLB Service Target Validation & Fix Route Rules being cleared fix: [M3-7521 & M3-7556 & M3-7581] - Update AGLB Service Target Validation & Fix Route Rules being cleared Jan 2, 2024
@mjac0bs
Copy link
Contributor

mjac0bs commented Jan 2, 2024

I meant to mention this, then forgot, and want to do so before I forget again: the help tooltip on the health check Protocol field looks slightly off center (too low) vertically, if that's an easy fix. 😅

image

@jaalah-akamai
Copy link
Contributor

the help tooltip on the health check Protocol field looks slightly off center (too low) vertically, if that's an easy fix.

I have a PR that will go up once this is merged that fixes the tooltip experience for our Textfield component. Currently focusing the icon using a keyboard causes the focus state to appear distorted/elongated.

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the fixes, I created service targets, rules, and renamed the service target. Validation is working as intended for me and the rules are in place upon renaming. ✅

Screenshot 2024-01-03 at 9 39 18 AM

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 3, 2024
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 5, 2024
@bnussman-akamai bnussman-akamai merged commit 5cd5707 into linode:develop Jan 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants