Skip to content

Settings: Disallow secure setting to exist in normal settings#23976

Merged
rjernst merged 4 commits intoelastic:masterfrom
rjernst:keystore9
Apr 7, 2017
Merged

Settings: Disallow secure setting to exist in normal settings#23976
rjernst merged 4 commits intoelastic:masterfrom
rjernst:keystore9

Conversation

@rjernst
Copy link
Copy Markdown
Member

@rjernst rjernst commented Apr 7, 2017

This commit removes the "legacy" feature of secure settings, which setup
a parallel setting that was a fallback in the insecure
elasticsearch.yml. This was previously used to allow the new secure
setting name to be that of the old setting name, but is now not in use
due to other refactorings. It is much cleaner to just have all secure
settings use new setting names. If in the future we want to reuse the
previous setting name, once support for the insecure settings have been
removed, we can then rename the secure setting. This also adds a test
for the behavior.

This commit removes the "legacy" feature of secure settings, which setup
a parallel setting that was a fallback in the insecure
elasticsearch.yml. This was previously used to allow the new secure
setting name to be that of the old setting name, but is now not in use
due to other refactorings. It is much cleaner to just have all secure
settings use new setting names. If in the future we want to reuse the
previous setting name, once support for the insecure settings have been
removed, we can then rename the secure setting.  This also adds a test
for the behavior.
@rjernst
Copy link
Copy Markdown
Member Author

rjernst commented Apr 7, 2017

Note that before this change, if a setting had allowLegacy=false (which all of them did), and the setting name existed in elasticsearch.yml, the value would have simply been ignored.

@rjernst rjernst requested a review from jasontedor April 7, 2017 18:45
Copy link
Copy Markdown
Member

@jasontedor jasontedor left a comment

Choose a reason for hiding this comment

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

LGTM.

@s1monw
Copy link
Copy Markdown
Contributor

s1monw commented Apr 7, 2017

looks great!

@rjernst rjernst merged commit 73b8aad into elastic:master Apr 7, 2017
@rjernst rjernst deleted the keystore9 branch April 7, 2017 21:18
rjernst added a commit that referenced this pull request Apr 7, 2017
This commit removes the "legacy" feature of secure settings, which setup
a parallel setting that was a fallback in the insecure
elasticsearch.yml. This was previously used to allow the new secure
setting name to be that of the old setting name, but is now not in use
due to other refactorings. It is much cleaner to just have all secure
settings use new setting names. If in the future we want to reuse the
previous setting name, once support for the insecure settings have been
removed, we can then rename the secure setting.  This also adds a test
for the behavior.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 8, 2017
* master:
  Discovery EC2: Remove region setting (elastic#23991)
  AWS Plugins: Remove signer type setting (elastic#23984)
  Settings: Disallow secure setting to exist in normal settings (elastic#23976)
  Add registration of new discovery settings
  Settings: Migrate ec2 discovery sensitive settings to elasticsearch keystore (elastic#23961)
  Fix throttled reindex_from_remote (elastic#23953)
  Add comment why we check for null fetch results during merge
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.

3 participants