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

Use origin for the client when running _features/_reset #88622

Merged
merged 11 commits into from
Jul 22, 2022

Conversation

grcevski
Copy link
Contributor

The current implementation of POST /_features/_reset doesn't pass the index descriptor origin when calling DELETE on the system indices, therefore, we fail to reset the respective indices.

Closes #88617

@grcevski grcevski added >bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.4.0 v8.3.4 labels Jul 19, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @grcevski, I've created a changelog YAML for you.

Copy link
Contributor

@gwbrown gwbrown left a comment

Choose a reason for hiding this comment

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

Left a few small comments but this looks like a good approach. Do we know what in particular requires the per-descriptor origin?

StringBuilder exceptions = new StringBuilder("[");
exceptions.append(errors.stream().map(e -> e.getException().getMessage()).collect(Collectors.joining(", ")));
exceptions.append(']');
listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString())));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add logging that captures the full stack traces? It's nitpicky but it's no fun when we hit a weird error and realize there's no way to get the actual stack trace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure sounds good.

}

@Nullable
static SystemIndexDescriptor findMatchingDescriptor(SystemIndexDescriptor[] indexDescriptors, String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not objecting necessarily, but why break this out? I assumed it was for testing in my first pass given it's pkg-private, but there's no tests that use this version looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, no need for that to be separate.

…csearch/integration/SecurityFeatureResetTests.java

Co-authored-by: Gordon Brown <[email protected]>
@grcevski
Copy link
Contributor Author

Left a few small comments but this looks like a good approach. Do we know what in particular requires the per-descriptor origin?

I'm not sure if there's a plugin where there's not 1-1 mapping of feature to origin. I simply erred on the side of caution since the origin is on the descriptor, I decided to split everything.

@@ -73,15 +74,15 @@ public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings sett
.put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1")
.build()
)
.setOrigin("net-new")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fake origins don't work anymore, since we actually switch the security. I picked tasks since it's not in a module.

Copy link
Contributor

@gwbrown gwbrown 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 iterating!

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM

@grcevski grcevski merged commit 3cb759b into elastic:master Jul 22, 2022
@grcevski grcevski deleted the bug/features_reset branch July 22, 2022 14:20
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.3 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 88622

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/Core Core issues without another label Team:Core/Infra Meta label for core/infra team v8.3.3 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

POST _features/reset doesn't use the system index origin
6 participants