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.

Sign up for GitHub

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

Jump to bottom

[BeatsCM] fix API for tokens to support any number #30335

Merged
merged 12 commits into from
Feb 21, 2019
Merged

[BeatsCM] fix API for tokens to support any number #30335

merged 12 commits into from
Feb 21, 2019

Conversation

mattapperson
Copy link
Contributor

This PR both fixes the API so it responds correctly with large number of tokens created/beats enrolled (there was a collision issue with the UUID).

Also added a manual testing CLI tool for this as automated testing alone did not catch the issue.

@elasticmachine
Copy link
Contributor

Pinging @elastic/beats

@mattapperson mattapperson mentioned this pull request Feb 6, 2019
Closed
@mattapperson mattapperson self-assigned this Feb 6, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@mattapperson
Copy link
Contributor Author

Failed test is red master

ph
ph previously requested changes Feb 8, 2019
View reviewed changes
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.

@mattapperson I've tested that PR and it breaks the UI, when I enroll beat and assign config block they do not stick to the tag.

If I edit a tag and try to add config block they also don't get associated with the tag.

@mattapperson
Copy link
Contributor Author

@ph that failure is related to #30549.
Please test using ES at version 6.7

@ph
Copy link
Contributor

ph commented Feb 11, 2019

From our discussion we are closing this and moving to a webtoken implementation.

@ph
Copy link
Contributor

ph commented Feb 19, 2019

I've tested it and forgot to update the issue directly, the new token does appear to work, but I had issues with the UI to create configuration block to a tag, I can create them in the first enrolling screen. But they are not persisted to the UI.

@mattapperson
Copy link
Contributor Author

@ph I am unable to replicate that issue

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ph
Copy link
Contributor

ph commented Feb 20, 2019

@mattapperson Not sure what I am doing wrong but I can reproduce it with the following steps.

preconditions:

  • clean kibana install
  • starting es with yarn es snapshot --license trial
  • starting kibana with yarn start
  • test done with chrome
  • cleared chrome cache.

steps:

  • click enroll
  • copy command
  • enroll filebeat
  • click continue
  • set tag name to "awesome"
  • click add configuration block
  • select filebeat module and choose the system module
  • click save
  • click add configuration block
  • select output and choose the elasticsearch output.
  • fill out the credentials and host.
  • click save
  • click save and continue
  • click configuration tags
  • click "awesome"
  • results I don't see any confguration blocks.

@ph
Copy link
Contributor

ph commented Feb 20, 2019

Also I don’t see any errors in the dev console, Kibana or ES and I have used the elastic/change me credentials combo.

@mattapperson
Copy link
Contributor Author

Bah. Just needed a merge with master. My bad @ph, I had merged master locally for the doc change and never pushed. The issue you were seeing was the config being saved but the query failing on the 8.0/7.0 ES snapshots. I just pushed the merge with master. Should be good now.

@elastic elastic deleted a comment from elasticmachine Feb 20, 2019
@mattapperson
Copy link
Contributor Author

Jenkins test this

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@chrisronline
Copy link
Contributor

I pulled this down and verified this is now working as intended.

How else should I test this?

@mattapperson
Copy link
Contributor Author

Please confirm @ph's issue is no longer present. Other then that (and a code review if you don't mind 😊 ) we are GTG

chrisronline
chrisronline approved these changes Feb 20, 2019
View reviewed changes
Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

Functionally, LGTM!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mattapperson mattapperson dismissed ph’s stale review February 21, 2019 00:38

PH is out of office and Chris approved, Verified the issue PH found was fixed

@mattapperson mattapperson merged commit d513817 into elastic:master Feb 21, 2019
@mattapperson mattapperson mentioned this pull request Feb 21, 2019
Merged
mattapperson added a commit to mattapperson/kibana that referenced this pull request Feb 21, 2019
@mattapperson
[BeatsCM] fix API for tokens to support any number (elastic#30335)
bb8a884 
* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…
@mattapperson mattapperson mentioned this pull request Feb 21, 2019
Merged
mattapperson added a commit to mattapperson/kibana that referenced this pull request Feb 21, 2019
@mattapperson
[BeatsCM] fix API for tokens to support any number (elastic#30335)
9b4c208 
* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…
mattapperson added a commit that referenced this pull request Feb 21, 2019
@mattapperson
[BeatsCM] fix API for tokens to support any number (#30335) (#31658)
a88a5f3 
* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…
@mattapperson mattapperson mentioned this pull request Feb 21, 2019
Merged
mattapperson added a commit to mattapperson/kibana that referenced this pull request Feb 21, 2019
@mattapperson
[BeatsCM] fix API for tokens to support any number (elastic#30335)
c785f60 
* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…

# Conflicts:
#	x-pack/plugins/beats_management/server/lib/adapters/framework/integration_tests/kibana.ts
mattapperson added a commit that referenced this pull request Feb 21, 2019
@mattapperson
[BeatsCM] fix API for tokens to support any number (#30335) (#31659)
fc9649a 
* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…
mattapperson added a commit that referenced this pull request Feb 21, 2019
@mattapperson
[6.7] [BeatsCM] fix API for tokens to support any number (#30335) (#3…
47d838e 
…1661)

* [BeatsCM] fix API for tokens to support any number (#30335)

* add basic script for standing up a fake env

* tweaks

* API needs to support any number of tokens not us

* [BeatsCM] Add testing script used to create test deployments

* Move to JWT for enrollment

* wrap dont scroll command

* Dont use token as token ID

* fix tests

* not sure why this file is enabled in this branch/PR…

# Conflicts:
#	x-pack/plugins/beats_management/server/lib/adapters/framework/integration_tests/kibana.ts

* remove dev only k7Design
@mattapperson mattapperson mentioned this pull request Mar 4, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrisronline chrisronline chrisronline approved these changes

@ph ph ph left review comments

@justinkambic justinkambic Awaiting requested review from justinkambic

Assignees

@mattapperson mattapperson

Labels
blocker Feature:beats-cm Team:Beats v6.7.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattapperson @elasticmachine @ph @chrisronline