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

Also enable node decom using string "true" #626

Merged
merged 1 commit into from Jul 4, 2017

Conversation

Projects
None yet
3 participants
@jaydoane
Contributor

jaydoane commented Jun 30, 2017

Overview

To decom a node, one normally inserts {"decom":true} into that node's
document via the admin /nodes endpoint. However, it's easy to
accidentally insert {"decom":"true"} instead, in which case the
current mem3 allowed_nodes logic will include that node as allowed,
which is probably not what was intended, and ultimately result in "500
badard" errors sent to the client.

This changes the allowed_nodes algorithm to also accept {"decom":"true"}
to decom a node.

Thanks to Nick Vatamaniuc for discovering this shortcoming.

Testing recommendations

I've included a new unit test for this code, but I wonder if it's more trouble than it's worth.

GitHub issue number

Related Pull Requests

Checklist

  • Code is written and works correctly;
  • Changes are covered by tests;
  • Documentation reflects the changes;
@eiri

This comment has been minimized.

Show comment
Hide comment
@eiri

eiri Jun 30, 2017

Member

Hmm, jiffy actually supporst JSON booleans

(node1@192.168.50.50)1> jiffy:decode(<<"{\"a\":true}">>).
{[{<<"a">>,true}]}
(node1@192.168.50.50)2> jiffy:decode(<<"{\"a\":\"true\"}">>).
{[{<<"a">>,<<"true">>}]}

So this is more about invalid doc field allowed on admin /nodes endpoint rather than mem3 bug.

I'd suggest either add validation on adding that "decom" field or make mem3 accept both boolean and string, after all you are trading one bug for another here.

Member

eiri commented Jun 30, 2017

Hmm, jiffy actually supporst JSON booleans

(node1@192.168.50.50)1> jiffy:decode(<<"{\"a\":true}">>).
{[{<<"a">>,true}]}
(node1@192.168.50.50)2> jiffy:decode(<<"{\"a\":\"true\"}">>).
{[{<<"a">>,<<"true">>}]}

So this is more about invalid doc field allowed on admin /nodes endpoint rather than mem3 bug.

I'd suggest either add validation on adding that "decom" field or make mem3 accept both boolean and string, after all you are trading one bug for another here.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Jun 30, 2017

Contributor

Agree with Eric. Since it's a user settable value maybe accept both values as truth?

Ideally a VDU or other validation to prevent a non-boolean value to be set would be better.

Contributor

nickva commented Jun 30, 2017

Agree with Eric. Since it's a user settable value maybe accept both values as truth?

Ideally a VDU or other validation to prevent a non-boolean value to be set would be better.

@jaydoane jaydoane changed the title from Don't include "decom" nodes in allowed_nodes to Also enable node decom using string "true" Jul 1, 2017

@jaydoane

This comment has been minimized.

Show comment
Hide comment
@jaydoane

jaydoane Jul 1, 2017

Contributor

@nickva @eiri thanks for the reviews!

I've updated this PR to allow "decom" to be either true or "true" to work.

Contributor

jaydoane commented Jul 1, 2017

@nickva @eiri thanks for the reviews!

I've updated this PR to allow "decom" to be either true or "true" to work.

@nickva

This comment has been minimized.

Show comment
Hide comment
@nickva

nickva Jul 3, 2017

Contributor

@jaydoane looks good!

+1 (but see comment about andalso)

Contributor

nickva commented Jul 3, 2017

@jaydoane looks good!

+1 (but see comment about andalso)

Also enable node decom using string "true"
To decom a node, one normally inserts {"decom":true} into that node's
document via the admin /nodes endpoint. However, it's easy to
accidentally insert {"decom":"true"} instead, in which case the
current mem3 allowed_nodes logic will include that node as allowed,
which is probably not what was intended, and ultimately result in "500
badard" errors sent to the client.

This changes the allowed_nodes algorithm to also accept {"decom":"true"}
to decom a node.

Thanks to Nick Vatamaniuc for discovering this shortcoming.
@jaydoane

This comment has been minimized.

Show comment
Hide comment
@jaydoane

jaydoane Jul 4, 2017

Contributor

Updated to use andalso. If acceptable, can a committer please merge as well? Thanks.

Contributor

jaydoane commented Jul 4, 2017

Updated to use andalso. If acceptable, can a committer please merge as well? Thanks.

@nickva nickva merged commit 409ea97 into apache:master Jul 4, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@nickva nickva deleted the cloudant:fix-mem3-allowed-nodes branch Jul 4, 2017

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