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

Return 500 instead of 403 when Riak is unavailable [JIRA: RCS-359] #1298

Merged
merged 2 commits into from
Mar 21, 2016

Conversation

kuenishi
Copy link
Contributor

@kuenishi kuenishi commented Mar 3, 2016

This commit adds new response 500 from an atom 'disconnected', which
stems from Riak client error. The first call during request handling
is fetching user information of requester via access key ID. It fails
when Riak is unavailable by network partition or node being offline.
Riak CS was handling this as authentication error thus returned 403.

Regression tests are added in regression_tests.erl as a function,
stopping all Riak nodes and trying S3 API requests to CS.

Rebased #1277 to latest 2.1

@kuenishi kuenishi added this to the 2.1.2 milestone Mar 3, 2016
@kuenishi kuenishi added the Bug label Mar 3, 2016
@kuenishi kuenishi force-pushed the ku/dont-403 branch 2 times, most recently from a8d9047 to dab2ca7 Compare March 3, 2016 06:26
@kuenishi kuenishi changed the title Don't return 403 when Riak is unavailable Return 500 instead of 403 when Riak is unavailable Mar 3, 2016
@kuenishi
Copy link
Contributor Author

kuenishi commented Mar 3, 2016

legacy_s3_rewrite_test-cs_multi_backend     : fail ( 115.4 sec)
gc_tests-cs_multi_backend                   : pass ( 105.0 sec)
external_client_tests-cs_multi_backend      : fail ( 168.0 sec)
error_response_test-cs_multi_backend        : fail (  45.9 sec)

Need fix.

@bashopatricia
Copy link

create jira issue

@Basho-JIRA Basho-JIRA changed the title Return 500 instead of 403 when Riak is unavailable Return 500 instead of 403 when Riak is unavailable [JIRA: RCS-359] Mar 4, 2016
This commit adds new response 500 from an atom 'disconnected', which
stems from Riak client error. The first call during request handling
is fetching user information of requester via access key ID. It fails
when Riak is unavailable by network partition or node being offline.
Riak CS was handling this as authentication error thus returned 403.

Regression tests are added in `regression_tests.erl` as a function,
stopping all Riak nodes and trying S3 API requests to CS.
@kuenishi
Copy link
Contributor Author

kuenishi commented Mar 4, 2016

Now all riak_tests have passed. Ready for review.

Test Results:
user_test-cs_multi_backend                  : pass (  62.0 sec)
upgrade_downgrade_test-cs_multi_backend     : pass ( 412.7 sec)
too_large_entity_test-cs_multi_backend      : pass (  58.0 sec)
storage_stats_test_2-cs_multi_backend       : pass (  66.1 sec)
storage_stats_test-cs_multi_backend         : pass (  66.0 sec)
storage_stats_detailed_test-cs_multi_backend: pass (  82.2 sec)
stats_test-cs_multi_backend                 : pass (  58.0 sec)
stanchion_switch_test-cs_multi_backend      : pass (  66.8 sec)
sibling_benchmark-cs_multi_backend          : pass ( 273.0 sec)
select_gc_bucket_test-cs_multi_backend      : pass (  10.4 sec)
riak_cs_debug_test-cs_multi_backend         : pass (  99.4 sec)
repl_v3_test-cs_multi_backend               : pass ( 205.3 sec)
regression_tests_2-cs_multi_backend         : pass ( 113.5 sec)
regression_tests-cs_multi_backend           : pass ( 140.8 sec)
put_copy_test-cs_multi_backend              : pass (  60.1 sec)
object_get_test-cs_multi_backend            : pass (  61.7 sec)
object_get_conditional_test-cs_multi_backend: pass (  58.2 sec)
mp_upload_test-cs_multi_backend             : pass (  67.2 sec)
migration_mixed_test-cs_multi_backend       : pass ( 394.6 sec)
migration_15_to_20_test-cs_multi_backend    : pass ( 364.4 sec)
mb_trans_test-cs_multi_backend              : pass ( 107.2 sec)
mb_trans2_test-cs_multi_backend             : pass ( 343.0 sec)
mb_pg_test-cs_multi_backend                 : pass ( 221.0 sec)
mb_disjoint_test-cs_multi_backend           : pass (  58.9 sec)
list_objects_v2_test-cs_multi_backend       : pass ( 394.9 sec)
list_objects_test-cs_multi_backend          : pass (  71.0 sec)
legacy_s3_rewrite_test-cs_multi_backend     : pass (  73.2 sec)
gc_tests-cs_multi_backend                   : pass ( 129.3 sec)
external_client_tests-cs_multi_backend      : pass ( 189.4 sec)
error_response_test-cs_multi_backend        : pass (  70.5 sec)
cs743_regression_test-cs_multi_backend      : pass (  79.5 sec)
buckets_test-cs_multi_backend               : pass (  60.1 sec)
block_audit_test-cs_multi_backend           : pass (  10.4 sec)
auth_bypass_test-cs_multi_backend           : pass (  57.9 sec)
active_delete_test-cs_multi_backend         : pass (  93.4 sec)
access_stats_test-cs_multi_backend          : pass (  80.6 sec)
---------------------------------------------
0 Tests Failed
36 Tests Passed
That's 100.0% for those keeping score

Props = erlcloud_s3:get_object(BucketName, ?KEY, UserConfig),
?assertEqual(proplists:get_value(content, Props), Data).
verify_cs756({UserConfig, {RiakNodes, _, _}}, BucketName) ->
%% Making sure API call from non-existent user all fails in 403, not 50x

Choose a reason for hiding this comment

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

This comment appears to say the exact opposite of what it should

@tburghart
Copy link

Copyright dates should be updated in all changed files.

@kuenishi
Copy link
Contributor Author

Updated to comments; please take a look again.

@tburghart
Copy link

+1 722276e

borshop added a commit that referenced this pull request Mar 21, 2016
Return 500 instead of 403 when Riak is unavailable [JIRA: RCS-359]

Reviewed-by: tburghart
@tburghart
Copy link

@borshop merge

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

Successfully merging this pull request may close these issues.

None yet

5 participants