This is an automated email from the ASF dual-hosted git repository. LiteSun pushed a commit to branch fix/untouched-inline-upstream in repository https://gitbox.apache.org/repos/asf/apisix-dashboard.git
commit 642cd9533513f1dff08f793c836ead31a8cf21c0 Author: LiteSun <[email protected]> AuthorDate: Thu Oct 15 16:48:51 2026 +0800 fix: do not send an inline upstream the user never touched The route, service and stream-route forms always render the inline upstream section, and its Select / Switch widgets register a value the moment they mount (scheme, type, hash_on, pass_host, tls.verify). A section the user never opened was therefore submitted as `upstream: { scheme: 'http', type: 'roundrobin', ... }`, which the Admin API rejects because an upstream needs nodes or discovery. A plugins-only route (the public-api metrics example) could not be created, and one created through the Admin API could not be edit-saved. `produceRmEmptyUpstreamFields`, shared by all three submit pipelines, now drops an inline upstream that holds nothing but those mount-time defaults; anything the user supplied keeps it in the request for the Admin API to validate. The bug was invisible to `vite dev` and to the e2e suite: both ran development React, under whose StrictMode double effects the shouldUnregister cleanup wiped the mount-time values again. The e2e image was built with NODE_ENV=test, which makes Vite bundle development React; it now builds with `pnpm build --mode test` so the tests run on the production React the release ships, and the Monaco test seam is keyed on the Vite mode instead of NODE_ENV. Fixes #3395 --- e2e/server/Dockerfile | 8 +- .../routes.plugins-only-no-upstream.spec.ts | 172 +++++++++++++ .../form-slice/FormPartUpstream/util.test.ts | 270 +++++++++++++++++++++ src/components/form-slice/FormPartUpstream/util.ts | 72 +++++- src/components/form/Editor.tsx | 8 +- 5 files changed, 523 insertions(+), 7 deletions(-) diff --git a/e2e/server/Dockerfile b/e2e/server/Dockerfile index 06f6d84c4..cd182a350 100644 --- a/e2e/server/Dockerfile +++ b/e2e/server/Dockerfile @@ -28,13 +28,17 @@ RUN \ # build ui FROM base AS builder -ENV NODE_ENV=test WORKDIR /app COPY --from=deps /app/node_modules ./node_modules COPY . . # we need git to get commit sha RUN apk add --no-cache git -RUN corepack enable pnpm && pnpm build +# `--mode test` (not NODE_ENV=test): the tests must run against the same +# production React the release ships. NODE_ENV=test made Vite bundle +# development React, whose StrictMode double effects wiped the form +# widgets' mount-time values and hid #3395 from every e2e run; the Vite +# mode still hides the devtools and enables the Monaco test seam. +RUN corepack enable pnpm && pnpm build --mode test # embed dashboard ui into apisix FROM apache/apisix:dev diff --git a/e2e/tests/regression/routes.plugins-only-no-upstream.spec.ts b/e2e/tests/regression/routes.plugins-only-no-upstream.spec.ts new file mode 100644 index 000000000..9434156d3 --- /dev/null +++ b/e2e/tests/regression/routes.plugins-only-no-upstream.spec.ts @@ -0,0 +1,172 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Regression: a plugins-only route (no upstream at all) must be creatable +// and editable from the Dashboard. +// Related issue: +// - apache/apisix-dashboard#3395 bind the route to the public-api plugin, +// upstream is a required option in the dashboard, but it is not +// actually needed +// The Admin API accepts a route with only `uri` + `plugins.public-api` +// (the official public-api example exposes /apisix/prometheus/metrics that +// way). The Dashboard's Route form always renders the inline upstream +// section, and its Select controls register defaults (scheme / type / +// hash_on / pass_host) the moment they mount — so an untouched upstream +// section must NOT turn into a half-built `upstream` object in the request +// body, which the Admin API rejects (upstream needs nodes or discovery). + +import { routesPom } from '@e2e/pom/routes'; +import { randomId } from '@e2e/utils/common'; +import { e2eReq } from '@e2e/utils/req'; +import { test } from '@e2e/utils/test'; +import { + uiFillMonacoEditor, + uiGetMonacoEditor, + uiGoto, + uiHasToastMsg, +} from '@e2e/utils/ui'; +import { expect, type Page, type Request } from '@playwright/test'; + +import { deleteAllRoutes, getRouteReq, putRouteReq } from '@/apis/routes'; +import { API_PREFIX, API_ROUTES } from '@/config/constant'; +import type { APISIXType } from '@/types/schema/apisix'; + +const pluginName = 'public-api'; + +const captureRouteWrites = (page: Page) => { + const bodies: Record<string, unknown>[] = []; + const isRouteWrite = (req: Request) => + ['POST', 'PUT', 'PATCH'].includes(req.method()) && + req.url().includes(`${API_PREFIX}${API_ROUTES}`); + page.on('request', (req) => { + if (isRouteWrite(req)) { + bodies.push(req.postDataJSON() as Record<string, unknown>); + } + }); + return bodies; +}; + +test.beforeAll(async () => { + await deleteAllRoutes(e2eReq); +}); + +test.afterAll(async () => { + await deleteAllRoutes(e2eReq); +}); + +test('add page: a route with only uri + public-api and no upstream is accepted', async ({ + page, +}) => { + const routeName = randomId('reg-plugins-only'); + const routeUri = `/regression/plugins-only/${routeName}`; + const bodies = captureRouteWrites(page); + + await routesPom.toAdd(page); + await routesPom.isAddPage(page); + + await test.step('fill name, uri and the public-api plugin only', async () => { + await page.getByLabel('Name', { exact: true }).first().fill(routeName); + await page.getByLabel('URI', { exact: true }).fill(routeUri); + + await page.getByRole('button', { name: 'Select Plugins' }).click(); + const selectPluginsDialog = page.getByRole('dialog', { + name: 'Select Plugins', + }); + await selectPluginsDialog.getByPlaceholder('Search').fill(pluginName); + await selectPluginsDialog + .getByTestId(`plugin-${pluginName}`) + .getByRole('button', { name: 'Add' }) + .click(); + + const addPluginDialog = page.getByRole('dialog', { name: 'Add Plugin' }); + const editor = await uiGetMonacoEditor(page, addPluginDialog); + await uiFillMonacoEditor(page, editor, '{}'); + await addPluginDialog.getByRole('button', { name: 'Add' }).click(); + await expect(addPluginDialog).toBeHidden(); + await expect( + page + .getByRole('group', { name: 'Plugins' }) + .getByTestId(`plugin-${pluginName}`) + ).toBeVisible(); + }); + + await test.step('submit succeeds and lands on the detail page', async () => { + await routesPom.getAddBtn(page).click(); + await uiHasToastMsg(page, { hasText: 'Add Route Successfully' }); + await routesPom.isDetailPage(page); + }); + + await test.step('request body carried no upstream object', async () => { + expect(bodies).toHaveLength(1); + expect(bodies[0]).not.toHaveProperty('upstream'); + expect(bodies[0]).not.toHaveProperty('upstream_id'); + expect(bodies[0]).toHaveProperty(['plugins', pluginName]); + }); + + await test.step('Admin API stored a plugins-only route', async () => { + const routeId = page.url().split('/').pop()!; + const route = (await getRouteReq(e2eReq, routeId)) + .value as APISIXType['Route']; + expect(route.uri).toBe(routeUri); + expect(route.plugins).toHaveProperty(pluginName); + expect(route.upstream).toBeUndefined(); + expect(route.upstream_id).toBeUndefined(); + expect(route.service_id).toBeUndefined(); + }); +}); + +test('edit page: a no-op save keeps an API-created plugins-only route upstream-free', async ({ + page, +}) => { + const routeName = randomId('reg-plugins-only-edit'); + const routeUri = `/regression/plugins-only-edit/${routeName}`; + const res = await putRouteReq(e2eReq, { + id: routeName, + name: routeName, + uri: routeUri, + plugins: { [pluginName]: {} }, + } as APISIXType['Route']); + const routeId = res.data.value.id; + const bodies = captureRouteWrites(page); + + await uiGoto(page, '/routes/detail/$id', { id: routeId }); + await routesPom.isDetailPage(page); + + await test.step('enter edit mode and save without changes', async () => { + await page.getByRole('button', { name: 'Edit' }).click(); + await expect( + page.getByLabel('Name', { exact: true }).first() + ).toBeEnabled(); + await page.getByRole('button', { name: 'Save' }).click(); + await uiHasToastMsg(page, { hasText: 'success' }); + await routesPom.isDetailPage(page); + }); + + await test.step('request body carried no upstream object', async () => { + expect(bodies).toHaveLength(1); + expect(bodies[0]).not.toHaveProperty('upstream'); + expect(bodies[0]).toHaveProperty(['plugins', pluginName]); + }); + + await test.step('Admin API route is still plugins-only', async () => { + const route = (await getRouteReq(e2eReq, routeId)) + .value as APISIXType['Route']; + expect(route.uri).toBe(routeUri); + expect(route.plugins).toHaveProperty(pluginName); + expect(route.upstream).toBeUndefined(); + }); +}); diff --git a/src/components/form-slice/FormPartUpstream/util.test.ts b/src/components/form-slice/FormPartUpstream/util.test.ts new file mode 100644 index 000000000..af592435a --- /dev/null +++ b/src/components/form-slice/FormPartUpstream/util.test.ts @@ -0,0 +1,270 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { describe, expect, it } from 'vitest'; + +import { produceRoute } from '@/components/form-slice/FormPartRoute/util'; +import { produceStreamRoute } from '@/components/form-slice/FormPartStreamRoute/util'; +import { produceRmUpstreamWhenHas } from '@/utils/form-producer'; +import { pipeProduce } from '@/utils/producer'; + +import { isUntouchedInlineUpstream, produceRmEmptyUpstreamFields } from './util'; + +// Regression for apache/apisix-dashboard#3395. +// +// The inline upstream section of the route / service / stream-route forms is +// always rendered, and its Select / Switch / JSON widgets register a value +// the moment they mount: scheme=http, type=roundrobin, hash_on=vars, +// pass_host=pass, tls.verify=false, discovery_args={}. react-hook-form keeps +// those in the submit values, so a form whose upstream section the user +// never opened still submitted `upstream: { scheme, type, hash_on, ... }` — +// an upstream with neither nodes nor discovery, which the Admin API rejects: +// property "upstream" validation failed: value should match only one +// schema, but matches none +// A plugins-only route (the public-api example) could not be created, and a +// route created that way through the Admin API could not be edit-saved. +// +// `vite dev` and the NODE_ENV=test e2e build never showed this: under React +// StrictMode the double-invoked controller effects run the shouldUnregister +// cleanup, which wipes the mount-time values again. Only a production build +// (the one shipped inside APISIX) sends them. + +/** exactly what react-hook-form holds for a never-opened upstream section */ +const untouchedInlineUpstream = () => ({ + name: '', + desc: '', + labels: {}, + service_name: '', + discovery_type: '', + discovery_args: {}, + nodes: undefined, + scheme: 'http', + type: 'roundrobin', + hash_on: 'vars', + key: '', + pass_host: 'pass', + upstream_host: '', + retries: undefined, + retry_timeout: undefined, + timeout: { connect: undefined, send: undefined, read: undefined }, + keepalive_pool: { + size: undefined, + idle_timeout: undefined, + requests: undefined, + }, + tls: { verify: false, client_cert: '', client_key: '', client_cert_id: '' }, +}); + +/** the whole Add Route form, submitted with only name, uri and a plugin */ +const pluginsOnlyRouteForm = () => ({ + name: 'metrics', + desc: '', + labels: {}, + status: 1, + priority: 0, + methods: [], + enable_websocket: false, + uri: '/apisix/prometheus/metrics', + uris: [], + host: '', + hosts: [], + remote_addr: '', + remote_addrs: [], + vars: '', + filter_func: '', + service_id: '', + timeout: { connect: undefined, send: undefined, read: undefined }, + upstream_id: '', + upstream: untouchedInlineUpstream(), + plugin_config_id: '', + plugins: { 'public-api': {} }, + __checksEnabled: false, + __checksPassiveEnabled: false, +}); + +describe('isUntouchedInlineUpstream', () => { + it('is true for the values a never-opened upstream section holds', () => { + expect(isUntouchedInlineUpstream(untouchedInlineUpstream())).toBe(true); + }); + + it('is true when the widgets left only their defaults after cleaning', () => { + expect( + isUntouchedInlineUpstream({ + scheme: 'http', + type: 'roundrobin', + hash_on: 'vars', + pass_host: 'pass', + tls: { verify: false }, + discovery_args: {}, + nodes: [], + }) + ).toBe(true); + }); + + it.each([ + ['array nodes', { nodes: [{ host: 'a.local', port: 80, weight: 1 }] }], + ['object nodes', { nodes: { 'a.local:80': 1 } }], + // a node row the user added but has not filled in yet is still input — + // the Admin API must be the one to reject the empty host + ['an empty node row', { nodes: [{ host: '', port: 80, weight: 1 }] }], + ['discovery', { discovery_type: 'dns', service_name: 'svc' }], + ['a non-default scheme', { scheme: 'https' }], + ['a non-default balancer', { type: 'chash' }], + ['a non-default hash_on', { hash_on: 'header' }], + ['a non-default pass_host', { pass_host: 'node' }], + ['tls.verify switched on', { tls: { verify: true } }], + ['a client cert', { tls: { verify: false, client_cert: 'cert' } }], + ['a name', { name: 'my-upstream' }], + ['labels', { labels: { env: 'prod' } }], + ['a zero retries count', { retries: 0 }], + ['a timeout', { timeout: { connect: 3 } }], + ['a keepalive pool size', { keepalive_pool: { size: 320 } }], + ['health checks', { checks: { active: { type: 'http' } } }], + ['discovery args', { discovery_args: { namespace: 'x' } }], + ])('is false once the user supplied %s', (_, touched) => { + expect( + isUntouchedInlineUpstream({ ...untouchedInlineUpstream(), ...touched }) + ).toBe(false); + }); + + it.each([undefined, null, '', 'nodes', 1, []])( + 'is false for a non-object value (%s)', + (value) => { + expect(isUntouchedInlineUpstream(value)).toBe(false); + } + ); + + it('does not mutate its argument', () => { + const upstream = untouchedInlineUpstream(); + const before = JSON.stringify(upstream); + isUntouchedInlineUpstream(upstream); + expect(JSON.stringify(upstream)).toBe(before); + }); +}); + +describe('produceRmEmptyUpstreamFields', () => { + it('drops an inline upstream the user never touched', () => { + const out = produceRmEmptyUpstreamFields({ + uri: '/x', + upstream: untouchedInlineUpstream(), + }); + expect(out).not.toHaveProperty('upstream'); + expect(out.uri).toBe('/x'); + }); + + it('keeps an inline upstream with nodes, defaults included', () => { + const out = produceRmEmptyUpstreamFields({ + upstream: { + ...untouchedInlineUpstream(), + nodes: [{ host: 'a.local', port: 80, weight: 1 }], + }, + }); + expect(out.upstream).toMatchObject({ + scheme: 'http', + type: 'roundrobin', + hash_on: 'vars', + pass_host: 'pass', + nodes: [{ host: 'a.local', port: 80, weight: 1 }], + }); + }); + + it('keeps an inline upstream that only has discovery', () => { + const out = produceRmEmptyUpstreamFields({ + upstream: { + ...untouchedInlineUpstream(), + discovery_type: 'dns', + service_name: 'svc.local', + }, + }); + expect(out.upstream).toMatchObject({ + discovery_type: 'dns', + service_name: 'svc.local', + }); + }); + + it('keeps a touched but incomplete inline upstream for the Admin API to judge', () => { + // silently dropping it would discard what the user typed (#3417) + const out = produceRmEmptyUpstreamFields({ + upstream: { ...untouchedInlineUpstream(), scheme: 'https' }, + }); + expect(out.upstream).toMatchObject({ scheme: 'https' }); + }); + + it('leaves the root-level upstream form (upstreams page) alone', () => { + // the upstreams page submits these fields at the root, not under + // `upstream`; whether that is a valid upstream is the Admin API's call + const out = produceRmEmptyUpstreamFields(untouchedInlineUpstream()); + expect(out).toMatchObject({ scheme: 'http', type: 'roundrobin' }); + }); +}); + +describe('submit pipelines with an untouched inline upstream', () => { + it('route add/edit: sends a plugins-only route without any upstream', () => { + const body = pipeProduce( + produceRmEmptyUpstreamFields, + produceRoute + )(pluginsOnlyRouteForm()) as Record<string, unknown>; + expect(body).not.toHaveProperty('upstream'); + expect(body).not.toHaveProperty('upstream_id'); + expect(body).not.toHaveProperty('service_id'); + expect(body).toMatchObject({ + name: 'metrics', + uri: '/apisix/prometheus/metrics', + plugins: { 'public-api': {} }, + }); + }); + + it('route: a filled inline upstream still goes through', () => { + const form = pluginsOnlyRouteForm(); + form.upstream.nodes = [ + { host: 'a.local', port: 80, weight: 1 }, + ] as never; + const body = pipeProduce( + produceRmEmptyUpstreamFields, + produceRoute + )(form) as { upstream?: Record<string, unknown> }; + expect(body.upstream).toMatchObject({ + scheme: 'http', + nodes: [{ host: 'a.local', port: 80, weight: 1 }], + }); + }); + + it('service add/edit: a service with no upstream at all is sent without one', () => { + const body = pipeProduce( + produceRmUpstreamWhenHas('upstream_id'), + produceRmEmptyUpstreamFields + )({ + name: 'svc', + upstream_id: '', + upstream: untouchedInlineUpstream(), + plugins: {}, + }) as Record<string, unknown>; + expect(body).not.toHaveProperty('upstream'); + expect(body.name).toBe('svc'); + }); + + it('stream route add/edit: an untouched inline upstream is not sent', () => { + const body = produceStreamRoute({ + server_addr: '127.0.0.1', + server_port: 9100, + upstream_id: '', + service_id: '', + upstream: untouchedInlineUpstream(), + } as never) as Record<string, unknown>; + expect(body).not.toHaveProperty('upstream'); + expect(body.server_port).toBe(9100); + }); +}); diff --git a/src/components/form-slice/FormPartUpstream/util.ts b/src/components/form-slice/FormPartUpstream/util.ts index 748118118..c7d353b88 100644 --- a/src/components/form-slice/FormPartUpstream/util.ts +++ b/src/components/form-slice/FormPartUpstream/util.ts @@ -14,10 +14,11 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -import { produce } from 'immer'; -import { isNotEmpty } from 'rambdax'; +import { current, isDraft, produce } from 'immer'; +import { equals, isNotEmpty } from 'rambdax'; -import type { APISIXType } from '@/types/schema/apisix'; +import { APISIX, type APISIXType } from '@/types/schema/apisix'; +import { deepCleanEmptyKeys, rmDoubleUnderscoreKeys } from '@/utils/producer'; import type { FormPartUpstreamType } from './schema'; @@ -63,6 +64,62 @@ const isAllUndefined = (obj: Record<string, unknown>) => (v) => v === undefined || v === null || v === '' || Number.isNaN(v) ); +/** + * What the inline upstream widgets register the moment they mount, before + * the user has opened the section at all: FormItemScheme, + * FormSectionLoadbalancing and FormSectionPassHost pass the first schema + * option as their Select default, and the TLS `verify` Switch falls back to + * false. Every other widget mounts empty (`''`, `undefined`, `{}`, `[]`), + * which the cleaner already strips. + * + * Keep in step with those widgets: a new widget that registers a non-empty + * default must be listed here, or an untouched section is sent again. + */ +const INLINE_UPSTREAM_MOUNT_DEFAULTS: Record<string, unknown> = { + scheme: APISIX.UpstreamSchemeL7.options[0].value, + type: APISIX.UpstreamBalancer.options[0].value, + hash_on: APISIX.UpstreamHashOn.options[0].value, + pass_host: APISIX.UpstreamPassHost.options[0].value, + tls: { verify: false }, +}; + +/** + * True when an inline `upstream` holds nothing but what its widgets + * registered on mount (#3395). + * + * The route / service / stream-route forms always render the inline upstream + * section, so react-hook-form submits `upstream: { scheme: 'http', type: + * 'roundrobin', hash_on: 'vars', pass_host: 'pass', tls: { verify: false } }` + * for a section the user never opened. The Admin API rejects that — an + * upstream needs nodes or discovery — so a plugins-only route (public-api) + * could not be created, and one created through the Admin API could not be + * edit-saved. Dropping such an upstream loses nothing: every value in it is + * the gateway default anyway. + * + * Anything the user did supply (a node row, even an empty one; a different + * scheme; a name; …) makes this false, and the upstream is sent as is for the + * Admin API to judge — silently discarding input is the #3417 failure mode. + */ +export const isUntouchedInlineUpstream = (upstream: unknown): boolean => { + if (!upstream || typeof upstream !== 'object' || Array.isArray(upstream)) { + return false; + } + // read-only: a plain snapshot of a (possibly nested) immer draft, cleaned + // into a fresh object so the caller's value is never touched + const plain = isDraft(upstream) ? current(upstream) : upstream; + const cleaned = deepCleanEmptyKeys(plain as Record<string, unknown>, { + cleanInPlace: false, + }) as Record<string, unknown>; + rmDoubleUnderscoreKeys(cleaned); + return Object.entries(cleaned).every( + ([key, value]) => + Object.prototype.hasOwnProperty.call( + INLINE_UPSTREAM_MOUNT_DEFAULTS, + key + ) && equals(INLINE_UPSTREAM_MOUNT_DEFAULTS[key], value) + ); +}; + export const produceRmEmptyUpstreamFields = produce( ( draft: { @@ -73,7 +130,7 @@ export const produceRmEmptyUpstreamFields = produce( timeout?: Record<string, unknown>; keepalive_pool?: Record<string, unknown>; tls?: Record<string, unknown>; - }; + } & Record<string, unknown>; } & Record<string, unknown> ) => { if (draft.timeout && isAllUndefined(draft.timeout)) { @@ -110,6 +167,13 @@ export const produceRmEmptyUpstreamFields = produce( if (draft.upstream.tls && isAllUndefined(draft.upstream.tls)) { delete draft.upstream.tls; } + + // the section was never opened: only the widgets' mount-time defaults + // are left, and the Admin API rejects an upstream with neither nodes + // nor discovery — send no upstream at all (#3395) + if (isUntouchedInlineUpstream(draft.upstream)) { + delete draft.upstream; + } } } ); diff --git a/src/components/form/Editor.tsx b/src/components/form/Editor.tsx index 4c6fb47c2..b59d12e67 100644 --- a/src/components/form/Editor.tsx +++ b/src/components/form/Editor.tsx @@ -163,7 +163,13 @@ export const FormItemEditor = <T extends FieldValues>( trigger(props.name); }} onMount={(editor) => { - if (process.env.NODE_ENV !== 'production') { + // test seam for the Playwright Monaco helpers. Keyed on the Vite + // mode, not NODE_ENV: the e2e image is a production build + // (`vite build --mode test`) so it runs the same React the + // release ships — a NODE_ENV=test build bundled development + // React, whose StrictMode double effects hid submit-body bugs + // (#3395) + if (import.meta.env.DEV || import.meta.env.MODE === 'test') { window.__monacoEditor__ = editor; } }}
