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

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

Merged
merged 12 commits into from
Oct 10, 2018

Conversation

kvch
Copy link
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 ph and andrewkroh September 27, 2018 08:28
@kvch kvch added the blocked label Sep 27, 2018
@kvch
Copy link
Contributor Author

kvch commented Sep 27, 2018

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

@ruflin
Copy link
Member

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
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!

@@ -45,6 +45,7 @@ https://github.com/elastic/beats/compare/v6.4.0...master[Check the HEAD diff]
- Add backoff support to x-pack monitoring outputs. {issue}7966[7966]
- Fix a race condition with the `add_host_metadata` and the event serialization. {pull}8223[8223]
- Enforce that data used by k8s or docker doesn't use any reference. {pull}8240[8240]
- Switch to different UUID lib: github.com/gofrs/uuid. {pull}8485[8485]
Copy link
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
Contributor Author

kvch commented Sep 27, 2018

Updated changelog entry and rebased on top of #8469

@ph
Copy link
Contributor

ph commented Sep 27, 2018

Make check fails on CI

Copy link
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.

vendor/vendor.json Show resolved Hide resolved
@@ -35,7 +36,11 @@ func init() {
beatMetrics = monitoring.Default.NewRegistry("beat")
monitoring.NewFunc(beatMetrics, "info", reportInfo, monitoring.Report)

ephemeralID = uuid.NewV4()
var err error
ephemeralID, err = uuid.NewV4()
Copy link
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
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.

key = uuid.NewV4()
key, err := uuid.NewV4()
if err != nil {
return uuid.UUID{}, err
Copy link
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
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 🙏

* no longer maintained
* contains critical bug which leads to generating similar and same UUIDs
@kvch
Copy link
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