feat: [M3-7160] - Add AGLB "Edit Certificate" drawer #9723

mjac0bs · 2023-09-26T21:18:32Z

Description 📝

Add an edit drawer for AGLB certificates.

Major Changes 🔄

Creates the Edit Certificate drawer with dynamic fields depending on certificate type
Adds the useLoadBalancerCertificateMutation
Adds a unit test for EditCertificateDrawer component
Updates the MSW PUT endpoint for */v4beta/aglb/:id/certificates/:certId to return a downstream cert in addition to the existing ca certs.

Preview 📷

Service Target Certificates	TLS Certificates

How to test 🧪

How to setup test environment?

Check out this PR.
yarn dev
Ensure AGLB feature flag is on and MSW is enabled.

How to verify changes?

Go to http://localhost:3000/loadbalancers/0/certificates/downstream.
Click on the Edit button in the action menu of the tls-certificate. Confirm that the edit drawer matches the mocks above for a TLS certificate.
Click on the Edit button in the action menu of any other certificate (they're all of type ca). Confirm that the edit drawer matches the mocks above for a Service Target certificate.
Confirm that a PUT request is sent by the MSW when the Update Certificate button is clicked.
Confirm that no PUT request is sent by the MSW when the drawer is Canceled or closed out of.
Confirm form validation:
- If a user edits the label of either certificate type, the form submits without error.
- If a user edits the certificate of a Service Target (ca) cert, the form submits without error.
- If a user edits the certificate of a TLS (downstream) cert and submits without a key, they should receive a validation error upon submission indicating Private Key is required in that case.

How to run Unit or E2E tests?
Unit:

yarn test EditCertificateDrawer

E2E:
Will be completed in M3-7134, since there was an existing ticket created for this.

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

mjac0bs · 2023-09-27T20:25:14Z

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

+          <TextField
+            errorText={errorMap.certificate}
+            label={labelMap[certificate.type]}
+            labelTooltipText="TODO: AGLB"


I'm not sure what the plan is for the copy in these tooltips, but given that we are not able to retrieve or display a certificate object's certificate or key fields for security reasons, we should probably mention that in this form so that the user does not think that their cert/key data is missing.

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

bnussman-akamai

Looks great! My only concerns regard consistency between the Create Drawer and this new Edit Drawer. We may need to ask Daniel for some clarification.

In the Create Service Target Cert drawer we say "TLS Certificate" but in the edit drawer we say "Service Certificate". I guess we should say "Service Certificate" in both places?
We have placeholders in the Create drawer but not the edit drawer

mjac0bs · 2023-09-29T16:15:25Z

@bnussman-akamai

In the Create Service Target Cert drawer we say "TLS Certificate" but in the edit drawer we say "Service Certificate". I guess we should say "Service Certificate" in both places?

Good catch on this! 🔍 Asked Daniel for clarification in a comment on the mocks and reached out to let him know.

We have placeholders in the Create drawer but not the edit drawer

Yeah, I'm not sure what's best here because we could use the same example placeholder as the Create drawer, but that might face the same problem as leaving it blank -- causing users to think that it is missing/incomplete, when really we're just not able to display the data. I also asked Daniel about this in the mocks.

mjac0bs · 2023-10-03T16:11:27Z

...nager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/CreateCertificateDrawer.tsx

@@ -91,7 +93,7 @@ export const CreateCertificateDrawer = (props: Props) => {
        />
        <TextField
          errorText={errorMap.certificate}
-          label="TLS Certificate"
+          label={labelMap[type]}


In the Create Service Target Cert drawer we say "TLS Certificate" but in the edit drawer we say "Service Certificate". I guess we should say "Service Certificate" in both places?

Daniel updated to mocks so that the Create drawer now labels the field as "Server Certificate" for ca/service target certificates and is "TLS Certificate" for downstream/TLS, so I made the change to dynamically render the label based on type in the Create drawer here.

mjac0bs · 2023-10-03T16:13:17Z

packages/manager/src/factories/aglb.ts

@@ -13,7 +13,7 @@ import {
 } from '@linode/api-v4/lib/aglb/types';
 import * as Factory from 'factory.ts';

-const certificate = `
+export const mockCertificate = `


I preemptively renamed in hope of making it clear that these are not real values that are a cause of security concern.

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

mjac0bs · 2023-10-03T16:18:31Z

packages/api-v4/src/aglb/types.ts

@@ -141,6 +141,7 @@ type CertificateType = 'ca' | 'downstream';
 export interface Certificate {
  id: number;
  label: string;
+  certificate: string;


The API spec has been updated to indicate that certificate will now be returned in GET /v4beta/aglb/{id}/certificates.

bnussman-akamai

When we make the PUT, I think we may want to omit certificate and key from the body if they are unchanged. I worry this might make the API clear out the certificate and key if we send an empty string or perhaps cause a validation error.

bnussman-akamai

Great job! 🎉

cpathipa

Over all looks great! left couple comments for minor suggestions.
Validated

Edit Certificate drawer with dynamic fields depending on certificate type
test coverage for EditCertificateDrawer

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx

mjac0bs · 2023-10-05T22:39:49Z

@bnussman-akamai , @cpathipa

Thanks for the reviews! I'll be merging this shortly. In my last commit, I addressed UX feedback to make the cert field contain the actual cert value instead of placeholder text. We now check whether the user is updating the cert from its original value and, if they are, require them to provide a key (as per the API spec). Otherwise, we can safely exclude the cert in the request and therefore avoid the user needing to submit their existing key, which we cannot return. (Validation seems to work correctly, as do request payloads; I think when I tried this initially, I was having unrelated validation issues.)

Creating follow up tickets to address additional UX feedback:

display the cert id in the table

mjac0bs · 2023-10-06T14:09:08Z

One relevant e2e failure fixed due to the typo correction in the Create Cert drawer; this test now passes:

mjac0bs added Work in Progress ACLB Relating to the Akamai Cloud Load Balancer labels Sep 26, 2023

mjac0bs self-assigned this Sep 26, 2023

mjac0bs force-pushed the M3-7160-edit-cert-drawer branch from b85ace7 to 411b570 Compare September 27, 2023 20:15

mjac0bs commented Sep 27, 2023

View reviewed changes

mjac0bs requested a review from bnussman-akamai September 27, 2023 20:30

mjac0bs added Ready for Review and removed Work in Progress labels Sep 27, 2023

mjac0bs marked this pull request as ready for review September 27, 2023 20:41

mjac0bs requested a review from a team as a code owner September 27, 2023 20:42

mjac0bs requested review from cpathipa and cliu-akamai and removed request for a team September 27, 2023 20:42

bnussman-akamai reviewed Sep 29, 2023

View reviewed changes

mjac0bs added 8 commits October 3, 2023 09:04

Initial set up, open edit drawer action

a0996b6

Update mock data, dynamic drawer display, and unit test

34a4ffd

Update tests to add TODOs and skip correctly

06f0886

Update payload for PUT request, update unit test

61f2999

Only include key in errorFields for a downstream cert type

1366934

Add validation

8e9854d

Fix schema

ae8cf08

Address feedback

499247b

mjac0bs force-pushed the M3-7160-edit-cert-drawer branch from 3a896c4 to 499247b Compare October 3, 2023 16:05

mjac0bs commented Oct 3, 2023

View reviewed changes

mjac0bs added 3 commits October 3, 2023 19:26

Re-enable cert and key fields; add cert field validation

caaf647

Add placeholder text to private key field

101f015

Address Slack feedback

6a6d1d6

bnussman-akamai reviewed Oct 4, 2023

View reviewed changes

Address feedback, but WIP validation

816ee30

Add type so validation triggers correctly; thanks @bnussman-akamai

f79299a

bnussman-akamai approved these changes Oct 4, 2023

View reviewed changes

cpathipa approved these changes Oct 5, 2023

View reviewed changes

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx Outdated Show resolved Hide resolved

...manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx Outdated Show resolved Hide resolved

Address PR and UX feedback

3269b33

mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels Oct 5, 2023

Fix failure in load-balancer-certificates.spec.ts due to label change

30e80a7

mjac0bs merged commit 459f7e7 into linode:develop Oct 6, 2023
11 of 12 checks passed

This was referenced Oct 16, 2023

Release v1.105.0 - Update API, Manager, and Validation changelogs #9795

Merged

Release v1.105.0 - staging → master (#9789) #9794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-7160] - Add AGLB "Edit Certificate" drawer #9723

feat: [M3-7160] - Add AGLB "Edit Certificate" drawer #9723

mjac0bs commented Sep 26, 2023 •

edited

Loading

mjac0bs Sep 27, 2023

bnussman-akamai left a comment

mjac0bs commented Sep 29, 2023

mjac0bs Oct 3, 2023

mjac0bs Oct 3, 2023

mjac0bs Oct 3, 2023

bnussman-akamai left a comment

bnussman-akamai left a comment

cpathipa left a comment

mjac0bs commented Oct 5, 2023 •

edited

Loading

mjac0bs commented Oct 6, 2023

feat: [M3-7160] - Add AGLB "Edit Certificate" drawer #9723

feat: [M3-7160] - Add AGLB "Edit Certificate" drawer #9723

Conversation

mjac0bs commented Sep 26, 2023 • edited Loading

Description 📝

Major Changes 🔄

Preview 📷

How to test 🧪

mjac0bs Sep 27, 2023

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

mjac0bs commented Sep 29, 2023

mjac0bs Oct 3, 2023

Choose a reason for hiding this comment

mjac0bs Oct 3, 2023

Choose a reason for hiding this comment

mjac0bs Oct 3, 2023

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

bnussman-akamai left a comment

Choose a reason for hiding this comment

cpathipa left a comment

Choose a reason for hiding this comment

mjac0bs commented Oct 5, 2023 • edited Loading

mjac0bs commented Oct 6, 2023

mjac0bs commented Sep 26, 2023 •

edited

Loading

mjac0bs commented Oct 5, 2023 •

edited

Loading