Skip to content

fix(cli): enforce exact label selector matching #418

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.

Sign up for GitHub

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

Closed
LiteSun wants to merge 4 commits into from
Closed

fix(cli): enforce exact label selector matching #418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa0f8bb
fix(cli): enforce exact label selector matching
LiteSun Mar 13, 2026
d8ee8b6
f
LiteSun Mar 13, 2026
2c1d0c5
f
LiteSun Mar 13, 2026
4be87bf
f
LiteSun Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions apps/cli/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
"@types/lodash": "catalog:",
"@types/supertest": "^6.0.3",
"@types/pluralize": "^0.0.33",
"@types/qs": "^6.9.15",
"@types/signale": "^1.4.7",
"@types/semver": "catalog:",
"supertest": "^7.1.4",
Expand All @@ -33,7 +32,6 @@
"commander": "^13.1.0",
"chalk": "^4.1.2",
"parse-duration": "^2.1.5",
"qs": "^6.14.1",
"dotenv": "^16.4.5",
"source-map-support": "^0.5.21",
"agentkeepalive": "^4.6.0",
Expand Down
50 changes: 46 additions & 4 deletions apps/cli/src/command/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,55 @@ import { has } from 'lodash';
import { existsSync } from 'node:fs';
import { resolve } from 'node:path';
import parseDuration from 'parse-duration';
import qs from 'qs';

export interface BaseOptions {
verbose: number;
}

export const parseLabelSelector = (
Copy link
Collaborator

@bzp2010 bzp2010 Mar 13, 2026

Choose a reason for hiding this comment

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

What is the difference from the old implementation based on qs?

Copy link
Contributor Author

@LiteSun LiteSun Mar 13, 2026

Choose a reason for hiding this comment

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

The main difference is that qs supports nested object notation—for example, a[b]=c is parsed as { a: { b: 'c' } } rather than { 'a[b]': 'c' }.
If a label value happens to contain [ or ] characters, qs would produce
an unexpected nested structure instead of a flat string value.

val: string,
previous: Record<string, string> = {},
) => {
const current = val.trim();
if (!current) {
throw new InvalidArgumentError(
'Label selector must be in the format key=value',
);
}

const parsed = current.split(',').reduce<Record<string, string>>(
(acc, pair) => {
const item = pair.trim();
const separatorIndex = item.indexOf('=');

if (
!item ||
separatorIndex <= 0 ||
separatorIndex === item.length - 1
) {
throw new InvalidArgumentError(
`Invalid label selector "${item}". Expected key=value`,
);
}

const key = item.slice(0, separatorIndex).trim();
const value = item.slice(separatorIndex + 1).trim();

if (!key || !value) {
throw new InvalidArgumentError(
`Invalid label selector "${item}". Expected key=value`,
);
}

acc[key] = value;
return acc;
},
{},
);

return Object.assign(previous, parsed);
};

export class BaseCommand<
OPTS extends BaseOptions = BaseOptions,
> extends Command {
Expand Down Expand Up @@ -129,9 +173,7 @@ export class BackendCommand<
new Option(
'--label-selector <labelKey=labelValue>',
'filter resources by labels',
).argParser((val, previous: Record<string, string> = {}) =>
Object.assign(previous, qs.parse(val, { delimiter: ',' })),
),
).argParser(parseLabelSelector),
)
.addOption(
new Option(
Expand Down
71 changes: 71 additions & 0 deletions apps/cli/src/command/utils.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import * as ADCSDK from '@api7/adc-sdk';
import { InvalidArgumentError } from 'commander';

import { parseLabelSelector } from './helper';
import {
fillLabels,
filterConfiguration,
recursiveRemoveMetadataField,
recursiveReplaceEnvVars,
} from './utils';
Expand Down Expand Up @@ -200,6 +203,74 @@ describe('CLI utils', () => {
});
});

it('should filter configuration by exact label key match', () => {
const [filtered, removed] = filterConfiguration(
{
services: [
{
name: 'match-by-name',
labels: { name: 'yanglao_wx_pgm' },
},
{
name: 'should-not-match-appname',
labels: { appname: 'yanglao_wx_pgm' },
},
{
name: 'should-not-match-different-value',
labels: { name: 'other' },
},
],
} as ADCSDK.Configuration,
{ name: 'yanglao_wx_pgm' },
);

expect(filtered.services).toEqual([
{
name: 'match-by-name',
labels: { name: 'yanglao_wx_pgm' },
},
]);
expect(removed.services).toEqual([
{
name: 'should-not-match-appname',
labels: { appname: 'yanglao_wx_pgm' },
},
{
name: 'should-not-match-different-value',
labels: { name: 'other' },
},
]);
});

