Skip to content

Set metadata on initial request in API key noop test#88507

Merged
n1v0lg merged 5 commits intoelastic:masterfrom
n1v0lg:fix/api-key-noop-test-metadata
Jul 14, 2022
Merged

Set metadata on initial request in API key noop test#88507
n1v0lg merged 5 commits intoelastic:masterfrom
n1v0lg:fix/api-key-noop-test-metadata

Conversation

@n1v0lg
Copy link
Copy Markdown
Contributor

@n1v0lg n1v0lg commented Jul 13, 2022 •
edited
Loading

This PR fixes API key integration test setup for noops. In the noop
test, we use the initial update request as a reference to choose
suitable values to force non-noop updates, including metadata. However,
metadata can be null on the initial request, meaning that the
underlying API key will retain the metadata it was assigned on
creation. This can lead to test failure when the metadata on the
initial request is null, and the subsequent metadata update matches
the metadata chosen at API key creation time.

Closes #88503.

@n1v0lg n1v0lg added >test Issues or PRs that are addressing/adding tests :Security/Security Security issues without another label labels Jul 13, 2022
@n1v0lg n1v0lg self-assigned this Jul 13, 2022
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Jul 13, 2022
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Jul 13, 2022

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

@n1v0lg n1v0lg changed the title Ensure initial requests sets metadata Set metadata on initial request in API key noop test Jul 13, 2022
@n1v0lg
Copy link
Copy Markdown
Contributor Author

n1v0lg commented Jul 13, 2022

@elasticmachine run elasticsearch-ci/bwc

@n1v0lg n1v0lg requested a review from ywangd July 13, 2022 15:27
ywangd
ywangd reviewed Jul 14, 2022
View reviewed changes
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.

I am not sure whether this is the right fix. The failure occurs at line 1805:

elasticsearch/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

Line 1805 in baadfb9

assertTrue(response.isUpdated());

The associated updateRequest is prepared to have a non-null metadata and also not the same as the initial metadata at line 1801:

elasticsearch/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

Line 1801 in baadfb9

randomValueOtherThanMany(md -> md == null || md.equals(initialRequest.getMetadata()), ApiKeyTests::randomMetadata)

So why forcing a non-null initial metadata would help?

Also, I think the following random logic is incorrect:

elasticsearch/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

Lines 1774 to 1781 in baadfb9

randomValueOtherThanMany(
rd -> (RoleDescriptorRequestValidator.validate(rd) != null) && initialRequest.getRoleDescriptors().contains(rd) == false,
() -> RoleDescriptorTests.randomRoleDescriptor(false)
),
randomValueOtherThanMany(
rd -> (RoleDescriptorRequestValidator.validate(rd) != null) && initialRequest.getRoleDescriptors().contains(rd) == false,
() -> RoleDescriptorTests.randomRoleDescriptor(false)
)

We want it the new descriptor to be (1) valid and (2) not contained by the initial descriptors. So I think it should be something like the follows?

randomValueOtherThanMany(
    rd -> RoleDescriptorRequestValidator.validate(rd) != null || initialRequest.getRoleDescriptors().contains(rd),
    () -> RoleDescriptorTests.randomRoleDescriptor(false)
),

@n1v0lg
Copy link
Copy Markdown
Contributor Author

n1v0lg commented Jul 14, 2022 •
edited
Loading

So why forcing a non-null initial metadata would help?

If metadata on initialRequest is null, then the metadata assigned during creation stays on the doc (since null metadata means don't update).

Suppose:

created.metadata = {"bar": "foo"}
initialRequest.metadata = null
then it can happen that on line 1805, metadata is {"bar": "foo"} matching the metadata on the doc and resulting in false for noop because we're comparing to initialRequest.metadata when picking a distinct metadata value

@n1v0lg
Copy link
Copy Markdown
Contributor Author

n1v0lg commented Jul 14, 2022

We want it the new descriptor to be (1) valid and (2) not contained by the initial descriptors. So I think it should be something like the follows?

Absolutely, De Morgan's law! Thanks for catching will fix.

@ywangd
Copy link
Copy Markdown
Member

ywangd commented Jul 14, 2022

So why forcing a non-null initial metadata would help?

If metadata on initialRequest is null, then the metadata assigned during creation stays on the doc (since null metadata means don't update).

Suppose:

created.metadata = {"bar": "foo"} initialRequest.metadata = null then it can happen that on line 1805, metadata is {"bar": "foo"} matching the metadata on the doc and resulting in false for noop because we're comparing to initialRequest.metadata when picking a distinct metadata value

Thanks for the explanation. I forgot about there is the initial creation!

ywangd
ywangd approved these changes Jul 14, 2022
View reviewed changes
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

@n1v0lg n1v0lg requested a review from ywangd July 14, 2022 07:55
@n1v0lg
Copy link
Copy Markdown
Contributor Author

n1v0lg commented Jul 14, 2022

@elasticmachine run elasticsearch-ci/part-1

@n1v0lg n1v0lg merged commit 4efc09c into elastic:master Jul 14, 2022
@n1v0lg n1v0lg deleted the fix/api-key-noop-test-metadata branch July 14, 2022 09:04
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Jul 15, 2022
@weizijun
Merge branch 'upstream/master' into number-support-single-type
0d878ac 
* upstream/master: (2974 commits)
  Reserved cluster state service (elastic#88527)
  Add transport action immutable state checks (elastic#88491)
  Remove suggest flag from index stats docs (elastic#85479)
  Polling cluster formation state for master-is-stable health indicator (elastic#88397)
  Add test execution guide in yamlRestTest asciidoc (elastic#88490)
  Add troubleshooting guide for corrupt repository (elastic#88391)
  [Transform] Finetune Schedule to be less noisy on retry and retry slower (elastic#88531)
  Updatable API keys - auto-update legacy RDs (elastic#88514)
  Fix typo in TransportForceMergeAction and TransportClearIndicesCacheA… (elastic#88064)
  Fixed NullPointerException on bulk request (elastic#88358)
  Avoid needless index metadata builders during reroute (elastic#88506)
  Set metadata on request in API key noop test (elastic#88507)
  Fix passing positional args to ES in Docker (elastic#88502)
  Improve description for task api detailed param (elastic#88493)
  Support cartesian shape with doc values (elastic#88487)
  Promote usage of Subjects in Authentication class (elastic#88494)
  Add CCx 2.0 feature flag (elastic#88451)
  Reword the watcher 'always' and 'never' condition docs (elastic#86105)
  Simplify azure discovery installation docs (elastic#88404)
  Breakup FIPS CI testing jobs
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
#	x-pack/plugin/mapper-aggregate-metric/src/main/java/org/elasticsearch/xpack/aggregatemetric/mapper/AggregateDoubleMetricFieldMapper.java
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ywangd ywangd ywangd approved these changes

Assignees

@n1v0lg n1v0lg

Labels

:Security/Security Security issues without another label Team:Security Meta label for security team >test Issues or PRs that are addressing/adding tests v8.4.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] ApiKeyIntegTests testNoopUpdateApiKey failing

4 participants

@n1v0lg @elasticmachine @ywangd @elasticsearchmachine