Preserve distinctEndpoint and metadataMapping when editing K8s endpoint aggregators

ikhoon · ikhoon · commit 33eb3367c1f0 · 2026-07-30T21:48:00.000+09:00
Motivation:
The xDS Kubernetes endpoint aggregator editor rebuilds the aggregator YAML from its form
model on every save. Because the form modeled only a subset of the schema, saving from the
console silently dropped distinctEndpoint, metadataMapping, and the aggregator-level policy
- fields the form has no editor for. An operator who edited an aggregator through the
console would unknowingly discard them; losing distinctEndpoint, for example, re-introduces
duplicate endpoints on the next rolling restart.

Modifications:
- Add distinctEndpoint to the watcher form as a checkbox next to "Trust certificates", read
  in parseToFormData and written in buildBody.
- Preserve metadataMapping verbatim across the load/save round-trip (carried as a
  react-hook-form field-array item property) and show a read-only note with the number of
  configured rules, since the form has no editor for them.
- Preserve the aggregator-level policy verbatim.
- Guard the new emissions so aggregators without these fields serialize exactly as before.
- Add regression tests that load an aggregator carrying these fields, perform an edit, and
  assert the committed YAML still contains them, including a multi-watcher removal case that
  verifies each mapping stays with its own watcher.

Result:
Editing a Kubernetes endpoint aggregator through the web console no longer drops
distinctEndpoint, metadataMapping, or policy. distinctEndpoint is now viewable and editable
in the form.
diff --git a/webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx b/webapp/src/dogma/features/xds/K8sAggregatorEditor.tsx
@@ -39,7 +39,15 @@ import * as jsYaml from 'js-yaml';
 import { default as RouteLink } from 'next/link';
 import Router from 'next/router';
 import { useEffect, useState } from 'react';
-import { Control, Controller, FieldErrors, useFieldArray, useForm, UseFormRegister } from 'react-hook-form';
+import {
+  Control,
+  Controller,
+  FieldErrors,
+  useFieldArray,
+  useForm,
+  UseFormRegister,
+  useWatch,
+} from 'react-hook-form';
 import { OptionBase, Select } from 'chakra-react-select';
 import { AiOutlineClose, AiOutlineDelete, AiOutlineEdit, AiOutlineEye } from 'react-icons/ai';
 import { FiSave } from 'react-icons/fi';
@@ -72,24 +80,41 @@ interface PropertyForm {
   value: string;
 }
 
+// A ServiceEndpointWatcher.metadata_mapping entry. The editor does not expose a UI for editing these yet,
+// but carries them verbatim so a Save from the form cannot silently drop them (see parseToFormData/buildBody).
+interface MetadataMappingForm {
+  resourceType?: string;
+  entryType?: string;
+  sourceKey?: string;
+  sourceKeyPrefix?: string;
+  metadataNamespace?: string;
+  metadataKey?: string;
+}
+
 interface WatcherForm {
   serviceName: string;
   portName: string;
   controlPlaneUrl: string;
   namespace: string;
   credentialId: string;
   trustCerts: boolean;
+  distinctEndpoint: boolean;
   priority: string;
   loadBalancingWeight: string;
   region: string;
   zone: string;
   subZone: string;
   additionalProperties: PropertyForm[];
+  // Preserved as-is across a load/save round-trip; not editable in the form.
+  metadataMapping: MetadataMappingForm[];
 }
 
 interface FormData {
   aggregatorId: string;
   watchers: WatcherForm[];
+  // The aggregator-level ClusterLoadAssignment policy. The form has no editor for it; it is preserved as-is so
+  // a Save cannot silently drop it (see parseToFormData/buildBody).
+  policy?: unknown;
 }
 
 const emptyWatcher: WatcherForm = {
@@ -99,12 +124,14 @@ const emptyWatcher: WatcherForm = {
   namespace: '',
   credentialId: '',
   trustCerts: false,
+  distinctEndpoint: false,
   priority: '',
   loadBalancingWeight: '',
   region: '',
   zone: '',
   subZone: '',
   additionalProperties: [],
+  metadataMapping: [],
 };
 
 // Parses a numeric form field, rejecting non-numeric input instead of silently serializing it as null
@@ -142,6 +169,14 @@ function buildBody(data: FormData, name?: string): string {
     if (Object.keys(additionalProperties).length > 0) {
       watcher.additionalProperties = additionalProperties;
     }
+    if (w.distinctEndpoint) {
+      watcher.distinctEndpoint = true;
+    }
+    // Re-emit metadata_mapping verbatim. The form has no editor for it, so dropping it here would silently
+    // discard the mappings whenever the aggregator is saved from the console.
+    if (w.metadataMapping && w.metadataMapping.length > 0) {
+      watcher.metadataMapping = w.metadataMapping;
+    }
     const entry: Record<string, unknown> = { watcher };
     const locality: Record<string, string> = {};
     if (w.region.trim()) {
@@ -165,6 +200,11 @@ function buildBody(data: FormData, name?: string): string {
     return entry;
   });
   const body: Record<string, unknown> = { localityLbEndpoints };
+  // Re-emit the aggregator-level policy verbatim; the form does not model it, so omitting it here would drop it
+  // from the stored file on every Save.
+  if (data.policy !== undefined && data.policy !== null) {
+    body.policy = data.policy;
+  }
   if (name) {
     body.name = name;
   }
@@ -189,6 +229,7 @@ function parseToFormData(aggregatorId: string, raw: any): FormData {
       namespace: e?.watcher?.kubeconfig?.namespace ?? '',
       credentialId: e?.watcher?.kubeconfig?.credentialId ?? '',
       trustCerts: !!e?.watcher?.kubeconfig?.trustCerts,
+      distinctEndpoint: !!e?.watcher?.distinctEndpoint,
       priority: e?.priority != null ? String(e.priority) : '',
       loadBalancingWeight: e?.loadBalancingWeight != null ? String(e.loadBalancingWeight) : '',
       region: e?.locality?.region ?? '',
@@ -198,9 +239,14 @@ function parseToFormData(aggregatorId: string, raw: any): FormData {
         key,
         value: String(value),
       })),
+      metadataMapping: Array.isArray(e?.watcher?.metadataMapping) ? e.watcher.metadataMapping : [],
     }),
   );
-  return { aggregatorId, watchers: watchers.length > 0 ? watchers : [{ ...emptyWatcher }] };
+  return {
+    aggregatorId,
+    watchers: watchers.length > 0 ? watchers : [{ ...emptyWatcher }],
+    policy: content?.policy,
+  };
 }
 
 interface CredentialOption extends OptionBase {
@@ -238,6 +284,9 @@ const WatcherFields = ({
     control,
     name: `watchers.${index}.additionalProperties` as `watchers.${number}.additionalProperties`,
   });
+  // metadata_mapping is preserved but not editable here; surface its presence so the carried value is visible.
+  const metadataMapping = useWatch({ control, name: `watchers.${index}.metadataMapping` });
+  const metadataMappingCount = Array.isArray(metadataMapping) ? metadataMapping.length : 0;
   return (
     <Box borderWidth="1px" borderRadius="md" p={4} mb={4} maxW="3xl">
       <Flex mb={2} align="center">
@@ -349,6 +398,11 @@ const WatcherFields = ({
             Trust certificates
           </Checkbox>
         </FormControl>
+        <FormControl display="flex" alignItems="center" pt={6}>
+          <Checkbox isReadOnly={readOnly} {...register(`watchers.${index}.distinctEndpoint`)}>
+            Distinct endpoint
+          </Checkbox>
+        </FormControl>
       </SimpleGrid>
 
       <Text mt={4} mb={1} fontSize="sm" fontWeight="semibold" color="gray.500">
@@ -423,6 +477,12 @@ const WatcherFields = ({
           Add property
         </Button>
       )}
+      {metadataMappingCount > 0 && (
+        <Text mt={3} fontSize="xs" color="gray.500">
+          {metadataMappingCount} metadata mapping rule(s) configured for this watcher — preserved on save (not
+          editable in this form).
+        </Text>
+      )}
     </Box>
   );
 };
diff --git a/webapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx b/webapp/tests/dogma/features/xds/K8sAggregatorEditor.test.tsx
@@ -191,3 +191,197 @@ describe('K8sAggregatorEditor – aggregator ID pattern validation', () => {
     });
   });
 });
+
+describe('K8sAggregatorEditor – round-trips fields the form has no editor for', () => {
+  // Metadata mapping in its fuller shape: an exact-key rule and a prefix rule carrying namespace + key. These
+  // must survive a load→save round-trip verbatim even though the form cannot edit them.
+  const METADATA_MAPPING = [
+    { resourceType: 'NODE', entryType: 'LABEL', sourceKey: 'topology.kubernetes.io/zone' },
+    {
+      resourceType: 'POD',
+      entryType: 'ANNOTATION',
+      sourceKeyPrefix: 'topology.kubernetes.io/',
+      metadataNamespace: 'envoy.lb',
+      metadataKey: 'zone',
+    },
+  ];
+  // A watcher carrying distinctEndpoint + metadataMapping, plus an aggregator-level policy — all fields the form
+  // does not model. Before the fix, saving from the form silently dropped every one of them.
+  const ADVANCED_CONTENT = {
+    policy: { overprovisioningFactor: 140 },
+    localityLbEndpoints: [
+      {
+        watcher: {
+          serviceName: 'my-service',
+          kubeconfig: { controlPlaneUrl: 'https://kubernetes.default.svc' },
+          additionalProperties: { nodeIpLabel: 'private-ip' },
+          distinctEndpoint: true,
+          metadataMapping: METADATA_MAPPING,
+        },
+      },
+    ],
+  };
+
+  let mockUpdate: jest.Mock;
+
+  const setupMocks = (content: unknown) => {
+    mockUpdate = jest.fn().mockReturnValue({ unwrap: () => Promise.resolve({}) });
+    jest
+      .mocked(xdsApiSlice.useCreateK8sAggregatorMutation)
+      .mockReturnValue([
+        jest.fn().mockReturnValue({ unwrap: () => Promise.resolve({}) }),
+        { isLoading: false },
+      ] as any);
+    jest
+      .mocked(xdsApiSlice.useUpdateK8sAggregatorMutation)
+      .mockReturnValue([mockUpdate, { isLoading: false }] as any);
+    jest
+      .mocked(xdsApiSlice.useDeleteK8sAggregatorMutation)
+      .mockReturnValue([jest.fn(), { isLoading: false }] as any);
+    jest
+      .mocked(xdsApiSlice.usePreviewK8sAggregatorMutation)
+      .mockReturnValue([jest.fn(), { isLoading: false }] as any);
+    jest.mocked(xdsApiSlice.useGetK8sAggregatorQuery).mockReturnValue({
+      data: { content: jsYaml.dump(content) },
+      isLoading: false,
+      error: undefined,
+    } as any);
+    jest.mocked(xdsApiSlice.useListCredentialsQuery).mockReturnValue({ data: [], error: null } as any);
+  };
+
+  beforeEach(() => {
+    setupMocks(ADVANCED_CONTENT);
+  });
+
+  const savedBody = () => jsYaml.load(mockUpdate.mock.calls[0][0].body) as any;
+  const savedWatcher = () => savedBody().localityLbEndpoints[0].watcher;
+
+  it('preserves distinctEndpoint, metadataMapping, and policy across an unrelated edit', async () => {
+    const user = userEvent.setup();
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('my-agg')).toBeInTheDocument());
+
+    await user.click(screen.getByRole('button', { name: /^edit$/i }));
+    // Make a genuinely unrelated change (rename the service) so the preserved fields ride through a real edit.
+    const serviceInput = screen.getByDisplayValue('my-service');
+    await user.clear(serviceInput);
+    await user.type(serviceInput, 'renamed-service');
+    await user.click(screen.getByRole('button', { name: /^save$/i }));
+
+    await waitFor(() => expect(mockUpdate).toHaveBeenCalled());
+    const watcher = savedWatcher();
+    expect(watcher.serviceName).toBe('renamed-service');
+    expect(watcher.distinctEndpoint).toBe(true);
+    // The full metadata_mapping shape (both rules, all sub-fields) is preserved verbatim.
+    expect(watcher.metadataMapping).toEqual(METADATA_MAPPING);
+    expect(savedBody().policy).toEqual({ overprovisioningFactor: 140 });
+  });
+
+  it('reflects the stored Distinct endpoint value and lets the user turn it off while preserving metadataMapping', async () => {
+    const user = userEvent.setup();
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('my-agg')).toBeInTheDocument());
+
+    expect(screen.getByRole('checkbox', { name: /distinct endpoint/i })).toBeChecked();
+
+    await user.click(screen.getByRole('button', { name: /^edit$/i }));
+    await user.click(screen.getByRole('checkbox', { name: /distinct endpoint/i }));
+    await user.click(screen.getByRole('button', { name: /^save$/i }));
+
+    await waitFor(() => expect(mockUpdate).toHaveBeenCalled());
+    const watcher = savedWatcher();
+    // Turned off → omitted from the committed body …
+    expect(watcher.distinctEndpoint).toBeUndefined();
+    // … while the metadata_mapping the form never touched is still preserved.
+    expect(watcher.metadataMapping).toEqual(METADATA_MAPPING);
+  });
+
+  it('writes distinctEndpoint when enabled on an aggregator that lacked it, and emits no policy key', async () => {
+    setupMocks({
+      localityLbEndpoints: [
+        {
+          watcher: {
+            serviceName: 'my-service',
+            kubeconfig: { controlPlaneUrl: 'https://kubernetes.default.svc' },
+          },
+        },
+      ],
+    });
+    const user = userEvent.setup();
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('my-agg')).toBeInTheDocument());
+
+    expect(screen.getByRole('checkbox', { name: /distinct endpoint/i })).not.toBeChecked();
+
+    await user.click(screen.getByRole('button', { name: /^edit$/i }));
+    await user.click(screen.getByRole('checkbox', { name: /distinct endpoint/i }));
+    await user.click(screen.getByRole('button', { name: /^save$/i }));
+
+    await waitFor(() => expect(mockUpdate).toHaveBeenCalled());
+    expect(savedWatcher().distinctEndpoint).toBe(true);
+    // An aggregator that had no policy must not gain one (no diff noise / no schema change).
+    expect(savedBody()).not.toHaveProperty('policy');
+  });
+
+  it('keeps each metadataMapping with its own watcher after removing another watcher', async () => {
+    setupMocks({
+      localityLbEndpoints: [
+        {
+          watcher: {
+            serviceName: 'svc-1',
+            kubeconfig: { controlPlaneUrl: 'https://kubernetes.default.svc' },
+            distinctEndpoint: true,
+            metadataMapping: [{ resourceType: 'NODE', entryType: 'LABEL', sourceKey: 'zone-1' }],
+          },
+        },
+        {
+          watcher: {
+            serviceName: 'svc-2',
+            kubeconfig: { controlPlaneUrl: 'https://kubernetes.default.svc' },
+            metadataMapping: [{ resourceType: 'POD', entryType: 'ANNOTATION', sourceKey: 'rack-2' }],
+          },
+        },
+      ],
+    });
+    const user = userEvent.setup();
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('svc-1')).toBeInTheDocument());
+
+    await user.click(screen.getByRole('button', { name: /^edit$/i }));
+    // Remove the first watcher; the second must keep its own mapping and not inherit the first's.
+    await user.click(screen.getAllByRole('button', { name: /^remove$/i })[0]);
+    await user.click(screen.getByRole('button', { name: /^save$/i }));
+
+    await waitFor(() => expect(mockUpdate).toHaveBeenCalled());
+    const endpoints = savedBody().localityLbEndpoints;
+    expect(endpoints).toHaveLength(1);
+    expect(endpoints[0].watcher.serviceName).toBe('svc-2');
+    expect(endpoints[0].watcher.metadataMapping).toEqual([
+      { resourceType: 'POD', entryType: 'ANNOTATION', sourceKey: 'rack-2' },
+    ]);
+    // svc-2 had no distinctEndpoint; removing svc-1 must not leak svc-1's onto it.
+    expect(endpoints[0].watcher.distinctEndpoint).toBeUndefined();
+  });
+
+  it('surfaces preserved metadata mapping rules as a read-only note', async () => {
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('my-agg')).toBeInTheDocument());
+    expect(screen.getByText(/2 metadata mapping rule/i)).toBeInTheDocument();
+  });
+
+  it('shows no metadata mapping note when the watcher has none', async () => {
+    setupMocks({
+      localityLbEndpoints: [
+        {
+          watcher: {
+            serviceName: 'my-service',
+            kubeconfig: { controlPlaneUrl: 'https://kubernetes.default.svc' },
+          },
+        },
+      ],
+    });
+    renderWithProviders(<K8sAggregatorEditor group="foo" id="my-agg" isNew={false} />);
+    await waitFor(() => expect(screen.getByDisplayValue('my-agg')).toBeInTheDocument());
+    expect(screen.queryByText(/metadata mapping rule/i)).not.toBeInTheDocument();
+  });
+});
