Add warnings for invalid realm order config (#51195) #51515

ywangd · 2020-01-28T05:18:33Z

The changes are to help users prepare for migration to next major
release (v8.0.0) regarding to the break change of realm order config.

Warnings are added for when:

A realm does not have an order config
Multiple realms have the same order config

The warning messages are added to both deprecation API and loggings.
The main reasons for doing this are: 1) there is currently no automatic relay
between the two; 2) deprecation API is under basic and we need logging
for OSS.

Issue warnings for both missing realm order and duplicated realm orders. The changes are to help users prepare for migration to next major release.

ywangd · 2020-01-28T06:38:46Z

@elasticmachine run elasticsearch-ci/default-distro

ywangd · 2020-01-28T10:26:08Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+        return new DeprecationIssue(
+            DeprecationIssue.Level.CRITICAL,
+            "Realm order will be required in next major release.",
+            "",


Not sure what url should be used here. The breaking change page will only be available once 8.0 is released?

There should be a link to the deprecation snippet, which is still to be added in this PR, seee my other comment #51515 (comment) .

ywangd · 2020-01-28T10:28:48Z

My understanding is that these changes make the _xpack/migration/deprecations API to return warning messages for the order config.
Is it necessary to also add warning messages in logs? What is the recommended practice? Thanks!

albertzaharovits · 2020-01-28T12:41:20Z

Is it necessary to also add warning messages in logs? What is the recommended practice?

Good point, I missed mentioning this in #51195 (comment) . This PR should also deprecate the lack of an order parameter (as well as the duplication). You can look for example uses of DeprecationLogger#deprecatedAndMaybeLog. There should also be a note in docs/reference/migration/migrate_7_7.asciidoc talking about this deprecation, similarly to the one for the removal in migrate_8.asciidoc.

For more info, here's a recent piece by Gordon https://github.com/elastic/elasticsearch-team/blob/master/onboarding-and-how-we-work/how-to-make-a-breaking-change.md .

ywangd · 2020-01-28T22:54:36Z

This PR should also deprecate the lack of an order parameter (as well as the duplication).

Yes the deprecation API returns warnings for both of the followings:

Missing realm order
Duplicated realm order

Thanks for mentioning the docs. I almost forgot about it. And I guess the updated docs will provide the URLs for DeprecationIssue objects.

tvernum · 2020-01-29T04:06:39Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+    static DeprecationIssue checkMissingRealmOrders(final Settings settings, final PluginsAndModules pluginsAndModules) {
+        final String orderNotConfiguredRealms = RealmSettings.getRealmSettings(settings).entrySet()
+                .stream()
+                .filter(e -> Objects.isNull(e.getValue().get(RealmSettings.ORDER_SETTING_KEY)))


I think you want to use Settings.hasValue instead here.

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

tvernum · 2020-01-29T04:26:06Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+                .stream()
+                .filter(e -> Objects.isNull(e.getValue().get(RealmSettings.ORDER_SETTING_KEY)))
+                .map(e -> e.getKey().getName())
+                .collect(Collectors.joining("; "));


I'd prefer to collect them in a Set instead of a String. No signficant reason, it just seems "cleaner" to me.

tvernum · 2020-01-29T04:29:48Z

...gin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecks.java

+        return new DeprecationIssue(
+            DeprecationIssue.Level.CRITICAL,
+            "Realm order will be required in next major release.",
+            "",


Per comments on the PR itself, we'll need to provide a URL here.

Updated the doc file and added the corresponding URL here.

tvernum · 2020-01-29T04:30:55Z

...eprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java

@@ -60,4 +62,48 @@ public void testCheckProcessors() {
        assertSettingDeprecationsAndWarnings(new Setting<?>[]{EsExecutors.PROCESSORS_SETTING});
    }

+    public void testCheckMissingRealmOrders() {


I'd like an inverse test as well - that realms with an order don't trigger an issue.

Added one test for correct realm order configs (2 realms of different orders)

tvernum · 2020-01-29T04:31:25Z

...eprecation/src/test/java/org/elasticsearch/xpack/deprecation/NodeDeprecationChecksTests.java

+            .put("xpack.security.authc.realms."
+                + randomAlphaOfLengthBetween(4, 12) + "." + randomAlphaOfLengthBetween(4, 12) + ".order", order)
+            .put("xpack.security.authc.realms." 
+                + randomAlphaOfLengthBetween(4, 12) + "." + randomAlphaOfLengthBetween(4, 12) + ".order", order)


Can we add another realm with order + 1 and verify that it isn't reported.

Yes we should. The test cases are not exhaustive. I have updated them to have better coverage.

tvernum · 2020-01-29T04:33:52Z

This needs an area label as well (:Security/Authentication)

tvernum · 2020-01-29T04:35:58Z

Is it necessary to also add warning messages in logs?

I think we should do one or the other.
If we want the checks from the API to also be in the logs we could automatically run them periodically and log them. I don't think it makes sense for every API check to be manually duplicated as a log warning too.

@gwbrown - thoughts?

…k/deprecation/NodeDeprecationChecks.java Co-Authored-By: Tim Vernum <[email protected]>

…earch into es-37614-realm-order-7.x

ywangd · 2020-01-29T12:37:06Z

Add breaking/deprecation notes for v8.0.0 realm order changes.
https://github.com/elastic/elasticsearch/pull/51515/files#diff-3da5c7727918592da984bb4bbd75efd5R34-R52

Just dropped the WIP label. This is now a formal PR.

elasticmachine · 2020-01-30T10:11:45Z

Pinging @elastic/es-security (:Security/Authentication)

albertzaharovits

LGTM .
Although I would prefer we always log deprecation messages even if they overlap with the API, to signal sooner that the configuration is deprecated; otherwise the API might only be called just before the upgrade and the user's experience would be as if all deprecations have been introduced in the last minor version.

ywangd · 2020-01-30T13:05:31Z

Thanks @albertzaharovits
I added deprecation logging code to Realms#initRealms. Will push and see how it goes.

gwbrown · 2020-01-30T23:49:06Z

Sorry for the delay in replying here.

Is it necessary to also add warning messages in logs?

We generally do both - log and add to the deprecation info API if possible. This does result in some duplication, but I think that's okay given that the logs are timestamped and it's more painful to miss that some functionality is deprecated than to sort through a few duplicated warnings. If it's a pain to log, then I don't think we need to (there are several conditions where it's difficult to log), but if it's straightforward to do so then I don't think there's much harm in both adding it to the API and emitting a log message. Tim said something about periodically checking and logging - while I haven't taken the time to truly understand what the invalid config we're checking for is here, emitting a log either on startup or when the config is loaded would be a good way to go.

gwbrown · 2020-01-30T23:57:50Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/Realms.java

+                .sorted()
+                .collect(Collectors.toList());
+        if (false == duplicatedRealmOrderSettingKeys.isEmpty()) {
+            new DeprecationLogger(logger).deprecated("Found multiple realms configured with the same order: [{}]. " +


One comment if we keep this: Generally we create one static DeprecationLogger for each class similar to the regular logger. It's not too much overhead to create new ones but it's still creating a few unnecessary objects.

tvernum · 2020-01-31T00:02:24Z

I don't think there's much harm in both adding it to the API and emitting a log message

My concern is twofold

If we think that logs and API are the better behaviour why doesn't the upgrade API just have that builtin? I'd rather we put time into making every node run the upgrade API methods on startup rather than having everyone write the same checks twice
If we write deprecation logs as well as implementing the API, then Write deprecation logs to an index #46106 would also cause the log to be in the API and duplicate the output to the user.

I think we ought to be planned about this, and decide on what we want, and do it centrally rather than ad-hoc.

ywangd · 2020-01-31T00:15:04Z

Thanks all for the feedback. I do agree there is code and logic duplication when implementing both API check and logging. It's great to see we do have plan to tackle them so that this will not be an issue in future.

In the meantime, I'd keep the logging code for this PR for following reasons:

It is for 7.x only which may not get benefit from Write deprecation logs to an index #46106.
Given the nature of this is a breaking change, so more serious than deprecation. It might be helpful to have higher visibility to users as Albert suggested.
And also because the work is already done.

We should definitely have one formal way of doing this. Is #46106 going to cover all the concerns (it doesn't mention anything about running deprecation API on launch time)? If not, we could create another issue for it.

gwbrown · 2020-01-31T00:17:58Z

If we think that logs and API are the better behaviour why doesn't the upgrade API just have that builtin? I'd rather we put time into making every node run the upgrade API methods on startup rather than having everyone write the same checks twice

~~The Deprecation Info API only runs on-demand, whereas emitting deprecation logs during the normal course of execution doesn't require the user to be aware of the Deprecation Info API at all.~~ Just reread your post, you did address this - I misread the first time. I wouldn't be opposed to running the checks on startup in general, but the next point still applies.

Plus, the Deprecation Info API is Elastic-licensed, and tying all deprecation warnings (or as many as we can) to Elastic-licensed code would create a much worse experience for users of the Apache2 distribution (vs. today where deprecation warnings are written into the Apache2 code as well).

That matters less in this and similar cases where all involved code is Elastic-licensed, but that's one reason why we don't want to centralize on putting as much in only the Deprecation Info API code as possible.

If we write deprecation logs as well as implementing the API, then #46106 would also cause the log to be in the API and duplicate the output to the user.

I agree, but we'll have to deal with that in several cases already when we get around to actually implementing #46106 (and also the follow-on #46107, plus the UI for it, which I don't think even has an open issue for it yet), and there will be already be a number of cases where we have to deduplicate log messages emitted multiple times. While I think #46106/#46107 are (very) good ideas, I think there's a long way to go before it gets to the point of actually becoming something we present alongside the standard Deprecation Info API, and we'll likely need to rethink some fairly low-level things about how we do deprecation logging for it to work smoothly enough that duplicated information between the Deprecation Info API and log messages is the most serious usability concern.

I think we ought to be planned about this, and decide on what we want, and do it centrally rather than ad-hoc.

This is a fantastic idea, and I would love to see us sit down and fundamentally rethink how we want to do deprecation warnings, but that doesn't seem to be something that's going to happen in the immediate future (if you know otherwise, please enlighten me! I would love to be wrong) so for now I think we should follow the patterns we have been for the past few releases.

tvernum · 2020-01-31T00:35:20Z

It took me sometime to understand your point, so let me spell it out for anyone who is as slow to catch on as I was.

We sometimes (often) deprecate OSS settings.
We then write a X-Pack deprecation API check for that
OSS builds will never have the code for that deprecation check
Therefore we also need something in OSS to inform users of the deprecation.
We could, in theory, simplify that process for X-Pack settings, but we'd still end up doing different things on a case by case basis.

That makes sense, I withdraw my concerns. Please continue to have both logging and API checks.

ywangd · 2020-01-31T00:42:49Z

It took me sometime to understand your point, so let me spell it out for anyone who is as slow to catch on as I was.

Thanks Tim. I also missed the issue around OSS release. It's quite amazing to see how many different releases that we support.
I just pushed new changes to address Gordon's comment. If there is no more concerns, I'll be merging this PR once CI jobs pass. Thanks all.

Add warnings for invalid realm order config (elastic#51195)

803bdaa

Issue warnings for both missing realm order and duplicated realm orders. The changes are to help users prepare for migration to next major release.

ywangd added the backport label Jan 28, 2020

ywangd requested review from tvernum and albertzaharovits January 28, 2020 05:18

ywangd commented Jan 28, 2020

View reviewed changes

albertzaharovits added >deprecation v7.7.0 and removed backport labels Jan 28, 2020

tvernum reviewed Jan 29, 2020

View reviewed changes

ywangd and others added 4 commits January 29, 2020 21:54

Merge branch '7.x' into es-37614-realm-order-7.x

1c7790c

Update x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpac…

0afe299

…k/deprecation/NodeDeprecationChecks.java Co-Authored-By: Tim Vernum <[email protected]>

Merge branch 'es-37614-realm-order-7.x' of github.com:ywangd/elastics…

d7ee0a5

…earch into es-37614-realm-order-7.x

Address feedbacks

661d554

ywangd requested a review from tvernum January 29, 2020 12:37

albertzaharovits added the :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) label Jan 30, 2020

albertzaharovits approved these changes Jan 30, 2020

View reviewed changes

ywangd added 2 commits January 31, 2020 00:19

Merge remote-tracking branch 'origin/7.x' into es-37614-realm-order-7.x

c3aa41e

Add deprecation loggings for realm order

cc45614

gwbrown reviewed Jan 30, 2020

View reviewed changes

Update to address feedback

d95353d

ywangd merged commit 77b00fc into elastic:7.x Jan 31, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket elastic/elasticsearch-net#4525

Closed

38 tasks

Add warnings for invalid realm order config (#51195) #51515

Add warnings for invalid realm order config (#51195) #51515

Uh oh!

Conversation

ywangd commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ywangd commented Jan 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ywangd commented Jan 28, 2020

Uh oh!

albertzaharovits commented Jan 28, 2020

Uh oh!

ywangd commented Jan 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Jan 29, 2020

Uh oh!

tvernum commented Jan 29, 2020

Uh oh!

ywangd commented Jan 29, 2020

Uh oh!

elasticmachine commented Jan 30, 2020

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

ywangd commented Jan 30, 2020

Uh oh!

gwbrown commented Jan 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Jan 31, 2020

Uh oh!

ywangd commented Jan 31, 2020

Uh oh!

gwbrown commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tvernum commented Jan 31, 2020

Uh oh!

ywangd commented Jan 31, 2020

Uh oh!

Uh oh!

ywangd commented Jan 28, 2020 •

edited

Loading

gwbrown commented Jan 31, 2020 •

edited

Loading