[Cases] Adding feature flag for sub cases #95122

jonathan-buttner · 2021-03-22T22:50:54Z

This PR adds a feature flag to disable the sub cases feature by default. This is necessary because the sub cases feature won't ship in 7.13.

To enable the feature set the ENABLE_CASE_CONNECTOR flag to true.

The cases plugin will no longer register case as a connector by default. It will only be registered if the flag is enabled. This is different than what is done in 7.12 and 7.11.

The work for this feature is being tracked here: #94115

API integration tests

I added some tests to ensure that the sub case routes resulted in a 404 response and some tests to ensure that the routes that used the subCaseId ignored it or errored with a 400 accordingly.

jonathan-buttner · 2021-03-22T22:51:43Z

.github/CODEOWNERS

@@ -339,6 +339,7 @@
 # Security Solution sub teams
 /x-pack/plugins/case @elastic/security-threat-hunting
 /x-pack/test/case_api_integration @elastic/security-threat-hunting
+/x-pack/test/case_api_no_sub_cases_integration @elastic/security-threat-hunting


Once the sub cases feature is released we can delete these tests

Is there any issue where we track what we should remove/enable when the sub cases feature is released?

Good idea, I added it as an item here: #94115

jonathan-buttner · 2021-03-23T14:03:53Z

x-pack/plugins/security_solution/common/constants.ts

@@ -171,7 +171,7 @@ export const ML_GROUP_IDS = [ML_GROUP_ID, LEGACY_ML_GROUP_ID];
 /*
  Rule notifications options
 */
-export const ENABLE_CASE_CONNECTOR = true;
+export const ENABLE_CASE_CONNECTOR = false;


@XavierM is there a better way to do this? could we tile this value to the subCasesEnabled feature flag?

I think so we do not need to have two flags for the same thing, SUB_CASES_ENABLED or CASE_CONNECTOR_ENABLED should be sufficient.

Hmm I think we might need two flags because the sub cases feature flag is stored in the config.ts and is passed into the case api integration tests. I don't think we can use that value in the security solution right? The flag needs to exist in both cases and security solution right?

I moved this flag to the cases plugin so we only have one flag now 👍

elasticmachine · 2021-03-23T14:04:10Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

…cases

x-pack/plugins/cases/server/client/cases/update.ts

cnasikas · 2021-03-24T12:05:33Z

x-pack/plugins/cases/server/connectors/case/index.ts