it('should parse label selectors in key=value format only', () => {
expect(parseLabelSelector('name=yanglao_wx_pgm')).toEqual({
name: 'yanglao_wx_pgm',
});
expect(parseLabelSelector('release=2026-03-13T12:00:00Z')).toEqual({
release: '2026-03-13T12:00:00Z',
});
expect(parseLabelSelector('url=http://a')).toEqual({
url: 'http://a',
});
expect(
parseLabelSelector('name=yanglao_wx_pgm,team=adc', { env: 'prod' }),
).toEqual({
env: 'prod',
name: 'yanglao_wx_pgm',
team: 'adc',
});
});

it('should reject invalid label selector formats', () => {
expect(() => parseLabelSelector('name:yanglao_wx_pgm')).toThrow(
InvalidArgumentError,
);
expect(() => parseLabelSelector('name=')).toThrow(InvalidArgumentError);
expect(() => parseLabelSelector('=yanglao_wx_pgm')).toThrow(
InvalidArgumentError,
);
});

it('should remove metadata from dump result', () => {
const config = {
services: [
Expand Down
18 changes: 10 additions & 8 deletions apps/cli/src/server/sync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { omit, toString } from 'lodash';
import { lastValueFrom, toArray } from 'rxjs';

import {
fillLabels,
filterConfiguration,
filterResourceType,
loadBackend,
Expand Down Expand Up @@ -52,11 +51,15 @@ export const syncHandler: RequestHandler<

// load local configuration and validate it
//TODO: merged with the listr task
const local = filterResourceType(
let local = filterResourceType(
task.config,
task.opts.includeResourceType,
task.opts.excludeResourceType,
) as ADCSDK.InternalConfiguration;
[local] = filterConfiguration(
local,
task.opts.labelSelector,
) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
Comment on lines +59 to +62
Copy link

@coderabbitai coderabbitai bot Mar 13, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if filterConfiguration handles undefined/empty rules gracefully
ast-grep --pattern $'export const filterConfiguration = ($_, $_, $_) => {
  $$$
}'

Repository: api7/adc

Length of output: 34

🏁 Script executed:

# Find filterConfiguration function definition
rg 'export.*filterConfiguration\s*=' -A 10

Repository: api7/adc

Length of output: 900

🏁 Script executed:

# Read sync.ts file around the flagged lines
cat -n apps/cli/src/server/sync.ts | sed -n '50,115p'

Repository: api7/adc

Length of output: 2463

🏁 Script executed:

# Find load_local.ts and check the guard pattern
find . -name 'load_local.ts' -type f

Repository: api7/adc

Length of output: 86

🏁 Script executed:

# Get full filterConfiguration implementation
cat -n apps/cli/src/command/utils.ts | grep -A 30 'export const filterConfiguration'

Repository: api7/adc

Length of output: 1221

🏁 Script executed:

# Check the labelFilter function definition and implementation
rg 'export.*labelFilter\s*=|function labelFilter' -A 15

Repository: api7/adc

Length of output: 34

🏁 Script executed:

# Read load_local.ts to see the guard pattern mentioned in the review
cat -n apps/cli/src/tasks/load_local.ts

Repository: api7/adc

Length of output: 4301

Add guards before calling filterConfiguration for consistency with load_local.ts.

The code unconditionally calls filterConfiguration even when task.opts.labelSelector is undefined or empty. While labelFilter has a default value (rules = {}), this results in unnecessary iteration and is inconsistent with the guard pattern in load_local.ts (lines 101-104). Add the same guard to both calls in sync.ts.

Suggested fix 
-    [local] = filterConfiguration(
-      local,
-      task.opts.labelSelector,
-    ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
+    if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) {
+      [local] = filterConfiguration(
+        local,
+        task.opts.labelSelector,
+      ) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
+    }

Apply the same guard to line 107:

-    [remote] = filterConfiguration(remote, task.opts.labelSelector);
+    if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) {
+      [remote] = filterConfiguration(remote, task.opts.labelSelector);
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[local] = filterConfiguration(
local,
task.opts.labelSelector,
) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
if (task.opts.labelSelector && Object.keys(task.opts.labelSelector).length > 0) {
[local] = filterConfiguration(
local,
task.opts.labelSelector,
) as [ADCSDK.InternalConfiguration, ADCSDK.Configuration];
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@apps/cli/src/server/sync.ts` around lines 59 - 62, The call site in sync.ts
unconditionally invokes filterConfiguration on local with
task.opts.labelSelector; add the same guard used in load_local.ts so you only
call filterConfiguration when task.opts.labelSelector is set/non-empty (e.g.,
check task.opts.labelSelector truthiness or length) before assigning [local] =
filterConfiguration(...); update both places in sync.ts where
filterConfiguration is used to match the guarded pattern and ensure local
remains unchanged if the selector is absent.

if (task.opts.lint) {
const result = check(local);
if (!result.success)
Expand All @@ -65,7 +68,6 @@ export const syncHandler: RequestHandler<
errors: result.error.issues,
});
}
fillLabels(local, task.opts.labelSelector);

// load and filter remote configuration
const backend = loadBackend(task.opts.backend, {
Expand Down Expand Up @@ -134,10 +136,10 @@ export const syncHandler: RequestHandler<
task.opts.backend !== 'apisix-standalone'
? generateOutput([results, successes, faileds])
: generateOutputForAPISIXStandalone(diff, [
results,
successes,
faileds,
]);
results,
successes,
faileds,
]);

logger.log({
level: 'debug',
Expand Down Expand Up @@ -173,7 +175,7 @@ const formatAxiosResponse = (axiosResponse: AxiosResponse) => ({
method: axiosResponse.config.method,
headers:
((axiosResponse.config.headers['X-API-KEY'] = '*****'),
axiosResponse.config.headers),
axiosResponse.config.headers),
data: axiosResponse.config.data,
},
});
Expand Down
50 changes: 50 additions & 0 deletions apps/cli/src/tasks/load_local.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import * as ADCSDK from '@api7/adc-sdk';
import { dump } from 'js-yaml';
import { Listr } from 'listr2';
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
import { tmpdir } from 'node:os';
import path from 'node:path';

import { LoadLocalConfigurationTask } from './load_local';

describe('LoadLocalConfigurationTask', () => {
it('filters local resources by exact label key match', async () => {
const dir = mkdtempSync(path.join(tmpdir(), 'adc-load-local-'));
const file = path.join(dir, 'adc.yaml');

writeFileSync(
file,
dump({
services: [
{
name: 'match-by-name',
labels: { name: 'yanglao_wx_pgm' },
Copy link
Collaborator

@bzp2010 bzp2010 Mar 13, 2026

Choose a reason for hiding this comment

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

What does this name signify? Who are you representing in submitting this PR?

Please ensure you always use neutral, non-specific terminology for testing purposes.

Copy link
Contributor Author

@LiteSun LiteSun Mar 13, 2026

Choose a reason for hiding this comment

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

You're right. I'll replace yanglao_wx_pgm with neutral placeholder values (e.g., test-service, foo) across all test cases.

},
{
name: 'should-not-match-appname',
labels: { appname: 'yanglao_wx_pgm' },
},
],
} satisfies ADCSDK.Configuration),
'utf8',
);

const ctx: { local: ADCSDK.Configuration } = { local: {} };

try {
await new Listr(
[LoadLocalConfigurationTask([file], { name: 'yanglao_wx_pgm' })],
{ ctx },
).run();
} finally {
rmSync(dir, { recursive: true, force: true });
}

expect(ctx.local.services).toEqual([
{
name: 'match-by-name',
labels: { name: 'yanglao_wx_pgm' },
},
]);
});
});
5 changes: 2 additions & 3 deletions apps/cli/src/tasks/load_local.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { ListrTask } from 'listr2';
import path from 'node:path';

import {
fillLabels,
filterConfiguration,
filterResourceType,
mergeKVConfigurations,
recursiveReplaceEnvVars,
Expand Down Expand Up @@ -100,8 +100,7 @@ export const LoadLocalConfigurationTask = (
title: 'Filter configuration',
enabled: !!labelSelector && Object.keys(labelSelector).length > 0,
task: async () => {
// Merge label selectors from CLI inputs to each resource
fillLabels(ctx.local, labelSelector);
[ctx.local] = filterConfiguration(ctx.local, labelSelector);
Copy link
Collaborator

@bzp2010 bzp2010 Mar 13, 2026

Choose a reason for hiding this comment

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

I don't understand what you're trying to do. The original design was intentional—label filtering always occurs remotely. For local resources, the ADC will only populate the label fields you want to filter, not perform the filtering itself.

I believe this stance remains unchanged.

If the label filter fails to accurately search and filter resources remotely, you should improve the specific backend and require the backend API implementation to ensure its filters are precise rather than fuzzy.

},
},
],
Expand Down
Loading
Loading