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.

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

[TIP] Enables TI plugin with kibana.yml feature flag #137838

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Aug 2, 2022

Summary

The Threat Intelligence plugin (introduced in this PR) and is not yet ready for General Availability. This PR hides the new plugin behind a feature flag. As the plugin is dynamically loaded within the Security Solution plugin, we're leveraging the already existing allowedExperimentalValues constant to:

  • show/hide the navbar entries in both old and new navigation
  • load the plugin

How to test

Threat Intelligence plugin disabled:

Screen.Recording.2022-08-02.at.2.51.21.PM.mov

Threat Intelligence plugin enabled:

  • in your kibana.yml file, add the following entry at the bottom xpack.securitySolution.enableExperimental: ['threatIntelligenceEnabled']
  • check that
Screen.Recording.2022-08-02.at.2.57.27.PM.mov

When the Threat Intelligence plugin is ready for General Availability, we should be able to simply do a revert of this commit! :)

https://github.com/elastic/security-team/issues/4505

Checklist

Delete any items that are not applicable to this PR.

@PhilippeOberti PhilippeOberti requested review from a team as code owners August 2, 2022 13:02
@@ -22,9 +22,10 @@ export const allowedExperimentalValues = Object.freeze({
pendingActionResponsesWithAck: true,
policyListEnabled: true,
policyResponseInFleetEnabled: true,
threatIntelligenceEnabled: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I was allowed to use this object but it seemed to make sense, as the Threat Intelligence plugin is loaded within the Security Solution plugin

): Promise<AppLinkItems> => {
const managementFilteredLinks = await getManagementFilteredLinks(core, plugins);

const threatHuntingFilteredLinks = {
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 hesitated to go the opposite way (adding the threatIntelligence entry to the threatHuntingLandingLinks.link property) but going this current route meant modifying one less file.

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to do this, links have support for experimentalFeatures, you can add the experimentalKey: 'threatIntelligenceEnabled', prop directly to the threat intelligence link definition at: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/threat_intelligence/links.ts

Copy link
Contributor Author

@PhilippeOberti PhilippeOberti Aug 4, 2022

Choose a reason for hiding this comment

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

haha I had took a note of this experimentalKey while working on another feature, tried it and didn't get it to work... I'll try again thank you :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that worked, I don't know what I did... must have forgotten to update my kibana.yml... 🤦‍♂️

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes 8.4 candidate v8.4.0 Team: Protections Experience backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Aug 2, 2022
@lgestc
Copy link
Contributor

lgestc commented Aug 2, 2022

How does it work with our e2e suite?:)

@PhilippeOberti
Copy link
Contributor Author

How does it work with our e2e suite?:)

crap I forgot about that, looking into it now 😆

@PhilippeOberti PhilippeOberti requested a review from a team as a code owner August 2, 2022 13:55
@PhilippeOberti
Copy link
Contributor Author

@PhilippeOberti PhilippeOberti requested a review from lgestc August 2, 2022 13:56
Copy link
Contributor

@maxcold maxcold left a comment

Choose a reason for hiding this comment

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

Tested, works as expected

@PhilippeOberti PhilippeOberti requested a review from a team as a code owner August 2, 2022 20:41
- show/hide navbar entries in both old and new navigation per feature flag value
- load plugin per feature flag value
- flag: xpack.securitySolution.enableExperimental: ['threatIntelligenceEnabled']

elastic/security-team#4505
): Promise<AppLinkItems> => {
const managementFilteredLinks = await getManagementFilteredLinks(core, plugins);

const threatHuntingFilteredLinks = {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to do this, links have support for experimentalFeatures, you can add the experimentalKey: 'threatIntelligenceEnabled', prop directly to the threat intelligence link definition at: https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/threat_intelligence/links.ts

@semd
Copy link
Contributor

semd commented Aug 4, 2022

Would it be possible to start the subPlugin normally in the plugin and check the experimental flag inside the threat_intelligence main component?

const ThreatIntelligenceRoutes = () => (
<TrackApplicationView viewId={SecurityPageName.threatIntelligence}>
<ThreatIntelligencePage />
</TrackApplicationView>
);
export const routes: SecuritySubPluginRoutes = [
{
path: THREAT_INTELLIGENCE_PATH,
render: ThreatIntelligenceRoutes,
},
];

So we don't have to do all those conditionals in the plugin.tsx and the types file. It will be easier to undo later on as well.
Suggestion:

import { useIsExperimentalFeatureEnabled } from '../common/hooks/use_experimental_features';

const ThreatIntelligenceRoutes = () => {
  const enabled = useIsExperimentalFeatureEnabled('threatIntelligenceEnabled');
  if (!enabled) {
    return null;
  }
  return (
    <TrackApplicationView viewId={SecurityPageName.threatIntelligence}>
      <ThreatIntelligencePage />
    </TrackApplicationView>
  );
};

export const routes: SecuritySubPluginRoutes = [
  {
    path: THREAT_INTELLIGENCE_PATH,
    component: ThreatIntelligenceRoutes,
  },
];

@PhilippeOberti
Copy link
Contributor Author

Would it be possible to start the subPlugin normally in the plugin and check the experimental flag inside the threat_intelligence main component?

@semd absolutely! I thought I was being smart by not having the plugin load at all, but if this is not required/desired I'll gladly remove all the logic and make this PR a lot smaller!

Plugin is now loaded all the time, and the logic to show the indicators page or route back to the Get Started page from Security is handled within the Threat Intelligence folder
Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM 🚀
thanks

@@ -52,6 +52,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) {
'riskyHostsEnabled',
'riskyUsersEnabled',
'insightsRelatedAlertsByProcessAncestry',
'threatIntelligenceEnabled',
Copy link
Member

Choose a reason for hiding this comment

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

Is there any plan of adding specific TI plugin tests on Security Solution Cypress tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MadameSheema we have some e2e Cypress tests in our plugin directly and this PR updates some e2e Cypress tests within the Security Solution plugin.

I wasn't thinking about adding any other tests, especially because this feature flag is temporary and should be very short lived. We're planning on releasing everything in 8.5 (plans could change though).

Did you have anything specify in mind you wanted me/us to add?

@PhilippeOberti
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.6MB 5.6MB +240.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 254.7KB 254.8KB +29.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Member

@MadameSheema MadameSheema left a comment

Choose a reason for hiding this comment

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

LGTM

@PhilippeOberti PhilippeOberti merged commit ff28e68 into elastic:main Aug 9, 2022
@PhilippeOberti PhilippeOberti deleted the tip-feature-flag branch August 9, 2022 10:00
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 9, 2022
- show/hide navbar entries in both old and new navigation per feature flag value
- load plugin per feature flag value
- flag: xpack.securitySolution.enableExperimental: ['threatIntelligenceEnabled']

elastic/security-team#4505
(cherry picked from commit ff28e68)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.4

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 9, 2022
…elastic#138373)

- show/hide navbar entries in both old and new navigation per feature flag value
- load plugin per feature flag value
- flag: xpack.securitySolution.enableExperimental: ['threatIntelligenceEnabled']

elastic/security-team#4505
(cherry picked from commit ff28e68)

Co-authored-by: Philippe Oberti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.4 candidate backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team: Protections Experience v8.4.0 v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants