[Fleet] Make integration names globally unique (elastic#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
kibanamachine · Nov 3, 2021 · f84cae3 · f84cae3
1 parent b37f680
commit f84cae3
Show file tree

Hide file tree

Showing 5 changed files with 141 additions and 58 deletions.
diff --git a/...ons/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx b/...ons/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<boolean>(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 ? (
       <FormGroupResponsiveFields

diff --git a/x-pack/plugins/fleet/server/services/agent_policy.ts b/x-pack/plugins/fleet/server/services/agent_policy.ts
@@ -410,12 +410,16 @@ class AgentPolicyService {
       options
     );
 
-    // Copy all package policies
+    // Copy all package policies and append (copy) to their names
     if (baseAgentPolicy.package_policies.length) {
       const newPackagePolicies = (baseAgentPolicy.package_policies as PackagePolicy[]).map(
         (packagePolicy: PackagePolicy) => {
           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
@@ -42,7 +42,6 @@ import type {
 } from '../../common';
 import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../constants';
 import {
-  HostedAgentPolicyRestrictionRelatedError,
   IngestManagerError,
   ingestErrorToResponseOptions,
   PackagePolicyIneligibleForUpgradeError,
@@ -108,24 +107,14 @@ class PackagePolicyService {
       skipEnsureInstalled?: boolean;
     }
   ): Promise<PackagePolicy> {
-    // 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
@@ -329,12 +318,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,
     };
@@ -377,19 +366,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
@@ -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
@@ -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 () {