Skip to content

Check feature flag before creating JWT test realm#84738

Merged
elasticsearchmachine merged 3 commits intoelastic:masterfrom
tvernum:feature/jwt/fix-84698-test
Mar 8, 2022
Merged

Check feature flag before creating JWT test realm#84738
elasticsearchmachine merged 3 commits intoelastic:masterfrom
tvernum:feature/jwt/fix-84698-test

Conversation

@tvernum
Copy link
Copy Markdown
Contributor

@tvernum tvernum commented Mar 7, 2022

SecurityRealmSettingsTests would fail on release builds because
the JWT realm is not registered as an internal realm

Resolves: #84698

SecurityRealmSettingsTests would fail on release builds because
the JWT realm is not registered as an internal realm

Resolves: elastic#84698
@tvernum tvernum added >test Issues or PRs that are addressing/adding tests :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) test-release Trigger CI checks against release build v8.2.0 labels Mar 7, 2022
@tvernum tvernum requested a review from ywangd March 7, 2022 22:39
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Mar 7, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

This works. But I wonder why you didn't choose to enable it explicitly so the test runs on all builds?

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 7, 2022

I wonder why you didn't choose to enable it explicitly so the test runs on all builds?

Because in a release build the feature is supposed to be disabled, so it felt better to test the intended behaviour rather than force it to use a different behaviour.

The fact that we run this tests with the realm disabled actually uncovered the bug to be fixed in #84740 because this test failed when comparing the configured settings to the expected realm types, but it should have failed when configuring the realm (because the setting shouldn't exist).

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 8, 2022

@elasticsearchmachine run elasticsearch-ci/release-tests please

:qa:smoke-test-http:integTest

org.elasticsearch.client.ResponseException: method [PUT], host [http://[::1]:44137], URI [_cluster/settings], status line [HTTP/1.1 500 Internal Server Error] |  
{"error":{"root_cause":[{"type":"process_cluster_event_timeout_exception","reason":"failed to process cluster event (reroute_after_cluster_update_settings) within 30s"}],"type":"exception","reason":"reroute after update settings failed","caused_by":{"type":"process_cluster_event_timeout_exception","reason":"failed to process cluster event (reroute_after_cluster_update_settings) within 30s"}},"status":500}

@tvernum
Copy link
Copy Markdown
Contributor Author

tvernum commented Mar 8, 2022

@elasticmachine update branch

@tvernum tvernum added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 8, 2022
@elasticsearchmachine elasticsearchmachine merged commit 54317d6 into elastic:master Mar 8, 2022
@tvernum tvernum deleted the feature/jwt/fix-84698-test branch March 8, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests test-release Trigger CI checks against release build v8.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CI] SecurityRealmSettingsTests testClusterStarted failing

4 participants