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] Create a new UIExtensionPoint to replace the integration define step #149653

Merged
merged 8 commits into from
Feb 1, 2023
Merged

[Fleet] Create a new UIExtensionPoint to replace the integration define step #149653

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2499179
add new extension
orouz Jan 27, 2023
48c18ac
simplify rendering of new extension point
orouz Jan 30, 2023
4aad0dd
remove integration info prop
orouz Jan 30, 2023
7af3d1d
throw on invalid extension combo
orouz Feb 1, 2023
e304afe
add and fix tests
orouz Feb 1, 2023
d63154a
Merge branch 'main' into fleet_replace_step_ext
orouz Feb 1, 2023
5d0274e
clean up
orouz Feb 1, 2023
5e0cec2
clean up
orouz Feb 1, 2023
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
26 changes: 25 additions & 1 deletion ...tions/fleet/sections/agent_policy/create_package_policy_page/single_page_layout/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,11 +267,34 @@ export const CreatePackagePolicySinglePage: CreatePackagePolicyParams = ({
);

const extensionView = useUIExtension(packagePolicy.package?.name ?? '', 'package-policy-create');
const replaceDefineStepView = useUIExtension(
packagePolicy.package?.name ?? '',
'package-policy-replace-define-step'
);

if (replaceDefineStepView && extensionView) {
// eslint-disable-next-line no-console
console.warn(
"'package-policy-create' is ignored when 'package-policy-replace-define-step' is defined"
);
}
Copy link
Contributor Author

@orouz orouz Jan 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

not sure logging here is good enough, we could add useUIExtensions which would be typed to not allow this combination

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think useUIExtensions or adding an overload to useUIExtension that takes an array of extensions would be the path forward here. Agree that logging is not enough to prevent this, as we should prevent it entirely at compile time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kpollich

thought about this again and i believe we shouldn't overload useUIExtension, because:

  • registration is not really constrained, so we could narrow the types but that won't make them true
    • we can't fully limit registration with static types, as it can be done multiple times.
    • we can make registerExtension take an array and type-check against invalid combinations, and run a full sweep on other plugins, which would somewhat mitigate possible future risk of misuse
  • there is actually not much to do with that information. we still need to account for both cases, and when doing those we don't need the other extension anyway. after making the types work, i noticed the distinction wasn't really used
  • overloading wasn't pretty

let me know what you think is best here/follow ups:

  • add/iterate on overloading commit
  • just drop/keep the comments
  • work on registerExtension
  • something else?

thanks!

also note i'd still want to merge #149137 as soon as we can because there are other PRs depending on it

Copy link
Member

Choose a reason for hiding this comment

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

Following up on our chat offline:

Let's just throw an error when we encounter this case. This should result in the Fleet UI's error boundary being displayed and should catch the "incompatible UI extension" state during development. I don't want to silently ignore a UI extension with a browser console log/warning because it will be non-obvious to someone developing in this situation.

if (replaceDefineStepView && extensionView) { 
  throw new Error(
    "'package-policy-create' and 'package-policy-replace-define-step' cannot both be registered as UI extensions"
  )
}

Thanks for investigating here and following up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this with some test updates that were needed


const stepConfigurePackagePolicy = useMemo(
() =>
isPackageInfoLoading || !isInitialized ? (
<Loading />
) : replaceDefineStepView && packageInfo?.name ? (
<ExtensionWrapper>
<replaceDefineStepView.Component
agentPolicy={agentPolicy}
packageInfo={packageInfo}
newPolicy={packagePolicy}
onChange={handleExtensionViewOnChange}
validationResults={validationResults}
integrationInfo={integrationInfo}
isEditPage={false}
/>
</ExtensionWrapper>
) : packageInfo ? (
<>
<StepDefinePackagePolicy
Expand Down Expand Up @@ -317,9 +340,10 @@ export const CreatePackagePolicySinglePage: CreatePackagePolicyParams = ({
updatePackagePolicy,
validationResults,
formState,
integrationInfo?.name,
extensionView,
handleExtensionViewOnChange,
integrationInfo,
replaceDefineStepView,
]
);

Expand Down
122 changes: 79 additions & 43 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 @@ -7,7 +7,7 @@

import React, { useState, useEffect, useCallback, useMemo, memo } from 'react';
import { omit } from 'lodash';
import { useRouteMatch } from 'react-router-dom';
import { useParams, useRouteMatch } from 'react-router-dom';
import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n-react';
import {
Expand Down Expand Up @@ -42,7 +42,7 @@ import {
} from '../../../components';
import { ConfirmDeployAgentPolicyModal } from '../components';
import { CreatePackagePolicySinglePageLayout } from '../create_package_policy_page/single_page_layout/components';
import type { EditPackagePolicyFrom } from '../create_package_policy_page/types';
import type { AddToPolicyParams, EditPackagePolicyFrom } from '../create_package_policy_page/types';
import {
StepConfigurePackagePolicy,
StepDefinePackagePolicy,
Expand Down Expand Up @@ -86,6 +86,7 @@ export const EditPackagePolicyForm = memo<{
forceUpgrade?: boolean;
from?: EditPackagePolicyFrom;
}>(({ packagePolicyId, forceUpgrade = false, from = 'edit' }) => {
const params = useParams<AddToPolicyParams>();
const { application, notifications } = useStartServices();
const {
agents: { enabled: isFleetEnabled },
Expand Down Expand Up @@ -116,6 +117,14 @@ export const EditPackagePolicyForm = memo<{
forceUpgrade,
});

const integrationInfo = useMemo(
() =>
packageInfo?.policy_templates?.find(
(policyTemplate) => policyTemplate.name === params.integration
),
[packageInfo?.policy_templates, params]
);

const canWriteIntegrationPolicies = useAuthz().integrations.writeIntegrationPolicies;

const policyId = agentPolicy?.id ?? '';
Expand Down Expand Up @@ -240,6 +249,10 @@ export const EditPackagePolicyForm = memo<{
};

const extensionView = useUIExtension(packagePolicy.package?.name ?? '', 'package-policy-edit');
const replaceDefineStepView = useUIExtension(
packagePolicy.package?.name ?? '',
'package-policy-replace-define-step'
);
const extensionTabsView = useUIExtension(
packagePolicy.package?.name ?? '',
'package-policy-edit-tabs'
Expand Down Expand Up @@ -274,55 +287,76 @@ export const EditPackagePolicyForm = memo<{
: [],
};

if (replaceDefineStepView && extensionView) {
// eslint-disable-next-line no-console
console.warn(
"'package-policy-edit' is ignored when 'package-policy-replace-define-step' is defined"
);
}

const configurePackage = useMemo(
() =>
agentPolicy && packageInfo ? (
<>
{selectedTab === 0 && (
<StepDefinePackagePolicy
replaceDefineStepView && originalPackagePolicy ? (
<ExtensionWrapper>
<replaceDefineStepView.Component
agentPolicy={agentPolicy}
packageInfo={packageInfo}
packagePolicy={packagePolicy}
updatePackagePolicy={updatePackagePolicy}
validationResults={validationResults!}
submitAttempted={formState === 'INVALID'}
isEditPage={true}
/>
)}

{/* Only show the out-of-box configuration step if a UI extension is NOT registered */}
{!extensionView && selectedTab === 0 && (
<StepConfigurePackagePolicy
packageInfo={packageInfo}
packagePolicy={packagePolicy}
updatePackagePolicy={updatePackagePolicy}
validationResults={validationResults!}
submitAttempted={formState === 'INVALID'}
policy={originalPackagePolicy}
newPolicy={packagePolicy}
onChange={handleExtensionViewOnChange}
validationResults={validationResults}
integrationInfo={integrationInfo}
isEditPage={true}
/>
)}

{extensionView &&
packagePolicy.policy_id &&
packagePolicy.package?.name &&
originalPackagePolicy && (
<ExtensionWrapper>
{selectedTab > 0 && tabsViews ? (
React.createElement(tabsViews[selectedTab - 1].Component, {
policy: originalPackagePolicy,
newPolicy: packagePolicy,
onChange: handleExtensionViewOnChange,
})
) : (
<extensionView.Component
policy={originalPackagePolicy}
newPolicy={packagePolicy}
onChange={handleExtensionViewOnChange}
/>
)}
</ExtensionWrapper>
</ExtensionWrapper>
) : (
<>
{selectedTab === 0 && (
<StepDefinePackagePolicy
agentPolicy={agentPolicy}
packageInfo={packageInfo}
packagePolicy={packagePolicy}
updatePackagePolicy={updatePackagePolicy}
validationResults={validationResults!}
submitAttempted={formState === 'INVALID'}
isEditPage={true}
/>
)}
</>
{/* Only show the out-of-box configuration step if a UI extension is NOT registered */}
{!extensionView && selectedTab === 0 && (
<StepConfigurePackagePolicy
packageInfo={packageInfo}
packagePolicy={packagePolicy}
updatePackagePolicy={updatePackagePolicy}
validationResults={validationResults!}
submitAttempted={formState === 'INVALID'}
isEditPage={true}
/>
)}

{extensionView &&
packagePolicy.policy_id &&
packagePolicy.package?.name &&
originalPackagePolicy && (
<ExtensionWrapper>
{selectedTab > 0 && tabsViews ? (
React.createElement(tabsViews[selectedTab - 1].Component, {
policy: originalPackagePolicy,
newPolicy: packagePolicy,
onChange: handleExtensionViewOnChange,
})
) : (
<extensionView.Component
policy={originalPackagePolicy}
newPolicy={packagePolicy}
onChange={handleExtensionViewOnChange}
/>
)}
</ExtensionWrapper>
)}
</>
)
) : null,
[
agentPolicy,
Expand All @@ -336,6 +370,8 @@ export const EditPackagePolicyForm = memo<{
handleExtensionViewOnChange,
selectedTab,
tabsViews,
integrationInfo,
replaceDefineStepView,
]
);

Expand Down
35 changes: 34 additions & 1 deletion 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 @@ -10,7 +10,16 @@ import type { ComponentType, LazyExoticComponent } from 'react';

import type { FleetServerAgentComponentUnit } from '../../common/types/models/agent';

import type { Agent, NewPackagePolicy, PackageInfo, PackagePolicy } from '.';
import type { PackagePolicyValidationResults } from '../services';

import type {
Agent,
AgentPolicy,
NewPackagePolicy,
PackageInfo,
PackagePolicy,
RegistryPolicyTemplate,
} from '.';

/** Register a Fleet UI extension */
export type UIExtensionRegistrationCallback = (extensionPoint: UIExtensionPoint) => void;
Expand All @@ -20,6 +29,23 @@ export interface UIExtensionsStorage {
[key: string]: Partial<Record<UIExtensionPoint['view'], UIExtensionPoint>>;
}

/**
* UI Component Extension is used to replace the Define Step on
* the pages displaying the ability to edit/create an Integration Policy
*/
export type PackagePolicyReplaceDefineStepExtensionComponent =
ComponentType<PackagePolicyReplaceDefineStepExtensionComponentProps>;

export type PackagePolicyReplaceDefineStepExtensionComponentProps = (
| (PackagePolicyEditExtensionComponentProps & { isEditPage: true })
| (PackagePolicyCreateExtensionComponentProps & { isEditPage: false })
) & {
validationResults?: PackagePolicyValidationResults;
agentPolicy?: AgentPolicy;
packageInfo: PackageInfo;
integrationInfo?: RegistryPolicyTemplate;
};
Copy link
Contributor Author

@orouz orouz Jan 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

since this extension replaces the first step entirely, and it slightly differs between edit and create, i've used the same props from the relevant existing extension points.

the added props are from the actual props StepDefinePackagePolicy receives, so any functionality it does inside an be replicated.


/**
* UI Component Extension is used on the pages displaying the ability to edit an
* Integration Policy
Expand Down Expand Up @@ -73,6 +99,12 @@ export interface PackageGenericErrorsListProps {
packageErrors: FleetServerAgentComponentUnit[];
}

export interface PackagePolicyReplaceDefineStepExtension {
package: string;
view: 'package-policy-replace-define-step';
Component: LazyExoticComponent<PackagePolicyReplaceDefineStepExtensionComponent>;
}

/** Extension point registration contract for Integration Policy Edit views */
export interface PackagePolicyEditExtension {
package: string;
Expand Down Expand Up @@ -184,6 +216,7 @@ export interface AgentEnrollmentFlyoutFinalStepExtension {

/** Fleet UI Extension Point */
export type UIExtensionPoint =
| PackagePolicyReplaceDefineStepExtension
| PackagePolicyEditExtension
| PackagePolicyResponseExtension
| PackagePolicyEditTabsExtension
Expand Down