Skip to content

Fixes SecurityIntegTestCase so it always adds at least one alias#33296

Merged
colings86 merged 2 commits intoelastic:masterfrom
colings86:fix/33123
Aug 31, 2018
Merged

Fixes SecurityIntegTestCase so it always adds at least one alias#33296
colings86 merged 2 commits intoelastic:masterfrom
colings86:fix/33123

Conversation

@colings86
Copy link
Copy Markdown
Contributor

@colings86 colings86 commented Aug 31, 2018 •
edited
Loading

SecurityIntegTestCase.createIndicesWithRandomAliases could randomly
fail because its not gauranteed that the randomness of which aliases to
add to the IndicesAliasesRequestBuilder would always select at least
one alias to add. This change fixes the problem by keeping track of
whether we have added an alias to teh request and forcing the last
alias to be added if no other aliases have been added so far.

Closes #30098
Closes #33123

@colings86
Fixes SecurityIntegTestCase so it always adds at least one alias
d35890e 
`SecurityIntegTestCase.createIndicesWithRandomAliases` could randomly
fail because its not gauranteed that the randomness of which aliases to
add to the `IndicesAliasesRequestBuilder` would always select at least
one alias to add. This change fixes the problem by keeping track of
whether we have added an alias to teh request and forcing the last
alias to be added if no other aliases have been added so far.

Closes #30098
Closes #33123e
@colings86 colings86 added >test Issues or PRs that are addressing/adding tests review v7.0.0 :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.5.0 v6.4.1 labels Aug 31, 2018
@colings86 colings86 self-assigned this Aug 31, 2018
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Aug 31, 2018

Pinging @elastic/es-security

jasontedor
jasontedor reviewed Aug 31, 2018
View reviewed changes
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.

I left a suggestion. Also, if you force the frequently() on line 423 to true, the frequently() on line 426 to false, and the randomBoolean() on line 434 to false, do all of the tests in ReadActionsTests pass?

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java Outdated
createIndex(indices);

