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

Jump to bottom

[Fleet] Append space ID to security solution tag #170789

Merged
merged 5 commits into from
Nov 10, 2023
Merged

[Fleet] Append space ID to security solution tag #170789

merged 5 commits into from
Nov 10, 2023

Conversation

kpollich
Copy link
Member

@kpollich kpollich commented Nov 7, 2023 •
edited by kibanamachine
Loading

Summary

Fixes #166798

Appends the current space ID to the ID of the security solution tag.

Note: If there are integrations suffering from the above bug (might be "stuck" in installing status, showing concurrent installation errors, etc), they should be reinstalled via the API in their corresponding space, e.g.

# In Kibana dev tools for the space in which the integration is installed
POST kbn:/api/fleet/epm/packages/cisco_asa/2.27.1
{
  "force": true
}

@kpollich kpollich added release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Nov 7, 2023
@kpollich kpollich self-assigned this Nov 7, 2023
@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@kpollich
Copy link
Member Author

kpollich commented Nov 8, 2023

@elasticmachine merge upstream

@kpollich kpollich requested a review from P1llus November 8, 2023 16:09
@kpollich kpollich marked this pull request as ready for review November 8, 2023 16:09
@kpollich kpollich requested a review from a team as a code owner November 8, 2023 16:09
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

criamico
criamico approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@P1llus
Copy link
Member

P1llus commented Nov 8, 2023

@criamico this was hardcoded for a reason before right? Because of the duplicate created by the SIEM UI? Mentioned it to @kpollich as well just in case, so we are not surprised after the merge :)

@criamico
Copy link
Contributor

criamico commented Nov 8, 2023

@P1llus so I went back to the discussion we had over this and it seems that the id security-solution-default was hardcoded as a temporary solution to tag the security assets. Here's a screenshot I had back then:

Screenshot 2023-08-23 at 15 07 09

The first tag in the screenshot is Fleet's one, and has security-solution-default id; the second tag is created by Security Solution and has a randomized id. I remember we said that we would hardcode Fleet's id for now and find a shared solution later to have a global, unique id, but I don't think we got to work on it.
So it should be fine for now to change the id (at least to fix this bug), but we still need to agree on a better solution going forward.

@kpollich
Copy link
Member Author

kpollich commented Nov 8, 2023

I have two separate recordings, one for the behavior on this branch and one for the behavior on main on a staging cloud instance:

Screenshare.-.2023-11-08.12_47_08.PM.mp4
Screenshare.-.2023-11-08.12_50_18.PM.mp4

(Please forgive my son shrieking in the background 👶)

To summarize the behavior in this PR:

  • When Fleet encounters a tag asset in an integration with a name of Security Solution, it will create a tag with that name and an ID of security-solution-{currentSpaceId} to avoid collision
  • The security solution dashboards UI queries for tags with name Security Solution, if one doesn't exist it will create one
  • If you navigate to the security solution dashboards page before installing a security solution tagged integration, you can wind up with this duplicate tag - this is current behavior in main
    • To fix this I think we'd need to update the security solution app to pass the same predictable ID here, however I don't think the underlying tag API support the ID parameter so we'd also need to touch core code
    • I can look a little more at including the above in this PR, but if I have to touch too many files owned by other teams I might punt on fixing this here

@kpollich
Copy link
Member Author

I've figured out where the duplicate tag creation logic comes from the in the security solution app. We check for the "Security Solution" tag existing at the same time as the request to install bulk install endpoint and security_detection_engine is processing. So, the tag logic completes first, as it's naturally much faster, doesn't see a tag by the name "Security Solution", and creates its own tag with an unpredictable ID.

We could fix this a few ways

  1. Provide the same predictable ID for the tag created by the security solution UI, and ensure we're tolerant of a saved object conflict
  2. Delete the tag creation code from security solution UI and rely on the package installation process to create the tag for us instead
  3. Orchestrate these requests to be more sequential, e.g. don't show the "tagged dashboards" list UI until the installation request has completed (potentially makes the UI too slow)

@elastic/security-solution (big ping, sorry - just using what's in CODEOWNERS for this codepath in question) FYI. I don't want to make any of those decisions for y'all in this bugfix PR, and since the Fleet changes aren't breaking any existing behavior (see comment above), I think we're good to merge this on our end to fix the conflict issues. Just want to bring to your team's attention that we have a potential case where duplicate "Security Solution" tags can be created.

@kpollich
Copy link
Member Author

@elasticmachine merge upstream

@kpollich kpollich enabled auto-merge (squash) November 10, 2023 14:36
@kpollich
Copy link
Member Author

Looks like #167694 already captures what I've listed above, so apologies for the needless ping 😶

@kpollich kpollich merged commit dd2fda2 into elastic:main Nov 10, 2023
28 checks passed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @kpollich

@kibanamachine kibanamachine mentioned this pull request Nov 10, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Nov 10, 2023
@kpollich
[Fleet] Append space ID to security solution tag (elastic#170789)
8516766 
## Summary

Fixes elastic#166798

Appends the current space ID to the ID of the security solution tag.

Note: If there are integrations suffering from the above bug (might be
"stuck" in `installing` status, showing concurrent installation errors,
etc), they should be reinstalled via the API in their corresponding
space, e.g.

```
# In Kibana dev tools for the space in which the integration is installed
POST kbn:/api/fleet/epm/packages/cisco_asa/2.27.1
{
  "force": true
}
```

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit dd2fda2)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.11

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Nov 10, 2023
@kibanamachine @kpollich
[8.11] [Fleet] Append space ID to security solution tag (#170789) (#1…
b07686d 
…71034)

# Backport

This will backport the following commits from `main` to `8.11`:
- [[Fleet] Append space ID to security solution tag
(#170789)](#170789)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Kyle
Pollich","email":"kyle.pollich@elastic.co"},"sourceCommit":{"committedDate":"2023-11-10T16:01:36Z","message":"[Fleet]
Append space ID to security solution tag (#170789)\n\n##
Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/166798\r\n\r\nAppends the
current space ID to the ID of the security solution tag.\r\n\r\nNote: If
there are integrations suffering from the above bug (might
be\r\n\"stuck\" in `installing` status, showing concurrent installation
errors,\r\netc), they should be reinstalled via the API in their
corresponding\r\nspace, e.g.\r\n\r\n```\r\n# In Kibana dev tools for the
space in which the integration is installed\r\nPOST
kbn:/api/fleet/epm/packages/cisco_asa/2.27.1\r\n{\r\n \"force\":
true\r\n}\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"dd2fda271187f718def78002516861736dc48cf7","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Fleet","backport:prev-minor","v8.12.0"],"number":170789,"url":"https://github.com/elastic/kibana/pull/170789","mergeCommit":{"message":"[Fleet]
Append space ID to security solution tag (#170789)\n\n##
Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/166798\r\n\r\nAppends the
current space ID to the ID of the security solution tag.\r\n\r\nNote: If
there are integrations suffering from the above bug (might
be\r\n\"stuck\" in `installing` status, showing concurrent installation
errors,\r\netc), they should be reinstalled via the API in their
corresponding\r\nspace, e.g.\r\n\r\n```\r\n# In Kibana dev tools for the
space in which the integration is installed\r\nPOST
kbn:/api/fleet/epm/packages/cisco_asa/2.27.1\r\n{\r\n \"force\":
true\r\n}\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"dd2fda271187f718def78002516861736dc48cf7"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/170789","number":170789,"mergeCommit":{"message":"[Fleet]
Append space ID to security solution tag (#170789)\n\n##
Summary\r\n\r\nFixes
https://github.com/elastic/kibana/issues/166798\r\n\r\nAppends the
current space ID to the ID of the security solution tag.\r\n\r\nNote: If
there are integrations suffering from the above bug (might
be\r\n\"stuck\" in `installing` status, showing concurrent installation
errors,\r\netc), they should be reinstalled via the API in their
corresponding\r\nspace, e.g.\r\n\r\n```\r\n# In Kibana dev tools for the
space in which the integration is installed\r\nPOST
kbn:/api/fleet/epm/packages/cisco_asa/2.27.1\r\n{\r\n \"force\":
true\r\n}\r\n```\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"dd2fda271187f718def78002516861736dc48cf7"}}]}]
BACKPORT-->

Co-authored-by: Kyle Pollich <kyle.pollich@elastic.co>
@logeekal logeekal mentioned this pull request Nov 16, 2023
Closed
@kpollich kpollich mentioned this pull request Nov 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@criamico criamico criamico approved these changes

@P1llus P1llus Awaiting requested review from P1llus

Assignees

@kpollich kpollich

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:Fleet Team label for Observability Data Collection Fleet team v8.11.1 v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Kibana] Upgrading Integrations leads to Saved object [tag/security-solution-default] conflict
7 participants
@kpollich @apmmachine @elasticmachine @P1llus @criamico @kibana-ci @kibanamachine