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

Add setFactory permission to GceDiscoveryPlugin #16860

Merged
merged 2 commits into from Mar 1, 2016

Conversation

Projects
None yet
6 participants
@s1monw
Copy link
Contributor

commented Feb 29, 2016

This commit adds a missing permission and a simple test that
ensures we discover other nodes via a mock http endpoint.

Closes #16485

@rjernst I had to make this integ test a ordinary testcase since it runs against an internal cluster. I didn't explore the test fixture path yet since it also requires a test plugin etc. I wonder if we can make this one test run against the internal cluster while others runs against external clusters? I am not super happy with the test but it's a major step forward since it really tests the integration of this plugins while everything else isn't really testing it. It also would have caught the security issue though

@dadoonet

View changes

plugins/discovery-gce/src/main/java/org/elasticsearch/cloud/gce/GceComputeService.java Outdated
@@ -30,6 +30,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.function.Function;

This comment has been minimized.

Copy link
@dadoonet

dadoonet Feb 29, 2016

Member

Why this import?

Add setFactory permission to GceDiscoveryPlugin
This commit adds a missing permission and a simple test that
ensures we discover other nodes via a mock http endpoint.

Closes #16485

@s1monw s1monw force-pushed the s1monw:issues/16485 branch to ecca717 Feb 29, 2016

@dadoonet

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

Interesting. I like the idea of using an HTTP server to serve the mock responses.
Thanks @s1monw for fixing this issue.

LGTM

@rjernst

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

This looks ok to me. It is good to have some kind of test for GCE. I'm apprehensive about checking in the keyfile, but I guess it is just for a test...

At another time, I am happy to look at moving this to a fixture, as well as generating the keyfile, but this PR at least gets us into a better state than we are now.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Feb 29, 2016

+1

@jasontedor

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

I have the same apprehension as @rjernst but given

At another time, I am happy to look at moving this to a fixture, as well as generating the keyhole

I'm good with the change too. Thanks for picking this up @s1monw.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 29, 2016

@rjernst @jasontedor I pushed a new commit generating the keystore from gradle

@rjernst

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

Looks good, thanks!

@jasontedor

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

LGTM.

s1monw added a commit that referenced this pull request Mar 1, 2016

Merge pull request #16860 from s1monw/issues/16485
Add setFactory permission to GceDiscoveryPlugin

This commit adds a missing permission and a simple test that
ensures we discover other nodes via a mock http endpoint.

Closes #16485

@s1monw s1monw merged commit 80e5c0a into elastic:master Mar 1, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:issues/16485 branch Mar 1, 2016

s1monw added a commit to s1monw/elasticsearch that referenced this pull request Mar 1, 2016

Backort add setFactory permission to GceDiscoveryPlugin
This commit adds a missing permission and a simple test that
ensures we discover other nodes via a mock http endpoint.

Backport of elastic#16860
Relates to elastic#16485

s1monw added a commit that referenced this pull request Mar 1, 2016

Add missing `setFactory` permission to cloud-gce
This commit adds the missing permission to `cloud-gce`. Since this is
a bugfix branch the commit doesn't contain the testcase added to 2.x and
master to keep the footprint of this change as low as possible.

Backport of #16860
Relates to #16485

s1monw added a commit that referenced this pull request Mar 1, 2016

Merge pull request #16881 from s1monw/backport_16860
Backport add setFactory permission to GceDiscoveryPlugin

This commit adds a missing permission and a simple test that
ensures we discover other nodes via a mock http endpoint.

Backport of #16860
Relates to #16485
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.