Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CCR] FollowingEngine should fail with 403 if operation has no seqno assigned #37213

Conversation

Projects
None yet
4 participants
@martijnvg
Copy link
Member

commented Jan 8, 2019

Fail with a 403 when indexing a document directly into a follower index.

In order to test this change, I had to disable assertions in the ccr
engine package. I think that is the right trade off.

[CCR] FollowingEngine should fail with 403 if operation has no seqno …
…assigned

Fail with a 403 when indexing a document directly into a follower index.

In order to test this change, I had to disable assertions in the ccr
engine package. I think that is the right trade off.
@elasticmachine

This comment has been minimized.

Copy link

commented Jan 8, 2019

@dnhatn

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@martijnvg Can we minimize the scope that we will disable the assertion? I think we can create a class that does only the PreFlight then disable the assertion only that class. WDYT?

extract assertions that are thrown before throwing an exception when
writing directly into a follower index into a separate class. So that
these can be disabled in specific tests.
@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

@dnhatn I've updated this PR.

@dnhatn
Copy link
Contributor

left a comment

Thanks @martijnvg. I left some minor comments.

*/
assert operation.seqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO;
if (operation.seqNo() == SequenceNumbers.UNASSIGNED_SEQ_NO) {
throw new ElasticsearchStatusException("a following engine does not accept operations without an assigned sequence number",

This comment has been minimized.

Copy link
@dnhatn

dnhatn Jan 9, 2019

Contributor

I think we should keep this check/throw in the FollowingEngine.

static void preFlight(final Engine.Operation operation) {
/*
* We assert here so that this goes uncaught in unit tests and fails nodes in standalone tests (we want a harsh failure so that we
* do not have a situation where a shard fails and is recovered elsewhere and a test subsequently passes). We throw an exception so

This comment has been minimized.

Copy link
@dnhatn

dnhatn Jan 9, 2019

Contributor

Can we make this method return true (and add prefix assert) and move this comment to the FollowingEngine before we call assert FollowingEngineAssertions. preFlight() in the FollowingEngine?

"invalid version_type in a following engine; version_type=" + operation.versionType() + "origin=" + operation.origin();
}

static void assertPrimaryIncomingSequenceNumber(final Engine.Operation.Origin origin, final long seqNo) {

This comment has been minimized.

Copy link
@dnhatn

dnhatn Jan 9, 2019

Contributor

Can we make this method return true and call assert assertPrimaryIncomingSequenceNumber in the FollowingEngine.

martijnvg added some commits Jan 9, 2019

@martijnvg

This comment has been minimized.

Copy link
Member Author

commented Jan 9, 2019

Thanks @dnhatn, I've updated the PR.

@dnhatn

dnhatn approved these changes Jan 9, 2019

Copy link
Contributor

left a comment

LGTM. Thanks @martijnvg.

martijnvg added some commits Jan 10, 2019

@martijnvg martijnvg merged commit 6d81e7c into elastic:master Jan 10, 2019

7 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci-1 Build finished.
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

martijnvg added a commit that referenced this pull request Jan 10, 2019

[CCR] FollowingEngine should fail with 403 if operation has no seqno …
…assigned (#37213)

Fail with a 403 when indexing a document directly into a follower index.

In order to test this change, I had to move specific assertions into a dedicated class and
disable assertions for that class in the rest qa module. I think that is the right trade off.

@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.