Make insecure construction explicit, to address #2614

7 files changed, 25 insertions, 17 deletions

diff --git a/src/ruby/ext/grpc/rb_server.c b/src/ruby/ext/grpc/rb_server.c
index 7e76349d2e..8958237510 100644
--- a/src/ruby/ext/grpc/rb_server.c
+++ b/src/ruby/ext/grpc/rb_server.c
@@ -49,6 +49,9 @@ static VALUE grpc_rb_cServer = Qnil;
 /* id_at is the constructor method of the ruby standard Time class. */
 static ID id_at;
 
+/* id_insecure_server is used to indicate that a server is insecure */
+static VALUE id_insecure_server;
+
 /* grpc_rb_server wraps a grpc_server.  It provides a peer ruby object,
   'mark' to minimize copying when a server is created from ruby. */
 typedef struct grpc_rb_server {
@@ -334,7 +337,7 @@ static VALUE grpc_rb_server_destroy(int argc, VALUE *argv, VALUE self) {
   call-seq:
     // insecure port
     insecure_server = Server.new(cq, {'arg1': 'value1'})
-    insecure_server.add_http2_port('mydomain:50051')
+    insecure_server.add_http2_port('mydomain:50051', :this_port_is_not_secure)
 
     // secure port
     server_creds = ...
@@ -342,21 +345,22 @@ static VALUE grpc_rb_server_destroy(int argc, VALUE *argv, VALUE self) {
     secure_server.add_http_port('mydomain:50051', server_creds)
 
     Adds a http2 port to server */
-static VALUE grpc_rb_server_add_http2_port(int argc, VALUE *argv, VALUE self) {
-  VALUE port = Qnil;
-  VALUE rb_creds = Qnil;
+static VALUE grpc_rb_server_add_http2_port(VALUE self, VALUE port,
+                                           VALUE rb_creds) {
   grpc_rb_server *s = NULL;
   grpc_server_credentials *creds = NULL;
   int recvd_port = 0;
 
-  /* "11" == 1 mandatory args, 1 (rb_creds) is optional */
-  rb_scan_args(argc, argv, "11", &port, &rb_creds);
-
   TypedData_Get_Struct(self, grpc_rb_server, &grpc_rb_server_data_type, s);
   if (s->wrapped == NULL) {
     rb_raise(rb_eRuntimeError, "destroyed!");
     return Qnil;
-  } else if (rb_creds == Qnil) {
+  } else if (TYPE(rb_creds) == T_SYMBOL) {
+    if (id_insecure_server != SYM2ID(rb_creds)) {
+      rb_raise(rb_eTypeError,
+               "bad creds symbol, want :this_port_is_insecure");
+      return Qnil;
+    }
     recvd_port =
         grpc_server_add_insecure_http2_port(s->wrapped, StringValueCStr(port));
     if (recvd_port == 0) {
@@ -398,8 +402,9 @@ void Init_grpc_server() {
   rb_define_alias(grpc_rb_cServer, "close", "destroy");
   rb_define_method(grpc_rb_cServer, "add_http2_port",
                    grpc_rb_server_add_http2_port,
-                   -1);
+                   2);
   id_at = rb_intern("at");
+  id_insecure_server = rb_intern("this_port_is_insecure");
 }
 
 /* Gets the wrapped server from the ruby wrapper */
diff --git a/src/ruby/spec/client_server_spec.rb b/src/ruby/spec/client_server_spec.rb
index 9aacea906f..387f2baec2 100644
--- a/src/ruby/spec/client_server_spec.rb
+++ b/src/ruby/spec/client_server_spec.rb
@@ -396,7 +396,7 @@ describe 'the http client/server' do
     @client_queue = GRPC::Core::CompletionQueue.new
     @server_queue = GRPC::Core::CompletionQueue.new
     @server = GRPC::Core::Server.new(@server_queue, nil)
-    server_port = @server.add_http2_port(server_host)
+    server_port = @server.add_http2_port(server_host, :this_port_is_insecure)
     @server.start
     @ch = Channel.new("0.0.0.0:#{server_port}", nil)
   end
diff --git a/src/ruby/spec/generic/active_call_spec.rb b/src/ruby/spec/generic/active_call_spec.rb
index fcd7bd082f..b05e3284fe 100644
--- a/src/ruby/spec/generic/active_call_spec.rb
+++ b/src/ruby/spec/generic/active_call_spec.rb
@@ -46,7 +46,7 @@ describe GRPC::ActiveCall do
     @server_queue = GRPC::Core::CompletionQueue.new
     host = '0.0.0.0:0'
     @server = GRPC::Core::Server.new(@server_queue, nil)
-    server_port = @server.add_http2_port(host)
+    server_port = @server.add_http2_port(host, :this_port_is_insecure)
     @server.start
     @ch = GRPC::Core::Channel.new("0.0.0.0:#{server_port}", nil)
   end
diff --git a/src/ruby/spec/generic/client_stub_spec.rb b/src/ruby/spec/generic/client_stub_spec.rb
index edcc962a7d..a05433df75 100644
--- a/src/ruby/spec/generic/client_stub_spec.rb
+++ b/src/ruby/spec/generic/client_stub_spec.rb
@@ -498,7 +498,7 @@ describe 'ClientStub' do
   def create_test_server
     @server_queue = GRPC::Core::CompletionQueue.new
     @server = GRPC::Core::Server.new(@server_queue, nil)
-    @server.add_http2_port('0.0.0.0:0')
+    @server.add_http2_port('0.0.0.0:0', :this_port_is_insecure)
   end
 
   def expect_server_to_be_invoked(notifier)
diff --git a/src/ruby/spec/generic/rpc_server_spec.rb b/src/ruby/spec/generic/rpc_server_spec.rb
index 1295fd7fdd..e484a9ea50 100644
--- a/src/ruby/spec/generic/rpc_server_spec.rb
+++ b/src/ruby/spec/generic/rpc_server_spec.rb
@@ -139,7 +139,7 @@ describe GRPC::RpcServer do
     @server_queue = GRPC::Core::CompletionQueue.new
     server_host = '0.0.0.0:0'
     @server = GRPC::Core::Server.new(@server_queue, nil)
-    server_port = @server.add_http2_port(server_host)
+    server_port = @server.add_http2_port(server_host, :this_port_is_insecure)
     @host = "localhost:#{server_port}"
     @ch = GRPC::Core::Channel.new(@host, nil)
   end
diff --git a/src/ruby/spec/pb/health/checker_spec.rb b/src/ruby/spec/pb/health/checker_spec.rb
index 6999a69105..d7b7535cbe 100644
--- a/src/ruby/spec/pb/health/checker_spec.rb
+++ b/src/ruby/spec/pb/health/checker_spec.rb
@@ -186,7 +186,7 @@ describe Grpc::Health::Checker do
       @server_queue = GRPC::Core::CompletionQueue.new
       server_host = '0.0.0.0:0'
       @server = GRPC::Core::Server.new(@server_queue, nil)
-      server_port = @server.add_http2_port(server_host)
+      server_port = @server.add_http2_port(server_host, :this_port_is_insecure)
       @host = "localhost:#{server_port}"
       @ch = GRPC::Core::Channel.new(@host, nil)
       @client_opts = { channel_override: @ch }
diff --git a/src/ruby/spec/server_spec.rb b/src/ruby/spec/server_spec.rb
index c52fe0d9b6..439b19fb8d 100644
--- a/src/ruby/spec/server_spec.rb
+++ b/src/ruby/spec/server_spec.rb
@@ -105,7 +105,7 @@ describe Server do
       it 'runs without failing' do
         blk = proc do
           s = Server.new(@cq, nil)
-          s.add_http2_port('localhost:0')
+          s.add_http2_port('localhost:0', :this_port_is_insecure)
           s.close(@cq)
         end
         expect(&blk).to_not raise_error
@@ -114,7 +114,10 @@ describe Server do
       it 'fails if the server is closed' do
         s = Server.new(@cq, nil)
         s.close(@cq)
-        expect { s.add_http2_port('localhost:0') }.to raise_error(RuntimeError)
+        blk = proc do
+          s.add_http2_port('localhost:0', :this_port_is_insecure)
+        end
+        expect(&blk).to raise_error(RuntimeError)
       end
     end
 
@@ -199,7 +202,7 @@ describe Server do
 
   def start_a_server
     s = Server.new(@cq, nil)
-    s.add_http2_port('0.0.0.0:0')
+    s.add_http2_port('0.0.0.0:0', :this_port_is_insecure)
     s.start
     s
   end