Skip to content

Switch to different UUID lib due to non-random generated UUIDs#8458

Merged
kvch merged 12 commits intoelastic:masterfrom
kvch:switch-uuid-libs
Oct 10, 2018
Merged

Switch to different UUID lib due to non-random generated UUIDs#8458
kvch merged 12 commits intoelastic:masterfrom
kvch:switch-uuid-libs

Conversation

@kvch
Copy link
Copy Markdown
Contributor

@kvch kvch commented Sep 27, 2018

The previously used github.com/satori/go-uuid lib has a critical bug and it is no longer maintained. The community moved to an active fork of this existing lib named github.com/gofrs/uuid. I haven't seen other UUID libs worth mentioning apart from this.

Changes compared to the previous dependency:

  • uuid.NewV4 returns an error if the function call failed
  • uuid.Equal is deprecated and removed. The Way is to use == instead.

TODO

  • update github.com/elastic/go-ucfg

Closes #8077

@kvch kvch added review libbeat needs_backport PR is waiting to be backported to other branches. labels Sep 27, 2018
@kvch kvch requested review from andrewkroh and ph September 27, 2018 08:28
@kvch kvch added the blocked label Sep 27, 2018
@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Sep 27, 2018

github.com/elastic/go-ucfg needs an update.

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Sep 27, 2018

Seems like go-ucfg uses the same library. We should do a PR there first and then update go-ucfg.

@kvch kvch added the in progress Pull request is currently in progress. label Sep 27, 2018
@kvch kvch changed the title Switch UUID libs Switch to different UUID lib: github.com/gofrs/uuid Sep 27, 2018
Copy link
Copy Markdown
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM just a minor things about the changelog.

Thanks for propagating this error!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we insert why we switch probably adding a word about collision in the old library.

@kvch kvch changed the title Switch to different UUID lib: github.com/gofrs/uuid Switch to different UUID lib due to non-random generated UUIDs Sep 27, 2018
@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Sep 27, 2018

Updated changelog entry and rebased on top of #8469

@ph
Copy link
Copy Markdown
Contributor

ph commented Sep 27, 2018

Make check fails on CI

Copy link
Copy Markdown
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Thanks for looking it to this issue. Only minor comments from me.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the impact to monitoring if the ephemeral ID is Nil?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The beat won't show up in some visualizations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use uuid.Nil here? It's the same but slightly more clear.

@exekias
Copy link
Copy Markdown
Contributor

exekias commented Oct 4, 2018

I just merged #8559, which adds a new use of this lib in libbeat/management/management.go, this will need a rebase + update that import 🙏

kvch added 5 commits October 5, 2018 19:47
* no longer maintained
* contains critical bug which leads to generating similar and same UUIDs
@kvch kvch force-pushed the switch-uuid-libs branch from 7302237 to e810af4 Compare October 5, 2018 17:50
@kvch
Copy link
Copy Markdown
Contributor Author

kvch commented Oct 5, 2018

This should be ready for the next round of review.

@kvch kvch removed blocked in progress Pull request is currently in progress. labels Oct 10, 2018
@kvch kvch merged commit 554ddcd into elastic:master Oct 10, 2018
@kvch kvch added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Oct 18, 2018
kvch added a commit to kvch/beats that referenced this pull request Oct 18, 2018
…ic#8458)

The previously used `github.com/satori/go-uuid` lib has a critical bug and it is no longer maintained. The community moved to an active fork of this existing lib named `github.com/gofrs/uuid`. I haven't seen other UUID libs worth mentioning apart from this.

Changes compared to the previous dependency:
* `uuid.NewV4` returns an error if the function call failed
* `uuid.Equal` is deprecated and removed. The Way is to use `==` instead.

Closes elastic#8077
(cherry picked from commit 554ddcd)
kvch added a commit that referenced this pull request Oct 18, 2018
#8640)

The previously used `github.com/satori/go-uuid` lib has a critical bug and it is no longer maintained. The community moved to an active fork of this existing lib named `github.com/gofrs/uuid`. I haven't seen other UUID libs worth mentioning apart from this.

Changes compared to the previous dependency:
* `uuid.NewV4` returns an error if the function call failed
* `uuid.Equal` is deprecated and removed. The Way is to use `==` instead.

Closes #8077
(cherry picked from commit 554ddcd)
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.

5 participants