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

elastic 7.x support #1061

Closed
wants to merge 1 commit into from
Closed

elastic 7.x support #1061

wants to merge 1 commit into from

Conversation

uberjew666
Copy link

@uberjew666 uberjew666 commented Nov 19, 2019

  • Update JVM startup options
  • Add node name to log4j2.properties
  • Rename augeas resource to prevent dependency loop
  • Rename xpack related parameters to new names
  • Add boolean to enable/disable xpack and required features
  • Add self to contributors list
  • Resolve variable scope warnings
  • Update unit tests
  • Remove ubuntu 14.04 acceptance tests - Not supported
    https://www.elastic.co/support/matrix

Fixes:

Pull request acceptance prerequisites:

  • Signed the CLA (if not already signed)
  • Rebased/up-to-date with base branch
  • Tests pass
  • Updated CHANGELOG.md with patch notes (if necessary)
  • Updated CONTRIBUTORS (if attribution is requested)

@elasticmachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@uberjew666
Copy link
Author

@tylerjl @fatmcgav

Evening gents, can either of you two approve?

@ernstae
Copy link

ernstae commented Dec 2, 2019

FWIW, I've been running this branch in my QA environment, and it has been behaving properly.

@specdevops
Copy link

@tylerjl Any update on this? Unsure if I should proceed to install 6 if 7 will be released soon... :)

@smasa90
Copy link

smasa90 commented Dec 18, 2019

Any contribution mention for this ?? #1040

@jake2184
Copy link

jake2184 commented Jan 2, 2020

Is there anything preventing this being merged? I am keen to use the functionality

@andrewbierbaum
Copy link

@tylerjl I am also eagerly waiting on official word regarding 7.x support.

@trunet
Copy link

trunet commented Jan 9, 2020

this PR is giving me this error:

java.lang.IllegalArgumentException: a key must be provided to run as a server. the key should be configured using the [xpack.security.http.ssl.key] or [xpack.security.http.ssl.keystore.path] setting

I think you have to set all xpack.security.http in addition to xpack.security.transport

@uberjew666
Copy link
Author

uberjew666 commented Jan 9, 2020

this PR is giving me this error:

java.lang.IllegalArgumentException: a key must be provided to run as a server. the key should be configured using the [xpack.security.http.ssl.key] or [xpack.security.http.ssl.keystore.path] setting

I think you have to set all xpack.security.http in addition to xpack.security.transport

You need to set elasticsearch::xpack: true This requires ca_certificate, certificate and private_key to be configured. This will add all required xpack settings to the config file.

@trunet
Copy link

trunet commented Jan 9, 2020

I did

@uberjew666
Copy link
Author

Set elasticsearch::instance::ssl to false.

@trunet
Copy link

trunet commented Jan 9, 2020

but this will disable http client -> elasticsearch tls communication and I want that.

you just have to also set where you set xpack.security.transport.*:

xpack.security.http.ssl.key
xpack.security.http.ssl.certificate
xpack.security.http.ssl.certificate_authorities

Documentation:
https://www.elastic.co/guide/en/elasticsearch/reference/current/configuring-tls.html

@trunet
Copy link

trunet commented Jan 10, 2020

Set elasticsearch::instance::ssl to false.

This is indeed needed, otherwise it tries to load the keystore that is not used on this PR.

I think the only thing this needs, is the xpack.security.http.ssl.* in addition to the xpack.security.transport.*

@@ -440,13 +444,51 @@
}
}

