Skip to content
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

[Fleet] Make integration names globally unique #115212

Merged
merged 5 commits into from
Nov 3, 2021
Merged

[Fleet] Make integration names globally unique #115212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
195455a
[Fleet] Make integration names globally unique
criamico Oct 21, 2021
d7e0807
Fix Jest tests
criamico Oct 25, 2021
de4c0fd
Append (copy) to names of integration packages belonging to duplicate…
criamico Oct 29, 2021
5fe3eaa
Update current policy maintaining its name
criamico Oct 29, 2021
565251b
Fix failing tests
criamico Nov 3, 2021
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
52 changes: 34 additions & 18 deletions ...ons/fleet/sections/agent_policy/create_package_policy_page/step_define_package_policy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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);

Expand All @@ -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
)
Expand All @@ -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
Expand Down
8 changes: 6 additions & 2 deletions x-pack/plugins/fleet/server/services/agent_policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
51 changes: 18 additions & 33 deletions x-pack/plugins/fleet/server/services/package_policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ import type {
} from '../../common';
import { PACKAGE_POLICY_SAVED_OBJECT_TYPE } from '../constants';
import {
HostedAgentPolicyRestrictionRelatedError,
IngestManagerError,
ingestErrorToResponseOptions,
PackagePolicyIneligibleForUpgradeError,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
};
Expand Down Expand Up @@ -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);
Comment on lines +370 to +374
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to do the filtering in the query?

Suggested change
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);
const existingPoliciesWithName = await this.list(soClient, {
perPage: 1,
kuery: `${PACKAGE_POLICY_SAVED_OBJECT_TYPE}.name: "${packagePolicy.name}" and not id:"${packagePolicy.id}"`,
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, thanks for the suggestion!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to update the query but I'm not able to directly filter by id. I'll keep it as it is :)


// 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) =>
Expand Down
54 changes: 51 additions & 3 deletions x-pack/test/fleet_api_integration/apis/package_policy/create.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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`
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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);
});
});
}
34 changes: 32 additions & 2 deletions x-pack/test/fleet_api_integration/apis/package_policy/update.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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 () {
Expand Down