From b063c87596f4e05e4aaac9f69541e01278920451 Mon Sep 17 00:00:00 2001 From: yang-g Date: Wed, 7 Oct 2015 11:40:13 -0700 Subject: [PATCH 01/17] mark unreachable code with a macro --- include/grpc/support/port_platform.h | 12 ++++++++++++ src/core/channel/client_channel.c | 4 +--- src/core/client_config/uri_parser.c | 4 ++-- src/core/httpcli/parser.c | 11 +++-------- src/core/iomgr/tcp_client_posix.c | 2 +- src/core/iomgr/tcp_server_posix.c | 2 +- src/core/surface/byte_buffer.c | 7 ++----- src/core/surface/call.c | 3 +-- src/core/surface/channel_connectivity.c | 8 ++------ src/core/surface/completion_queue.c | 3 +-- src/core/transport/chttp2/frame_data.c | 4 +--- src/core/transport/chttp2/frame_goaway.c | 4 +--- src/core/transport/chttp2/hpack_parser.c | 8 ++------ src/core/transport/chttp2/parsing.c | 16 ++++------------ src/core/transport/chttp2/stream_encoder.c | 6 +++--- src/cpp/server/server.cc | 3 +-- 16 files changed, 38 insertions(+), 59 deletions(-) diff --git a/include/grpc/support/port_platform.h b/include/grpc/support/port_platform.h index 434dd6de874..f98038da520 100644 --- a/include/grpc/support/port_platform.h +++ b/include/grpc/support/port_platform.h @@ -181,6 +181,7 @@ #ifndef _BSD_SOURCE #define _BSD_SOURCE #endif +#define GPR_FORBID_UNREACHABLE_CODE #define GPR_MSG_IOVLEN_TYPE int #if TARGET_OS_IPHONE #define GPR_PLATFORM_STRING "ios" @@ -336,4 +337,15 @@ typedef uintptr_t gpr_uintptr; #endif #endif +#ifdef GPR_FORBID_UNREACHABLE_CODE +#define GPR_UNREACHABLE_CODE(STATEMENT) +#else +#define GPR_UNREACHABLE_CODE(STATEMENT) \ + do { \ + gpr_log(GPR_ERROR, "Should never reach here."); \ + abort(); \ + STATEMENT; \ + } while (0) +#endif /* GPR_FORBID_UNREACHABLE_CODE */ + #endif /* GRPC_SUPPORT_PORT_PLATFORM_H */ diff --git a/src/core/channel/client_channel.c b/src/core/channel/client_channel.c index b59b62a6aa7..4b5cd04dc11 100644 --- a/src/core/channel/client_channel.c +++ b/src/core/channel/client_channel.c @@ -645,9 +645,7 @@ static void destroy_call_elem(grpc_exec_ctx *exec_ctx, case CALL_WAITING_FOR_CONFIG: case CALL_WAITING_FOR_CALL: case CALL_WAITING_FOR_SEND: - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - break; + GPR_UNREACHABLE_CODE(return ); } } diff --git a/src/core/client_config/uri_parser.c b/src/core/client_config/uri_parser.c index df9f32d403c..cbdfffcf8e8 100644 --- a/src/core/client_config/uri_parser.c +++ b/src/core/client_config/uri_parser.c @@ -37,6 +37,7 @@ #include #include +#include #include /** a size_t default value... maps to all 1's */ @@ -120,8 +121,7 @@ static int parse_fragment_or_query(const char *uri_text, size_t *i) { } else { return 1; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return 0); default: (*i) += advance; break; diff --git a/src/core/httpcli/parser.c b/src/core/httpcli/parser.c index 404906d5ae2..046770c0944 100644 --- a/src/core/httpcli/parser.c +++ b/src/core/httpcli/parser.c @@ -139,8 +139,7 @@ static int finish_line(grpc_httpcli_parser *parser) { } break; case GRPC_HTTPCLI_BODY: - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return 0); } parser->cur_line_length = 0; @@ -165,8 +164,7 @@ static int addbyte(grpc_httpcli_parser *parser, gpr_uint8 byte) { } else { return 1; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return 0); case GRPC_HTTPCLI_BODY: if (parser->r.body_length == parser->body_capacity) { parser->body_capacity = GPR_MAX(8, parser->body_capacity * 3 / 2); @@ -177,10 +175,7 @@ static int addbyte(grpc_httpcli_parser *parser, gpr_uint8 byte) { parser->r.body_length++; return 1; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - - return 0; + GPR_UNREACHABLE_CODE(return 0); } void grpc_httpcli_parser_init(grpc_httpcli_parser *parser) { diff --git a/src/core/iomgr/tcp_client_posix.c b/src/core/iomgr/tcp_client_posix.c index aca2691c416..fe200392641 100644 --- a/src/core/iomgr/tcp_client_posix.c +++ b/src/core/iomgr/tcp_client_posix.c @@ -191,7 +191,7 @@ static void on_writable(grpc_exec_ctx *exec_ctx, void *acp, int success) { goto finish; } - abort(); + GPR_UNREACHABLE_CODE(return ); finish: if (fd != NULL) { diff --git a/src/core/iomgr/tcp_server_posix.c b/src/core/iomgr/tcp_server_posix.c index 13bd67576f5..99c76dcbe9a 100644 --- a/src/core/iomgr/tcp_server_posix.c +++ b/src/core/iomgr/tcp_server_posix.c @@ -352,7 +352,7 @@ static void on_read(grpc_exec_ctx *exec_ctx, void *arg, int success) { gpr_free(addr_str); } - abort(); + GPR_UNREACHABLE_CODE(return ); error: gpr_mu_lock(&sp->server->mu); diff --git a/src/core/surface/byte_buffer.c b/src/core/surface/byte_buffer.c index a930949f2d4..fb39c4531d9 100644 --- a/src/core/surface/byte_buffer.c +++ b/src/core/surface/byte_buffer.c @@ -75,9 +75,7 @@ grpc_byte_buffer *grpc_byte_buffer_copy(grpc_byte_buffer *bb) { return grpc_raw_byte_buffer_create(bb->data.raw.slice_buffer.slices, bb->data.raw.slice_buffer.count); } - gpr_log(GPR_INFO, "should never get here"); - abort(); - return NULL; + GPR_UNREACHABLE_CODE(return NULL); } void grpc_byte_buffer_destroy(grpc_byte_buffer *bb) { @@ -95,6 +93,5 @@ size_t grpc_byte_buffer_length(grpc_byte_buffer *bb) { case GRPC_BB_RAW: return bb->data.raw.slice_buffer.length; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return 0); } diff --git a/src/core/surface/call.c b/src/core/surface/call.c index d15a3bcbade..bc1b95a305d 100644 --- a/src/core/surface/call.c +++ b/src/core/surface/call.c @@ -429,8 +429,7 @@ static grpc_cq_completion *allocate_completion(grpc_call *call) { gpr_mu_unlock(&call->completion_mu); return &call->completions[i]; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return NULL); } static void done_completion(grpc_exec_ctx *exec_ctx, void *call, diff --git a/src/core/surface/channel_connectivity.c b/src/core/surface/channel_connectivity.c index f72fb041426..ca3c02c536a 100644 --- a/src/core/surface/channel_connectivity.c +++ b/src/core/surface/channel_connectivity.c @@ -100,9 +100,7 @@ static void finished_completion(grpc_exec_ctx *exec_ctx, void *pw, switch (w->phase) { case WAITING: case CALLED_BACK: - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - break; + GPR_UNREACHABLE_CODE(return ); case CALLING_BACK: w->phase = CALLED_BACK; break; @@ -149,9 +147,7 @@ static void partly_done(grpc_exec_ctx *exec_ctx, state_watcher *w, w->phase = CALLING_BACK_AND_FINISHED; break; case CALLING_BACK_AND_FINISHED: - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - break; + GPR_UNREACHABLE_CODE(return ); case CALLED_BACK: delete = 1; break; diff --git a/src/core/surface/completion_queue.c b/src/core/surface/completion_queue.c index e818ccba488..9e552c2cdf8 100644 --- a/src/core/surface/completion_queue.c +++ b/src/core/surface/completion_queue.c @@ -254,8 +254,7 @@ static void del_plucker(grpc_completion_queue *cc, void *tag, return; } } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return ); } grpc_event grpc_completion_queue_pluck(grpc_completion_queue *cc, void *tag, diff --git a/src/core/transport/chttp2/frame_data.c b/src/core/transport/chttp2/frame_data.c index acfa7c002eb..07179a4571e 100644 --- a/src/core/transport/chttp2/frame_data.c +++ b/src/core/transport/chttp2/frame_data.c @@ -168,7 +168,5 @@ grpc_chttp2_parse_error grpc_chttp2_data_parser_parse( } } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - return GRPC_CHTTP2_CONNECTION_ERROR; + GPR_UNREACHABLE_CODE(return GRPC_CHTTP2_CONNECTION_ERROR); } diff --git a/src/core/transport/chttp2/frame_goaway.c b/src/core/transport/chttp2/frame_goaway.c index 2ff1eda89b6..c5758bcb714 100644 --- a/src/core/transport/chttp2/frame_goaway.c +++ b/src/core/transport/chttp2/frame_goaway.c @@ -152,9 +152,7 @@ grpc_chttp2_parse_error grpc_chttp2_goaway_parser_parse( } return GRPC_CHTTP2_PARSE_OK; } - gpr_log(GPR_ERROR, "Should never end up here"); - abort(); - return GRPC_CHTTP2_CONNECTION_ERROR; + GPR_UNREACHABLE_CODE(return GRPC_CHTTP2_CONNECTION_ERROR); } void grpc_chttp2_goaway_append(gpr_uint32 last_stream_id, gpr_uint32 error_code, diff --git a/src/core/transport/chttp2/hpack_parser.c b/src/core/transport/chttp2/hpack_parser.c index 20ea5133751..20d8312d547 100644 --- a/src/core/transport/chttp2/hpack_parser.c +++ b/src/core/transport/chttp2/hpack_parser.c @@ -1166,9 +1166,7 @@ static int append_string(grpc_chttp2_hpack_parser *p, const gpr_uint8 *cur, append_bytes(str, decoded, 3); goto b64_byte0; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - return 1; + GPR_UNREACHABLE_CODE(return 1); } /* append a null terminator to a string */ @@ -1313,9 +1311,7 @@ static int parse_value_string(grpc_chttp2_hpack_parser *p, const gpr_uint8 *cur, return 0; } /* Add code to prevent return without value error */ - gpr_log(GPR_ERROR, "Should never reach beyond switch in parse_value_string"); - abort(); - return 0; + GPR_UNREACHABLE_CODE(return 0); } static int parse_value_string_with_indexed_key(grpc_chttp2_hpack_parser *p, diff --git a/src/core/transport/chttp2/parsing.c b/src/core/transport/chttp2/parsing.c index f7a0a10581b..a0977ccaf6c 100644 --- a/src/core/transport/chttp2/parsing.c +++ b/src/core/transport/chttp2/parsing.c @@ -417,14 +417,10 @@ int grpc_chttp2_perform_read(grpc_exec_ctx *exec_ctx, transport_parsing->incoming_frame_size -= (gpr_uint32)(end - cur); return 1; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return 0); } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - - return 0; + GPR_UNREACHABLE_CODE(return 0); } static int init_frame_parser(grpc_chttp2_transport_parsing *transport_parsing) { @@ -580,9 +576,7 @@ static int init_data_frame_parser( case GRPC_CHTTP2_CONNECTION_ERROR: return 0; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - return 0; + GPR_UNREACHABLE_CODE(return 0); } static void free_timeout(void *p) { gpr_free(p); } @@ -820,7 +814,5 @@ static int parse_frame_slice(grpc_exec_ctx *exec_ctx, case GRPC_CHTTP2_CONNECTION_ERROR: return 0; } - gpr_log(GPR_ERROR, "should never reach here"); - abort(); - return 0; + GPR_UNREACHABLE_CODE(return 0); } diff --git a/src/core/transport/chttp2/stream_encoder.c b/src/core/transport/chttp2/stream_encoder.c index 83227e677db..6c7f7a9ea7f 100644 --- a/src/core/transport/chttp2/stream_encoder.c +++ b/src/core/transport/chttp2/stream_encoder.c @@ -428,7 +428,7 @@ static grpc_mdelem *hpack_enc(grpc_chttp2_hpack_compressor *c, emit_lithdr_noidx(c, dynidx(c, indices_key), elem, st); return elem; } - abort(); + GPR_UNREACHABLE_CODE(return NULL); } indices_key = c->indices_keys[HASH_FRAGMENT_3(key_hash)]; @@ -442,7 +442,7 @@ static grpc_mdelem *hpack_enc(grpc_chttp2_hpack_compressor *c, emit_lithdr_noidx(c, dynidx(c, indices_key), elem, st); return elem; } - abort(); + GPR_UNREACHABLE_CODE(return NULL); } /* no elem, key in the table... fall back to literal emission */ @@ -454,7 +454,7 @@ static grpc_mdelem *hpack_enc(grpc_chttp2_hpack_compressor *c, emit_lithdr_noidx_v(c, elem, st); return elem; } - abort(); + GPR_UNREACHABLE_CODE(return NULL); } #define STRLEN_LIT(x) (sizeof(x) - 1) diff --git a/src/cpp/server/server.cc b/src/cpp/server/server.cc index a44e1d20250..e09744b842b 100644 --- a/src/cpp/server/server.cc +++ b/src/cpp/server/server.cc @@ -153,8 +153,7 @@ class Server::SyncRequest GRPC_FINAL : public CompletionQueueTag { GPR_ASSERT((*req)->in_flight_); return true; } - gpr_log(GPR_ERROR, "Should never reach here"); - abort(); + GPR_UNREACHABLE_CODE(return false); } void SetupRequest() { cq_ = grpc_completion_queue_create(nullptr); } From a6a9a6df85797227f3435dd043325791f93b06a4 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Wed, 7 Oct 2015 16:40:04 -0700 Subject: [PATCH 02/17] Add incompressible responses and status echoing to Node interop server --- src/node/interop/interop_server.js | 63 +++++++++++++++++------------- 1 file changed, 36 insertions(+), 27 deletions(-) diff --git a/src/node/interop/interop_server.js b/src/node/interop/interop_server.js index 3e83304faa3..5321005c863 100644 --- a/src/node/interop/interop_server.js +++ b/src/node/interop/interop_server.js @@ -44,6 +44,9 @@ var testProto = grpc.load({ var ECHO_INITIAL_KEY = 'x-grpc-test-echo-initial'; var ECHO_TRAILING_KEY = 'x-grpc-test-echo-trailing-bin'; +var incompressible_data = fs.readFileSync( + __dirname + '/../../../test/cpp/interop/rnd.dat'); + /** * Create a buffer filled with size zeroes * @param {number} size The length of the buffer @@ -83,6 +86,19 @@ function getEchoTrailer(call) { return response_trailer; } +function getPayload(payload_type, size) { + if (payload_type === 'RANDOM') { + payload_type = ['COMPRESSABLE', + 'UNCOMPRESSABLE'][Math.random() < 0.5 ? 0 : 1]; + } + var body; + switch (payload_type) { + case 'COMPRESSABLE': body = zeroBuffer(size); break; + case 'UNCOMPRESSABLE': incompressible_data.slice(size); break; + } + return {type: payload_type, body: body}; +} + /** * Respond to an empty parameter with an empty response. * NOTE: this currently does not work due to issue #137 @@ -104,13 +120,14 @@ function handleEmpty(call, callback) { function handleUnary(call, callback) { echoHeader(call); var req = call.request; - var zeros = zeroBuffer(req.response_size); - var payload_type = req.response_type; - if (payload_type === 'RANDOM') { - payload_type = ['COMPRESSABLE', - 'UNCOMPRESSABLE'][Math.random() < 0.5 ? 0 : 1]; + if (req.response_status) { + var status = req.response_status; + status.metadata = getEchoTrailer(call); + callback(status); + return; } - callback(null, {payload: {type: payload_type, body: zeros}}, + var payload = getPayload(req.response_type, req.response_size); + callback(null, {payload: payload}, getEchoTrailer(call)); } @@ -139,18 +156,14 @@ function handleStreamingInput(call, callback) { function handleStreamingOutput(call) { echoHeader(call); var req = call.request; - var payload_type = req.response_type; - if (payload_type === 'RANDOM') { - payload_type = ['COMPRESSABLE', - 'UNCOMPRESSABLE'][Math.random() < 0.5 ? 0 : 1]; + if (req.response_status) { + var status = req.response_status; + status.metadata = getEchoTrailer(call); + call.emit('error', status); + return; } _.each(req.response_parameters, function(resp_param) { - call.write({ - payload: { - body: zeroBuffer(resp_param.size), - type: payload_type - } - }); + call.write({payload: getPayload(req.response_type, resp_param.size)}); }); call.end(getEchoTrailer(call)); } @@ -163,18 +176,14 @@ function handleStreamingOutput(call) { function handleFullDuplex(call) { echoHeader(call); call.on('data', function(value) { - var payload_type = value.response_type; - if (payload_type === 'RANDOM') { - payload_type = ['COMPRESSABLE', - 'UNCOMPRESSABLE'][Math.random() < 0.5 ? 0 : 1]; + if (value.response_status) { + var status = value.response_status; + status.metadata = getEchoTrailer(call); + call.emit('error', status); + return; } _.each(value.response_parameters, function(resp_param) { - call.write({ - payload: { - body: zeroBuffer(resp_param.size), - type: payload_type - } - }); + call.write({payload: getPayload(value.response_type, resp_param.size)}); }); }); call.on('end', function() { @@ -188,7 +197,7 @@ function handleFullDuplex(call) { * @param {Call} call Call to handle */ function handleHalfDuplex(call) { - throw new Error('HalfDuplexCall not yet implemented'); + call.emit('error', Error('HalfDuplexCall not yet implemented')); } /** From 83815eab40568e142f05376dae48c2cef41bfefd Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 8 Oct 2015 09:31:45 -0700 Subject: [PATCH 03/17] Added interval_us delay in Node interop server --- src/node/interop/interop_server.js | 47 ++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/node/interop/interop_server.js b/src/node/interop/interop_server.js index 5321005c863..cc527364b4a 100644 --- a/src/node/interop/interop_server.js +++ b/src/node/interop/interop_server.js @@ -35,6 +35,7 @@ var fs = require('fs'); var path = require('path'); +var async = require('async'); var _ = require('lodash'); var grpc = require('..'); var testProto = grpc.load({ @@ -86,6 +87,22 @@ function getEchoTrailer(call) { return response_trailer; } +/** + * @typedef Payload + * @type {object} + * @property {string} payload_type The payload type + * @property {Buffer} body The payload body + */ + +/** + * Get a payload of the specified type and size. If the requested payload is + * COMPRESSABLE, it returns a zero buffer. If the type is UNCOMRESSABLE, it + * returns a slice of pre-loaded uncompressable data. If the type is RANDOM, + * it returns one of the other choices, chosen at random. + * @param {string} payload_type The type of payload to return + * @param {Number} size The size of the payload body + * @return {Payload} The requested payload + */ function getPayload(payload_type, size) { if (payload_type === 'RANDOM') { payload_type = ['COMPRESSABLE', @@ -99,6 +116,15 @@ function getPayload(payload_type, size) { return {type: payload_type, body: body}; } +function respondWithStream(call, request, callback) { + async.eachSeries(request.response_parameters, function(resp_param, callback) { + setTimeout(function() { + call.write({payload: getPayload(request.response_type, resp_param.size)}); + callback(); + }, resp_param.interval_us/1000); + }, callback); +} + /** * Respond to an empty parameter with an empty response. * NOTE: this currently does not work due to issue #137 @@ -162,10 +188,13 @@ function handleStreamingOutput(call) { call.emit('error', status); return; } - _.each(req.response_parameters, function(resp_param) { - call.write({payload: getPayload(req.response_type, resp_param.size)}); + respondWithStream(call, req, function(err) { + if (err) { + call.emit(err); + } else { + call.end(getEchoTrailer(call)); + } }); - call.end(getEchoTrailer(call)); } /** @@ -175,6 +204,7 @@ function handleStreamingOutput(call) { */ function handleFullDuplex(call) { echoHeader(call); + var call_ended; call.on('data', function(value) { if (value.response_status) { var status = value.response_status; @@ -182,12 +212,17 @@ function handleFullDuplex(call) { call.emit('error', status); return; } - _.each(value.response_parameters, function(resp_param) { - call.write({payload: getPayload(value.response_type, resp_param.size)}); + call.pause(); + respondWithStream(call, value, function(err) { + call.resume(); + if (call_ended) { + call.end(getEchoTrailer(call)); + } }); }); call.on('end', function() { - call.end(getEchoTrailer(call)); + call_ended = true; + }); } From 30df27aee191cfeea3ceb9e0993ef91d48343cad Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 8 Oct 2015 10:54:22 -0700 Subject: [PATCH 04/17] Made Node interop client use all specified command line flags --- src/node/interop/interop_client.js | 97 ++++++++++++++---------------- 1 file changed, 45 insertions(+), 52 deletions(-) diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index 14cc6c0efe1..ac820dce3e3 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -44,13 +44,6 @@ var GoogleAuth = require('google-auth-library'); var assert = require('assert'); -var AUTH_SCOPE = 'https://www.googleapis.com/auth/xapi.zoo'; -var AUTH_SCOPE_RESPONSE = 'xapi.zoo'; -var AUTH_USER = ('155450119199-vefjjaekcc6cmsd5914v6lqufunmh9ue' + - '@developer.gserviceaccount.com'); -var COMPUTE_ENGINE_USER = ('155450119199-r5aaqa2vqoa9g5mv2m6s3m1l293rlmel' + - '@developer.gserviceaccount.com'); - var ECHO_INITIAL_KEY = 'x-grpc-test-echo-initial'; var ECHO_TRAILING_KEY = 'x-grpc-test-echo-trailing-bin'; @@ -369,7 +362,7 @@ function authTest(expected_user, scope, client, done) { assert.strictEqual(resp.payload.body.length, 314159); assert.strictEqual(resp.username, expected_user); if (scope) { - assert.strictEqual(resp.oauth_scope, AUTH_SCOPE_RESPONSE); + assert(scope.indexOf(resp.oauth_scope) > -1); } if (done) { done(); @@ -377,56 +370,49 @@ function authTest(expected_user, scope, client, done) { }); } -function oauth2Test(expected_user, scope, per_rpc, client, done) { - (new GoogleAuth()).getApplicationDefault(function(err, credential) { - assert.ifError(err); +function computeEngineCreds(client, done, extra) { + authTest(extra.service_account, null, client, done); +} + +function serviceAccountCreds(client, done, extra) { + authTest(extra.default_service_account, extra.oauth_scope, client, done); +} + +function jwtTokenCreds(client, done, extra) { + authTest(extra.default_service_account, null, client, done); +} + +function oauth2Test(client, done, extra) { var arg = { fill_username: true, fill_oauth_scope: true }; - credential = credential.createScoped(scope); - credential.getAccessToken(function(err, token) { - assert.ifError(err); - var updateMetadata = function(authURI, metadata, callback) { - metadata.add('authorization', 'Bearer ' + token); - callback(null, metadata); - }; - var makeTestCall = function(error, client_metadata) { - assert.ifError(error); - client.unaryCall(arg, function(err, resp) { - assert.ifError(err); - assert.strictEqual(resp.username, expected_user); - assert.strictEqual(resp.oauth_scope, AUTH_SCOPE_RESPONSE); - if (done) { - done(); - } - }, client_metadata); - }; - if (per_rpc) { - updateMetadata('', new grpc.Metadata(), makeTestCall); - } else { - client.$updateMetadata = updateMetadata; - makeTestCall(null, new grpc.Metadata()); - } - }); + client.unaryCall(arg, function(err, resp) { + assert.ifError(err); + assert.strictEqual(resp.username, extra.service_account); + assert(extra.oauth_scope.indexOf(resp.oauth_scope) > -1); + if (done) { + done(); + } }); } -function perRpcAuthTest(expected_user, scope, per_rpc, client, done) { +function perRpcAuthTest(client, done, extra) { (new GoogleAuth()).getApplicationDefault(function(err, credential) { assert.ifError(err); var arg = { fill_username: true, fill_oauth_scope: true }; + var scope = extra.oauth_scope; if (credential.createScopedRequired() && scope) { credential = credential.createScoped(scope); } var creds = grpc.credentials.createFromGoogleCredential(credential); client.unaryCall(arg, function(err, resp) { assert.ifError(err); - assert.strictEqual(resp.username, expected_user); - assert.strictEqual(resp.oauth_scope, AUTH_SCOPE_RESPONSE); + assert.strictEqual(resp.username, extra.service_account); + assert(extra.oauth_scope.indexOf(resp.oauth_scope) > -1); if (done) { done(); } @@ -483,15 +469,15 @@ var test_cases = { cancel_after_first_response: {run: cancelAfterFirstResponse}, timeout_on_sleeping_server: {run: timeoutOnSleepingServer}, custom_metadata: {run: customMetadata}, - compute_engine_creds: {run: _.partial(authTest, COMPUTE_ENGINE_USER, null), - getCreds: _.partial(getApplicationCreds, null)}, - service_account_creds: {run: _.partial(authTest, AUTH_USER, AUTH_SCOPE), - getCreds: _.partial(getApplicationCreds, AUTH_SCOPE)}, - jwt_token_creds: {run: _.partial(authTest, AUTH_USER, null), - getCreds: _.partial(getApplicationCreds, null)}, - oauth2_auth_token: {run: _.partial(oauth2Test, AUTH_USER, AUTH_SCOPE, false), - getCreds: _.partial(getOauth2Creds, AUTH_SCOPE)}, - per_rpc_creds: {run: _.partial(perRpcAuthTest, AUTH_USER, AUTH_SCOPE, true)} + compute_engine_creds: {run: computeEngineCreds, + getCreds: getApplicationCreds}, + service_account_creds: {run: serviceAccountCreds, + getCreds: getApplicationCreds}, + jwt_token_creds: {run: jwtTokenCreds, + getCreds: getApplicationCreds}, + oauth2_auth_token: {run: oauth2Test, + getCreds: getOauth2Creds}, + per_rpc_creds: {run: perRpcAuthTest} }; /** @@ -504,8 +490,9 @@ var test_cases = { * @param {bool} tls Indicates that a secure channel should be used * @param {function} done Callback to call when the test is completed. Included * primarily for use with mocha + * @param {object=} extra Extra options for some tests */ -function runTest(address, host_override, test_case, tls, test_ca, done) { +function runTest(address, host_override, test_case, tls, test_ca, done, extra) { // TODO(mlumish): enable TLS functionality var options = {}; var creds; @@ -534,7 +521,7 @@ function runTest(address, host_override, test_case, tls, test_ca, done) { }; if (test.getCreds) { - test.getCreds(function(err, new_creds) { + test.getCreds(extra.oauth_scope, function(err, new_creds) { execute(err, grpc.credentials.combineChannelCredentials( creds, new_creds)); }); @@ -547,13 +534,19 @@ if (require.main === module) { var parseArgs = require('minimist'); var argv = parseArgs(process.argv, { string: ['server_host', 'server_host_override', 'server_port', 'test_case', - 'use_tls', 'use_test_ca'] + 'use_tls', 'use_test_ca', 'default_service_account', 'oauth_scope', + 'service_account_key_file'] }); + var extra_args = { + service_account: argv.default_service_account, + oauth_scope: argv.oauth_scope, + service_account_key_file: argv.service_account_key_file + }; runTest(argv.server_host + ':' + argv.server_port, argv.server_host_override, argv.test_case, argv.use_tls === 'true', argv.use_test_ca === 'true', function () { console.log('OK:', argv.test_case); - }); + }, extra_args); } /** From b8ceb7c5500e70b669329878dda35156a9306175 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Thu, 8 Oct 2015 11:07:16 -0700 Subject: [PATCH 05/17] Added remaining implementable node interop tests, except compression --- src/node/interop/interop_client.js | 78 +++++++++++++++++++++++----- src/node/src/server.js | 2 +- src/node/test/interop_sanity_test.js | 8 +++ 3 files changed, 75 insertions(+), 13 deletions(-) diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index ac820dce3e3..54a256f277f 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -338,6 +338,41 @@ function customMetadata(client, done) { stream.end(); } +function statusCodeAndMessage(client, done) { + done = multiDone(done, 2); + var arg = { + response_status: { + code: 2, + message: "test status message" + } + }; + client.unaryCall(arg, function(err, resp) { + assert(err); + assert.strictEqual(err.code, 2); + assert.strictEqual(err.message, "test status message"); + done(); + }); + var duplex = client.fullDuplexCall(); + duplex.on('status', function(status) { + assert(status); + assert.strictEqual(status.code, 2); + assert.strictEqual(status.details, "test status message"); + done(); + }); + duplex.on('error', function(){}); + duplex.write(arg); + duplex.end(); +} + +function unimplementedMethod(client, done) { + client.unimplementedCall({}, function(err, resp) { + assert(err); + assert.strictEqual(err.code, grpc.status.UNIMPLEMENTED); + assert(!err.message); + done(); + }); +} + /** * Run one of the authentication tests. * @param {string} expected_user The expected username in the response @@ -459,25 +494,44 @@ function getOauth2Creds(scope, callback) { * Map from test case names to test functions */ var test_cases = { - empty_unary: {run: emptyUnary}, - large_unary: {run: largeUnary}, - client_streaming: {run: clientStreaming}, - server_streaming: {run: serverStreaming}, - ping_pong: {run: pingPong}, - empty_stream: {run: emptyStream}, - cancel_after_begin: {run: cancelAfterBegin}, - cancel_after_first_response: {run: cancelAfterFirstResponse}, - timeout_on_sleeping_server: {run: timeoutOnSleepingServer}, - custom_metadata: {run: customMetadata}, + empty_unary: {run: emptyUnary, + Client: testProto.TestService}, + large_unary: {run: largeUnary, + Client: testProto.TestService}, + client_streaming: {run: clientStreaming, + Client: testProto.TestService}, + server_streaming: {run: serverStreaming, + Client: testProto.TestService}, + ping_pong: {run: pingPong, + Client: testProto.TestService}, + empty_stream: {run: emptyStream, + Client: testProto.TestService}, + cancel_after_begin: {run: cancelAfterBegin, + Client: testProto.TestService}, + cancel_after_first_response: {run: cancelAfterFirstResponse, + Client: testProto.TestService}, + timeout_on_sleeping_server: {run: timeoutOnSleepingServer, + Client: testProto.TestService}, + custom_metadata: {run: customMetadata, + Client: testProto.TestService}, + status_code_and_message: {run: statusCodeAndMessage, + Client: testProto.TestService}, + unimplemented_method: {run: unimplementedMethod, + Client: testProto.UnimplementedService}, compute_engine_creds: {run: computeEngineCreds, + Client: testProto.TestService, getCreds: getApplicationCreds}, service_account_creds: {run: serviceAccountCreds, + Client: testProto.TestService, getCreds: getApplicationCreds}, jwt_token_creds: {run: jwtTokenCreds, + Client: testProto.TestService, getCreds: getApplicationCreds}, oauth2_auth_token: {run: oauth2Test, + Client: testProto.TestService, getCreds: getOauth2Creds}, - per_rpc_creds: {run: perRpcAuthTest} + per_rpc_creds: {run: perRpcAuthTest, + Client: testProto.TestService} }; /** @@ -516,7 +570,7 @@ function runTest(address, host_override, test_case, tls, test_ca, done, extra) { var execute = function(err, creds) { assert.ifError(err); - var client = new testProto.TestService(address, creds, options); + var client = new test.Client(address, creds, options); test.run(client, done); }; diff --git a/src/node/src/server.js b/src/node/src/server.js index 87b5b7ad06e..a974d593c95 100644 --- a/src/node/src/server.js +++ b/src/node/src/server.js @@ -629,7 +629,7 @@ function Server(options) { (new Metadata())._getCoreRepresentation(); batch[grpc.opType.SEND_STATUS_FROM_SERVER] = { code: grpc.status.UNIMPLEMENTED, - details: 'This method is not available on this server.', + details: '', metadata: {} }; batch[grpc.opType.RECV_CLOSE_ON_SERVER] = true; diff --git a/src/node/test/interop_sanity_test.js b/src/node/test/interop_sanity_test.js index 804c1d45e4a..f008a87585c 100644 --- a/src/node/test/interop_sanity_test.js +++ b/src/node/test/interop_sanity_test.js @@ -94,4 +94,12 @@ describe('Interop tests', function() { interop_client.runTest(port, name_override, 'custom_metadata', true, true, done); }); + it('should pass status_code_and_message', function(done) { + interop_client.runTest(port, name_override, 'status_code_and_message', + true, true, done); + }); + it('should pass unimplemented_method', function(done) { + interop_client.runTest(port, name_override, 'unimplemented_method', + true, true, done); + }); }); From 9a52908f210b5ed86c9ecaff6b11623c3a1cab82 Mon Sep 17 00:00:00 2001 From: Julien Boeuf Date: Thu, 8 Oct 2015 13:12:14 -0700 Subject: [PATCH 06/17] Fixing #3680 The server auth filter needs a reference on the server credentials so that the processor that belongs to the creds is not destroyed when the server auth filter is still using it. The server auth filter also does not need the security connector but just the auth context. --- src/core/security/credentials.c | 42 ++++++++++++++++++++++++ src/core/security/credentials.h | 8 ++++- src/core/security/security_context.c | 26 ++++++++++----- src/core/security/security_context.h | 11 +++---- src/core/security/server_auth_filter.c | 36 ++++++++++---------- src/core/security/server_secure_chttp2.c | 4 +-- 6 files changed, 91 insertions(+), 36 deletions(-) diff --git a/src/core/security/credentials.c b/src/core/security/credentials.c index 398db20e8cf..5e155d83b9c 100644 --- a/src/core/security/credentials.c +++ b/src/core/security/credentials.c @@ -181,6 +181,48 @@ void grpc_server_credentials_set_auth_metadata_processor( creds->processor = processor; } +static void server_credentials_pointer_arg_destroy(void *p) { + grpc_server_credentials_unref(p); +} + +static void *server_credentials_pointer_arg_copy(void *p) { + return grpc_server_credentials_ref(p); +} + +grpc_arg grpc_server_credentials_to_arg(grpc_server_credentials *p) { + grpc_arg arg; + memset(&arg, 0, sizeof(grpc_arg)); + arg.type = GRPC_ARG_POINTER; + arg.key = GRPC_SERVER_CREDENTIALS_ARG; + arg.value.pointer.p = p; + arg.value.pointer.copy = server_credentials_pointer_arg_copy; + arg.value.pointer.destroy = server_credentials_pointer_arg_destroy; + return arg; +} + +grpc_server_credentials *grpc_server_credentials_from_arg( + const grpc_arg *arg) { + if (strcmp(arg->key, GRPC_SERVER_CREDENTIALS_ARG) != 0) return NULL; + if (arg->type != GRPC_ARG_POINTER) { + gpr_log(GPR_ERROR, "Invalid type %d for arg %s", arg->type, + GRPC_SERVER_CREDENTIALS_ARG); + return NULL; + } + return arg->value.pointer.p; +} + +grpc_server_credentials *grpc_find_server_credentials_in_args( + const grpc_channel_args *args) { + size_t i; + if (args == NULL) return NULL; + for (i = 0; i < args->num_args; i++) { + grpc_server_credentials *p = + grpc_server_credentials_from_arg(&args->args[i]); + if (p != NULL) return p; + } + return NULL; +} + /* -- Ssl credentials. -- */ static void ssl_destruct(grpc_credentials *creds) { diff --git a/src/core/security/credentials.h b/src/core/security/credentials.h index b213e052d34..01203b08f1f 100644 --- a/src/core/security/credentials.h +++ b/src/core/security/credentials.h @@ -215,7 +215,6 @@ typedef struct { grpc_server_credentials *c, grpc_security_connector **sc); } grpc_server_credentials_vtable; -/* TODO(jboeuf): Add a refcount. */ struct grpc_server_credentials { const grpc_server_credentials_vtable *vtable; const char *type; @@ -231,6 +230,13 @@ grpc_server_credentials *grpc_server_credentials_ref( void grpc_server_credentials_unref(grpc_server_credentials *creds); +#define GRPC_SERVER_CREDENTIALS_ARG "grpc.server_credentials" + +grpc_arg grpc_server_credentials_to_arg(grpc_server_credentials *c); +grpc_server_credentials *grpc_server_credentials_from_arg(const grpc_arg *arg); +grpc_server_credentials *grpc_find_server_credentials_in_args( + const grpc_channel_args *args); + /* -- Ssl credentials. -- */ typedef struct { diff --git a/src/core/security/security_context.c b/src/core/security/security_context.c index fb905e0b228..f544c1d943d 100644 --- a/src/core/security/security_context.c +++ b/src/core/security/security_context.c @@ -305,33 +305,43 @@ void grpc_auth_property_reset(grpc_auth_property *property) { memset(property, 0, sizeof(grpc_auth_property)); } -grpc_arg grpc_auth_metadata_processor_to_arg(grpc_auth_metadata_processor *p) { +static void auth_context_pointer_arg_destroy(void *p) { + GRPC_AUTH_CONTEXT_UNREF(p, "auth_context_pointer_arg"); +} + +static void *auth_context_pointer_arg_copy(void *p) { + return GRPC_AUTH_CONTEXT_REF(p, "auth_context_pointer_arg"); +} + +grpc_arg grpc_auth_context_to_arg(grpc_auth_context *p) { grpc_arg arg; memset(&arg, 0, sizeof(grpc_arg)); arg.type = GRPC_ARG_POINTER; - arg.key = GRPC_AUTH_METADATA_PROCESSOR_ARG; + arg.key = GRPC_AUTH_CONTEXT_ARG; arg.value.pointer.p = p; + arg.value.pointer.copy = auth_context_pointer_arg_copy; + arg.value.pointer.destroy = auth_context_pointer_arg_destroy; return arg; } -grpc_auth_metadata_processor *grpc_auth_metadata_processor_from_arg( +grpc_auth_context *grpc_auth_context_from_arg( const grpc_arg *arg) { - if (strcmp(arg->key, GRPC_AUTH_METADATA_PROCESSOR_ARG) != 0) return NULL; + if (strcmp(arg->key, GRPC_AUTH_CONTEXT_ARG) != 0) return NULL; if (arg->type != GRPC_ARG_POINTER) { gpr_log(GPR_ERROR, "Invalid type %d for arg %s", arg->type, - GRPC_AUTH_METADATA_PROCESSOR_ARG); + GRPC_AUTH_CONTEXT_ARG); return NULL; } return arg->value.pointer.p; } -grpc_auth_metadata_processor *grpc_find_auth_metadata_processor_in_args( +grpc_auth_context *grpc_find_auth_context_in_args( const grpc_channel_args *args) { size_t i; if (args == NULL) return NULL; for (i = 0; i < args->num_args; i++) { - grpc_auth_metadata_processor *p = - grpc_auth_metadata_processor_from_arg(&args->args[i]); + grpc_auth_context *p = + grpc_auth_context_from_arg(&args->args[i]); if (p != NULL) return p; } return NULL; diff --git a/src/core/security/security_context.h b/src/core/security/security_context.h index a9a03064108..2bbdc4be973 100644 --- a/src/core/security/security_context.h +++ b/src/core/security/security_context.h @@ -103,13 +103,12 @@ typedef struct { grpc_server_security_context *grpc_server_security_context_create(void); void grpc_server_security_context_destroy(void *ctx); -/* --- Auth metadata processing. --- */ -#define GRPC_AUTH_METADATA_PROCESSOR_ARG "grpc.auth_metadata_processor" +/* --- Channel args for auth context --- */ +#define GRPC_AUTH_CONTEXT_ARG "grpc.auth_context" -grpc_arg grpc_auth_metadata_processor_to_arg(grpc_auth_metadata_processor *p); -grpc_auth_metadata_processor *grpc_auth_metadata_processor_from_arg( - const grpc_arg *arg); -grpc_auth_metadata_processor *grpc_find_auth_metadata_processor_in_args( +grpc_arg grpc_auth_context_to_arg(grpc_auth_context *c); +grpc_auth_context *grpc_auth_context_from_arg(const grpc_arg *arg); +grpc_auth_context *grpc_find_auth_context_in_args( const grpc_channel_args *args); #endif /* GRPC_INTERNAL_CORE_SECURITY_SECURITY_CONTEXT_H */ diff --git a/src/core/security/server_auth_filter.c b/src/core/security/server_auth_filter.c index 30ca9f57a25..2e18369fe8c 100644 --- a/src/core/security/server_auth_filter.c +++ b/src/core/security/server_auth_filter.c @@ -34,7 +34,7 @@ #include #include "src/core/security/auth_filters.h" -#include "src/core/security/security_connector.h" +#include "src/core/security/credentials.h" #include "src/core/security/security_context.h" #include @@ -58,8 +58,8 @@ typedef struct call_data { } call_data; typedef struct channel_data { - grpc_security_connector *security_connector; - grpc_auth_metadata_processor processor; + grpc_auth_context *auth_context; + grpc_server_credentials *creds; grpc_mdctx *mdctx; } channel_data; @@ -160,12 +160,12 @@ static void auth_on_recv(grpc_exec_ctx *exec_ctx, void *user_data, grpc_stream_op *op = &ops[i]; if (op->type != GRPC_OP_METADATA || calld->got_client_metadata) continue; calld->got_client_metadata = 1; - if (chand->processor.process == NULL) continue; + if (chand->creds->processor.process == NULL) continue; calld->md_op = op; calld->md = metadata_batch_to_md_array(&op->data.metadata); - chand->processor.process(chand->processor.state, calld->auth_context, - calld->md.metadata, calld->md.count, - on_md_processing_done, elem); + chand->creds->processor.process( + chand->creds->processor.state, calld->auth_context, + calld->md.metadata, calld->md.count, on_md_processing_done, elem); return; } } @@ -221,7 +221,7 @@ static void init_call_elem(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, } server_ctx = grpc_server_security_context_create(); server_ctx->auth_context = - grpc_auth_context_create(chand->security_connector->auth_context); + grpc_auth_context_create(chand->auth_context); server_ctx->auth_context->pollset = initial_op->bind_pollset; initial_op->context[GRPC_CONTEXT_SECURITY].value = server_ctx; initial_op->context[GRPC_CONTEXT_SECURITY].destroy = @@ -241,9 +241,8 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel *master, const grpc_channel_args *args, grpc_mdctx *mdctx, int is_first, int is_last) { - grpc_security_connector *sc = grpc_find_security_connector_in_args(args); - grpc_auth_metadata_processor *processor = - grpc_find_auth_metadata_processor_in_args(args); + grpc_auth_context *auth_context = grpc_find_auth_context_in_args(args); + grpc_server_credentials *creds = grpc_find_server_credentials_in_args(args); /* grab pointers to our data from the channel element */ channel_data *chand = elem->channel_data; @@ -252,15 +251,14 @@ static void init_channel_elem(grpc_exec_ctx *exec_ctx, path */ GPR_ASSERT(!is_first); GPR_ASSERT(!is_last); - GPR_ASSERT(sc != NULL); - GPR_ASSERT(processor != NULL); + GPR_ASSERT(auth_context != NULL); + GPR_ASSERT(creds != NULL); /* initialize members */ - GPR_ASSERT(!sc->is_client_side); - chand->security_connector = - GRPC_SECURITY_CONNECTOR_REF(sc, "server_auth_filter"); + chand->auth_context = + GRPC_AUTH_CONTEXT_REF(auth_context, "server_auth_filter"); + chand->creds = grpc_server_credentials_ref(creds); chand->mdctx = mdctx; - chand->processor = *processor; } /* Destructor for channel data */ @@ -268,8 +266,8 @@ static void destroy_channel_elem(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem) { /* grab pointers to our data from the channel element */ channel_data *chand = elem->channel_data; - GRPC_SECURITY_CONNECTOR_UNREF(chand->security_connector, - "server_auth_filter"); + GRPC_AUTH_CONTEXT_UNREF(chand->auth_context, "server_auth_filter"); + grpc_server_credentials_unref(chand->creds); } const grpc_channel_filter grpc_server_auth_filter = { diff --git a/src/core/security/server_secure_chttp2.c b/src/core/security/server_secure_chttp2.c index 881e44a3fed..82c639e8301 100644 --- a/src/core/security/server_secure_chttp2.c +++ b/src/core/security/server_secure_chttp2.c @@ -93,9 +93,9 @@ static void setup_transport(grpc_exec_ctx *exec_ctx, void *statep, grpc_server_secure_state *state = statep; grpc_channel_args *args_copy; grpc_arg args_to_add[2]; - args_to_add[0] = grpc_security_connector_to_arg(state->sc); + args_to_add[0] = grpc_server_credentials_to_arg(state->creds); args_to_add[1] = - grpc_auth_metadata_processor_to_arg(&state->creds->processor); + grpc_auth_context_to_arg(state->sc->auth_context); args_copy = grpc_channel_args_copy_and_add( grpc_server_get_channel_args(state->server), args_to_add, GPR_ARRAY_SIZE(args_to_add)); From 90540b4338943f388434ccfade4bb20cace4c777 Mon Sep 17 00:00:00 2001 From: yang-g Date: Thu, 8 Oct 2015 15:13:35 -0700 Subject: [PATCH 07/17] add another return in case the macro is not defined and the compiler complains about no return statement --- src/core/surface/call.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/surface/call.c b/src/core/surface/call.c index c8152b6eae9..f9e6a596741 100644 --- a/src/core/surface/call.c +++ b/src/core/surface/call.c @@ -435,6 +435,7 @@ static grpc_cq_completion *allocate_completion(grpc_call *call) { return &call->completions[i]; } GPR_UNREACHABLE_CODE(return NULL); + return NULL; } static void done_completion(grpc_exec_ctx *exec_ctx, void *call, From 6374f9dc8368d3cbd65293e129a28a448adb75e9 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 9 Oct 2015 11:36:33 -0700 Subject: [PATCH 08/17] Fix a couple of issues with the node interop client This fixes how the node interop client handles authentication failure and how it checks the service account email responses. --- src/node/interop/interop_client.js | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index 54a256f277f..6ee271f431b 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -44,6 +44,9 @@ var GoogleAuth = require('google-auth-library'); var assert = require('assert'); +var SERVICE_ACCOUNT_EMAIL = require( + process.env.GOOGLE_APPLICATION_CREDENTIALS).client_email; + var ECHO_INITIAL_KEY = 'x-grpc-test-echo-initial'; var ECHO_TRAILING_KEY = 'x-grpc-test-echo-trailing-bin'; @@ -410,21 +413,21 @@ function computeEngineCreds(client, done, extra) { } function serviceAccountCreds(client, done, extra) { - authTest(extra.default_service_account, extra.oauth_scope, client, done); + authTest(SERVICE_ACCOUNT_EMAIL, extra.oauth_scope, client, done); } function jwtTokenCreds(client, done, extra) { - authTest(extra.default_service_account, null, client, done); + authTest(SERVICE_ACCOUNT_EMAIL, null, client, done); } function oauth2Test(client, done, extra) { - var arg = { - fill_username: true, - fill_oauth_scope: true - }; + var arg = { + fill_username: true, + fill_oauth_scope: true + }; client.unaryCall(arg, function(err, resp) { assert.ifError(err); - assert.strictEqual(resp.username, extra.service_account); + assert.strictEqual(resp.username, SERVICE_ACCOUNT_EMAIL); assert(extra.oauth_scope.indexOf(resp.oauth_scope) > -1); if (done) { done(); @@ -446,7 +449,7 @@ function perRpcAuthTest(client, done, extra) { var creds = grpc.credentials.createFromGoogleCredential(credential); client.unaryCall(arg, function(err, resp) { assert.ifError(err); - assert.strictEqual(resp.username, extra.service_account); + assert.strictEqual(resp.username, SERVICE_ACCOUNT_EMAIL); assert(extra.oauth_scope.indexOf(resp.oauth_scope) > -1); if (done) { done(); @@ -571,11 +574,12 @@ function runTest(address, host_override, test_case, tls, test_ca, done, extra) { var execute = function(err, creds) { assert.ifError(err); var client = new test.Client(address, creds, options); - test.run(client, done); + test.run(client, done, extra); }; if (test.getCreds) { test.getCreds(extra.oauth_scope, function(err, new_creds) { + assert.ifError(err); execute(err, grpc.credentials.combineChannelCredentials( creds, new_creds)); }); @@ -593,8 +597,7 @@ if (require.main === module) { }); var extra_args = { service_account: argv.default_service_account, - oauth_scope: argv.oauth_scope, - service_account_key_file: argv.service_account_key_file + oauth_scope: argv.oauth_scope }; runTest(argv.server_host + ':' + argv.server_port, argv.server_host_override, argv.test_case, argv.use_tls === 'true', argv.use_test_ca === 'true', From 0221ecbdfda51cf2f4e081be9a520bf210723182 Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Fri, 9 Oct 2015 02:32:25 +0200 Subject: [PATCH 09/17] Ensuring docker dependencies present in the container. --- tools/jenkins/grpc_jenkins_slave/Dockerfile | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tools/jenkins/grpc_jenkins_slave/Dockerfile b/tools/jenkins/grpc_jenkins_slave/Dockerfile index 5f2b425c8c2..129a8db24fa 100644 --- a/tools/jenkins/grpc_jenkins_slave/Dockerfile +++ b/tools/jenkins/grpc_jenkins_slave/Dockerfile @@ -157,6 +157,12 @@ RUN apt-get update && apt-get install -y \ RUN apt-get install -y libzookeeper-mt-dev +################## +# Docker "inception". +# Note this is quite the ugly hack. +# This makes sure that the docker binary we inject has its dependencies. +RUN curl https://get.docker.com/ | sh +RUN apt-get remove --purge -y docker-engine RUN mkdir /var/local/jenkins From 98c0be52284bb6b4a8a4a3c32e5157a2d1f7776d Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Oct 2015 14:33:34 -0700 Subject: [PATCH 10/17] add retries for docker port command --- tools/run_tests/dockerjob.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/tools/run_tests/dockerjob.py b/tools/run_tests/dockerjob.py index 266edd4375b..0d4a1fc117e 100755 --- a/tools/run_tests/dockerjob.py +++ b/tools/run_tests/dockerjob.py @@ -49,10 +49,19 @@ def docker_kill(cid): return subprocess.call(['docker','kill', str(cid)]) == 0 -def docker_mapped_port(cid, port): +def docker_mapped_port(cid, port, timeout_seconds=15): """Get port mapped to internal given internal port for given container.""" - output = subprocess.check_output('docker port %s %s' % (cid, port), shell=True) - return int(output.split(':', 2)[1]) + started = time.time() + while time.time() - started < timeout_seconds: + try: + output = subprocess.check_output('docker port %s %s' % (cid, port), + stderr=_DEVNULL + shell=True) + return int(output.split(':', 2)[1]) + except subprocess.CalledProcessError as e: + pass + raise Exception('Failed to get exposed port %s for container %s.' % + (port, cid)) def finish_jobs(jobs): @@ -68,7 +77,7 @@ def image_exists(image): """Returns True if given docker image exists.""" return subprocess.call(['docker','inspect', image], stdout=_DEVNULL, - stderr=_DEVNULL) == 0 + stderr=subprocess.STDOUT) == 0 def remove_image(image, skip_nonexistent=False, max_retries=10): From 0a14f62e178f8557ed84b689ca6076b3c0372747 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Oct 2015 14:34:29 -0700 Subject: [PATCH 11/17] dont use + character in generated container names --- tools/run_tests/run_interop_tests.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/tools/run_tests/run_interop_tests.py b/tools/run_tests/run_interop_tests.py index 4d09ae7fcda..dea33f7ab20 100755 --- a/tools/run_tests/run_interop_tests.py +++ b/tools/run_tests/run_interop_tests.py @@ -71,6 +71,7 @@ class CXXLanguage: self.client_cmdline_base = ['bins/opt/interop_client'] self.client_cwd = None self.server_cwd = None + self.safename = 'cxx' def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -96,6 +97,7 @@ class CSharpLanguage: self.client_cmdline_base = ['mono', 'Grpc.IntegrationTesting.Client.exe'] self.client_cwd = 'src/csharp/Grpc.IntegrationTesting.Client/bin/Debug' self.server_cwd = 'src/csharp/Grpc.IntegrationTesting.Server/bin/Debug' + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -121,6 +123,7 @@ class JavaLanguage: self.client_cmdline_base = ['./run-test-client.sh'] self.client_cwd = '../grpc-java' self.server_cwd = '../grpc-java' + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -147,6 +150,7 @@ class GoLanguage: # TODO: this relies on running inside docker self.client_cwd = '/go/src/google.golang.org/grpc/interop/client' self.server_cwd = '/go/src/google.golang.org/grpc/interop/server' + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -172,6 +176,7 @@ class NodeLanguage: self.client_cmdline_base = ['node', 'src/node/interop/interop_client.js'] self.client_cwd = None self.server_cwd = None + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -196,6 +201,7 @@ class PHPLanguage: def __init__(self): self.client_cmdline_base = ['src/php/bin/interop_client.sh'] self.client_cwd = None + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -218,6 +224,7 @@ class RubyLanguage: self.client_cmdline_base = ['ruby', 'src/ruby/bin/interop/interop_client.rb'] self.client_cwd = None self.server_cwd = None + self.safename = str(self) def cloud_to_prod_args(self): return (self.client_cmdline_base + _CLOUD_TO_PROD_BASE_ARGS + @@ -337,7 +344,7 @@ def cloud_to_prod_jobspec(language, test_case, docker_image=None, auth=False): cmdline = bash_login_cmdline(cmdline) if docker_image: - container_name = dockerjob.random_name('interop_client_%s' % language) + container_name = dockerjob.random_name('interop_client_%s' % language.safename) cmdline = docker_run_cmdline(cmdline, image=docker_image, cwd=cwd, @@ -370,7 +377,7 @@ def cloud_to_cloud_jobspec(language, test_case, server_name, server_host, '--server_port=%s' % server_port ]) cwd = language.client_cwd if docker_image: - container_name = dockerjob.random_name('interop_client_%s' % language) + container_name = dockerjob.random_name('interop_client_%s' % language.safename) cmdline = docker_run_cmdline(cmdline, image=docker_image, cwd=cwd, @@ -393,7 +400,7 @@ def cloud_to_cloud_jobspec(language, test_case, server_name, server_host, def server_jobspec(language, docker_image): """Create jobspec for running a server""" - container_name = dockerjob.random_name('interop_server_%s' % language) + container_name = dockerjob.random_name('interop_server_%s' % language.safename) cmdline = bash_login_cmdline(language.server_args() + ['--port=%s' % _DEFAULT_SERVER_PORT]) docker_cmdline = docker_run_cmdline(cmdline, @@ -411,10 +418,10 @@ def server_jobspec(language, docker_image): def build_interop_image_jobspec(language, tag=None): """Creates jobspec for building interop docker image for a language""" - safelang = str(language).replace("+", "x") if not tag: - tag = 'grpc_interop_%s:%s' % (safelang, uuid.uuid4()) - env = {'INTEROP_IMAGE': tag, 'BASE_NAME': 'grpc_interop_%s' % safelang} + tag = 'grpc_interop_%s:%s' % (language.safename, uuid.uuid4()) + env = {'INTEROP_IMAGE': tag, + 'BASE_NAME': 'grpc_interop_%s' % language.safename} if not args.travis: env['TTY_FLAG'] = '-t' build_job = jobset.JobSpec( From 1f1c5c541ad9b6c0d17085d3b18639563a5fb250 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Oct 2015 14:36:28 -0700 Subject: [PATCH 12/17] run empty_stream interop test --- tools/run_tests/run_interop_tests.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tools/run_tests/run_interop_tests.py b/tools/run_tests/run_interop_tests.py index dea33f7ab20..6daa967bba4 100755 --- a/tools/run_tests/run_interop_tests.py +++ b/tools/run_tests/run_interop_tests.py @@ -258,11 +258,9 @@ _LANGUAGES = { # languages supported as cloud_to_cloud servers _SERVERS = ['c++', 'node', 'csharp', 'java', 'go', 'ruby'] -# TODO(jtattermusch): add empty_stream once PHP starts supporting it. # TODO(jtattermusch): add timeout_on_sleeping_server once java starts supporting it. -# TODO(jtattermusch): add support for auth tests. _TEST_CASES = ['large_unary', 'empty_unary', 'ping_pong', - 'client_streaming', 'server_streaming', + 'empty_stream', 'client_streaming', 'server_streaming', 'cancel_after_begin', 'cancel_after_first_response'] _AUTH_TEST_CASES = ['compute_engine_creds', 'jwt_token_creds', From be13d818240bd2f37737d8717d4f77a417f44074 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 9 Oct 2015 14:45:30 -0700 Subject: [PATCH 13/17] Fixed some issues with the tests --- src/node/interop/interop_client.js | 16 +++++++++++----- src/node/test/async_test.js | 2 +- src/node/test/channel_test.js | 2 +- src/node/test/credentials_test.js | 10 +++++----- src/node/test/interop_sanity_test.js | 2 +- 5 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/node/interop/interop_client.js b/src/node/interop/interop_client.js index 6ee271f431b..cb55083d1ae 100644 --- a/src/node/interop/interop_client.js +++ b/src/node/interop/interop_client.js @@ -44,8 +44,14 @@ var GoogleAuth = require('google-auth-library'); var assert = require('assert'); -var SERVICE_ACCOUNT_EMAIL = require( - process.env.GOOGLE_APPLICATION_CREDENTIALS).client_email; +var SERVICE_ACCOUNT_EMAIL; +try { + SERVICE_ACCOUNT_EMAIL = require( + process.env.GOOGLE_APPLICATION_CREDENTIALS).client_email; +} catch (e) { + // This will cause the tests to fail if they need that string + SERVICE_ACCOUNT_EMAIL = null; +} var ECHO_INITIAL_KEY = 'x-grpc-test-echo-initial'; var ECHO_TRAILING_KEY = 'x-grpc-test-echo-trailing-bin'; @@ -346,20 +352,20 @@ function statusCodeAndMessage(client, done) { var arg = { response_status: { code: 2, - message: "test status message" + message: 'test status message' } }; client.unaryCall(arg, function(err, resp) { assert(err); assert.strictEqual(err.code, 2); - assert.strictEqual(err.message, "test status message"); + assert.strictEqual(err.message, 'test status message'); done(); }); var duplex = client.fullDuplexCall(); duplex.on('status', function(status) { assert(status); assert.strictEqual(status.code, 2); - assert.strictEqual(status.details, "test status message"); + assert.strictEqual(status.details, 'test status message'); done(); }); duplex.on('error', function(){}); diff --git a/src/node/test/async_test.js b/src/node/test/async_test.js index ce3ce50a2d6..6d71ea24f54 100644 --- a/src/node/test/async_test.js +++ b/src/node/test/async_test.js @@ -57,7 +57,7 @@ describe('Async functionality', function() { grpc.ServerCredentials.createInsecure()); server.start(); math_client = new math.Math('localhost:' + port_num, - grpc.Credentials.createInsecure()); + grpc.credentials.createInsecure()); done(); }); after(function() { diff --git a/src/node/test/channel_test.js b/src/node/test/channel_test.js index b86f89b8a85..05269f7b6e4 100644 --- a/src/node/test/channel_test.js +++ b/src/node/test/channel_test.js @@ -149,7 +149,7 @@ describe('channel', function() { afterEach(function() { channel.close(); }); - it.only('should time out if called alone', function(done) { + it('should time out if called alone', function(done) { var old_state = channel.getConnectivityState(); var deadline = new Date(); deadline.setSeconds(deadline.getSeconds() + 1); diff --git a/src/node/test/credentials_test.js b/src/node/test/credentials_test.js index 8eb91ee69e6..7fc311a888d 100644 --- a/src/node/test/credentials_test.js +++ b/src/node/test/credentials_test.js @@ -130,8 +130,8 @@ describe('client credentials', function() { callback(null, metadata); }; var creds = grpc.credentials.createFromMetadataGenerator(metadataUpdater); - var combined_creds = grpc.credentials.combineCredentials(client_ssl_creds, - creds); + var combined_creds = grpc.credentials.combineChannelCredentials( + client_ssl_creds, creds); var client = new Client('localhost:' + port, combined_creds, client_options); var call = client.unary({}, function(err, data) { @@ -150,8 +150,8 @@ describe('client credentials', function() { callback(null, metadata); }; var creds = grpc.credentials.createFromMetadataGenerator(metadataUpdater); - var combined_creds = grpc.credentials.combineCredentials(client_ssl_creds, - creds); + var combined_creds = grpc.credentials.combineChannelCredentials( + client_ssl_creds, creds); var client = new Client('localhost:' + port, combined_creds, client_options); var call = client.unary({}, function(err, data) { @@ -231,7 +231,7 @@ describe('client credentials', function() { updater_creds, alt_updater_creds); var call = client.unary({}, function(err, data) { assert.ifError(err); - }, null, {credentials: updater_creds}); + }, null, {credentials: combined_updater}); call.on('metadata', function(metadata) { assert.deepEqual(metadata.get('plugin_key'), ['plugin_value']); assert.deepEqual(metadata.get('other_plugin_key'), diff --git a/src/node/test/interop_sanity_test.js b/src/node/test/interop_sanity_test.js index f008a87585c..f8c0b141377 100644 --- a/src/node/test/interop_sanity_test.js +++ b/src/node/test/interop_sanity_test.js @@ -71,7 +71,7 @@ describe('Interop tests', function() { interop_client.runTest(port, name_override, 'server_streaming', true, true, done); }); - it('should pass ping_pong', function(done) { + it.only('should pass ping_pong', function(done) { interop_client.runTest(port, name_override, 'ping_pong', true, true, done); }); it('should pass empty_stream', function(done) { From 5dbbd9178db5a26b9a765534a561cf1e0cba5e94 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Fri, 9 Oct 2015 14:45:55 -0700 Subject: [PATCH 14/17] Revert "Added interval_us delay in Node interop server" This reverts commit 83815eab40568e142f05376dae48c2cef41bfefd. --- src/node/interop/interop_server.js | 47 ++++-------------------------- 1 file changed, 6 insertions(+), 41 deletions(-) diff --git a/src/node/interop/interop_server.js b/src/node/interop/interop_server.js index cc527364b4a..5321005c863 100644 --- a/src/node/interop/interop_server.js +++ b/src/node/interop/interop_server.js @@ -35,7 +35,6 @@ var fs = require('fs'); var path = require('path'); -var async = require('async'); var _ = require('lodash'); var grpc = require('..'); var testProto = grpc.load({ @@ -87,22 +86,6 @@ function getEchoTrailer(call) { return response_trailer; } -/** - * @typedef Payload - * @type {object} - * @property {string} payload_type The payload type - * @property {Buffer} body The payload body - */ - -/** - * Get a payload of the specified type and size. If the requested payload is - * COMPRESSABLE, it returns a zero buffer. If the type is UNCOMRESSABLE, it - * returns a slice of pre-loaded uncompressable data. If the type is RANDOM, - * it returns one of the other choices, chosen at random. - * @param {string} payload_type The type of payload to return - * @param {Number} size The size of the payload body - * @return {Payload} The requested payload - */ function getPayload(payload_type, size) { if (payload_type === 'RANDOM') { payload_type = ['COMPRESSABLE', @@ -116,15 +99,6 @@ function getPayload(payload_type, size) { return {type: payload_type, body: body}; } -function respondWithStream(call, request, callback) { - async.eachSeries(request.response_parameters, function(resp_param, callback) { - setTimeout(function() { - call.write({payload: getPayload(request.response_type, resp_param.size)}); - callback(); - }, resp_param.interval_us/1000); - }, callback); -} - /** * Respond to an empty parameter with an empty response. * NOTE: this currently does not work due to issue #137 @@ -188,13 +162,10 @@ function handleStreamingOutput(call) { call.emit('error', status); return; } - respondWithStream(call, req, function(err) { - if (err) { - call.emit(err); - } else { - call.end(getEchoTrailer(call)); - } + _.each(req.response_parameters, function(resp_param) { + call.write({payload: getPayload(req.response_type, resp_param.size)}); }); + call.end(getEchoTrailer(call)); } /** @@ -204,7 +175,6 @@ function handleStreamingOutput(call) { */ function handleFullDuplex(call) { echoHeader(call); - var call_ended; call.on('data', function(value) { if (value.response_status) { var status = value.response_status; @@ -212,17 +182,12 @@ function handleFullDuplex(call) { call.emit('error', status); return; } - call.pause(); - respondWithStream(call, value, function(err) { - call.resume(); - if (call_ended) { - call.end(getEchoTrailer(call)); - } + _.each(value.response_parameters, function(resp_param) { + call.write({payload: getPayload(value.response_type, resp_param.size)}); }); }); call.on('end', function() { - call_ended = true; - + call.end(getEchoTrailer(call)); }); } From ab5bc721a099df03181fa6dc4ab06bff86a2d9d9 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Oct 2015 14:46:29 -0700 Subject: [PATCH 15/17] prevent polluting output by subprocesses --- tools/run_tests/dockerjob.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/run_tests/dockerjob.py b/tools/run_tests/dockerjob.py index 0d4a1fc117e..6427b8e0220 100755 --- a/tools/run_tests/dockerjob.py +++ b/tools/run_tests/dockerjob.py @@ -55,7 +55,7 @@ def docker_mapped_port(cid, port, timeout_seconds=15): while time.time() - started < timeout_seconds: try: output = subprocess.check_output('docker port %s %s' % (cid, port), - stderr=_DEVNULL + stderr=_DEVNULL, shell=True) return int(output.split(':', 2)[1]) except subprocess.CalledProcessError as e: @@ -85,7 +85,9 @@ def remove_image(image, skip_nonexistent=False, max_retries=10): if skip_nonexistent and not image_exists(image): return True for attempt in range(0, max_retries): - if subprocess.call(['docker','rmi', '-f', image]) == 0: + if subprocess.call(['docker','rmi', '-f', image], + stdout=_DEVNULL, + stderr=subprocess.STDOUT) == 0: return True time.sleep(2) print 'Failed to remove docker image %s' % image From cc6a2b89fc88ec31972d0fb52f5d215d10747773 Mon Sep 17 00:00:00 2001 From: Jan Tattermusch Date: Fri, 9 Oct 2015 14:53:51 -0700 Subject: [PATCH 16/17] eat output from docker kill as well --- tools/run_tests/dockerjob.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/run_tests/dockerjob.py b/tools/run_tests/dockerjob.py index 6427b8e0220..1d67fe3033e 100755 --- a/tools/run_tests/dockerjob.py +++ b/tools/run_tests/dockerjob.py @@ -46,7 +46,9 @@ def random_name(base_name): def docker_kill(cid): """Kills a docker container. Returns True if successful.""" - return subprocess.call(['docker','kill', str(cid)]) == 0 + return subprocess.call(['docker','kill', str(cid)], + stdout=_DEVNULL, + stderr=subprocess.STDOUT) == 0 def docker_mapped_port(cid, port, timeout_seconds=15): From 0ca8c06b19a3074b69ef311fdfb607365ad7206d Mon Sep 17 00:00:00 2001 From: "Nicolas \"Pixel\" Noble" Date: Fri, 9 Oct 2015 23:46:34 +0200 Subject: [PATCH 17/17] Adding basic redirect for the node coverage report html. --- tools/run_tests/run_node.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/run_tests/run_node.sh b/tools/run_tests/run_node.sh index 780969089da..0a11e87c37a 100755 --- a/tools/run_tests/run_node.sh +++ b/tools/run_tests/run_node.sh @@ -46,6 +46,8 @@ then lcov --base-directory . --directory . -c -o coverage.info genhtml -o ../reports/node_ext_coverage --num-spaces 2 \ -t 'Node gRPC test coverage' coverage.info + echo '' > \ + ../reports/node_coverage/index.html else ./node_modules/mocha/bin/mocha --timeout 8000 src/node/test fi