Add enabled status for token and api key service by bizybot · Pull Request #38687 · elastic/elasticsearch

bizybot · 2019-02-11T06:15:26Z

Right now there is no way to determine whether the
token service or API key service is enabled or not.
This commit adds support for the enabled status of
token and API key service to security feature set
usage API /_xpack/usage.

Closes #38535

Right now there is no way to determine whether the token service or API key service is enabled or not. This commit adds the enabled status for token and API key service to security feature set usage API `/_xpack/usage`.

elasticmachine · 2019-02-11T06:15:28Z

Pinging @elastic/es-security

albertzaharovits · 2019-02-11T19:12:29Z

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityFeatureSetUsage.java

        realmsUsage = in.readMap();
        rolesStoreUsage = in.readMap();
        sslUsage = in.readMap();
+        if (in.getVersion().onOrAfter(Version.V_7_1_0)) {


If I got it right should be Version.CURRENT and do the dance 😢 : commit to master, change to Version.V_7_1_0 in the backport commit, disable bwc tests, commit to 7.x, wait for green intake, then change to Version.V_7_1_0 in the master, and enable bwc tests . If you learn of an easier method lemme know please! I'll admit it, in the past I cut a few corners on this...

I think you are right, I have changed this to CURRENT with a TODO, will address it on backport to 7.x.
Thank you for your comment.

albertzaharovits

LGTM

jaymode

I left a suggestion to improve the test and you need to disable BWC tests in

elasticsearch/build.gradle

Lines 156 to 163 in 9631c1a

    
           /* 
        
            * When adding backcompat behavior that spans major versions, temporarily 
        
            * disabling the backcompat tests is necessary. This flag controls 
        
            * the enabled state of every bwc task. It should be set back to true 
        
            * after the backport of the backcompat code is complete. 
        
            */ 
        
           final boolean bwc_tests_enabled = true 
        
           final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */

. Then after backporting and a successful intake build, you'd re-enable the BWC tests

jaymode · 2019-02-13T22:36:10Z

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityFeatureSetUsage.java

        realmsUsage = in.readMap();
        rolesStoreUsage = in.readMap();
        sslUsage = in.readMap();
+        if (in.getVersion().onOrAfter(Version.CURRENT)) { // TODO change the version to V_7_1_0 on backporting


I think you should use V_8_0_0 instead of current but it doesn't matter much since it will change later on

Done, Thank you.

jaymode · 2019-02-13T22:37:33Z

.../plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityFeatureSetTests.java

        final boolean transportSSLEnabled = randomBoolean();
        settings.put("xpack.security.transport.ssl.enabled", transportSSLEnabled);
+
+        boolean configureEnabledFlagForTokenAndApiKeyServices = randomBoolean();


I think it would be best to separate these two services enabled in the test. That way we wouldn't miss a bug that mistakenly reports the wrong value if one service is enabled and the other is not

bizybot · 2019-02-14T02:19:07Z

Hi @jaymode, I have addressed your review comments, please take a look when you get some time. Thank you.

Right now there is no way to determine whether the token service or API key service is enabled or not. This commit adds support for the enabled status of token and API key service to the security feature set usage API `/_xpack/usage`. Closes elastic#38535

Right now there is no way to determine whether the token service or API key service is enabled or not. This commit adds support for the enabled status of token and API key service to the security feature set usage API `/_xpack/usage`. Closes #38535

* elastic/master: Remove immediate operation retry after mapping update (elastic#38873) Remove mentioning of types from bulk API docs (elastic#38896) SQL: change JDBC setup URL in the documentation (elastic#38564) Skip BWC tests in checkPart1 and checkPart2 (elastic#38730) Enable silent FollowersCheckerTest (elastic#38851) Update TESTING.asciidoc with platform specific instructions (elastic#38802) Use consistent view of realms for authentication (elastic#38815) Stabilize RareClusterState (elastic#38671) Increase Timeout in UnicastZenPingTests (elastic#38893) Do not recommend installing vagrant-winrm elastic#38887 _cat/indices with Security, hide names when wildcard (elastic#38824) SQL: fall back to using the field name for column label (elastic#38842) Fix LocalIndexFollowingIT#testRemoveRemoteConnection() test (elastic#38709) Remove joda time mentions in documentation (elastic#38720) Add enabled status for token and api key service (elastic#38687)

jasontedor · 2019-02-14T18:18:28Z

Unless I am missing something, I don't think this change needed to disable all BWC testing. I think a targeted subset of BWC tests could have been disabled.

jaymode · 2019-02-14T18:46:14Z

You are probably right; it was an oversight on my part since this API is only called from a few tests.

jaymode · 2019-02-14T18:47:45Z

...plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityFeatureSetUsage.java

        out.writeMap(realmsUsage);
        out.writeMap(rolesStoreUsage);
        out.writeMap(sslUsage);
+        out.writeMap(tokenServiceUsage);


@bizybot I missed this in my review but there is a bug here; we write the map always without checking the version. We need the same guards on both reading and writing

True I missed this as well, Thanks for addressing this.

This change makes the writing of new usage data conditional based on the version that is being written to. A test has also been added to ensure serialization works as expected to an older version. Relates elastic#38687, elastic#38917

This change makes the writing of new usage data conditional based on the version that is being written to. A test has also been added to ensure serialization works as expected to an older version. Relates #38687, #38917

Add enabled status for token and api key service

c075aac

Right now there is no way to determine whether the token service or API key service is enabled or not. This commit adds the enabled status for token and API key service to security feature set usage API `/_xpack/usage`.

bizybot added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.2.0 labels Feb 11, 2019

bizybot requested review from albertzaharovits and jaymode February 11, 2019 06:15

albertzaharovits reviewed Feb 11, 2019

View reviewed changes

albertzaharovits approved these changes Feb 11, 2019

View reviewed changes

Yogesh Gaikwad added 2 commits February 12, 2019 12:08

Merge branch 'master' into add-enabled-status-api-token-service

25082ab

Use CURRENT version and change it to 7.1.0 once backported

0c78be6

jaymode requested changes Feb 13, 2019

View reviewed changes

Address review comments

65fbbb4

jaymode approved these changes Feb 14, 2019

View reviewed changes

bizybot merged commit d227f8e into elastic:master Feb 14, 2019

bizybot added the backport pending label Feb 14, 2019

cbuescher mentioned this pull request Feb 14, 2019

Adapt FullClusterRestartIT on master #38856

Merged

jasontedor mentioned this pull request Feb 14, 2019

Adjust BWC version for token/API key service #38917

Merged

jaymode reviewed Feb 14, 2019

View reviewed changes

jaymode mentioned this pull request Feb 14, 2019

Fix writing of SecurityFeatureSetUsage to pre-7.1 #38922

Merged

bizybot removed the backport pending label Feb 16, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	/*
	* When adding backcompat behavior that spans major versions, temporarily
	* disabling the backcompat tests is necessary. This flag controls
	* the enabled state of every bwc task. It should be set back to true
	* after the backport of the backcompat code is complete.
	*/
	final boolean bwc_tests_enabled = true
	final String bwc_tests_disabled_issue = "" /* place a PR link here when committing bwc changes */

Conversation

bizybot commented Feb 11, 2019

Uh oh!

elasticmachine commented Feb 11, 2019

Uh oh!

albertzaharovits Feb 11, 2019

Choose a reason for hiding this comment

Uh oh!

bizybot Feb 12, 2019

Choose a reason for hiding this comment

Uh oh!

albertzaharovits left a comment

Choose a reason for hiding this comment

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

jaymode Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

bizybot Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

jaymode Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

bizybot commented Feb 14, 2019

Uh oh!

jasontedor commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaymode commented Feb 14, 2019

Uh oh!

jaymode Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

bizybot Feb 15, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasontedor commented Feb 14, 2019 •

edited

Loading