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

[Rest Api Compatibility] Licence accept_enterprise and response changes #75479

Merged
merged 10 commits into from
Jul 27, 2021

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Jul 19, 2021

in #50067 for _license the accept_enterprise = false was no longer supported. This
commit allows it under rest compatibility and is showing enterprise licenses as platinum
The same change had to be applied to _xpack endpoint #58217

in #50735 max_resource_units was introduced to be more accurate. For v7 requests, which do not know about enterprise license we will return max_node which will be set from max_resource_units (it assumes that one resource unit is 32GB - which corresponds to 1 node)

relates #51816

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

in elastic#50067 the accept_enterprise = false was no longer supported. This
commit allows it under rest compatibility and ignores.

in elastic#5073 there were changes when max_nodes and max_resource_units can
exist. This does not require compatible changes as it feels to be
behaviour change. Requires some testing transformation though

relates elastic#51816
@pgomulka pgomulka self-assigned this Jul 19, 2021
@pgomulka pgomulka added :Core/Infra/REST API REST infrastructure and utilities :Security/License License functionality for commercial features labels Jul 19, 2021
@elasticmachine elasticmachine added Team:Security Meta label for security team Team:Core/Infra Meta label for core/infra team labels Jul 19, 2021
@elasticmachine
Copy link
Collaborator

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

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@pgomulka
Copy link
Contributor Author

we also no longer parse hide_enterprise but it was present in 7.x?

@tvernum
Copy link
Contributor

tvernum commented Jul 20, 2021

I find this compatibility change to be a bit weird. We're going out of our way to accept a parameter value so that we can ignore it and not do the thing that the client asked for.
I haven't kept up to date with the objectives of the compatibility project, but this one really puzzles me. The client is explicitly saying "don't show me an enterprise licenses" and our response is "yes, we hear you, and that's fine, but we're going to show you one anyway". I don't understand why that's helpful.

On this one I really think we should go all the way and hide enterprise licenses from 7.x clients, or reject the parameter. Accepting it but refusing to honor it doesn't make sense to me.

we also no longer parse hide_enterprise but it was present in 7.x?

We never supported hide_enterprise in the Rest API. It was only used as an override parameter to the XContent generation of the license body.
We'd only need to add it back if we want to continue to support hiding enterprise licenses.

@pgomulka
Copy link
Contributor Author

I might have prematurely hit ready for review when this was meant to be a draft helping me to ask a question offline.
thank you for explanation @tvernum I did not notice at first that the change to allow enterprise was only in 7.x.
And I agree, I think we should honor the parameter and only show enterprise licenses to a 7.x client when he asked for this.

@pgomulka pgomulka requested review from ywangd and tvernum July 21, 2021 06:56
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.

The existing (7.x) behaviours of accept_enterprise seem inconsistent to me between GetLicense and GetXPackInfo APIs. For this review, I assume we are not trying to fix the inconsistency and just to ensure 8.x behaves the same as 7.x when rest compat is requested. Happy to chat if you need clarification for any of the comments.

x-pack/plugin/build.gradle Show resolved Hide resolved
Comment on lines 550 to 563
if (version >= VERSION_ENTERPRISE) {
if (hideEnterprise && builder.getRestApiVersion().matches(onOrAfter(RestApiVersion.V_7))) {
builder.field(Fields.MAX_NODES,maxNodes == -1 ? maxResourceUnits : maxNodes);
}else if (version >= VERSION_ENTERPRISE ) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I don't fully understand the intended behaviour for hideEnterprise. In GetLicense 7.x, it seems that it is only honored if version < VERSION_ENTERPRISE. But this is already inconsistent with RestXPackInfo 7.x which always hide enterprise if requested.

Anyway, if we just want to keep whatever 7.x have as is and be compatible, I think we can just copy the code from 7.x, i.e.:

if (version >= VERSION_ENTERPRISE) {
    builder.field(Fields.MAX_NODES, maxNodes == -1 ? null : maxNodes);
    builder.field(Fields.MAX_RESOURCE_UNITS, maxResourceUnits == -1 ? null : maxResourceUnits);
} else if (hideEnterprise && maxNodes == -1) {
    builder.field(Fields.MAX_NODES, maxResourceUnits);
} else {
    builder.field(Fields.MAX_NODES, maxNodes);
}

The value of hideEnterprise is already a result of checking rest compat, so I don't think we need check it again here.

Copy link
Member

@ywangd ywangd Jul 26, 2021

Choose a reason for hiding this comment

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

We discussed lively and realised that 7.x sets the license version is set to before enterprise when accept_enterprise=false. So it does always hide it similar to RestGetXPackInfo.
We also decided that the V7 compat should just behave the same way as 7.x (by mostly adding relevant code from 7.x).

@pgomulka pgomulka requested a review from ywangd July 27, 2021 06:50
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

@pgomulka
Copy link
Contributor Author

@elasticmachine update branch

@pgomulka pgomulka merged commit 6b93aff into elastic:master Jul 27, 2021
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
…es (elastic#75479)

in elastic#50067 for _license the accept_enterprise = false was no longer supported. This
commit allows it under rest compatibility and is showing enterprise licenses as platinum
The same change had to be applied to _xpack endpoint elastic#58217

in elastic#50735 max_resource_units was introduced to be more accurate. For v7 requests, which do not know about enterprise license we will return max_node which will be set from max_resource_units (it assumes that one resource unit is 32GB - which corresponds to 1 node)

relates elastic#51816
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 :Security/License License functionality for commercial features Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants