diff --git a/packages/api-v4/src/aglb/types.ts b/packages/api-v4/src/aglb/types.ts index 14a541d0f1c..c6a416f7a2f 100644 --- a/packages/api-v4/src/aglb/types.ts +++ b/packages/api-v4/src/aglb/types.ts @@ -179,7 +179,7 @@ type CertificateType = 'ca' | 'downstream'; export interface Certificate { id: number; label: string; - certificate: string; + certificate?: string; // Not returned for Alpha type: CertificateType; } diff --git a/packages/manager/.changeset/pr-9941-upcoming-features-1701203325929.md b/packages/manager/.changeset/pr-9941-upcoming-features-1701203325929.md new file mode 100644 index 00000000000..c3cefb0cbb6 --- /dev/null +++ b/packages/manager/.changeset/pr-9941-upcoming-features-1701203325929.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Upcoming Features +--- + +Add AGLB Configuration e2e tests, improve error handling, and improve UX ([#9941](https://github.com/linode/manager/pull/9941)) diff --git a/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts b/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts index 126a4e1d1e3..588c9c27e17 100644 --- a/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts +++ b/packages/manager/cypress/e2e/core/loadBalancers/load-balancer-configurations.spec.ts @@ -18,6 +18,8 @@ import { mockGetLoadBalancerCertificates, mockGetLoadBalancerConfigurations, mockGetLoadBalancerRoutes, + mockUpdateLoadBalancerConfiguration, + mockUpdateLoadBalancerConfigurationError, } from 'support/intercepts/load-balancers'; import { ui } from 'support/ui'; @@ -353,6 +355,151 @@ describe('Akamai Global Load Balancer configurations page', () => { } }); }); + + describe('update', () => { + it('edits a HTTPS configuration', () => { + const configuration = configurationFactory.build({ protocol: 'https' }); + const loadbalancer = loadbalancerFactory.build({ + configurations: [{ id: configuration.id, label: configuration.label }], + }); + const certificates = certificateFactory.buildList(3); + const routes = routeFactory.buildList(3); + + mockGetLoadBalancer(loadbalancer).as('getLoadBalancer'); + mockGetLoadBalancerConfigurations(loadbalancer.id, [configuration]).as( + 'getConfigurations' + ); + mockGetLoadBalancerCertificates(loadbalancer.id, certificates).as( + 'getCertificates' + ); + mockUpdateLoadBalancerConfiguration(loadbalancer.id, configuration).as( + 'updateConfiguration' + ); + mockGetLoadBalancerRoutes(loadbalancer.id, routes).as('getRoutes'); + + cy.visitWithLogin( + `/loadbalancers/${loadbalancer.id}/configurations/${configuration.id}` + ); + + cy.wait([ + '@getFeatureFlags', + '@getClientStream', + '@getLoadBalancer', + '@getConfigurations', + '@getRoutes', + '@getCertificates', + ]); + + // In edit mode, we will disable the "save" button if the user hasn't made any changes + ui.button + .findByTitle('Save Configuration') + .should('be.visible') + .should('be.disabled'); + + cy.findByLabelText('Configuration Label') + .should('be.visible') + .clear() + .type('new-label'); + + cy.findByLabelText('Port').should('be.visible').clear().type('444'); + + ui.button + .findByTitle('Apply More Certificates') + .should('be.visible') + .should('be.enabled') + .click(); + + ui.drawer.findByTitle('Apply Certificates').within(() => { + cy.findByLabelText('Host Header').type('example-1.com'); + + cy.findByLabelText('Certificate').click(); + + ui.autocompletePopper.findByTitle(certificates[1].label).click(); + + ui.button + .findByTitle('Save') + .should('be.visible') + .should('be.enabled') + .click(); + }); + + ui.button + .findByTitle('Save Configuration') + .should('be.visible') + .should('be.enabled') + .click(); + + cy.wait('@updateConfiguration'); + }); + + it('shows API errors when editing', () => { + const configuration = configurationFactory.build({ protocol: 'https' }); + const loadbalancer = loadbalancerFactory.build({ + configurations: [{ id: configuration.id, label: configuration.label }], + }); + + mockGetLoadBalancer(loadbalancer).as('getLoadBalancer'); + mockGetLoadBalancerConfigurations(loadbalancer.id, [configuration]).as( + 'getConfigurations' + ); + + const errors = [ + { field: 'label', reason: 'Bad Label.' }, + { field: 'port', reason: 'Port number is too high.' }, + { field: 'protocol', reason: 'This protocol is not supported.' }, + { + field: 'certificates[0].id', + reason: 'Certificate 0 does not exist.', + }, + { + field: 'certificates[0].hostname', + reason: 'That hostname is too long.', + }, + { field: 'route_ids', reason: 'Some of these routes do not exist.' }, + ]; + + mockUpdateLoadBalancerConfigurationError( + loadbalancer.id, + configuration.id, + errors + ).as('updateConfigurationError'); + + cy.visitWithLogin( + `/loadbalancers/${loadbalancer.id}/configurations/${configuration.id}` + ); + + cy.wait([ + '@getFeatureFlags', + '@getClientStream', + '@getLoadBalancer', + '@getConfigurations', + ]); + + // In edit mode, we will disable the "save" button if the user hasn't made any changes + ui.button + .findByTitle('Save Configuration') + .should('be.visible') + .should('be.disabled'); + + cy.findByLabelText('Configuration Label') + .should('be.visible') + .clear() + .type('new-label'); + + ui.button + .findByTitle('Save Configuration') + .should('be.visible') + .should('be.enabled') + .click(); + + cy.wait('@updateConfigurationError'); + + for (const error of errors) { + cy.findByText(error.reason).should('be.visible'); + } + }); + }); + describe('delete', () => { it('deletes a configuration', () => { const loadbalancer = loadbalancerFactory.build(); diff --git a/packages/manager/cypress/support/intercepts/load-balancers.ts b/packages/manager/cypress/support/intercepts/load-balancers.ts index 687240aea62..ff7ed60fbe2 100644 --- a/packages/manager/cypress/support/intercepts/load-balancers.ts +++ b/packages/manager/cypress/support/intercepts/load-balancers.ts @@ -119,6 +119,45 @@ export const mockCreateLoadBalancerConfiguration = ( ); }; +/** + * Intercepts PUT request to update an AGLB configuration. + * + * @param loadBalancerId - ID of load balancer for which to update the configuration. + * @param configuration - The AGLB configuration being updated. + * + * @returns Cypress chainable. + */ +export const mockUpdateLoadBalancerConfiguration = ( + loadBalancerId: number, + configuration: Configuration +) => { + return cy.intercept( + 'PUT', + apiMatcher(`/aglb/${loadBalancerId}/configurations/${configuration.id}`), + makeResponse(configuration) + ); +}; + +/** + * Intercepts PUT request to update an AGLB configuration. + * + * @param loadBalancerId - ID of load balancer for which to update the configuration. + * @param configuration - The AGLB configuration being updated. + * + * @returns Cypress chainable. + */ +export const mockUpdateLoadBalancerConfigurationError = ( + loadBalancerId: number, + configurationId: number, + errors: APIError[] +) => { + return cy.intercept( + 'PUT', + apiMatcher(`/aglb/${loadBalancerId}/configurations/${configurationId}`), + makeResponse({ errors }, 400) + ); +}; + /** * Intercepts POST request to create an AGLB configuration and returns an error. * diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx index 1bb38a1df5a..7efd1e5a9fa 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.test.tsx @@ -1,4 +1,3 @@ -import { Certificate } from '@linode/api-v4'; import { act, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import React from 'react'; @@ -8,18 +7,19 @@ import { renderWithTheme } from 'src/utilities/testHelpers'; import { EditCertificateDrawer } from './EditCertificateDrawer'; -const mockTLSCertificate: Certificate = { +const mockTLSCertificate = { certificate: mockCertificate, id: 0, label: 'test-tls-cert', type: 'downstream', -}; -const mockCACertificate: Certificate = { +} as const; + +const mockCACertificate = { certificate: mockCertificate, id: 0, label: 'test-ca-cert', type: 'ca', -}; +} as const; describe('EditCertificateDrawer', () => { it('should contain the name of the cert in the drawer title and label field', () => { diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx index e9d20cfb6ab..9b9ca01ebe6 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/EditCertificateDrawer.tsx @@ -35,7 +35,7 @@ export const EditCertificateDrawer = (props: Props) => { const formik = useFormik({ enableReinitialize: true, initialValues: { - certificate: certificate?.certificate.trim(), + certificate: certificate?.certificate?.trim(), key: '', label: certificate?.label ?? '', type: certificate?.type, @@ -43,7 +43,7 @@ export const EditCertificateDrawer = (props: Props) => { async onSubmit(values) { // The user has not edited their cert or the private key, so we exclude both cert and key from the request. const shouldIgnoreField = - certificate?.certificate.trim() === values.certificate && + certificate?.certificate?.trim() === values.certificate && values.key === ''; try { diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx index d6503b2b618..c823a573672 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx @@ -85,6 +85,7 @@ export const ApplyCertificatesDrawer = (props: Props) => { ) } errorText={formik.errors.certificates?.[index]?.['id']} + filter={{ type: 'downstream' }} loadbalancerId={loadbalancerId} value={id} /> diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx index 00154f2b99b..26822ecd702 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx @@ -8,18 +8,21 @@ import { TableCell } from 'src/components/TableCell'; import { TableHead } from 'src/components/TableHead'; import { TableRow } from 'src/components/TableRow'; import { TableRowEmpty } from 'src/components/TableRowEmpty/TableRowEmpty'; +import { Typography } from 'src/components/Typography'; import { useLoadBalancerCertificatesQuery } from 'src/queries/aglb/certificates'; -import type { Configuration } from '@linode/api-v4'; +import type { CertificateConfig, Configuration } from '@linode/api-v4'; +import type { FormikErrors } from 'formik'; interface Props { certificates: Configuration['certificates']; + errors: FormikErrors[] | string[]; loadbalancerId: number; onRemove: (index: number) => void; } export const CertificateTable = (props: Props) => { - const { certificates, loadbalancerId, onRemove } = props; + const { certificates, errors, loadbalancerId, onRemove } = props; const { data } = useLoadBalancerCertificatesQuery( loadbalancerId, @@ -40,10 +43,27 @@ export const CertificateTable = (props: Props) => { {certificates.length === 0 && } {certificates.map((cert, idx) => { const certificate = data?.data.find((c) => c.id === cert.id); + const error = errors[idx]; + const hostnameError = + typeof error !== 'string' ? error?.hostname : undefined; + const idError = typeof error !== 'string' ? error?.id : undefined; + + const generalRowError = typeof error === 'string' ? error : idError; + return ( - {certificate?.label ?? cert.id} - {cert.hostname} + + {certificate?.label ?? cert.id} + {generalRowError && ( + {generalRowError} + )} + + + {cert.hostname} + {hostnameError && ( + {hostnameError} + )} + { ? useLoadBalancerConfigurationMutation : useLoadBalancerConfigurationCreateMutation; - const { error, isLoading, mutateAsync } = useMutation( + const { error, isLoading, mutateAsync, reset } = useMutation( loadbalancerId, configuration?.id ?? -1 ); @@ -102,10 +102,24 @@ export const ConfigurationForm = (props: CreateProps | EditProps) => { helpers.setErrors(getFormikErrorsFromAPIErrors(error)); } }, - validationSchema, + validate(values) { + // We must use `validate` insted of validationSchema because Formik decided to convert + // "" to undefined before passing the values to yup. This makes it hard to validate `label`. + // See https://github.com/jaredpalmer/formik/issues/805 + try { + validationSchema.validateSync(values, { abortEarly: false }); + return {}; + } catch (error) { + return yupToFormErrors(error); + } + }, + // Prevent errors from being cleared when we show API errors + validateOnBlur: !error, + validateOnChange: !error, }); const handleRemoveCert = (index: number) => { + formik.setFieldTouched('certificates'); formik.values.certificates.splice(index, 1); formik.setFieldValue('certificates', formik.values.certificates); }; @@ -119,16 +133,22 @@ export const ConfigurationForm = (props: CreateProps | EditProps) => { }; const handleAddCerts = (certificates: Configuration['certificates']) => { + formik.setFieldTouched('certificates'); formik.setFieldValue('certificates', [ ...formik.values.certificates, ...certificates, ]); }; + const handleReset = () => { + formik.resetForm(); + reset(); + }; + const generalErrors = error?.reduce((acc, { field, reason }) => { if ( !field || - !['certificates', 'label', 'port', 'protocol'].includes(field) + (formik.values.protocol !== 'https' && field.startsWith('certificates')) ) { return acc ? `${acc}, ${reason}` : reason; } @@ -197,6 +217,11 @@ export const ConfigurationForm = (props: CreateProps | EditProps) => { /> )} { + )} + {mode === 'edit' && ( + + )} + {mode === 'create' && ( + + )} - diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerServiceTargets.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerServiceTargets.tsx index 1fbcec94796..c8b0e318bee 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerServiceTargets.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerServiceTargets.tsx @@ -1,7 +1,7 @@ import { ServiceTarget } from '@linode/api-v4'; import CloseIcon from '@mui/icons-material/Close'; import { Hidden, IconButton } from '@mui/material'; -import React, { useEffect, useState } from 'react'; +import React, { useState } from 'react'; import { useParams } from 'react-router-dom'; import { Button } from 'src/components/Button/Button'; @@ -66,13 +66,6 @@ export const LoadBalancerServiceTargets = () => { setSelectedServiceTarget(serviceTarget); }; - // Once the drawer is closed, clear the selected service target for the correct add/edit drawer data. - useEffect(() => { - if (!isDrawerOpen) { - setSelectedServiceTarget(undefined); - } - }, [isDrawerOpen]); - // If the user types in a search query, filter results by label. if (query) { filter['label'] = { '+contains': query }; @@ -148,10 +141,10 @@ export const LoadBalancerServiceTargets = () => { Algorithm @@ -197,8 +190,11 @@ export const LoadBalancerServiceTargets = () => { pageSize={pagination.pageSize} /> { + setIsDrawerOpen(false); + setSelectedServiceTarget(undefined); + }} loadbalancerId={Number(loadbalancerId)} - onClose={() => setIsDrawerOpen(false)} open={isDrawerOpen} serviceTarget={selectedServiceTarget} /> diff --git a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/RuleDrawer.tsx b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/RuleDrawer.tsx index dc77a6bafbc..680c9da69ff 100644 --- a/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/RuleDrawer.tsx +++ b/packages/manager/src/features/LoadBalancers/LoadBalancerDetail/Routes/RuleDrawer.tsx @@ -89,6 +89,7 @@ export const RuleDrawer = (props: Props) => { } await updateRoute({ + label: route?.label, protocol: route?.protocol, // If we are editing, send the updated rules, otherwise // append a new rule to the end. diff --git a/packages/manager/src/queries/aglb/serviceTargets.ts b/packages/manager/src/queries/aglb/serviceTargets.ts index c417d06a939..1dcc5560aa3 100644 --- a/packages/manager/src/queries/aglb/serviceTargets.ts +++ b/packages/manager/src/queries/aglb/serviceTargets.ts @@ -97,7 +97,7 @@ export const useLoadBalancerServiceTargetsInfiniteQuery = ( filter: Filter = {} ) => { return useInfiniteQuery, APIError[]>( - [QUERY_KEY, 'loadbalancer', id, 'service-targets', 'infinite', filter], + [QUERY_KEY, 'aglb', id, 'service-targets', 'infinite', filter], ({ pageParam }) => getLoadbalancerServiceTargets( id, diff --git a/packages/validation/src/loadbalancers.schema.ts b/packages/validation/src/loadbalancers.schema.ts index af4f3d80127..cd02d0b72bd 100644 --- a/packages/validation/src/loadbalancers.schema.ts +++ b/packages/validation/src/loadbalancers.schema.ts @@ -134,6 +134,7 @@ const BaseRuleSchema = object({ return sum === 100; } ) + .min(1, 'Rules must have at least 1 Service Target.') .required(), });