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

Do not return metadata customs by default #30857

Closed
wants to merge 6 commits into from

Conversation

jasontedor
Copy link
Member

Returning metadata customs by default is dangerous because they could be from a plugin that is not installed on a transport client. This commit modifies the cluster state action so that metadata customs are not returned instead of explicitly asked for. We make the same change on the REST layer, and we require that if metadata customs are requested, so is metadata.

Relates #30731

@jasontedor jasontedor added >bug review v7.0.0 v6.3.0 :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v6.4.0 labels May 25, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@jasontedor jasontedor requested a review from s1monw May 25, 2018 02:38
Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

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

The same deserialization issue exists with cluster state customs (which are still enabled by default). I'm not sure that we need to add another request parameter here, but can just reuse the existing customs parameter, defaulting it to false and have it apply to metadata customs as well.

@jasontedor
Copy link
Member Author

retest this please

@jasontedor
Copy link
Member Author

@ywelsch Are you okay with that being a follow-up?

@jasontedor
Copy link
Member Author

retest this please

@jasontedor
Copy link
Member Author

run gradle build tests

Returning metadata customs by default is dangerous because they could be
from a plugin that is not installed on a transport client. This commit
modifies the cluster state action so that metadata customs are not
returned instead of explicitly asked for. We make the same change on the
REST layer, and we require that if metadata customs are requested, so is
metadata.
@ywelsch
Copy link
Contributor

ywelsch commented May 28, 2018

With the newly introduced flag, users with the default distribution now have to choose between upgrading to the x-pack client or not showing any custom metadata at all (not even OSS one). The PR is also a breaking change as we will stop showing OSS custom metadata by default (e.g. index graveyard, ingest pipelines, repositories). In more detail, with the current PR:

  • 6.2/6.3 clients will by default stop returning custom metadata (even if that custom metadata was from OSS, e.g. index graveyard, ingest pipelines, repositories, ...).
  • OSS 6.3 client cannot get clusterstate from default distrib 6.3 cluster if specifying all() on request and not explicitly excluding metadata customs afterwards while the same worked on a default distrib 6.2 cluster.
  • OSS 6.3 client cannot get OSS custom metadata from a default distrib 6.3 cluster. When specifying metaCustoms, it has to choose between all or nothing.

I'm doubtful about the use of the new flag and that it truly solves the problem, and my suggestion is therefore for the short term to go with the following approach instead:

  • not return x-pack custom metadata through the cluster state API. This will only affect ML metadata. It already has dedicated APIs to access the information, so there's no need to expose this through the cluster state API. If need be, we could add a system property to allow exposing this for duration of the 6.x series.
  • For persistent tasks, which has moved to OSS in 6.3, we can return it if streamoutput is on 6.3 at least.

@s1monw
Copy link
Contributor

s1monw commented May 30, 2018

I'm doubtful about the use of the new flag and that it truly solves the problem, and my suggestion is therefore for the short term to go with the following approach instead:

I am not sure this is useful to anybody and I will push hard to remove this stuff form this API in 7.0 it's been a mistake to expose this to begin with.

not return x-pack custom metadata through the cluster state API. This will only affect ML metadata. It already has dedicated APIs to access the information, so there's no need to expose this through the cluster state API. If need be, we could add a system property to allow exposing this for duration of the 6.x series.

+1 we should never return plugin metadata here at any time

For persistent tasks, which has moved to OSS in 6.3, we can return it if streamoutput is on 6.3 at least.

this sounds good to me too

@jasontedor jasontedor closed this May 31, 2018
@jasontedor
Copy link
Member Author

Superseded by #31020

@jasontedor jasontedor deleted the metadata-customs branch June 1, 2018 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Distributed Coordination/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. >non-issue v6.3.0 v6.4.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants