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

Clarify difference between public and secure client settings in discu… #31469

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

MorrieAtElastic
Copy link
Contributor

@MorrieAtElastic MorrieAtElastic commented Jun 20, 2018

…ssion of S3 client configuration.

The original discussion of client settings was carried over from earlier versions before TLS. The existing language in the documentation can make users believe that any client setting can be added to elasticsearch.yml, which is not the case. The wording I am proposing makes it clear from the beginning of the discussion that there are now 2 classes of client settings for S3 snapshots: public and secure, and that each class must be specified differently.

…ssion of S3 client configuration.

See Support Ticket 00231008 : https://elastic.my.salesforce.com/ui/support/servicedesk/ServiceDeskPage#/5006100000M5DFG 
and also https://github.com/elastic/support-dev-help-elasticsearch/issues/737. 

The original discussion of client settings was carried over  from doc versions  before there were sensitive client settings. The existing language in the documentation can make customers believe that any client setting can be added to elasticsearch.yml. The new wording that I am proposing above makes it clear from the beginning of the discussion that there are now 2 classes of client settings for S3 snapshots: public and secure, and that each class must be specified differently.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs

@rjernst
Copy link
Member

rjernst commented Jun 21, 2018

I think someone from the docs team needs to review this (@lcawl?). I am apprehensive about introducing "public settings" terminology, as we have not used it anywhere else in our docs to differentiate from secure settings. This would also affect more than this file, as the pattern of noting settings that must be placed in the keystore is used elsewhere in the docs (eg x-pack secure settings documentation).

@lcawl
Copy link
Contributor

lcawl commented Jun 21, 2018

It's true, we refer to secure settings in several places (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/security-settings.html, https://www.elastic.co/guide/en/beats/winlogbeat/6.3/keystore.html, https://www.elastic.co/guide/en/kibana/6.3/secure-settings.html, https://www.elastic.co/guide/en/elasticsearch/reference/6.3/secure-settings.html) but we don't refer to all other settings as "public" or "unsecure" or anything like that at this point.
I'll make some suggestions and invite @Sue-Gallagher to weigh in too.

the form `s3.client.CLIENT_NAME.SETTING_NAME` and specified inside `elasticsearch.yml`. The
default client name looked up by a `s3` repository is called `default`, but can be customized
with the repository setting `client`. For example:
The client used to connect to S3 has a number of settings available. Settings are either "public" or "secure", Public client settings may be specified in the `elasticsearch.yml` file and are of the form `s3.client.CLIENT_NAME.SETTING_NAME`. The default client name looked up by a `s3` repository is called `default`, but can be customized with the repository setting `client`. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than introducing the idea of "public" settings, I'd suggest something like this:
"The client ... has a number of settings available. All of these settings can be added to the elasticsearch.yml configuration file, with the exception of the secure settings, which you add to the Elasticsearch keystore. The settings have the form..."

@@ -63,7 +59,7 @@ bin/elasticsearch-keystore add s3.client.default.secret_key
----

The following are the available client settings. Those that must be stored in the keystore
are marked as `Secure`.
are marked as `Secure`; all other settings are public and may be included in the `elasticsearch.yml` file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Per above, I'd just say "...all other settings can be included in the elasticsearch.yml file".

@@ -63,7 +59,7 @@ bin/elasticsearch-keystore add s3.client.default.secret_key
----

The following are the available client settings. Those that must be stored in the keystore
are marked as `Secure`.
are marked as `Secure`; all other settings are public and may be included in the `elasticsearch.yml` file.

`access_key`::
Copy link
Contributor

Choose a reason for hiding this comment

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

In other docs (e.g. https://www.elastic.co/guide/en/elasticsearch/reference/master/security-settings.html), we put the "secure" identifier directly after the setting name (instead of after the description) and it's a link to the secure settings page. I suggest doing the same here.

@MorrieAtElastic
Copy link
Contributor Author

MorrieAtElastic commented Jun 22, 2018 via email

@javanna
Copy link
Member

javanna commented Aug 16, 2018

hi @MorrieAtElastic do you have time to address comments or shall the docs team take this over? cc @lcawl

@tomcallahan
Copy link
Contributor

@lcawl you want to take this one over?

@lcawl
Copy link
Contributor

lcawl commented Jan 4, 2019

I've added a commit that implements my suggestions.
P.S. I've noticed that this page has changed in subsequent versions (with respect to reloadable settings), so it should be cherry-picked cautiously.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

The new edits LGTM, just one comment about wording.

The following are the available client settings. Those that must be stored in the keystore
are marked as `Secure`.
The following list contains the available client settings. Those that must be
stored in the keystore are marked as "secure"; all other settings can be
Copy link
Member

Choose a reason for hiding this comment

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

"can" implies (to me) that this is optional. Yet the secure and non secure settings are disjoint; they cannot be present in the other location. I'm not sure what word would better relate that requirement, though...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback! I changed it to "all other settings belong in ..."

@lcawl lcawl merged commit 14e0677 into 6.3 Jan 9, 2019
lcawl pushed a commit that referenced this pull request Jan 9, 2019
lcawl pushed a commit that referenced this pull request Jan 9, 2019
lcawl pushed a commit that referenced this pull request Jan 9, 2019
lcawl pushed a commit that referenced this pull request Jan 9, 2019
lcawl pushed a commit to lcawl/elasticsearch that referenced this pull request Jan 9, 2019
@colings86 colings86 added the >docs General docs changes label Jan 11, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@colings86 colings86 deleted the MorrieAtElastic-patch-1 branch May 27, 2020 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants