Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Cherrypick es keystore entries #757

Conversation

Bernhard-Fluehmann
Copy link
Contributor

@Bernhard-Fluehmann Bernhard-Fluehmann commented Jan 21, 2021

#Add support to add,update and delete elasticsearch.keystore entries.
Obsoletes #685
Fix #684

@bndabbs Please have a look

@elasticmachine
Copy link
Collaborator

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?

This was referenced Jan 21, 2021
@Bernhard-Fluehmann
Copy link
Contributor Author

@jmlrt Could you add the feature label and assignement as from #685 to this PR? thanks

@schallee
Copy link

Would it be better to use something like:

- command: stdin: "{{passphrase}}" argv: - "{{keystore_cmd}}" - add - '-x' - '-f' - "xpack.security.{{ item }}.secure_password"

instead of echo pipe? I worry that certain values (eg: randomly generated password) could contain shell meta characters which would cause problems.

@Bernhard-Fluehmann
Copy link
Contributor Author

@schallee Thank yor for bringing this up. I have refactored it already and updated the pull request.

@bndabbs
Copy link

bndabbs commented Feb 1, 2021

Thanks for the contribution @BernardIsibor. I'm hoping to review this PR either today or tomorrow.

bndabbs
bndabbs previously approved these changes Feb 1, 2021
Copy link

@bndabbs bndabbs left a comment

Choose a reason for hiding this comment

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

Looks good!

@bndabbs
Copy link

bndabbs commented Feb 1, 2021

jenkins test this please

@@ -28,12 +28,62 @@

- name: Create Bootstrap password for elastic user
become: yes
shell: echo {{ es_api_basic_auth_password | quote }} | {{ es_home }}/bin/elasticsearch-keystore add -x 'bootstrap.password'
command:
Copy link
Member

@jmlrt jmlrt Feb 2, 2021

Choose a reason for hiding this comment

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

CI tests using this task are failing (example: https://devops-ci.elastic.co/job/elastic+ansible-elasticsearch+pull-request/237/OS=ubuntu-1404,TEST_TYPE=xpack-upgrade,label=linux/console).

I did a few local tests and see 2 issues:

  • argv syntax for command module was released in Ansible 2.6 but our role is still supporting Ansible 2.5

    min_ansible_version: 2.5.0

    I would prefer not remove 2.5 support if that's possible as it is the default version used in Ubuntu 18.04 packages and in our ubuntu-1804 tests.

  • The generated command don't seem valid. When using it with a more recent Ansible version and enabling log for this task, I get the following error:

    TASK [elasticsearch : Create Bootstrap password for elastic user] **************
    fatal: [localhost]: FAILED! => {"changed": false, "cmd": "changeme /usr/share/elasticsearch/bin/elasticsearch-keystore add -x bootstrap.password", "msg": "[Errno 2] No such file or directory", "rc": 2}
    

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can have a look at this and change it back. How should we address the problem mentioned by @schalle about autogenerated passwords?

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK the Ansible quote filter should handle the special characters correctly so that we don't have any issue.

I did a try with a few passwords like elastic|$&01 and didn't have any issue with my small samples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Got it. I have fixed it and tried to push it but used a wrong git user. Can you delete this pull request please, I will do it again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmlrt Or even better, just accept it.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure that we can merge the PR with CLA failing on some commits.

Can you try changing the author name of these 2 commits with this procedure for example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmlrt I have tried it. Can you please check? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

This is still failing and I still see your other author name and emails in some commits. Then if you prefer creating a new branch/PR please do, you can then close this one or I'll do it when the new one will be created.

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 2, 2021

❌ Author of the following commits did not sign a Contributor Agreement:
, , , , , 077ca68, 17079be, 2cde079, , 5d38e70

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

jmlrt and others added 7 commits February 2, 2021 12:18
This commit create dedicated Jenkins jobs for 6.x for master branch and
PRs. This is required to allow having different test suites for 7.x and
6.x in a following PR.
This commit fix the test job templates following elastic#760.
- add VERSION parameter for 7.x jobs
- move axis to jobs templates
This commit fix PR test job for 6.x following elastic#762.
This commit fix PR test job for 6.x following elastic#762.
@jmlrt
Copy link
Member

jmlrt commented Feb 2, 2021

cla/check

@Bernhard-Fluehmann Bernhard-Fluehmann deleted the cherrypick_es_keystore_entries branch February 2, 2021 12:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keystore Configuration
5 participants