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] Add support for "Edit Package Policy" extensions using latest version of a package #114914

Merged
merged 15 commits into from
Oct 20, 2021
Merged

[Fleet] Add support for "Edit Package Policy" extensions using latest version of a package #114914

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
86d59fc
Add support for extensions using latest version of a package and forc…
kpollich Oct 13, 2021
6964c66
Merge branch 'master' into 114785-use-latest-version-extension-point
kpollich Oct 15, 2021
b88dc36
Merge branch 'master' into 114785-use-latest-version-extension-point
kibanamachine Oct 15, 2021
0e0c2b7
Merge branch 'master' into 114785-use-latest-version-extension-point
kibanamachine Oct 18, 2021
e4dba75
Merge branch 'master' into 114785-use-latest-version-extension-point
kpollich Oct 18, 2021
84dbe21
Fix isUpgrade flag on integrations UI version of edit page
kpollich Oct 18, 2021
1afedd3
Treat non-validation errors as general failures in server and UI
kpollich Oct 18, 2021
c4abf7d
Mergh branch 'master' into 114785-use-latest-version-extension-point
kpollich Oct 19, 2021
61bea60
Merge branch 'master' into 114785-use-latest-version-extension-point
kibanamachine Oct 19, 2021
eb0c5bc
Fix tests + don't call upgrade API when saving
kpollich Oct 19, 2021
6c9a1f6
fix i18n
kpollich Oct 19, 2021
76840c7
Fix default name always appearing when editing package policies via e…
kpollich Oct 19, 2021
1d4f414
Merge branch 'master' into 114785-use-latest-version-extension-point
kibanamachine Oct 20, 2021
859a333
Merge branch 'master' into 114785-use-latest-version-extension-point
kibanamachine Oct 20, 2021
db57f16
Opt security solution plugin out of new extension option
kpollich Oct 20, 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
1 change: 1 addition & 0 deletions x-pack/plugins/apm/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,7 @@ export class ApmPlugin implements Plugin<ApmPluginSetup, ApmPluginStart> {
fleet.registerExtension({
package: 'apm',
view: 'package-policy-edit',
useLatestPackageVersion: true,
Component: getLazyAPMPolicyEditExtension(),
});

Expand Down
6 changes: 5 additions & 1 deletion ...applications/fleet/sections/agent_policy/create_package_policy_page/components/layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@ export const CreatePackagePolicyPageLayout: React.FunctionComponent<{
const isEdit = useMemo(() => ['edit', 'package-edit'].includes(from), [from]);
const isUpgrade = useMemo(
() =>
['upgrade-from-fleet-policy-list', 'upgrade-from-integrations-policy-list'].includes(from),
[
'upgrade-from-fleet-policy-list',
'upgrade-from-integrations-policy-list',
'upgrade-from-extension',
].includes(from),
[from]
);

Expand Down
4 changes: 3 additions & 1 deletion ...fleet/public/applications/fleet/sections/agent_policy/create_package_policy_page/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ export type EditPackagePolicyFrom =
| 'policy'
| 'edit'
| 'upgrade-from-fleet-policy-list'
| 'upgrade-from-integrations-policy-list';
| 'upgrade-from-integrations-policy-list'
| 'upgrade-from-extension';

export type PackagePolicyFormState =
| 'VALID'
| 'INVALID'
Expand Down
49 changes: 22 additions & 27 deletions .../fleet/public/applications/fleet/sections/agent_policy/edit_package_policy_page/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@ import {
sendGetOneAgentPolicy,
sendGetOnePackagePolicy,
sendGetPackageInfoByKey,
sendUpgradePackagePolicy,
sendUpgradePackagePolicyDryRun,
} from '../../../hooks';
import { useBreadcrumbs as useIntegrationsBreadcrumbs } from '../../../../integrations/hooks';
import {
useBreadcrumbs as useIntegrationsBreadcrumbs,
useGetOnePackagePolicy,
} from '../../../../integrations/hooks';
import { Loading, Error, ExtensionWrapper } from '../../../components';
import { ConfirmDeployAgentPolicyModal } from '../components';
import { CreatePackagePolicyPageLayout } from '../create_package_policy_page/components';
Expand All @@ -68,7 +70,23 @@ export const EditPackagePolicyPage = memo(() => {
params: { packagePolicyId },
} = useRouteMatch<{ policyId: string; packagePolicyId: string }>();

return <EditPackagePolicyForm packagePolicyId={packagePolicyId} />;
const packagePolicy = useGetOnePackagePolicy(packagePolicyId);

const extensionView = useUIExtension(
packagePolicy.data?.item?.package?.name ?? '',
'package-policy-edit'
);

return (
<EditPackagePolicyForm
packagePolicyId={packagePolicyId}
// If an extension opts in to this `useLatestPackageVersion` flag, we want to display
// the edit form in an "upgrade" state regardless of whether the user intended to
// "edit" their policy or "upgrade" it. This ensures the new policy generated will be
// set to use the latest version of the package, not its current version.
isUpgrade={extensionView?.useLatestPackageVersion}
/>
);
});

export const EditPackagePolicyForm = memo<{
Expand Down Expand Up @@ -345,29 +363,6 @@ export const EditPackagePolicyForm = memo<{

const { error } = await savePackagePolicy();
if (!error) {
if (isUpgrade) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@joshdover @jen-huang

My change to return a bad request in the case of general failures, and instead allowing only validation errors to be considered non-fatal errors in 61bea60 revealed an interesting issue:

This API request to /upgrade the package policy was always failing before, we just didn't return an error-coded HTTP response. We returned a 200 with success: false in the body. However, the actual "upgrade" process has been and remains successful to this date. Why is that?

What we were actually doing here in the policy editor was:

  1. Run the dry run to get the new "proposed" shape of the package policy w/ any new inputs/streams/variables, and any existing values merged into them
  2. Use the new "proposed" package policy as our backing data model for the policy editor form
  3. Save the policy, including the new values and package version
  4. Call the /upgrade endpoint, and receive an error because the policy has already been saved with the new package version

So, the /upgrade endpoint in its non-dry-run context was essentially useless in this workflow. And it is largely useless overall because we don't support editing at the time of upgrade. A consumer would have to make their edits to the outdated package policy prior to upgrading, which doesn't make sense because it'd result in an invalid package policy. We'd be adding values for variables defined in the new version of the package, then validating them against the outdated version of the package.

Instead, by removing this /upgrade call, we have a workflow that stops at #3 above. So our functionality is entirely the same as it was before, we've just done away with an erroneous call at the end of the process.

It seems to me like maybe we should formalize this and re-do some of the ergonomics of our upgrade process. The only purpose of the non-dry-run upgrade endpoint, at this point, would to be to perform an upgrade with no edits, then allow the user to make edits to their policy after the upgrade is complete. It seems like this is not a realistic user experience based on our implementation in Fleet, so it doesn't make sense to me to even support it. Instead, we should focus on the path of "Generate proposed upgraded policy -> Allow for edits to proposed policy -> Persist proposed policy".

I know this is long but I would love to get some thoughts here. Happy to sync offline as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

The actual upgrade service method still serves a purpose in upgrading package policies automatically, to be clear. We want to attempt an upgrade and fail if there are any conflicts. I am just proposing that we do away with the API endpoint that serves that same purpose.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, we should focus on the path of "Generate proposed upgraded policy -> Allow for edits to proposed policy -> Persist proposed policy".

This makes sense to me. It seems this endpoint is only useful in the context of attempting to automate an upgrade without any user intervention. If that's the case, then I think we should remove the ability to specify any edits / input vars in the upgrade endpoint and instead only use it for attempting to upgrade a policy automatically without any edits.

It'd then make sense to move the logic for executing a dry run + getting the new proposed policy to a separate endpoint.

To make sure I'm understanding correctly, if we followed this logic, we'd end up with something like:

  • POST /package-policy/<id>/upgrade for attempting automated upgrades without edits. Does not accept any inputs or request body at all. Will fail if there's a conflict.
  • GET /package-policy/<id>/upgrade for executing a dry run and getting the "proposed package policy"
  • PUT /package-policy/<id> for saving an existing policy, potentially with a higher version. Accepts all input fields. Will fail if validation fails for the given package version.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that sounds like a great improvement for the layout of this particular API.

POST /package-policy/<id>/upgrade for attempting automated upgrades without edits. Does not accept any inputs or request body at all. Will fail if there's a conflict.

This is how the POST /package-policy/<id>/upgrade endpoint works today.

GET /package-policy/<id>/upgrade for executing a dry run and getting the "proposed package policy"

This is the current functionality of the POST /package-policy/<id>/upgrade { dryRun: true } API call

PUT /package-policy/<id> for saving an existing policy, potentially with a higher version. Accepts all input fields. Will fail if validation fails for the given package version.

This is existing functionality as well. So the only change here should be moving away from the dryRun: true request parameter, and creating a distinct GET /upgrade endpoint wired up to that particular service method instead.

I created #115570 to capture this piece of work. I tagged it as technical debt for now.

const { error: upgradeError } = await sendUpgradePackagePolicy([packagePolicyId]);

if (upgradeError) {
notifications.toasts.addError(upgradeError, {
title: i18n.translate('xpack.fleet.upgradePackagePolicy.failedNotificationTitle', {
defaultMessage: 'Error upgrading {packagePolicyName}',
values: {
packagePolicyName: packagePolicy.name,
},
}),
toastMessage: i18n.translate(
'xpack.fleet.editPackagePolicy.failedConflictNotificationMessage',
{
defaultMessage: `Data is out of date. Refresh the page to get the latest policy.`,
}
),
});

return;
}
}

application.navigateToUrl(successRedirectPath);
notifications.toasts.addSuccess({
title: i18n.translate('xpack.fleet.editPackagePolicy.updatedNotificationTitle', {
Expand Down Expand Up @@ -426,7 +421,7 @@ export const EditPackagePolicyForm = memo<{
const [selectedTab, setSelectedTab] = useState(0);

const layoutProps = {
from,
from: extensionView?.useLatestPackageVersion ? 'upgrade-from-extension' : from,
cancelUrl,
agentPolicy,
packageInfo,
Expand Down
16 changes: 15 additions & 1 deletion x-pack/plugins/fleet/public/applications/integrations/sections/epm/screens/policy/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,25 @@ import { useRouteMatch } from 'react-router-dom';

// TODO: Needs to be moved
import { EditPackagePolicyForm } from '../../../../../fleet/sections/agent_policy/edit_package_policy_page';
import { useGetOnePackagePolicy, useUIExtension } from '../../../../hooks';

export const Policy = memo(() => {
const {
params: { packagePolicyId },
} = useRouteMatch<{ packagePolicyId: string }>();

return <EditPackagePolicyForm packagePolicyId={packagePolicyId} from="package-edit" />;
const packagePolicy = useGetOnePackagePolicy(packagePolicyId);

const extensionView = useUIExtension(
packagePolicy.data?.item?.package?.name ?? '',
'package-policy-edit'
);

return (
<EditPackagePolicyForm
packagePolicyId={packagePolicyId}
from="package-edit"
isUpgrade={extensionView?.useLatestPackageVersion}
/>
);
});
7 changes: 7 additions & 0 deletions x-pack/plugins/fleet/public/hooks/use_request/package_policy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ export function useGetPackagePolicies(query: GetPackagePoliciesRequest['query'])
});
}

export const useGetOnePackagePolicy = (packagePolicyId: string) => {
return useRequest<GetOnePackagePolicyResponse>({
path: packagePolicyRouteService.getInfoPath(packagePolicyId),
method: 'get',
});
};

export const sendGetOnePackagePolicy = (packagePolicyId: string) => {
return sendRequest<GetOnePackagePolicyResponse>({
path: packagePolicyRouteService.getInfoPath(packagePolicyId),
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/public/types/ui_extensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ export interface PackagePolicyEditExtensionComponentProps {
export interface PackagePolicyEditExtension {
package: string;
view: 'package-policy-edit';
useLatestPackageVersion?: boolean;
Component: LazyExoticComponent<PackagePolicyEditExtensionComponent>;
}

Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/fleet/server/errors/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ export class PackageCacheError extends IngestManagerError {}
export class PackageOperationNotSupportedError extends IngestManagerError {}
export class ConcurrentInstallOperationError extends IngestManagerError {}
export class AgentReassignmentError extends IngestManagerError {}
export class PackagePolicyIneligibleForUpgradeError extends IngestManagerError {}
export class PackagePolicyValidationError extends IngestManagerError {}
export class HostedAgentPolicyRestrictionRelatedError extends IngestManagerError {
constructor(message = 'Cannot perform that action') {
super(
Expand Down
49 changes: 31 additions & 18 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 @@ -45,6 +45,8 @@ import {
HostedAgentPolicyRestrictionRelatedError,
IngestManagerError,
ingestErrorToResponseOptions,
PackagePolicyIneligibleForUpgradeError,
PackagePolicyValidationError,
} from '../errors';
import { NewPackagePolicySchema, UpdatePackagePolicySchema } from '../types';
import type {
Expand Down Expand Up @@ -528,25 +530,25 @@ class PackagePolicyService {
pkgName: packagePolicy.package.name,
pkgVersion: installedPackage?.version ?? '',
});
}

const isInstalledVersionLessThanOrEqualToPolicyVersion = semverLte(
installedPackage?.version ?? '',
packagePolicy.package.version
);
const isInstalledVersionLessThanOrEqualToPolicyVersion = semverLte(
packageInfo?.version ?? '',
packagePolicy.package.version
);

if (isInstalledVersionLessThanOrEqualToPolicyVersion) {
throw new IngestManagerError(
i18n.translate('xpack.fleet.packagePolicy.ineligibleForUpgradeError', {
defaultMessage:
"Package policy {id}'s package version {version} of package {name} is up to date with the installed package. Please install the latest version of {name}.",
values: {
id: packagePolicy.id,
name: packagePolicy.package.name,
version: packagePolicy.package.version,
},
})
);
}
if (isInstalledVersionLessThanOrEqualToPolicyVersion) {
throw new PackagePolicyIneligibleForUpgradeError(
i18n.translate('xpack.fleet.packagePolicy.ineligibleForUpgradeError', {
defaultMessage:
"Package policy {id}'s package version {version} of package {name} is up to date with the installed package. Please install the latest version of {name}.",
values: {
id: packagePolicy.id,
name: packagePolicy.package.name,
version: packagePolicy.package.version,
},
})
);
}

return {
Expand Down Expand Up @@ -600,6 +602,13 @@ class PackagePolicyService {
currentVersion: packageInfo.version,
});
} catch (error) {
// We only want to specifically handle validation errors for the new package policy. If a more severe or
// general error is thrown elsewhere during the upgrade process, we want to surface that directly in
// order to preserve any status code mappings, etc that might be included w/ the particular error type
if (!(error instanceof PackagePolicyValidationError)) {
throw error;
}

result.push({
id,
success: false,
Expand Down Expand Up @@ -653,6 +662,10 @@ class PackagePolicyService {
hasErrors,
};
} catch (error) {
if (!(error instanceof PackagePolicyValidationError)) {
throw error;
}

return {
hasErrors: true,
...ingestErrorToResponseOptions(error),
Expand Down Expand Up @@ -1089,7 +1102,7 @@ export function overridePackageInputs(
return { ...resultingPackagePolicy, errors: responseFormattedValidationErrors };
}

throw new IngestManagerError(
throw new PackagePolicyValidationError(
i18n.translate('xpack.fleet.packagePolicyInvalidError', {
defaultMessage: 'Package policy is invalid: {errors}',
values: {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/public/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S
registerExtension({
package: 'endpoint',
view: 'package-policy-edit',
useLatestPackageVersion: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a requirement for the Endpoint package since Elastic Agent has the ability to dynamically run different versions of the endpoint binary. That said, I can't really think of any downsides here since the user is already going to be pushing out updates to all of their endpoints.

Let me figure out the best person to talk to about this.

Component: getLazyEndpointPolicyEditExtension(core, plugins),
});

Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/uptime/public/apps/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ export class UptimePlugin
registerExtension({
package: 'synthetics',
view: 'package-policy-edit',
useLatestPackageVersion: true,
Component: LazySyntheticsPolicyEditExtension,
});

Expand Down
Loading