Skip to content

Commit

Permalink
[UII] Support integration policies without agent policy references (a…
Browse files Browse the repository at this point in the history
…ka orphaned integration policies) (#190649)

## Summary

Resolves #182220.

This PR allows integration polices to be saved without being added to
any agent policies. These integration policies can be considered
"orphaned." Through the API, an empty `policy_ids: []` array can be
passed during create operations to add it to no agent policies. The same
empty array can be passed during update operations to clear it from all
previous agent policies. Clearing agent policies references this way
will also set the deprecated `policy_id` field to `null`.

Clearing agent policy references requires the same licensing as the
general reusable integration policies feature.

I spotted a bug where removing one or more agent policy references does
not bump their revision. Revision was only bumped for only newly added
references This has been fixed in this PR as well.

On the UI side, orphaned integration policies can only be discovered on
the Integrations details > Policies table:

<img width="1388" alt="image"
src="https://github.com/user-attachments/assets/bcaeb4e7-629a-4ce6-81df-a48a997ed85d">


Agent policies can be unattached in the manage agent policies modal:

<img width="1068" alt="image"
src="https://github.com/user-attachments/assets/7efd48a8-1466-48ee-a48d-18cfbcc4a9a6">


Integration policy create/edit form allows agent policies field to be
cleared, with a confirmation modal as a heads up to the user. If the
user previously had agent policies attached, and is now clearing it, the
normal `This action will update the selected agent policies` will be
shown:

<img width="1108" alt="image"
src="https://github.com/user-attachments/assets/1eade1d1-a456-4d2d-a85f-65801ec55857">


### Checklist

- ~~Consider interaction with space aware policies~~ Will be done in
#190727
- [x] Test with integrations that register custom policy editors
- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
  - Will be done in elastic/ingest-docs#1261
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
jen-huang authored Aug 28, 2024
1 parent b56d7dc commit c9d8292
Show file tree
Hide file tree
Showing 37 changed files with 348 additions and 213 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ properties:
- enabled
policy_id:
type: string
nullable: true
deprecated: true
policy_ids:
type: array
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,17 @@ properties:
type: string
description: The package policy namespace. Leave blank to inherit the agent policy's namespace.
example: 'customnamespace'
output_id:
type: string
description: Output ID to send package data to
example: 'output-id'
nullable: true
policy_id:
type: string
description: Agent policy ID where that package policy will be added
example: 'agent-policy-id'
deprecated: true
nullable: true
policy_ids:
type: array
items:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export type SimplifiedInputs = Record<

export interface SimplifiedPackagePolicy {
id?: string;
policy_id?: string;
policy_id?: string | null;
policy_ids: string[];
output_id?: string;
namespace: string;
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/fleet/common/types/models/package_policy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ export interface NewPackagePolicy {
namespace?: string;
enabled: boolean;
is_managed?: boolean;
/** @deprecated */
policy_id?: string;
/** @deprecated Nullable to allow user to clear existing policy id */
policy_id?: string | null;
policy_ids: string[];
// Nullable to allow user to reset to default outputs
output_id?: string | null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export type PostDeletePackagePoliciesResponse = Array<{
name?: string;
success: boolean;
package?: PackagePolicyPackage;
policy_id?: string;
policy_id?: string | null;
policy_ids?: string[];
output_id?: string;
// Support generic errors
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/fleet/cypress/tasks/integrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ import { CONFIRM_MODAL } from '../screens/navigation';
import { request } from './common';

export const addIntegration = ({ useExistingPolicy } = { useExistingPolicy: false }) => {
cy.intercept('/api/fleet/agent_status?*').as('agentStatus');

cy.getBySel(ADD_INTEGRATION_POLICY_BTN).click();
if (useExistingPolicy) {
cy.getBySel(EXISTING_HOSTS_TAB).click();
cy.wait('@agentStatus');
} else {
// speeding up creating with unchecking system and agent integration
cy.getBySel(AGENT_POLICY_SYSTEM_MONITORING_CHECKBOX).uncheck({ force: true });
Expand All @@ -33,7 +36,8 @@ export const addIntegration = ({ useExistingPolicy } = { useExistingPolicy: fals
force: true,
});
}
cy.getBySel(CREATE_PACKAGE_POLICY_SAVE_BTN).click();
cy.getBySel(CREATE_PACKAGE_POLICY_SAVE_BTN).should('be.enabled').click();

// sometimes agent is assigned to default policy, sometimes not
cy.getBySel(CONFIRM_MODAL.CONFIRM_BUTTON).click();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,16 @@ describe('ConfirmDeployAgentPolicyModal', () => {
'Agent policies that will be updated to remove this integration policy:Agent policy 3'
);
});

it('should render no agent policies message when not adding/removing agent policies', () => {
testRenderer = createFleetTestRendererMock();
render({
agentCount: 0,
agentPolicies: [],
agentPoliciesToRemove: [],
});
const calloutText = renderResult.getByTestId('confirmNoPoliciesCallout').textContent;
expect(calloutText).toContain('No agent policies selected');
expect(calloutText).toContain('This integration will not be added to any agent policies.');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,46 @@ export const ConfirmDeployAgentPolicyModal: React.FunctionComponent<{
agentPoliciesToAdd = [],
agentPoliciesToRemove = [],
}) => {
return (
return agentPolicies.length === 0 && agentPoliciesToRemove.length === 0 ? (
<EuiConfirmModal
title={
<FormattedMessage
id="xpack.fleet.agentPolicy.noPolicies.confirmModalTitle"
defaultMessage="Save changes"
/>
}
onCancel={onCancel}
onConfirm={onConfirm}
cancelButtonText={
<FormattedMessage
id="xpack.fleet.agentPolicy.noPolicies.confirmModalCancelButtonLabel"
defaultMessage="Cancel"
/>
}
confirmButtonText={
<FormattedMessage
id="xpack.fleet.agentPolicy.noPolicies.confirmModalConfirmButtonLabel"
defaultMessage="Save changes"
/>
}
buttonColor="primary"
>
<EuiCallOut
iconType="iInCircle"
data-test-subj="confirmNoPoliciesCallout"
title={i18n.translate('xpack.fleet.agentPolicy.noPolicies.confirmModalCalloutTitle', {
defaultMessage: 'No agent policies selected',
})}
>
<div className="eui-textBreakWord">
<FormattedMessage
id="xpack.fleet.agentPolicy.noPolicies.confirmModalCalloutDescription"
defaultMessage="This integration will not be added to any agent policies."
/>
</div>
</EuiCallOut>
</EuiConfirmModal>
) : (
<EuiConfirmModal
title={
<FormattedMessage
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,6 @@ describe('step select agent policy', () => {
await act(async () => {
const select = renderResult.container.querySelector('[data-test-subj="agentPolicySelect"]');
expect((select as any)?.value).toEqual('');

expect(renderResult.getByText('At least one agent policy is required.')).toBeVisible();
});
});

Expand Down Expand Up @@ -179,8 +177,6 @@ describe('step select agent policy', () => {
'[data-test-subj="agentPolicyMultiSelect"]'
);
expect((select as any)?.value).toEqual(undefined);

expect(renderResult.getByText('At least one agent policy is required.')).toBeVisible();
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,19 +101,23 @@ export const StepSelectAgentPolicy: React.FunctionComponent<{
updateAgentPolicies([]);
}
};
if (isLoading || selectedPolicyIds.length === 0) {
if (isLoading || isFirstLoad) {
return;
}
const agentPolicyIds = agentPolicies.map((policy) => policy.id);
const agentPoliciesHaveAllSelectedIds = selectedPolicyIds.every((id) =>
agentPolicies.map((policy) => policy.id).includes(id)
agentPolicyIds.includes(id)
);
if (agentPolicies.length === 0 || !agentPoliciesHaveAllSelectedIds) {
if (
(agentPolicies.length === 0 && selectedPolicyIds.length !== 0) ||
!agentPoliciesHaveAllSelectedIds
) {
fetchAgentPolicyInfo();
} else if (agentPoliciesHaveAllSelectedIds && selectedPolicyIds.length < agentPolicies.length) {
setSelectedAgentPolicyError(undefined);
updateAgentPolicies(agentPolicies.filter((policy) => selectedPolicyIds.includes(policy.id)));
}
}, [selectedPolicyIds, agentPolicies, updateAgentPolicies, isLoading]);
}, [selectedPolicyIds, agentPolicies, updateAgentPolicies, isLoading, isFirstLoad]);

// Try to select default agent policy
useEffect(() => {
Expand Down Expand Up @@ -152,9 +156,11 @@ export const StepSelectAgentPolicy: React.FunctionComponent<{

// Bubble up any issues with agent policy selection
useEffect(() => {
if (selectedPolicyIds.length > 0 && !selectedAgentPolicyError) {
if (!selectedAgentPolicyError) {
setHasAgentPolicyError(false);
} else setHasAgentPolicyError(true);
} else {
setHasAgentPolicyError(true);
}
}, [selectedAgentPolicyError, selectedPolicyIds, setHasAgentPolicyError]);

const onChange = useCallback(
Expand Down Expand Up @@ -237,16 +243,9 @@ export const StepSelectAgentPolicy: React.FunctionComponent<{
/>
) : null
}
isInvalid={Boolean(
selectedPolicyIds.length === 0 || someNewAgentPoliciesHaveLimitedPackage
)}
isInvalid={Boolean(someNewAgentPoliciesHaveLimitedPackage)}
error={
selectedPolicyIds.length === 0 ? (
<FormattedMessage
id="xpack.fleet.createPackagePolicy.StepSelectPolicy.noPolicySelectedError"
defaultMessage="At least one agent policy is required."
/>
) : someNewAgentPoliciesHaveLimitedPackage ? (
someNewAgentPoliciesHaveLimitedPackage ? (
<FormattedMessage
id="xpack.fleet.createPackagePolicy.StepSelectPolicy.cannotAddLimitedIntegrationError"
defaultMessage="This integration can only be added once per agent policy."
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ describe('StepSelectHosts', () => {
).toContain('Agent policy 1');
});

it('should display dropdown without preselected value when Existing hosts selected with mulitple agent policies', () => {
it('should display dropdown without preselected value when Existing hosts selected with mulitple agent policies', async () => {
(useGetAgentPolicies as jest.MockedFunction<any>).mockReturnValue({
data: {
items: [
Expand All @@ -174,8 +174,9 @@ describe('StepSelectHosts', () => {
fireEvent.click(renderResult.getByText('Existing hosts').closest('button')!);
});

waitFor(() => {
expect(renderResult.getByText('At least one agent policy is required.')).toBeInTheDocument();
await act(async () => {
const select = renderResult.container.querySelector('[data-test-subj="agentPolicySelect"]');
expect((select as any)?.value).toEqual('');
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
import { useCallback, useMemo, useEffect, useRef } from 'react';
import type { ApplicationStart } from '@kbn/core-application-browser';

import { PLUGIN_ID } from '../../../../constants';
import { PLUGIN_ID, INTEGRATIONS_PLUGIN_ID } from '../../../../constants';
import { pkgKeyFromPackageInfo } from '../../../../services';
import { useStartServices, useLink, useIntraAppState } from '../../../../hooks';
import type {
CreatePackagePolicyRouteState,
Expand Down Expand Up @@ -86,18 +87,22 @@ export const useOnSaveNavigate = (params: UseOnSaveNavigateParams) => {
if (!doOnSaveNavigation.current) {
return;
}
const packagePolicyPath = getPath('policy_details', {
policyId: packagePolicy.policy_ids[0], // TODO navigates to first policy
});

const hasNoAgentPolicies = packagePolicy.policy_ids.length === 0;
const packagePolicyPath = hasNoAgentPolicies
? getPath('integration_details_policies', {
pkgkey: pkgKeyFromPackageInfo(packagePolicy.package!),
})
: getPath('policy_details', {
policyId: packagePolicy.policy_ids[0], // TODO navigates to first policy
});
const [onSaveNavigateTo, onSaveQueryParams]: [
Parameters<ApplicationStart['navigateToApp']>,
CreatePackagePolicyRouteState['onSaveQueryParams']
] = routeState?.onSaveNavigateTo
? [routeState.onSaveNavigateTo, routeState?.onSaveQueryParams]
: [
[
PLUGIN_ID,
hasNoAgentPolicies ? INTEGRATIONS_PLUGIN_ID : PLUGIN_ID,
{
path: packagePolicyPath,
},
Expand All @@ -123,7 +128,15 @@ export const useOnSaveNavigate = (params: UseOnSaveNavigateParams) => {
navigateToApp(...onSaveNavigateTo);
}
},
[packagePolicy.policy_ids, getPath, navigateToApp, routeState, queryParamsPolicyId]
[
packagePolicy.policy_ids,
packagePolicy.package,
getPath,
routeState?.onSaveNavigateTo,
routeState?.onSaveQueryParams,
queryParamsPolicyId,
navigateToApp,
]
);

return onSaveNavigate;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
sendCreatePackagePolicy,
sendBulkInstallPackages,
sendGetPackagePolicies,
useMultipleAgentPolicies,
} from '../../../../../hooks';
import { isVerificationError, packageToPackagePolicy } from '../../../../../services';
import {
Expand Down Expand Up @@ -154,6 +155,8 @@ export function useOnSubmit({
const { notifications } = useStartServices();
const confirmForceInstall = useConfirmForceInstall();
const spaceSettings = useSpaceSettingsContext();
const { canUseMultipleAgentPolicies } = useMultipleAgentPolicies();

// only used to store the resulting package policy once saved
const [savedPackagePolicy, setSavedPackagePolicy] = useState<PackagePolicy>();
// Form state
Expand Down Expand Up @@ -184,14 +187,10 @@ export function useOnSubmit({
if (isEqual(updatedAgentPolicies, agentPolicies)) {
return;
}
if (updatedAgentPolicies.length > 0) {
setAgentPolicies(updatedAgentPolicies);
if (packageInfo) {
setHasAgentPolicyError(false);
}
} else {
setHasAgentPolicyError(true);
setAgentPolicies([]);

setAgentPolicies(updatedAgentPolicies);
if (packageInfo) {
setHasAgentPolicyError(false);
}

// eslint-disable-next-line no-console
Expand Down Expand Up @@ -235,18 +234,17 @@ export function useOnSubmit({
? validationHasErrors(newValidationResults)
: false;
const hasAgentPolicy =
newPackagePolicy.policy_ids.length > 0 && newPackagePolicy.policy_ids[0] !== '';
if (
hasPackage &&
(hasAgentPolicy || selectedPolicyTab === SelectedPolicyTab.NEW) &&
!hasValidationErrors
) {
(newPackagePolicy.policy_ids.length > 0 && newPackagePolicy.policy_ids[0] !== '') ||
selectedPolicyTab === SelectedPolicyTab.NEW;
const isOrphaningPolicy =
canUseMultipleAgentPolicies && newPackagePolicy.policy_ids.length === 0;
if (hasPackage && (hasAgentPolicy || isOrphaningPolicy) && !hasValidationErrors) {
setFormState('VALID');
} else {
setFormState('INVALID');
}
},
[packagePolicy, setFormState, updatePackagePolicyValidation, selectedPolicyTab]
[packagePolicy, updatePackagePolicyValidation, selectedPolicyTab, canUseMultipleAgentPolicies]
);

// Initial loading of package info
Expand Down Expand Up @@ -315,7 +313,8 @@ export function useOnSubmit({
return;
}
if (
agentCount !== 0 &&
(agentCount !== 0 ||
(agentPolicies.length === 0 && selectedPolicyTab !== SelectedPolicyTab.NEW)) &&
!(isAgentlessIntegration(packageInfo) || isAgentlessPackagePolicy(packagePolicy)) &&
formState !== 'CONFIRM'
) {
Expand Down Expand Up @@ -401,7 +400,7 @@ export function useOnSubmit({
setSavedPackagePolicy(data!.item);

const promptForAgentEnrollment =
!(agentCount && agentPolicies.length > 0) &&
(createdPolicy || (agentPolicies.length > 0 && !agentCount)) &&
!isAgentlessConfigured &&
hasFleetAddAgentsPrivileges;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -498,7 +498,7 @@ export const CreatePackagePolicySinglePage: CreatePackagePolicyParams = ({
<Suspense fallback={<Loading />}>
<PliAuthBlockWrapper>
<EuiErrorBoundary>
{formState === 'CONFIRM' && agentPolicies.length > 0 && (
{formState === 'CONFIRM' && (
<ConfirmDeployAgentPolicyModal
agentCount={agentCount}
agentPolicies={agentPolicies}
Expand Down
Loading

0 comments on commit c9d8292

Please sign in to comment.