From 7432b9b6bdf3b77d98c2099fafd38556f8c4158a Mon Sep 17 00:00:00 2001 From: Cristina Amico Date: Wed, 3 Nov 2021 17:41:11 +0100 Subject: [PATCH] [Fleet] Make integration names globally unique (#115212) * [Fleet] Make integration names globally unique * Fix Jest tests * Append (copy) to names of integration packages belonging to duplicated policy * Update current policy maintaining its name * Fix failing tests --- .../step_define_package_policy.tsx | 52 +++++++++++------- .../fleet/server/services/agent_policy.ts | 8 ++- .../fleet/server/services/package_policy.ts | 51 +++++++----------- .../apis/package_policy/create.ts | 54 +++++++++++++++++-- .../apis/package_policy/update.ts | 34 +++++++++++- 5 files changed, 141 insertions(+), 58 deletions(-) diff --git a/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx index e11aaabb4fd951..63e9ba64ad7534 100644 --- a/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx +++ b/x-pack/plugins/fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx @@ -22,16 +22,12 @@ import { import styled from 'styled-components'; -import type { - AgentPolicy, - PackageInfo, - PackagePolicy, - NewPackagePolicy, - RegistryVarsEntry, -} from '../../../types'; +import type { AgentPolicy, PackageInfo, NewPackagePolicy, RegistryVarsEntry } from '../../../types'; import { packageToPackagePolicy, pkgKeyFromPackageInfo } from '../../../services'; import { Loading } from '../../../components'; -import { useStartServices } from '../../../hooks'; +import { useStartServices, useGetPackagePolicies } from '../../../hooks'; +import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../../../../../constants'; +import { SO_SEARCH_LIMIT } from '../../../../../../common'; import { isAdvancedVar } from './services'; import type { PackagePolicyValidationResults } from './services'; @@ -65,6 +61,14 @@ export const StepDefinePackagePolicy: React.FunctionComponent<{ submitAttempted, }) => { const { docLinks } = useStartServices(); + + // Fetch all packagePolicies having the package name + const { data: packagePolicyData, isLoading: isLoadingPackagePolicies } = useGetPackagePolicies({ + perPage: SO_SEARCH_LIMIT, + page: 1, + kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.package.name:${packageInfo.name}`, + }); + // Form show/hide states const [isShowingAdvanced, setIsShowingAdvanced] = useState(false); @@ -84,33 +88,37 @@ export const StepDefinePackagePolicy: React.FunctionComponent<{ // Update package policy's package and agent policy info useEffect(() => { + if (isLoadingPackagePolicies) { + return; + } const pkg = packagePolicy.package; const currentPkgKey = pkg ? pkgKeyFromPackageInfo(pkg) : ''; const pkgKey = pkgKeyFromPackageInfo(packageInfo); // If package has changed, create shell package policy with input&stream values based on package info if (currentPkgKey !== pkgKey) { - // Existing package policies on the agent policy using the package name, retrieve highest number appended to package policy name + // Retrieve highest number appended to package policy name and increment it by one const pkgPoliciesNamePattern = new RegExp(`${packageInfo.name}-(\\d+)`); - const pkgPoliciesWithMatchingNames = agentPolicy - ? (agentPolicy.package_policies as PackagePolicy[]) + const pkgPoliciesWithMatchingNames = packagePolicyData?.items + ? packagePolicyData.items .filter((ds) => Boolean(ds.name.match(pkgPoliciesNamePattern))) .map((ds) => parseInt(ds.name.match(pkgPoliciesNamePattern)![1], 10)) .sort((a, b) => a - b) : []; + const incrementedName = `${packageInfo.name}-${ + pkgPoliciesWithMatchingNames.length + ? pkgPoliciesWithMatchingNames[pkgPoliciesWithMatchingNames.length - 1] + 1 + : 1 + }`; + updatePackagePolicy( packageToPackagePolicy( packageInfo, agentPolicy?.id || '', packagePolicy.output_id, packagePolicy.namespace, - packagePolicy.name || - `${packageInfo.name}-${ - pkgPoliciesWithMatchingNames.length - ? pkgPoliciesWithMatchingNames[pkgPoliciesWithMatchingNames.length - 1] + 1 - : 1 - }`, + packagePolicy.name || incrementedName, packagePolicy.description, integrationToEnable ) @@ -124,7 +132,15 @@ export const StepDefinePackagePolicy: React.FunctionComponent<{ namespace: agentPolicy.namespace, }); } - }, [packagePolicy, agentPolicy, packageInfo, updatePackagePolicy, integrationToEnable]); + }, [ + packagePolicy, + agentPolicy, + packageInfo, + updatePackagePolicy, + integrationToEnable, + packagePolicyData, + isLoadingPackagePolicies, + ]); return validationResults ? ( { const { id: packagePolicyId, version, ...newPackagePolicy } = packagePolicy; - return newPackagePolicy; + const updatedPackagePolicy = { + ...newPackagePolicy, + name: `${packagePolicy.name} (copy)`, + }; + return updatedPackagePolicy; } ); await packagePolicyService.bulkCreate( diff --git a/x-pack/plugins/fleet/server/services/package_policy.ts b/x-pack/plugins/fleet/server/services/package_policy.ts index 8968b1b4af3fdb..985351c3e981b5 100644 --- a/x-pack/plugins/fleet/server/services/package_policy.ts +++ b/x-pack/plugins/fleet/server/services/package_policy.ts @@ -42,7 +42,6 @@ import type { } from '../../common'; import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../constants'; import { - HostedAgentPolicyRestrictionRelatedError, IngestManagerError, ingestErrorToResponseOptions, PackagePolicyIneligibleForUpgradeError, @@ -99,24 +98,14 @@ class PackagePolicyService { skipEnsureInstalled?: boolean; } ): Promise { - // Check that its agent policy does not have a package policy with the same name - const parentAgentPolicy = await agentPolicyService.get(soClient, packagePolicy.policy_id); - if (!parentAgentPolicy) { - throw new Error('Agent policy not found'); - } - if (parentAgentPolicy.is_managed && !options?.force) { - throw new HostedAgentPolicyRestrictionRelatedError( - `Cannot add integrations to hosted agent policy ${parentAgentPolicy.id}` - ); - } - if ( - (parentAgentPolicy.package_policies as PackagePolicy[]).find( - (siblingPackagePolicy) => siblingPackagePolicy.name === packagePolicy.name - ) - ) { - throw new IngestManagerError( - 'There is already a package with the same name on this agent policy' - ); + const existingPoliciesWithName = await this.list(soClient, { + perPage: 1, + kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name: "${packagePolicy.name}"`, + }); + + // Check that the name does not exist already + if (existingPoliciesWithName.items.length > 0) { + throw new IngestManagerError('There is already a package with the same name'); } let elasticsearch: PackagePolicy['elasticsearch']; // Add ids to stream @@ -320,12 +309,12 @@ class PackagePolicyService { }); return { - items: packagePolicies.saved_objects.map((packagePolicySO) => ({ + items: packagePolicies?.saved_objects.map((packagePolicySO) => ({ id: packagePolicySO.id, version: packagePolicySO.version, ...packagePolicySO.attributes, })), - total: packagePolicies.total, + total: packagePolicies?.total, page, perPage, }; @@ -369,19 +358,15 @@ class PackagePolicyService { if (!oldPackagePolicy) { throw new Error('Package policy not found'); } + // Check that the name does not exist already but exclude the current package policy + const existingPoliciesWithName = await this.list(soClient, { + perPage: 1, + kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name: "${packagePolicy.name}"`, + }); + const filtered = (existingPoliciesWithName?.items || []).filter((p) => p.id !== id); - // Check that its agent policy does not have a package policy with the same name - const parentAgentPolicy = await agentPolicyService.get(soClient, packagePolicy.policy_id); - if (!parentAgentPolicy) { - throw new Error('Agent policy not found'); - } - if ( - (parentAgentPolicy.package_policies as PackagePolicy[]).find( - (siblingPackagePolicy) => - siblingPackagePolicy.id !== id && siblingPackagePolicy.name === packagePolicy.name - ) - ) { - throw new Error('There is already a package with the same name on this agent policy'); + if (filtered.length > 0) { + throw new IngestManagerError('There is already a package with the same name'); } let inputs = restOfPackagePolicy.inputs.map((input) => diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/create.ts b/x-pack/test/fleet_api_integration/apis/package_policy/create.ts index 6a0d46a6053866..6817289d389f35 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/create.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/create.ts @@ -68,7 +68,7 @@ export default function (providerContext: FtrProviderContext) { .post(`/api/fleet/package_policies`) .set('kbn-xsrf', 'xxxx') .send({ - name: 'filetest-1', + name: 'filetest', description: '', namespace: 'default', policy_id: hostedPolicy.id, @@ -85,7 +85,7 @@ export default function (providerContext: FtrProviderContext) { expect(responseWithoutForce.statusCode).to.be(400); expect(responseWithoutForce.message).to.contain( - 'Cannot add integrations to hosted agent policy' + 'Cannot update integrations of hosted agent policy' ); // try same request with `force: true` @@ -122,7 +122,7 @@ export default function (providerContext: FtrProviderContext) { .post(`/api/fleet/package_policies`) .set('kbn-xsrf', 'xxxx') .send({ - name: 'filetest-1', + name: 'filetest-2', description: '', namespace: 'default', policy_id: agentPolicyId, @@ -276,5 +276,53 @@ export default function (providerContext: FtrProviderContext) { }) .expect(400); }); + + it('should return a 400 if there is a package policy with the same name on a different policy', async function () { + const { body: agentPolicyResponse } = await supertest + .post(`/api/fleet/agent_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'Test policy 2', + namespace: 'default', + }); + const otherAgentPolicyId = agentPolicyResponse.item.id; + + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'same-name-test-2', + description: '', + namespace: 'default', + policy_id: otherAgentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(200); + await supertest + .post(`/api/fleet/package_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'same-name-test-2', + description: '', + namespace: 'default', + policy_id: agentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); + }); }); } diff --git a/x-pack/test/fleet_api_integration/apis/package_policy/update.ts b/x-pack/test/fleet_api_integration/apis/package_policy/update.ts index 315ca276c393ff..7d62ea3bf7ec34 100644 --- a/x-pack/test/fleet_api_integration/apis/package_policy/update.ts +++ b/x-pack/test/fleet_api_integration/apis/package_policy/update.ts @@ -158,7 +158,7 @@ export default function (providerContext: FtrProviderContext) { }); }); - it('should return a 500 if there is another package policy with the same name', async function () { + it('should return a 400 if there is another package policy with the same name', async function () { await supertest .put(`/api/fleet/package_policies/${packagePolicyId2}`) .set('kbn-xsrf', 'xxxx') @@ -176,7 +176,37 @@ export default function (providerContext: FtrProviderContext) { version: '0.1.0', }, }) - .expect(500); + .expect(400); + }); + + it('should return a 400 if there is another package policy with the same name on a different policy', async function () { + const { body: agentPolicyResponse } = await supertest + .post(`/api/fleet/agent_policies`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'Test policy 2', + namespace: 'default', + }); + const otherAgentPolicyId = agentPolicyResponse.item.id; + + await supertest + .put(`/api/fleet/package_policies/${packagePolicyId2}`) + .set('kbn-xsrf', 'xxxx') + .send({ + name: 'filetest-1', + description: '', + namespace: 'updated_namespace', + policy_id: otherAgentPolicyId, + enabled: true, + output_id: '', + inputs: [], + package: { + name: 'filetest', + title: 'For File Tests', + version: '0.1.0', + }, + }) + .expect(400); }); it('should work with frozen input vars', async function () {