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

[Fleet] Don't throw concurrent installation error when encountering conflicts #173190

Merged

Conversation

kpollich
Copy link
Member

@kpollich kpollich commented Dec 12, 2023

Summary

Closes #171986

How to test

You'll need to get into a state where a tag with a conflicting ID exists in a second Kibana space outside of the default space.

It seems like the tags API doesn't support providing an ID at least from the REST API, so that makes creating duplicate tags in other spaces a bit challenging.

e.g.

POST kbn:api/saved_objects_tagging/tags/create
{
  "id": "my-tag",
  "name": "My tag",
  "description": "",
  "color": "#FFFFFF"
}

results in

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "[request body.id]: definition for this key is missing"
}

So, in order to enable creating tags with ID's easily, I made some quick code changes to the tags service

diff --git a/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts b/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
index 0c48168eed2..fa0e7fbe7b9 100644
--- a/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
+++ b/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
@@ -6,6 +6,7 @@
  */
 
 import { schema } from '@kbn/config-schema';
+import { omit } from 'lodash';
 import type { TagsPluginRouter } from '../../types';
 import { TagValidationError } from '../../services/tags';
 
@@ -15,6 +16,7 @@ export const registerCreateTagRoute = (router: TagsPluginRouter) => {
       path: '/api/saved_objects_tagging/tags/create',
       validate: {
         body: schema.object({
+          id: schema.maybe(schema.string()),
           name: schema.string(),
           description: schema.string(),
           color: schema.string(),
@@ -32,7 +34,7 @@ export const registerCreateTagRoute = (router: TagsPluginRouter) => {
           });
         }
 
-        const tag = await tagsClient.create(req.body);
+        const tag = await tagsClient.create(omit(req.body, 'id'), { id: req.body.id });
         return res.ok({
           body: {
             tag,

With those changes in place (I don't think committing them is necessary), I was able to create a tag in my second space e.g.

POST kbn:api/saved_objects_tagging/tags/create
{
  "id": "fleet-pkg-nginx-default",
  "name": "Nginx duplicate tag",
  "description": "",
  "color": "#DADADA"
}

Then, from my default space I installed the nginx package

POST kbn:/api/fleet/epm/packages/nginx

This throws the concurrent install error as expected on main

{
  "statusCode": 409,
  "error": "Conflict",
  "message": "Concurrent installation or upgrade of nginx-1.17.0 detected, aborting. Original error: Saved object [tag/fleet-pkg-nginx-default] conflict"
}

With this PR, we get this error instead

{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "Saved Object conflict encountered while installing nginx-1.17.0. There may be a conflicting Saved Object saved to another Space. Original error: Saved object [tag/fleet-pkg-nginx-default] conflict"
}

@kpollich kpollich self-assigned this Dec 12, 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 kpollich added release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team labels Dec 12, 2023
@nchaulet
Copy link
Member

nchaulet commented Jan 3, 2024

I think you can reproduce the conflict error by manually creating a tag with the same id in a different space, before installing a package

@kpollich
Copy link
Member Author

kpollich commented Jan 4, 2024

@elasticmachine merge upstream

@kpollich
Copy link
Member Author

kpollich commented Jan 4, 2024

I have added testing instructions + a basic unit test that ensures we throw the right error type in this instance, but it's not the most valuable test imo.

I would have liked to try and add some logic to https://github.com/elastic/kibana/blob/main/x-pack/test/fleet_api_integration/apis/epm/install_integration_in_multiple_spaces.ts for a better integration test but

  1. That test suite is skipped for flakiness
  2. We'd need to add some API helpers to create tags in secondary spaces, as they don't exist today
  3. It might be better to figure out loading a data archive or something with the tags set up the way we want, then trying various install operations - and I don't really know how to get started on that. This might also be the path to fix the flakiness of that suite, and we'd want to refactor all the existing tests to use this strategy if we pursue it.

@kpollich kpollich marked this pull request as ready for review January 4, 2024 15:59
@kpollich kpollich requested a review from a team as a code owner January 4, 2024 15:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@kpollich
Copy link
Member Author

@elasticmachine merge upstream

@kpollich
Copy link
Member Author

I would have liked to add an integration test for this, but there's not a great way to create tags in other spaces to force this conflict case from FTR tests that I've found. Rather than hit my head on that for any longer, I think it's okay to consider this ready to merge.

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 🚢

@kpollich kpollich enabled auto-merge (squash) January 10, 2024 16:01
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

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

cc @kpollich

@kpollich kpollich merged commit 88b74ba into elastic:main Jan 10, 2024
21 checks passed
@kibanamachine kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 10, 2024
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 11, 2024
…onflicts (elastic#173190)

## Summary

Closes elastic#171986

## How to test

You'll need to get into a state where a tag with a conflicting ID exists
in a second Kibana space outside of the default space.

It seems like the tags API doesn't support providing an ID at least from
the REST API, so that makes creating duplicate tags in other spaces a
bit challenging.

e.g. 

```
POST kbn:api/saved_objects_tagging/tags/create
{
  "id": "my-tag",
  "name": "My tag",
  "description": "",
  "color": "#FFFFFF"
}
```

results in 

```
{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "[request body.id]: definition for this key is missing"
}
```

So, in order to enable creating tags with ID's easily, I made some quick
code changes to the tags service

```diff
diff --git a/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts b/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
index 0c48168eed2..fa0e7fbe7b9 100644
--- a/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
+++ b/x-pack/plugins/saved_objects_tagging/server/routes/tags/create_tag.ts
@@ -6,6 +6,7 @@
  */
 
 import { schema } from '@kbn/config-schema';
+import { omit } from 'lodash';
 import type { TagsPluginRouter } from '../../types';
 import { TagValidationError } from '../../services/tags';
 
@@ -15,6 +16,7 @@ export const registerCreateTagRoute = (router: TagsPluginRouter) => {
       path: '/api/saved_objects_tagging/tags/create',
       validate: {
         body: schema.object({
+          id: schema.maybe(schema.string()),
           name: schema.string(),
           description: schema.string(),
           color: schema.string(),
@@ -32,7 +34,7 @@ export const registerCreateTagRoute = (router: TagsPluginRouter) => {
           });
         }
 
-        const tag = await tagsClient.create(req.body);
+        const tag = await tagsClient.create(omit(req.body, 'id'), { id: req.body.id });
         return res.ok({
           body: {
             tag,

```

With those changes in place (I don't think committing them is
necessary), I was able to create a tag in my second space e.g.

```
POST kbn:api/saved_objects_tagging/tags/create
{
  "id": "fleet-pkg-nginx-default",
  "name": "Nginx duplicate tag",
  "description": "",
  "color": "#DADADA"
}
```

Then, from my default space I installed the nginx package

```
POST kbn:/api/fleet/epm/packages/nginx
```

This throws the concurrent install error as expected on `main`

```
{
  "statusCode": 409,
  "error": "Conflict",
  "message": "Concurrent installation or upgrade of nginx-1.17.0 detected, aborting. Original error: Saved object [tag/fleet-pkg-nginx-default] conflict"
}
```

With this PR, we get this error instead

```
{
  "statusCode": 400,
  "error": "Bad Request",
  "message": "Saved Object conflict encountered while installing nginx-1.17.0. There may be a conflicting Saved Object saved to another Space. Original error: Saved object [tag/fleet-pkg-nginx-default] conflict"
}
```

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fleet] Saved objects conflicts shouldn't throw Concurrent Installation error
7 participants