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

Replace deprecated _xpack endpoints #9656

Merged
merged 16 commits into from
Feb 5, 2019

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Dec 19, 2018

In 7.0.0, Elasticsearch is deprecating most _xpack/* endpoints. See elastic/elasticsearch#35958.

This PR updates the Beats codebase, except test fixtures, with the appropriate replacements for the deprecated endpoints.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Will this break compatibility with older versions? I think we should support both paths at least during some versions.

@ycombinator
Copy link
Contributor Author

Will this break compatibility with older versions? I think we should support both paths at least during some versions.

Yes, you are correct. Given that ES is deprecating the old endpoints only starting with 7.0.0, things would break for a user that ran Beats 7.0.0 (with the changes in this PR) against an ES 6.7.0 node. I'm assuming this combination will be supported, following from https://www.elastic.co/support/matrix#matrix_compatibility.

Now, if ES could perform the deprecation in 6.7.0 itself, then the changes in this PR would not cause a break. Note that the changes in this PR will go only into 7.0.0/master and not be backported.

Do you agree with my assessment above? If so, should I ask on elastic/elasticsearch#35958 if they would be willing to perform the deprecations starting in 6.7.0 itself?

@ycombinator
Copy link
Contributor Author

@jsoriano Any thoughts about my last comment?

@jsoriano
Copy link
Member

jsoriano commented Jan 3, 2019

@ycombinator oh sorry, I didn't see this comment.

Yes, I guess that having both paths already in ES 6.7.0 can also be an option.

@ycombinator
Copy link
Contributor Author

ycombinator commented Jan 3, 2019

@jsoriano Looks like the new X-Pack APIs used in this PR all exist in ES 6.7.0 already, so there should be no breakage if a user tried to use Beats 7.0.0 (with this PR's changes in it) against ES 6.7.0.

Related: elastic/elasticsearch#35958 (comment)

@ycombinator ycombinator requested review from a team as code owners January 3, 2019 20:03
@ycombinator
Copy link
Contributor Author

@ph @kvch @urso Would be good to get one of your 👀 on this PR too, I think. Thanks!

Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

We want Beats 6.7 and ES 7.0 to work, but also Beats 7.0 and ES 6.7 to work correctly. In fact, it is recommended to update Elasticsearch first, but yet we're used to Beats users running against older ES versions.

The Connect on the beats ES client fetches the Elasticsearch version. You can access the vesion via client.GetVersion() I think. Maybe we can add some logic to take the version into account. E.g.

func apiCall(v *common.Version, api, path string) string {
  if v.Major >= 7 || (v.Major == 6 && v.Minor == 7) {
    return fmt.Sprintf("/_%v/%v", api, path)
  }
  return fmt.Sprintf("/_xpack/%v/%v", api, path)
}

and create the URL via:

client.Request("GET", apiCall(client.GetVersion(), "ml", "anomaly_detectors"), ...)

Or some ApiCall struct getting you the path by version:

var mlAnomalyCall = apiCall{"ml", "anomaly_detectors"}

...

client.Request("GET", mlAnonamlyCall.Path(client.GetVersion()), ...)

As _xpack namespace is still available in 7.0 I'd rather fallback to use _xpack if the version is unknown in the code (which is the behavior of the apiCall function).

WDYT?

Copy link

@urso urso left a comment

Choose a reason for hiding this comment

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

We want Beats 6.7 and ES 7.0 to work, but also Beats 7.0 and ES 6.7 to work correctly. In fact, it is recommended to update Elasticsearch first, but yet we're used to Beats users running against older ES versions.

The Connect on the beats ES client fetches the Elasticsearch version. You can access the vesion via client.GetVersion() I think. Maybe we can add some logic to take the version into account. E.g.

func apiCall(v *common.Version, api, path string) string {
  if v.Major >= 7 || (v.Major == 6 && v.Minor == 7) {
    return fmt.Sprintf("/_%v/%v", api, path)
  }
  return fmt.Sprintf("/_xpack/%v/%v", api, path)
}

and create the URL via:

client.Request("GET", apiCall(client.GetVersion(), "ml", "anomaly_detectors"), ...)

Or some ApiCall struct getting you the path by version:

var mlAnomalyCall = apiCall{"ml", "anomaly_detectors"}

...

client.Request("GET", mlAnonamlyCall.Path(client.GetVersion()), ...)

As _xpack namespace is still available in 7.0 I'd rather fallback to use _xpack if the version is unknown in the code (which is the behavior of the apiCall function).

WDYT?

@jsoriano
Copy link
Member

jsoriano commented Jan 4, 2019

I'd also prefer something like @urso proposal

@ycombinator
Copy link
Contributor Author

ycombinator commented Jan 4, 2019

I'm not sure we need the conditional logic, but maybe I'm missing something.

We want Beats 6.7 and ES 7.0 to work, but also Beats 7.0 and ES 6.7 to work correctly.

Let's break this statement into two, but in reverse order:

  1. We want ... Beats 7.0 and ES 6.7 to work correctly.

    This PR targets master, meaning the changes in this PR will go into Beats 7.0. As I mentioned in my earlier comment, the newer ES API endpoints (the ones without _xpack) are available in ES 6.7 as well. So the changes in this PR (i.e. calling the newer ES API endpoints) should work with ES 6.7.

  2. We want Beats 6.7 and ES 7.0 to work.

    I wasn't planning on backporting this PR to 6.x, meaning the changes in this PR will not go into Beats 6.7. If a user runs Beats 6.7 against ES 7.0, that will still work as the older (_xpack) API endpoints are still available in ES 7.0.

@ycombinator
Copy link
Contributor Author

ycombinator commented Jan 4, 2019

Chatted with @urso off-PR. He explained to me that we want to be even more liberal in our compatibility story than what's documented in the support matrix. So I'm going to implement his version check proposal in this PR now.

@ph
Copy link
Contributor

ph commented Jan 4, 2019

@ycombinator
Copy link
Contributor Author

@ph if I'm following that code correctly, it is calling the GET /_xpack API. AFAIK, that endpoint is not going away, even in ES 7.0.

@ycombinator
Copy link
Contributor Author

jenkins, test this

1 similar comment
@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator
Copy link
Contributor Author

jenkins, test this

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. One minor question.

Potentially worth a changelog even though users should not be impacted?

We should add this to our manual testing checklist (if we already have one). I didn't test it locally.

@@ -33,7 +33,7 @@ func init() {
}

const (
jobPath = "/_xpack/ml/anomaly_detectors/_all/_stats"
jobPathSuffix = "/anomaly_detectors/_all/_stats"
Copy link
Member

Choose a reason for hiding this comment

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

This has no ml prefix anymore?

Copy link
Contributor Author

@ycombinator ycombinator Feb 4, 2019

Choose a reason for hiding this comment

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

In 7.0.0+, the preferred path is _ml/anomaly_detectors/_all/_stats (notice the underscore in _ml). Before that version, the path is _xpack/ml/anomaly_detectors/_all/_stats (notice that ml does not contain the underscore).

Given this difference I decided to only store the suffix, anomaly_detectors/_all/_stats, in the variable here. Further below I concatenate either /_ml or _xpack/ml to the suffix, depending on the ES version we're talking to.

@ycombinator
Copy link
Contributor Author

ycombinator commented Feb 4, 2019

One minor question.

I think the question was about the ML prefix? If so, I've answered it in the comment thread.

Potentially worth a changelog even though users should not be impacted?

If a user runs a Beat < 7.0.0 with ES >= 7.0.0, they will see deprecation logs in their Elasticsearch logs for Elasticsearch X-Pack API calls made from their Beat. So I added a CHANGELOG entry that says that they won't see such calls with this PR 😄.

We should add this to our manual testing checklist (if we already have one). I didn't test it locally.

++ where is this checklist kept? I can add this item to it.

@ycombinator
Copy link
Contributor Author

jenkins, test this

@ycombinator ycombinator merged commit fd98c11 into elastic:master Feb 5, 2019
@ycombinator ycombinator deleted the xpack-endpoints branch February 5, 2019 03:40
@ruflin
Copy link
Member

ruflin commented Feb 5, 2019

For the list, not sure if it exists yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants