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

Enable Serverless API protections dynamically #97079

Merged
merged 8 commits into from Jun 28, 2023

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jun 26, 2023

This commit changes the method for enabling Serverless API protections (see #93607) from a system property to a runtime property.

Within this project there is no external way to configure this property - it must be controlled by a plugin - but the RestController has been changed to dynamically adapt to a change in that property.

This commit changes the method for enabling Serverless API protections
(see elastic#93607) from a system property to a runtime property.

Within this project there is no external way to configure this
property - it must be controlled by a plugin - but the RestController
has been changed to dynamically adapt to a change in that property.
@tvernum tvernum added >enhancement :Core/Infra/REST API REST infrastructure and utilities v8.10.0 labels Jun 26, 2023
@tvernum tvernum requested review from masseyke and ywangd June 26, 2023 08:25
@elasticsearchmachine
Copy link
Collaborator

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

@tvernum
Copy link
Contributor Author

tvernum commented Jun 26, 2023

This is draft (no tests, and probably breaks existing tests) but it works in local testing, so I wanted to put it up for a first pass review in case someone hates the direction.

@tvernum tvernum marked this pull request as ready for review June 27, 2023 04:02
@tvernum
Copy link
Contributor Author

tvernum commented Jun 27, 2023

This PR is ready for review now

@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jun 27, 2023
@elasticsearchmachine
Copy link
Collaborator

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

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

As commented on the other PR, there is a way to register a setting update listener without registering the setting itself. This would be my personal choice of implementation. But since the whole thing is temporary, the difference likely does not really matter.


package org.elasticsearch.rest;

public class ServerlessApiProtections {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we just call it ApiProtections?

Copy link
Contributor Author

@tvernum tvernum Jun 27, 2023

Choose a reason for hiding this comment

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

I don't mind. I made it explicit because the annotation is @ServerlessScope, so it's not intended to ever be a generic API protections facility.

@tvernum
Copy link
Contributor Author

tvernum commented Jun 28, 2023

As commented on the other PR, there is a way to register a setting update listener without registering the setting itself.

I'm going to leave it as-is, mostly because I don't like the idea of having a setting in server that isn't registered.
There's ugliness either way, but this feels like it's going to be less work over time to get it back to where it needs to be.

@tvernum
Copy link
Contributor Author

tvernum commented Jun 28, 2023

@elasticsearchmachine run elasticsearch-ci/part-3 please.

@tvernum tvernum merged commit ca1bec1 into elastic:main Jun 28, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >enhancement Team:Core/Infra Meta label for core/infra team v8.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants