Remove Watcher Account "unsecure" settings by albertzaharovits · Pull Request #36736 · elastic/elasticsearch

albertzaharovits · 2018-12-17T20:21:11Z

Removes all the settings for watcher notifications accounts that are dynamic cluster filtered, and have a corresponding secure sibling.
They are deprecated in favor of the sibling that is stored in the keystore.

~~Some documentation is still pending.~~

Follow-up on #36403

elasticmachine · 2018-12-17T20:21:18Z

Pinging @elastic/es-core-features

hub-cap

I had a general question on the PR

hub-cap · 2018-12-17T21:59:18Z

...gin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/jira/JiraAccount.java

-                throw requiredSettingException(accountName, settingName);
-            }
-            value = secureString.toString();
+    private static String getSetting(String accountName, Settings settings, Setting<SecureString> secureSetting) {


This seems like something we could put in Account

Agreed, but I think that's a job for another PR (see prev comment)

hub-cap · 2018-12-17T22:00:37Z

...plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/Account.java

+                    return secureString;
                } else {
-                    return new SecureString(value.toCharArray());
+                    return null;


this seems a bit different from the other getSecureSettings that are in the other *Account classes, and somewhat trappy. Why would we be returning null instead of throwing up like we do in the others? I ask because I hope we can consolidate the functionality into only Account, if thats at all possible, instead of having a method that does this validation in every *Account class.

@hub-cap There is no Account base class. Also I tried to touch as less as possible to make reviewing easier. IMO we should be using the SecureSetting from the corresponding Service (EmailService in this case). But I prefer to leave this for another time. WDYT?

Sounds great to me

albertzaharovits · 2019-01-13T13:47:23Z

test this please

albertzaharovits · 2019-01-13T14:18:23Z

test this please

albertzaharovits · 2019-01-15T12:33:15Z

@hub-cap is this LGTMd or you still need to look into it?

hub-cap

lgtm! sorry about the wait :(

albertzaharovits · 2019-01-17T10:01:06Z

@elasticmachine run elasticsearch-ci-1

albertzaharovits · 2019-01-17T10:03:22Z

run gradle build test 1

albertzaharovits · 2019-01-17T10:04:46Z

run gradle build tests 1

albertzaharovits · 2019-01-17T10:04:52Z

run gradle build tests 2

albertzaharovits · 2019-01-17T11:02:36Z

run gradle build tests 1

albertzaharovits · 2019-01-17T11:02:50Z

run gradle build tests 2

albertzaharovits · 2019-01-17T11:32:13Z

run gradle build tests 1

albertzaharovits · 2019-01-18T15:18:54Z

Hey @hub-cap ,

Apparently I've not adapted the tests, waiting for the reviews (a bad habit I got into), and then forgot about it. I have done it now. It was just grunt work but it was more extensive than I thought it would be.

May I please ask you to cast an eye, maybe it rings some bell, just to be surer I haven't subtly messed smth up.

Nevertheless, I will be crawling through CI...

albertzaharovits · 2019-01-18T17:03:28Z

run gradle build tests 2

Upgrade checks for watcher notifications account deprecations in #36403 and #36736 .

* elastic/master: Remove Watcher Account "unsecure" settings (elastic#36736) Add cache cleaning task for ML snapshot (elastic#37505) Update jdk used by the docker builds (elastic#37621) Remove an unused constant in PutMappingRequest. Update get users to allow unknown fields (elastic#37593) Do not add index event listener if CCR disabled (elastic#37432) Add local session timeouts to leader node (elastic#37438)

* elastic/master: (104 commits) Permission for restricted indices (elastic#37577) Remove Watcher Account "unsecure" settings (elastic#36736) Add cache cleaning task for ML snapshot (elastic#37505) Update jdk used by the docker builds (elastic#37621) Remove an unused constant in PutMappingRequest. Update get users to allow unknown fields (elastic#37593) Do not add index event listener if CCR disabled (elastic#37432) Add local session timeouts to leader node (elastic#37438) Add some deprecation optimizations (elastic#37597) refactor inner geogrid classes to own class files (elastic#37596) Remove obsolete deprecation checks (elastic#37510) ML: Add support for single bucket aggs in Datafeeds (elastic#37544) ML: creating ML State write alias and pointing writes there (elastic#37483) Deprecate types in the put mapping API. (elastic#37280) [ILM] Add unfollow action (elastic#36970) Packaging: Update marker used to allow ELASTIC_PASSWORD (elastic#37243) Fix setting openldap realm ssl config Document the need for JAVA11_HOME (elastic#37589) SQL: fix object extraction from sources (elastic#37502) Nit in settings.gradle for Eclipse ...

albertzaharovits added 2 commits December 17, 2018 21:47

Done!

e6ce4a2

Done, but no docs

ac53fb9

albertzaharovits added >breaking v7.0.0 :Distributed/Watcher labels Dec 17, 2018

albertzaharovits self-assigned this Dec 17, 2018

albertzaharovits requested review from hub-cap and spinscale December 17, 2018 20:21

AthenaEryma mentioned this pull request Dec 17, 2018

Update Deprecation Info API for 7.0 breaking changes #36024

Closed

26 tasks

albertzaharovits mentioned this pull request Dec 17, 2018

Watcher deprecate notification service settings #36403

Merged

hub-cap reviewed Dec 17, 2018

View reviewed changes

albertzaharovits added 3 commits December 20, 2018 14:14

Merge branch 'master' into remove_watcher_secure_settings

444b715

Docs

73e9282

Forgot about slack

3aac147

albertzaharovits mentioned this pull request Dec 20, 2018

Watcher notification settings Upgrade checks #36907

Merged

hub-cap approved these changes Jan 15, 2019

View reviewed changes

Merge branch 'master' into remove_watcher_secure_settings

cf102df

albertzaharovits added 2 commits January 17, 2019 19:28

Merge branch 'master' into remove_watcher_secure_settings

a284719

Fix tests

1e48dd0

albertzaharovits added 3 commits January 18, 2019 14:15

Checkstyle

25fde98

Fix tests

b080066

Unused imports

66bd17e

x-pack/qa/smoke-test-watcher-with-security

a4d4354

albertzaharovits merged commit 5308746 into elastic:master Jan 20, 2019

albertzaharovits deleted the remove_watcher_secure_settings branch January 20, 2019 10:51

albertzaharovits added a commit that referenced this pull request Jan 20, 2019

Watcher notification settings Upgrade checks (#36907)

6ab4d18

Upgrade checks for watcher notifications account deprecations in #36403 and #36736 .

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

albertzaharovits commented Dec 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 17, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

hub-cap Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

hub-cap Dec 17, 2018

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

hub-cap Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Jan 13, 2019

Uh oh!

albertzaharovits commented Jan 13, 2019

Uh oh!

albertzaharovits commented Jan 15, 2019

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 17, 2019

Uh oh!

albertzaharovits commented Jan 18, 2019

Uh oh!

albertzaharovits commented Jan 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

albertzaharovits commented Dec 17, 2018 •

edited

Loading