if (frequently()) {
boolean noAliasAdded = true;
Copy link
Copy Markdown
Member

@jasontedor jasontedor Aug 31, 2018

Choose a reason for hiding this comment

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

I think aliasAdded = false; is clearer.

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java Outdated
if (frequently()) {
//one alias per index with prefix "alias-"
builder.addAlias(index, "alias-" + index);
noAliasAdded = false;
Copy link
Copy Markdown
Member

@jasontedor jasontedor Aug 31, 2018

Choose a reason for hiding this comment

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

aliasAdded = true;

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java Outdated
if (randomBoolean()) {
// If we get to this point and we haven't added an alias to the request we need to add one
// or the request will fail so use noAliasAdded to force adding the alias in this case
if (noAliasAdded || randomBoolean()) {
Copy link
Copy Markdown
Member

@jasontedor jasontedor Aug 31, 2018

Choose a reason for hiding this comment

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

aliasAdded == false

@colings86
Copy link
Copy Markdown
Contributor Author

colings86 commented Aug 31, 2018 •
edited
Loading

if you force the frequently() on line 423 to true, the frequently() on line 426 to false, and the randomBoolean() on line 434 to false, do all of the tests in ReadActionsTests pass?

Tested and can confirm passes

jasontedor
jasontedor approved these changes Aug 31, 2018
View reviewed changes
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.

🚢

@colings86 colings86 merged commit 436d5c4 into elastic:master Aug 31, 2018
colings86 added a commit that referenced this pull request Aug 31, 2018
@colings86
Fixes SecurityIntegTestCase so it always adds at least one alias
ff56588 
(#33296)

* Fixes SecurityIntegTestCase so it always adds at least one alias

`SecurityIntegTestCase.createIndicesWithRandomAliases` could randomly
fail because its not gauranteed that the randomness of which aliases to
add to the `IndicesAliasesRequestBuilder` would always select at least
one alias to add. This change fixes the problem by keeping track of
whether we have added an alias to teh request and forcing the last
alias to be added if no other aliases have been added so far.

Closes #30098
Closes #33123e

* Addresses review comments
colings86 added a commit that referenced this pull request Aug 31, 2018
@colings86
Fixes SecurityIntegTestCase so it always adds at least one alias
f9cba69 
(#33296)

* Fixes SecurityIntegTestCase so it always adds at least one alias

`SecurityIntegTestCase.createIndicesWithRandomAliases` could randomly
fail because its not gauranteed that the randomness of which aliases to
add to the `IndicesAliasesRequestBuilder` would always select at least
one alias to add. This change fixes the problem by keeping track of
whether we have added an alias to teh request and forcing the last
alias to be added if no other aliases have been added so far.

Closes #30098
Closes #33123e

* Addresses review comments
dnhatn added a commit that referenced this pull request Sep 1, 2018
@dnhatn
Merge branch 'master' into ccr
b935076 
* master:
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  Adjust soft-deletes version after backport into 6.5
  completely drop `index.shard.check_on_startup: fix` for 7.0 (#33194)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  drop `index.shard.check_on_startup: fix` (#32279)
  tracked at
  [DOCS] Moves ml folder from x-pack/docs to docs (#33248)
  [DOCS] Move rollup APIs to docs (#31450)
  [DOCS] Rename X-Pack Commands section (#33005)
  TEST: Disable soft-deletes in ParentChildTestCase
  Fixes SecurityIntegTestCase so it always adds at least one alias (#33296)
  Fix pom for build-tools (#33300)
  Lazy evaluate java9home (#33301)
  SQL: test coverage for JdbcResultSet (#32813)
  Work around to be able to generate eclipse projects (#33295)
  Highlight that index_phrases only works if no slop is used (#33303)
  Different handling for security specific errors in the CLI. Fix for #33230 (#33255)
  [ML] Refactor delimited file structure detection (#33233)
  SQL: Support multi-index format as table identifier (#33278)
  MINOR: Remove Dead Code from PathTrie (#33280)
  Enable forbiddenapis server java9 (#33245)
dnhatn added a commit that referenced this pull request Sep 1, 2018
@dnhatn
Merge branch '6.x' into ccr-6.x
a6312a5 
* 6.x:
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  TEST: Disable soft-deletes in ParentChildTestCase
  TEST: Disable randomized soft-deletes settings
  Integrates soft-deletes into Elasticsearch (#33222)
  drop `index.shard.check_on_startup: fix` (#32279)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  [DOCS] Moves ml folder from x-pack/docs to docs (#33248)
  TEST: mute more SmokeTestWatcherWithSecurityIT tests
  [DOCS] Move rollup APIs to docs (#31450)
  [DOCS] Rename X-Pack Commands section (#33005)
  Fixes SecurityIntegTestCase so it always adds at least one alias (#33296)
  TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307)
  MINOR: Remove Dead Code from PathTrie (#33280) (#33306)
  Fix pom for build-tools (#33300)
  Lazy evaluate java9home (#33301)
  SQL: test coverage for JdbcResultSet (#32813)
  Work around to be able to generate eclipse projects (#33295)
  Different handling for security specific errors in the CLI. Fix for #33230 (#33255)
  [ML] Refactor delimited file structure detection (#33233)
  SQL: Support multi-index format as table identifier (#33278)
  Enable forbiddenapis server java9 (#33245)
  [MUTE] SmokeTestWatcherWithSecurityIT flaky tests
  Add region ISO code to GeoIP Ingest plugin (#31669) (#33276)
  Don't be strict for 6.x
  Update serialization versions for custom IndexMetaData backport
  Replace IndexMetaData.Custom with Map-based custom metadata (#32749)
  Painless: Fix Bindings Bug (#33274)
  SQL: prevent duplicate generation for repeated aggs (#33252)
  TEST: Mute testMonitorClusterHealth
  Fix serialization of empty field capabilities response (#33263)
  Fix nested _source retrieval with includes/excludes (#33180)
  [DOCS] TLS file resources are reloadable (#33258)
  Watcher: Ensure TriggerEngine start replaces existing watches (#33157)
  Ignore module-info in jar hell checks (#33011)
  Fix docs build after #33241
  [DOC] Repository GCS ADC not supported (#33238)
  Upgrade to latest Gradle 4.10  (#32801)
  Fix/30904 cluster formation part2 (#32877)
  Move file-based discovery to core (#33241)
  HLRC: add client side RefreshPolicy (#33209)
  [Kerberos] Add unsupported languages for tests (#33253)
  Watcher: Reload properly on remote shard change (#33167)
  Fix classpath security checks for external tests. (#33066)
  [Rollup] Only allow aggregating on multiples of configured interval (#32052)
  Added deprecation warning for rescore in scroll queries (#33070)
  Apply settings filter to get cluster settings API (#33247)
  [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability   (#32743)
  HLRC: create base timed request class (#33216)
  HLRC: Use Optional in validation logic (#33104)
  Painless: Add Bindings (#33042)
jasontedor added a commit to martijnvg/elasticsearch that referenced this pull request Sep 2, 2018
@jasontedor
Merge branch 'master' into ccr_auto_follow
6907ce9 
* master: (64 commits)
  HLREST: add update by query API (elastic#32760)
  TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333)
  HLRC: ML Flush job (elastic#33187)
  HLRC: Adding ML Job stats (elastic#33183)
  LLREST: Drop deprecated methods (elastic#33223)
  Mute testSyncerOnClosingShard
  [DOCS] Moves machine learning APIs to docs folder (elastic#31118)
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  Adjust soft-deletes version after backport into 6.5
  completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  drop `index.shard.check_on_startup: fix` (elastic#32279)
  tracked at
  [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248)
  [DOCS] Move rollup APIs to docs (elastic#31450)
  [DOCS] Rename X-Pack Commands section (elastic#33005)
  TEST: Disable soft-deletes in ParentChildTestCase
  Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296)
  ...
jasontedor added a commit to debadair/elasticsearch that referenced this pull request Sep 2, 2018
@jasontedor
Merge branch 'master' into HEAD
0e3d6c5 
* master: (283 commits)
  HLREST: add update by query API (elastic#32760)
  TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333)
  HLRC: ML Flush job (elastic#33187)
  HLRC: Adding ML Job stats (elastic#33183)
  LLREST: Drop deprecated methods (elastic#33223)
  Mute testSyncerOnClosingShard
  [DOCS] Moves machine learning APIs to docs folder (elastic#31118)
  Mute test watcher usage stats output
  [Rollup] Fix FullClusterRestart test
  Adjust soft-deletes version after backport into 6.5
  completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194)
  Fix AwaitsFix issue number
  Mute SmokeTestWatcherWithSecurityIT testsi
  drop `index.shard.check_on_startup: fix` (elastic#32279)
  tracked at
  [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248)
  [DOCS] Move rollup APIs to docs (elastic#31450)
  [DOCS] Rename X-Pack Commands section (elastic#33005)
  TEST: Disable soft-deletes in ParentChildTestCase
  Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296)
  ...
@colings86 colings86 deleted the fix/33123 branch September 3, 2018 08:22
@jaymode
Copy link
Copy Markdown
Member

jaymode commented Sep 4, 2018

thanks @colings86 and @jasontedor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasontedor jasontedor jasontedor approved these changes

@jaymode jaymode Awaiting requested review from jaymode

Assignees

@colings86 colings86

Labels

:Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >test Issues or PRs that are addressing/adding tests v6.4.1 v6.5.0 v7.0.0-beta1

Projects

None yet

Milestone

No milestone

4 participants

@colings86 @elasticmachine @jaymode @jasontedor