if $xpack {
if (($ca_certificate == undef) and ($certificate == undef) and ($private_key == undef)) {
Copy link

@TJM TJM Jan 13, 2020

Choose a reason for hiding this comment

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

Shouldn't these be or instead of and ... or != undef ?

The way this is written, if any of them are defined, it will skip by this error check, it will only catch if all three are undef.

Copy link
Author

Choose a reason for hiding this comment

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

Good spot, it would be better to use or.

Copy link
Author

Choose a reason for hiding this comment

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

This has been added also

@@ -290,8 +294,8 @@
$tls_config = {
'xpack.security.transport.ssl.enabled' => true,
'xpack.security.http.ssl.enabled' => true,
'xpack.ssl.keystore.path' => $_keystore_path,
'xpack.ssl.keystore.password' => $keystore_password,
'xpack.http.ssl.keystore.path' => $_keystore_path,
Copy link

Choose a reason for hiding this comment

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

Are these options backwards compatible? Otherwise there should probably be a check on the major version and a switch between these two configurations?

Copy link

Choose a reason for hiding this comment

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

We had to add a check for major version on our cluster settings... this may be useful:

  $es_major_version = $elasticsearch::version.split(/\./)[0] # 7.5.1 -> 7

Copy link
Author

Choose a reason for hiding this comment

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

This has been added for ssl section

manifests/package.pp Show resolved Hide resolved
-Djna.nosys=true.
-Dlog4j.shutdownHookEnabled=false.
-Dlog4j2.disable.jmx=true.
-XX:\+AlwaysPreTouch.
-XX:\+HeapDumpOnOutOfMemoryError.
-XX:\+PrintGCDateStamps.
-XX:\+PrintGCDetails.
-XX:\+PrintTenuringDistribution.
-XX:\+UseCMSInitiatingOccupancyOnly.
-XX:\+UseConcMarkSweepGC.
Copy link

Choose a reason for hiding this comment

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

This should have an `8:' too

Copy link

Choose a reason for hiding this comment

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

I realize that the test isn't the only change but the point was the -XX:+UseConcMarkSweepGC should have an 8: prefix as it was deprecated in 9.

Copy link
Author

Choose a reason for hiding this comment

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

This has been added.

@Antiarchitect
Copy link

Really need this feature too. Anything I could help with?

@TJM
Copy link

TJM commented Jan 23, 2020

Really need this feature too. Anything I could help with?

I took the code from this pull request and added the changes I mentioned above. I also had to modify my "profile" code to form the cluster with the new parameters. It is working for us. I figured @uberjew666 would make the changes I suggested so that it could hopefully be merged, but I can re-submit if they are unable to.

Tommy

@uberjew666
Copy link
Author

@TJM Sorry, I've not had much time to work on this. Luckily, this module is required for a project I'm working on so I will try and get the amendments sorted and pushed soon.

@Antiarchitect
Copy link

@TJM @uberjew666 Any news on this? Can I help somehow?

@TJM
Copy link

TJM commented Feb 2, 2020

@Antiarchitect I just pushed the version we are using internally: https://github.com/TJM/puppet-elasticsearch/tree/es7-fix

@Antiarchitect
Copy link

Antiarchitect commented Feb 2, 2020

@TJM Thank you so much! Anyway, I and my team have some time. So I'll wait this merged into master.

@fatmcgav
Copy link
Contributor

fatmcgav commented Feb 4, 2020

Hi there,

Firstly, thank you for taking the time to contribute, and apologies for the radio silence from Elastic on this PR.

I'm going to be working on this module over the next few weeks, with an ultimate aim of updating the module to support the elasticsearch 7.x and simplifying the module to make it easier to use.

I'll be reviewing all the open issues and PRs over the next few days, and will merge or provide feedback where appropriate.

So please hang in there whilst we give this module some TLC.

Thanks again.

@cla-checker-service
Copy link

Author of the following commits did not sign a Contributor Agreement:
, 83a5648, a4b9222

Please, read and sign the above mentioned agreement if you want to contribute to this project

- Update JVM startup options
- Add node name to log4j2.properties
- Rename augeas resource to prevent dependency loop
- Add boolean to enable/disable xpack and required features
- Add self to contributors list
- Add check for required certificate variables
- Add version check for xpack related settings
  Maintains backwards compatibility
- Resolve variable scope warnings
- Update unit tests
- Remove ubuntu 14.04 acceptance tests - Not supported
  https://www.elastic.co/support/matrix
@Antiarchitect
Copy link

Any progress on this, guys?

@crazymind1337
Copy link
Member

@uberjew666 looks like you have conflicts within your branch.

@crazymind1337
Copy link
Member

Since there is no progress in here I have created a new PR which is rebased: #1082

@fatmcgav
Copy link
Contributor

Elasticsearch 7 support has been added. See #1068 (comment)

Thanks

@fatmcgav fatmcgav closed this Dec 24, 2020
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.