@@ -86,6 +86,9 @@ async function executor(
    userActionService,
    alertsService,
    logger,
+    // sub-cases-enabled: force this to true because we should never be able to call execute if the feature is disabled in the first
+    // place. The plugin won't register case as a connector if subCasesEnabled is false.
+    subCasesEnabled: true,


As a precaution measure maybe we should throw an error if subCasesEnabled: true so we don't allow the execution of the connector at all.

Good idea, I added it.

x-pack/plugins/cases/server/plugin.ts

cnasikas · 2021-03-24T12:16:38Z

x-pack/plugins/cases/server/routes/api/cases/comments/delete_all_comments.ts

 }: RouteDeps) {
+  const querySchema = subCasesEnabled


Do we document this API? If yes, then conditional params will be difficult to documented it. Why don't we leave the schema as it is but do not take into account the subCaseId at all and throw an error.

I was thinking the same way, can we just have a global const for now

cnasikas · 2021-03-24T12:16:59Z

x-pack/plugins/cases/server/routes/api/cases/comments/delete_comment.ts

 }: RouteDeps) {
+  const querySchema = subCasesEnabled


https://github.com/elastic/kibana/pull/95122/files#r600423216

cnasikas · 2021-03-24T12:19:16Z

x-pack/plugins/cases/server/routes/api/cases/comments/find_comments.ts

-          FindQueryParamsRt.decode(request.query),
-          fold(throwErrors(Boom.badRequest), identity)
-        );
+        const query = {


I think is best to leave the schema as it was and just const subCaseId = subCasesEnabled ? query.subCaseId : undefined and then relay on this variable for all check afterwards.

I throw if the query parameter is defined now.

cnasikas · 2021-03-24T12:21:32Z

x-pack/plugins/cases/server/routes/api/cases/comments/get_all_comment.ts

@@ -14,20 +14,25 @@ import { flattenCommentSavedObjects, wrapError } from '../../utils';
 import { CASE_COMMENTS_URL } from '../../../../../common/constants';
 import { defaultSortField } from '../../../../common';

-export function initGetAllCommentsApi({ caseService, router, logger }: RouteDeps) {
+export function initGetAllCommentsApi({ caseService, router, logger, subCasesEnabled }: RouteDeps) {
+  const querySchema = subCasesEnabled


It seems that this pattern is being applied in our routes. Why we need to remove the subCases parameters?

I removed the flag from being passed around just import the global directly now.

cnasikas · 2021-03-24T12:33:03Z

x-pack/test/case_api_no_sub_cases_integration/basic/tests/cases/comments/find_comments.ts

+
+      it('fails to find comments for a sub case', async () => {
+        const { body } = await supertest
+          .get(`${CASES_URL}/invalid-id/comments/_find?subCaseId=somevalue`)


Maybe I am missing something, but I cannot see how this test test the scenario described. We don't have comments, caseId is invalid, subCaseId is invalid so the total will always be 0.

Good point, I have the routes through now 👍

x-pack/test/case_api_no_sub_cases_integration/basic/tests/cases/comments/delete_comment.ts

x-pack/test/case_api_no_sub_cases_integration/basic/tests/cases/comments/get_all_comments.ts

…cases

jonathan-buttner · 2021-03-24T20:30:19Z

x-pack/test/case_api_integration/basic/tests/cases/comments/get_all_comments.ts

@@ -63,47 +56,78 @@ export default ({ getService }: FtrProviderContext): void => {
      expect(comments.length).to.eql(2);
    });

-    it('should get comments from a case and its sub cases', async () => {


These tests were indented and placed in a new describe block.

jonathan-buttner · 2021-03-24T20:31:34Z

x-pack/test/case_api_integration/basic/tests/cases/sub_cases/delete_sub_cases.ts

-      const { newSubCaseInfo: caseInfo } = await createSubCase({ supertest, actionID });
-      expect(caseInfo.subCases![0].id).to.not.eql(undefined);
+    // ENABLE_SUB_CASES: once the case connector feature is completed unskip these tests
+    describe.skip('delete_sub_cases', () => {


These were indented.

jonathan-buttner · 2021-03-24T20:53:11Z

x-pack/plugins/cases/common/constants.ts

@@ -5,8 +5,6 @@
 * 2.0.
 */

-import { DEFAULT_MAX_SIGNALS } from '../../security_solution/common/constants';


I need to remove this import to avoid the circular import issue.

XavierM

Nicely done, good job!!!

kibanamachine · 2021-03-25T17:23:06Z

💚 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`	7.2MB	7.2MB	-2.6KB

Page load bundle

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

id	before	after	diff
`securitySolution`	211.3KB	214.0KB	+2.7KB

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`cases-sub-case`	17	-	-17

History

💚 Build #115054 succeeded 1d8a1e2
💛 Build #114709 was flaky dcf1907
💚 Build #114708 succeeded 1ae829f
💔 Build #114567 failed 24cd187
💚 Build #114452 succeeded 8c1ce31

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

* Adding feature flag for sub cases * Disabling case as a connector in security solution * Fix connector test * Switching feature flag to global variable * Removing this.config use * Fixing circular import and renaming flag

Adding feature flag for sub cases

8c1ce31

jonathan-buttner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Feature:Cases Cases feature v7.13.0 labels Mar 22, 2021

jonathan-buttner requested review from cnasikas and XavierM March 22, 2021 22:51

jonathan-buttner commented Mar 22, 2021

View reviewed changes

Disabling case as a connector in security solution

24cd187

jonathan-buttner commented Mar 23, 2021

View reviewed changes

jonathan-buttner marked this pull request as ready for review March 23, 2021 14:04

jonathan-buttner requested review from a team as code owners March 23, 2021 14:04

jonathan-buttner added 2 commits March 23, 2021 15:20

Fix connector test

1ae829f

Merge branch 'master' of github.com:elastic/kibana into disabled-sub-…

dcf1907

…cases

jonathan-buttner mentioned this pull request Mar 23, 2021

[Cases] Fix _find API total bug #95235

Merged

cnasikas reviewed Mar 24, 2021

View reviewed changes

x-pack/plugins/cases/server/client/cases/update.ts Show resolved Hide resolved

cnasikas reviewed Mar 24, 2021

View reviewed changes

x-pack/plugins/cases/server/plugin.ts Outdated Show resolved Hide resolved

cnasikas reviewed Mar 24, 2021

View reviewed changes

x-pack/test/case_api_no_sub_cases_integration/basic/tests/cases/comments/delete_comment.ts Outdated Show resolved Hide resolved

cnasikas reviewed Mar 24, 2021

View reviewed changes

x-pack/test/case_api_no_sub_cases_integration/basic/tests/cases/comments/get_all_comments.ts Outdated Show resolved Hide resolved

jonathan-buttner added 2 commits March 24, 2021 16:00

Switching feature flag to global variable

3fa266a

Merge branch 'master' of github.com:elastic/kibana into disabled-sub-…

801c920

…cases

jonathan-buttner commented Mar 24, 2021

View reviewed changes

jonathan-buttner added 2 commits March 24, 2021 16:38

Removing this.config use

39436e8

Fixing circular import and renaming flag

1d8a1e2

jonathan-buttner commented Mar 24, 2021

View reviewed changes

XavierM approved these changes Mar 25, 2021

View reviewed changes

Fixing estype merge conflicts

38f48e0

jonathan-buttner merged commit 07f32d0 into elastic:master Mar 25, 2021

jonathan-buttner deleted the disabled-sub-cases branch March 25, 2021 17:36

jonathan-buttner mentioned this pull request Mar 25, 2021

[7.x] [Cases] Adding feature flag for sub cases (#95122) #95453

Merged

jonathan-buttner mentioned this pull request Mar 25, 2021

[Cases] Disable Case Connector #95469

Closed

cnasikas mentioned this pull request Mar 30, 2021

[Security Solution] [Cases] Deleting files from security solution and removing feature flag #95176

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Adding feature flag for sub cases #95122

[Cases] Adding feature flag for sub cases #95122

jonathan-buttner commented Mar 22, 2021 •

edited

Loading

jonathan-buttner Mar 22, 2021

cnasikas Mar 24, 2021

jonathan-buttner Mar 24, 2021

jonathan-buttner Mar 23, 2021

XavierM Mar 23, 2021 •

edited

Loading

jonathan-buttner Mar 23, 2021

jonathan-buttner Mar 24, 2021

elasticmachine commented Mar 23, 2021

cnasikas Mar 24, 2021

jonathan-buttner Mar 24, 2021

cnasikas Mar 24, 2021 •

edited

Loading

XavierM Mar 24, 2021

cnasikas Mar 24, 2021

cnasikas Mar 24, 2021

jonathan-buttner Mar 24, 2021

cnasikas Mar 24, 2021 •

edited

Loading

jonathan-buttner Mar 24, 2021

cnasikas Mar 24, 2021 •

edited

Loading

jonathan-buttner Mar 24, 2021

jonathan-buttner Mar 24, 2021

jonathan-buttner Mar 24, 2021

jonathan-buttner Mar 24, 2021

XavierM left a comment

kibanamachine commented Mar 25, 2021

[Cases] Adding feature flag for sub cases #95122

[Cases] Adding feature flag for sub cases #95122

Conversation

jonathan-buttner commented Mar 22, 2021 • edited Loading

API integration tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Mar 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XavierM left a comment

Choose a reason for hiding this comment

kibanamachine commented Mar 25, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

Saved Objects .kibana field count

History

jonathan-buttner commented Mar 22, 2021 •

edited

Loading

XavierM Mar 23, 2021 •

edited

Loading

cnasikas Mar 24, 2021 •

edited

Loading

cnasikas Mar 24, 2021 •

edited

Loading

cnasikas Mar 24, 2021 •

edited

Loading