Merge branch '5824-keep-web' into 5824-keep-web-workbench
authorTom Clegg <tom@curoverse.com>
Fri, 30 Oct 2015 18:30:13 +0000 (14:30 -0400)
committerTom Clegg <tom@curoverse.com>
Fri, 30 Oct 2015 18:30:13 +0000 (14:30 -0400)
Conflicts:
sdk/python/tests/run_test_server.py
services/keepproxy/keepproxy_test.go

14 files changed:
apps/workbench/app/controllers/collections_controller.rb
apps/workbench/config/application.default.yml
apps/workbench/test/controllers/collections_controller_test.rb
apps/workbench/test/helpers/download_helper.rb [new file with mode: 0644]
apps/workbench/test/integration/collection_upload_test.rb
apps/workbench/test/integration/download_test.rb [new file with mode: 0644]
apps/workbench/test/integration_helper.rb
apps/workbench/test/test_helper.rb
sdk/python/tests/nginx.conf
sdk/python/tests/run_test_server.py
services/api/app/controllers/database_controller.rb
services/api/test/fixtures/api_client_authorizations.yml
services/keepproxy/keepproxy.go
services/keepproxy/keepproxy_test.go

index e01151ca408b567ec9908349bdfd7a51bcd15a2f..38b58a16500bbdf5808b11ad77791426f3451042 100644 (file)
@@ -1,4 +1,6 @@
 require "arvados/keep"
+require "uri"
+require "cgi"
 
 class CollectionsController < ApplicationController
   include ActionController::Live
@@ -130,11 +132,27 @@ class CollectionsController < ApplicationController
     usable_token = find_usable_token(tokens) do
       coll = Collection.find(params[:uuid])
     end
+    if usable_token.nil?
+      # Response already rendered.
+      return
+    end
+
+    if Rails.configuration.keep_web_url
+      opts = {}
+      if usable_token == params[:reader_token]
+        opts[:path_token] = usable_token
+      elsif usable_token == Rails.configuration.anonymous_user_token
+        # Don't pass a token at all
+      else
+        # We pass the current user's real token only if it's necessary
+        # to read the collection.
+        opts[:query_token] = usable_token
+      end
+      return redirect_to keep_web_url(params[:uuid], params[:file], opts)
+    end
 
     file_name = params[:file].andand.sub(/^(\.\/|\/|)/, './')
-    if usable_token.nil?
-      return  # Response already rendered.
-    elsif file_name.nil? or not coll.manifest.has_file?(file_name)
+    if file_name.nil? or not coll.manifest.has_file?(file_name)
       return render_not_found
     end
 
@@ -305,6 +323,21 @@ class CollectionsController < ApplicationController
     return nil
   end
 
+  def keep_web_url(uuid_or_pdh, file, opts)
+    fmt = {uuid_or_pdh: uuid_or_pdh.sub('+', '-')}
+    uri = URI.parse(Rails.configuration.keep_web_url % fmt)
+    uri.path += '/' unless uri.path.end_with? '/'
+    if opts[:path_token]
+      uri.path += 't=' + opts[:path_token] + '/'
+    end
+    uri.path += '_/'
+    uri.path += CGI::escape(file)
+    if opts[:query_token]
+      uri.query = 'api_token=' + CGI::escape(opts[:query_token])
+    end
+    uri.to_s
+  end
+
   # Note: several controller and integration tests rely on stubbing
   # file_enumerator to return fake file content.
   def file_enumerator opts
index 00959bbb3bea30568e62ddb82c9f0d7f733fb44d..5504fd29a726383544c78d45aa38266fe7458500 100644 (file)
@@ -225,3 +225,11 @@ common:
   # E.g., using a name-based proxy server to forward connections to shell hosts:
   # https://%{hostname}.webshell.uuid_prefix.arvadosapi.com/
   shell_in_a_box_url: false
+
+  # Format of download/preview links. If false, use Workbench's
+  # download facility.
+  #
+  # Examples:
+  # keep_web_url: https://%{uuid_or_pdh}.dl.zzzzz.your.domain
+  # keep_web_url: https://%{uuid_or_pdh}--dl.zzzzz.your.domain
+  keep_web_url: false
index 13644e00bdce28db3460aa2f722f679deb107c7e..b4e7dd36f4b4c0d6dc99527bf2ce5c56b7e8baeb 100644 (file)
@@ -514,4 +514,55 @@ class CollectionsControllerTest < ActionController::TestCase
     get :show, {id: api_fixture('collections')['user_agreement']['uuid']}, session_for(:active)
     assert_not_includes @response.body, '<a href="#Upload"'
   end
+
+  def setup_for_keep_web cfg='https://%{uuid_or_pdh}.dl.zzzzz.example'
+    Rails.configuration.keep_web_url = cfg
+    @controller.expects(:file_enumerator).never
+  end
+
+  %w(uuid portable_data_hash).each do |id_type|
+    test "Redirect to keep_web_url via #{id_type}" do
+      setup_for_keep_web
+      tok = api_fixture('api_client_authorizations')['active']['api_token']
+      id = api_fixture('collections')['w_a_z_file'][id_type]
+      get :show_file, {uuid: id, file: "w a z"}, session_for(:active)
+      assert_response :redirect
+      assert_equal "https://#{id.sub '+', '-'}.dl.zzzzz.example/_/w+a+z?api_token=#{tok}", @response.redirect_url
+    end
+
+    test "Redirect to keep_web_url via #{id_type} with reader token" do
+      setup_for_keep_web
+      tok = api_fixture('api_client_authorizations')['active']['api_token']
+      id = api_fixture('collections')['w_a_z_file'][id_type]
+      get :show_file, {uuid: id, file: "w a z", reader_token: tok}, session_for(:expired)
+      assert_response :redirect
+      assert_equal "https://#{id.sub '+', '-'}.dl.zzzzz.example/t=#{tok}/_/w+a+z", @response.redirect_url
+    end
+
+    test "Redirect to keep_web_url via #{id_type} with no token" do
+      setup_for_keep_web
+      Rails.configuration.anonymous_user_token =
+        api_fixture('api_client_authorizations')['anonymous']['api_token']
+      id = api_fixture('collections')['public_text_file'][id_type]
+      get :show_file, {uuid: id, file: "Hello World.txt"}
+      assert_response :redirect
+      assert_equal "https://#{id.sub '+', '-'}.dl.zzzzz.example/_/Hello+World.txt", @response.redirect_url
+    end
+
+    test "Redirect to keep_web_url via #{id_type} using -attachment-only-host mode" do
+      setup_for_keep_web 'https://dl.zzzzz.example/c=%{uuid_or_pdh}'
+      tok = api_fixture('api_client_authorizations')['active']['api_token']
+      id = api_fixture('collections')['w_a_z_file'][id_type]
+      get :show_file, {uuid: id, file: "w a z"}, session_for(:active)
+      assert_response :redirect
+      assert_equal "https://dl.zzzzz.example/c=#{id.sub '+', '-'}/_/w+a+z?api_token=#{tok}", @response.redirect_url
+    end
+  end
+
+  test "No redirect to keep_web_url if collection not found" do
+    setup_for_keep_web
+    id = api_fixture('collections')['w_a_z_file']['uuid']
+    get :show_file, {uuid: id, file: "w a z"}, session_for(:spectator)
+    assert_response 404
+  end
 end
diff --git a/apps/workbench/test/helpers/download_helper.rb b/apps/workbench/test/helpers/download_helper.rb
new file mode 100644 (file)
index 0000000..21fb4cd
--- /dev/null
@@ -0,0 +1,21 @@
+module DownloadHelper
+  module_function
+
+  def path
+    Rails.root.join 'tmp', 'downloads'
+  end
+
+  def clear
+    FileUtils.rm_f path
+    begin
+      Dir.mkdir path
+    rescue Errno::EEXIST
+    end
+  end
+
+  def done
+    Dir[path.join '*'].reject do |f|
+      /\.part$/ =~ f
+    end
+  end
+end
index 62efee4d67e6b4e5a84e2340bcc55902b18ba30d..903df90fb419c7ba231ae3c42d679abc85af41ed 100644 (file)
@@ -7,9 +7,19 @@ class CollectionUploadTest < ActionDispatch::IntegrationTest
         io.write content
       end
     end
+    # Database reset doesn't restore KeepServices; we have to
+    # save/restore manually.
+    use_token :admin do
+      @keep_services = KeepService.all.to_a
+    end
   end
 
   teardown do
+    use_token :admin do
+      @keep_services.each do |ks|
+        KeepService.find(ks.uuid).update_attributes(ks.attributes)
+      end
+    end
     testfiles.each do |filename, _|
       File.unlink(testfile_path filename)
     end
@@ -42,7 +52,7 @@ class CollectionUploadTest < ActionDispatch::IntegrationTest
     assert_match /_text":"\. d41d8\S+ 0:0:empty.txt\\n\. d41d8\S+ 0:0:empty\\\\040\(1\).txt\\n"/, body
   end
 
-  test "Upload non-empty files, report errors" do
+  test "Upload non-empty files" do
     need_selenium "to make file uploads work"
     visit page_with_token 'active', sandbox_path
     find('.nav-tabs a', text: 'Upload').click
@@ -50,24 +60,17 @@ class CollectionUploadTest < ActionDispatch::IntegrationTest
     attach_file 'file_selector', testfile_path('foo.txt')
     assert_selector 'button:not([disabled])', text: 'Start'
     click_button 'Start'
-    if "test environment does not have a keepproxy yet, see #4534" != "fixed"
-      using_wait_time 20 do
-        assert_text :visible, 'error'
-      end
-    else
-      assert_text :visible, 'Done!'
-      visit sandbox_path+'.json'
-      assert_match /_text":"\. 0cc1\S+ 0:1:a\\n\. acbd\S+ 0:3:foo.txt\\n"/, body
-    end
+    assert_text :visible, 'Done!'
+    visit sandbox_path+'.json'
+    assert_match /_text":"\. 0cc1\S+ 0:1:a\\n\. acbd\S+ 0:3:foo.txt\\n"/, body
   end
 
   test "Report mixed-content error" do
     skip 'Test suite does not use TLS'
     need_selenium "to make file uploads work"
-    begin
-      use_token :admin
-      proxy = KeepService.find(api_fixture('keep_services')['proxy']['uuid'])
-      proxy.update_attributes service_ssl_flag: false
+    use_token :admin do
+      KeepService.where(service_type: 'proxy').first.
+        update_attributes(service_ssl_flag: false)
     end
     visit page_with_token 'active', sandbox_path
     find('.nav-tabs a', text: 'Upload').click
@@ -82,11 +85,12 @@ class CollectionUploadTest < ActionDispatch::IntegrationTest
 
   test "Report network error" do
     need_selenium "to make file uploads work"
-    begin
-      use_token :admin
-      proxy = KeepService.find(api_fixture('keep_services')['proxy']['uuid'])
-      # Even if you somehow do port>2^16, surely nx.example.net won't respond
-      proxy.update_attributes service_host: 'nx.example.net', service_port: 99999
+    use_token :admin do
+      # Even if you somehow do port>2^16, surely nx.example.net won't
+      # respond
+      KeepService.where(service_type: 'proxy').first.
+        update_attributes(service_host: 'nx.example.net',
+                          service_port: 99999)
     end
     visit page_with_token 'active', sandbox_path
     find('.nav-tabs a', text: 'Upload').click
diff --git a/apps/workbench/test/integration/download_test.rb b/apps/workbench/test/integration/download_test.rb
new file mode 100644 (file)
index 0000000..9e4fd56
--- /dev/null
@@ -0,0 +1,45 @@
+require 'integration_helper'
+require 'helpers/download_helper'
+
+class DownloadTest < ActionDispatch::IntegrationTest
+  setup do
+    portfile = File.expand_path '../../../../../tmp/keep-web-ssl.port', __FILE__
+    @kwport = File.read portfile
+    Rails.configuration.keep_web_url = "https://localhost:#{@kwport}/c=%{uuid_or_pdh}"
+    CollectionsController.any_instance.expects(:file_enumerator).never
+
+    # Make sure Capybara can download files.
+    need_selenium 'for downloading', :selenium_with_download
+    DownloadHelper.clear
+
+    # Keep data isn't populated by fixtures, so we have to write any
+    # data we expect to read.
+    unless /^acbd/ =~ `echo -n foo | arv-put --no-progress --raw -` && $?.success?
+      raise $?.to_s
+    end
+  end
+
+  test "download from keep-web with a reader token" do
+    uuid = api_fixture('collections')['foo_file']['uuid']
+    token = api_fixture('api_client_authorizations')['active_all_collections']['api_token']
+    visit "/collections/download/#{uuid}/#{token}/"
+    within "#collection_files" do
+      click_link "foo"
+    end
+    data = nil
+    tries = 0
+    while tries < 20
+      sleep 0.1
+      tries += 1
+      data = File.read(DownloadHelper.path.join 'foo') rescue nil
+    end
+    assert_equal 'foo', data
+  end
+
+  # TODO(TC): test "view pages hosted by keep-web, using session
+  # token". We might persuade selenium to send
+  # "collection-uuid.dl.example" requests to localhost by configuring
+  # our test nginx server to work as its forward proxy. Until then,
+  # we're relying on the "Redirect to keep_web_url via #{id_type}"
+  # test in CollectionsControllerTest (and keep-web's tests).
+end
index 39fdf4b260abd68be05f252158f29629aa199199..5750a1b0c20a4b5a5c8f62092db0eb6039ed9862 100644 (file)
@@ -19,6 +19,17 @@ Capybara.register_driver :poltergeist_without_file_api do |app|
   Capybara::Poltergeist::Driver.new app, POLTERGEIST_OPTS.merge(extensions: [js])
 end
 
+Capybara.register_driver :selenium_with_download do |app|
+  profile = Selenium::WebDriver::Firefox::Profile.new
+  profile['browser.download.dir'] = DownloadHelper.path.to_s
+  profile['browser.download.downloadDir'] = DownloadHelper.path.to_s
+  profile['browser.download.defaultFolder'] = DownloadHelper.path.to_s
+  profile['browser.download.folderList'] = 2 # "save to user-defined location"
+  profile['browser.download.manager.showWhenStarting'] = false
+  profile['browser.helperApps.alwaysAsk.force'] = false
+  Capybara::Selenium::Driver.new app, profile: profile
+end
+
 module WaitForAjax
   Capybara.default_wait_time = 5
   def wait_for_ajax
@@ -73,8 +84,8 @@ module HeadlessHelper
     end
   end
 
-  def need_selenium reason=nil
-    Capybara.current_driver = :selenium
+  def need_selenium reason=nil, driver=:selenium
+    Capybara.current_driver = driver
     unless ENV['ARVADOS_TEST_HEADFUL'] or @headless
       @headless = HeadlessSingleton.get
       @headless.start
index 89d15c67d8de3708c4d74540518b9707e09aa432..41592af993261b6affd55fa6cba0f05780d475ec 100644 (file)
@@ -176,7 +176,10 @@ class ApiServerForTests
       # though it doesn't need to start up a new server).
       env_script = check_output %w(python ./run_test_server.py start --auth admin)
       check_output %w(python ./run_test_server.py start_arv-git-httpd)
+      check_output %w(python ./run_test_server.py start_keep-web)
       check_output %w(python ./run_test_server.py start_nginx)
+      # This one isn't a no-op, even under run-tests.sh.
+      check_output %w(python ./run_test_server.py start_keep)
     end
     test_env = {}
     env_script.each_line do |line|
@@ -192,9 +195,11 @@ class ApiServerForTests
 
   def stop_test_server
     Dir.chdir PYTHON_TESTS_DIR do
+      check_output %w(python ./run_test_server.py stop_keep)
       # These are no-ops if we're running within run-tests.sh
       check_output %w(python ./run_test_server.py stop_nginx)
       check_output %w(python ./run_test_server.py stop_arv-git-httpd)
+      check_output %w(python ./run_test_server.py stop_keep-web)
       check_output %w(python ./run_test_server.py stop)
     end
     @@server_is_running = false
index d8a207f2cf2ce506d9406a646272e92c51c201e6..b14c674523f067469cbdfebd96dd035597b4bb44 100644 (file)
@@ -28,4 +28,18 @@ http {
       proxy_pass http://keepproxy;
     }
   }
+  upstream keep-web {
+    server localhost:{{KEEPWEBPORT}};
+  }
+  server {
+    listen *:{{KEEPWEBSSLPORT}} ssl default_server;
+    server_name ~^(?<request_host>.*)$;
+    ssl_certificate {{SSLCERT}};
+    ssl_certificate_key {{SSLKEY}};
+    location  / {
+      proxy_pass http://keep-web;
+      proxy_set_header Host $request_host:{{KEEPWEBPORT}};
+      proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
+    }
+  }
 }
index d325b4eb6ecb086d15effa34bc26db3e95c9ad15..f8f8b18d7637bed76966dc637a622af3dbcb9020 100644 (file)
@@ -342,7 +342,7 @@ def run_keep(blob_signing_key=None, enforce_permissions=False, num_servers=2):
         token=os.environ['ARVADOS_API_TOKEN'],
         insecure=True)
 
-    for d in api.keep_services().list().execute()['items']:
+    for d in api.keep_services().list(filters=[['service_type','=','disk']]).execute()['items']:
         api.keep_services().delete(uuid=d['uuid']).execute()
     for d in api.keep_disks().list().execute()['items']:
         api.keep_disks().delete(uuid=d['uuid']).execute()
@@ -360,6 +360,12 @@ def run_keep(blob_signing_key=None, enforce_permissions=False, num_servers=2):
             'keep_disk': {'keep_service_uuid': svc['uuid'] }
         }).execute()
 
+    # If keepproxy is running, send SIGHUP to make it discover the new
+    # keepstore services.
+    proxypidfile = _pidfile('keepproxy')
+    if os.path.exists(proxypidfile):
+        os.kill(int(open(proxypidfile).read()), signal.SIGHUP)
+
 def _stop_keep(n):
     kill_server_pid(_pidfile('keep{}'.format(n)), 0)
     if os.path.exists("{}/keep{}.volume".format(TEST_TMPDIR, n)):
@@ -435,10 +441,35 @@ def stop_arv_git_httpd():
         return
     kill_server_pid(_pidfile('arv-git-httpd'), wait=0)
 
+def run_keep_web():
+    if 'ARVADOS_TEST_PROXY_SERVICES' in os.environ:
+        return
+    stop_keep_web()
+
+    keepwebport = find_available_port()
+    env = os.environ.copy()
+    env.pop('ARVADOS_API_TOKEN', None)
+    keepweb = subprocess.Popen(
+        ['keep-web',
+         '-attachment-only-host=localhost:'+str(keepwebport),
+         '-address=:'+str(keepwebport)],
+        env=env, stdin=open('/dev/null'), stdout=sys.stderr)
+    with open(_pidfile('keep-web'), 'w') as f:
+        f.write(str(keepweb.pid))
+    _setport('keep-web', keepwebport)
+    _wait_until_port_listens(keepwebport)
+
+def stop_keep_web():
+    if 'ARVADOS_TEST_PROXY_SERVICES' in os.environ:
+        return
+    kill_server_pid(_pidfile('keep-web'), wait=0)
+
 def run_nginx():
     if 'ARVADOS_TEST_PROXY_SERVICES' in os.environ:
         return
     nginxconf = {}
+    nginxconf['KEEPWEBPORT'] = _getport('keep-web')
+    nginxconf['KEEPWEBSSLPORT'] = find_available_port()
     nginxconf['KEEPPROXYPORT'] = _getport('keepproxy')
     nginxconf['KEEPPROXYSSLPORT'] = find_available_port()
     nginxconf['GITPORT'] = _getport('arv-git-httpd')
@@ -474,6 +505,7 @@ def run_nginx():
     cat_access = subprocess.Popen(
         ['cat', nginxconf['ACCESSLOG']],
         stdout=sys.stderr)
+    _setport('keep-web-ssl', nginxconf['KEEPWEBSSLPORT'])
     _setport('keepproxy-ssl', nginxconf['KEEPPROXYSSLPORT'])
     _setport('arv-git-httpd-ssl', nginxconf['GITSSLPORT'])
 
@@ -555,6 +587,7 @@ class TestCaseWithServers(unittest.TestCase):
     MAIN_SERVER = None
     KEEP_SERVER = None
     KEEP_PROXY_SERVER = None
+    KEEP_WEB_SERVER = None
 
     @staticmethod
     def _restore_dict(src, dest):
@@ -573,7 +606,8 @@ class TestCaseWithServers(unittest.TestCase):
         for server_kwargs, start_func, stop_func in (
                 (cls.MAIN_SERVER, run, reset),
                 (cls.KEEP_SERVER, run_keep, stop_keep),
-                (cls.KEEP_PROXY_SERVER, run_keep_proxy, stop_keep_proxy)):
+                (cls.KEEP_PROXY_SERVER, run_keep_proxy, stop_keep_proxy),
+                (cls.KEEP_WEB_SERVER, run_keep_web, stop_keep_web)):
             if server_kwargs is not None:
                 start_func(**server_kwargs)
                 cls._cleanup_funcs.append(stop_func)
@@ -599,6 +633,7 @@ if __name__ == "__main__":
         'start', 'stop',
         'start_keep', 'stop_keep',
         'start_keep_proxy', 'stop_keep_proxy',
+        'start_keep-web', 'stop_keep-web',
         'start_arv-git-httpd', 'stop_arv-git-httpd',
         'start_nginx', 'stop_nginx',
     ]
@@ -638,6 +673,10 @@ if __name__ == "__main__":
         run_arv_git_httpd()
     elif args.action == 'stop_arv-git-httpd':
         stop_arv_git_httpd()
+    elif args.action == 'start_keep-web':
+        run_keep_web()
+    elif args.action == 'stop_keep-web':
+        stop_keep_web()
     elif args.action == 'start_nginx':
         run_nginx()
     elif args.action == 'stop_nginx':
index 64818da375ecc4fb0277a10be0f3f7e6b552a2e8..21c8e4710cb5e62dbe224a6034c68ea1bd40b05e 100644 (file)
@@ -29,6 +29,10 @@ class DatabaseController < ApplicationController
     fixturesets = Dir.glob(Rails.root.join('test', 'fixtures', '*.yml')).
       collect { |yml| yml.match(/([^\/]*)\.yml$/)[1] }
 
+    # Don't reset keep_services: clients need to discover our
+    # integration-testing keepstores, not test fixtures.
+    fixturesets -= %w[keep_services]
+
     table_names = '"' + ActiveRecord::Base.connection.tables.join('","') + '"'
 
     attempts_left = 20
index 9199d178f6bcdfec3c8536d8da9f7e6b22613898..cb96295064a244073bbe1865f436dd951db890bb 100644 (file)
@@ -87,7 +87,7 @@ active_all_collections:
   user: active
   api_token: activecollectionsabcdefghijklmnopqrstuvwxyz1234567
   expires_at: 2038-01-01 00:00:00
-  scopes: ["GET /arvados/v1/collections/", "GET /arvados/v1/keep_disks"]
+  scopes: ["GET /arvados/v1/collections/", "GET /arvados/v1/keep_services/accessible"]
 
 active_userlist:
   api_client: untrusted
index 1a1189658d7ba1473aa33036cce60276932cb9b8..bad0d22bf1a81868799d8a437860f941d6fbe770 100644 (file)
@@ -22,8 +22,8 @@ import (
 )
 
 // Default TCP address on which to listen for requests.
-// Initialized by the -listen flag.
-const DEFAULT_ADDR = ":25107"
+// Override with -listen.
+const DefaultAddr = ":25107"
 
 var listener net.Listener
 
@@ -42,7 +42,7 @@ func main() {
        flagset.StringVar(
                &listen,
                "listen",
-               DEFAULT_ADDR,
+               DefaultAddr,
                "Interface on which to listen for requests, in the format "+
                        "ipaddr:port. e.g. -listen=10.0.1.24:8000. Use -listen=:port "+
                        "to listen on all network interfaces.")
@@ -79,16 +79,6 @@ func main() {
 
        flagset.Parse(os.Args[1:])
 
-       arv, err := arvadosclient.MakeArvadosClient()
-       if err != nil {
-               log.Fatalf("Error setting up arvados client %s", err.Error())
-       }
-
-       kc, err := keepclient.MakeKeepClient(&arv)
-       if err != nil {
-               log.Fatalf("Error setting up keep client %s", err.Error())
-       }
-
        if pidfile != "" {
                f, err := os.Create(pidfile)
                if err != nil {
@@ -99,16 +89,23 @@ func main() {
                defer os.Remove(pidfile)
        }
 
+       arv, err := arvadosclient.MakeArvadosClient()
+       if err != nil {
+               log.Fatalf("setting up arvados client: %v", err)
+       }
+       kc, err := keepclient.MakeKeepClient(&arv)
+       if err != nil {
+               log.Fatalf("setting up keep client: %v", err)
+       }
        kc.Want_replicas = default_replicas
-
        kc.Client.Timeout = time.Duration(timeout) * time.Second
+       go RefreshServicesList(kc, 5*time.Minute, 3*time.Second)
 
        listener, err = net.Listen("tcp", listen)
        if err != nil {
                log.Fatalf("Could not listen on %v", listen)
        }
-
-       go RefreshServicesList(kc)
+       log.Printf("Arvados Keep proxy started listening on %v", listener.Addr())
 
        // Shut down the server gracefully (by closing the listener)
        // if SIGTERM is received.
@@ -121,9 +118,7 @@ func main() {
        signal.Notify(term, syscall.SIGTERM)
        signal.Notify(term, syscall.SIGINT)
 
-       log.Printf("Arvados Keep proxy started listening on %v", listener.Addr())
-
-       // Start listening for requests.
+       // Start serving requests.
        http.Serve(listener, MakeRESTRouter(!no_get, !no_put, kc))
 
        log.Println("shutting down")
@@ -135,27 +130,39 @@ type ApiTokenCache struct {
        expireTime int64
 }
 
-// Refresh the keep service list every five minutes.
-func RefreshServicesList(kc *keepclient.KeepClient) {
+// Refresh the keep service list on SIGHUP; when the given interval
+// has elapsed since the last refresh; and (if the last refresh
+// failed) the given errInterval has elapsed.
+func RefreshServicesList(kc *keepclient.KeepClient, interval, errInterval time.Duration) {
        var previousRoots = []map[string]string{}
-       var delay time.Duration = 0
+
+       timer := time.NewTimer(interval)
+       gotHUP := make(chan os.Signal, 1)
+       signal.Notify(gotHUP, syscall.SIGHUP)
+
        for {
-               time.Sleep(delay * time.Second)
-               delay = 300
+               select {
+               case <-gotHUP:
+               case <-timer.C:
+               }
+               timer.Reset(interval)
+
                if err := kc.DiscoverKeepServers(); err != nil {
-                       log.Println("Error retrieving services list:", err)
-                       delay = 3
+                       log.Println("Error retrieving services list: %v (retrying in %v)", err, errInterval)
+                       timer.Reset(errInterval)
                        continue
                }
                newRoots := []map[string]string{kc.LocalRoots(), kc.GatewayRoots()}
+
                if !reflect.DeepEqual(previousRoots, newRoots) {
                        log.Printf("Updated services list: locals %v gateways %v", newRoots[0], newRoots[1])
+                       previousRoots = newRoots
                }
+
                if len(newRoots[0]) == 0 {
-                       log.Print("WARNING: No local services. Retrying in 3 seconds.")
-                       delay = 3
+                       log.Printf("WARNING: No local services (retrying in %v)", errInterval)
+                       timer.Reset(errInterval)
                }
-               previousRoots = newRoots
        }
 }
 
@@ -191,12 +198,8 @@ func (this *ApiTokenCache) RecallToken(token string) bool {
 }
 
 func GetRemoteAddress(req *http.Request) string {
-       if realip := req.Header.Get("X-Real-IP"); realip != "" {
-               if forwarded := req.Header.Get("X-Forwarded-For"); forwarded != realip {
-                       return fmt.Sprintf("%s (X-Forwarded-For %s)", realip, forwarded)
-               } else {
-                       return realip
-               }
+       if xff := req.Header.Get("X-Forwarded-For"); xff != "" {
+               return xff + "," + req.RemoteAddr
        }
        return req.RemoteAddr
 }
index 2c75ec1616e3b404a9547b6e2f969ce9fc1fe9f2..e4f09b4e74a071b62797da22c3649ddd1f423938 100644 (file)
@@ -2,21 +2,19 @@ package main
 
 import (
        "crypto/md5"
-       "crypto/tls"
        "fmt"
        "git.curoverse.com/arvados.git/sdk/go/arvadosclient"
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
        "git.curoverse.com/arvados.git/sdk/go/keepclient"
-       . "gopkg.in/check.v1"
-       "io"
        "io/ioutil"
        "log"
        "net/http"
-       "net/url"
        "os"
        "strings"
        "testing"
        "time"
+
+       . "gopkg.in/check.v1"
 )
 
 // Gocheck boilerplate
@@ -36,6 +34,8 @@ var _ = Suite(&NoKeepServerSuite{})
 // Test with no keepserver to simulate errors
 type NoKeepServerSuite struct{}
 
+var TestProxyUUID = "zzzzz-bi6l4-lrixqc4fxofbmzz"
+
 // Wait (up to 1 second) for keepproxy to listen on a port. This
 // avoids a race condition where we hit a "connection refused" error
 // because we start testing the proxy too soon.
@@ -83,106 +83,31 @@ func (s *NoKeepServerSuite) TearDownSuite(c *C) {
        arvadostest.StopAPI()
 }
 
-func setupProxyService() {
-
-       client := &http.Client{Transport: &http.Transport{
-               TLSClientConfig: &tls.Config{InsecureSkipVerify: true}}}
-
-       var req *http.Request
-       var err error
-       if req, err = http.NewRequest("POST", fmt.Sprintf("https://%s/arvados/v1/keep_services", os.Getenv("ARVADOS_API_HOST")), nil); err != nil {
-               panic(err.Error())
-       }
-       req.Header.Add("Authorization", fmt.Sprintf("OAuth2 %s", os.Getenv("ARVADOS_API_TOKEN")))
-
-       reader, writer := io.Pipe()
-
-       req.Body = reader
-
-       go func() {
-               data := url.Values{}
-               data.Set("keep_service", `{
-  "service_host": "localhost",
-  "service_port": 29950,
-  "service_ssl_flag": false,
-  "service_type": "proxy"
-}`)
-
-               writer.Write([]byte(data.Encode()))
-               writer.Close()
-       }()
-
-       var resp *http.Response
-       if resp, err = client.Do(req); err != nil {
-               panic(err.Error())
-       }
-       if resp.StatusCode != 200 {
-               panic(resp.Status)
-       }
-}
+func runProxy(c *C, args []string, bogusClientToken bool) *keepclient.KeepClient {
+       args = append([]string{"keepproxy"}, args...)
+       os.Args = append(args, "-listen=:0")
+       listener = nil
+       go main()
+       waitForListener()
 
-func runProxy(c *C, args []string, port int, bogusClientToken bool) *keepclient.KeepClient {
-       if bogusClientToken {
-               os.Setenv("ARVADOS_API_TOKEN", "bogus-token")
-       }
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, Equals, nil)
-       kc := keepclient.KeepClient{
-               Arvados:       &arv,
-               Want_replicas: 2,
-               Using_proxy:   true,
-               Client:        &http.Client{},
-       }
-       locals := map[string]string{
-               "proxy": fmt.Sprintf("http://localhost:%v", port),
-       }
-       writableLocals := map[string]string{
-               "proxy": fmt.Sprintf("http://localhost:%v", port),
-       }
-       kc.SetServiceRoots(locals, writableLocals, nil)
-       c.Check(kc.Using_proxy, Equals, true)
-       c.Check(len(kc.LocalRoots()), Equals, 1)
-       for _, root := range kc.LocalRoots() {
-               c.Check(root, Equals, fmt.Sprintf("http://localhost:%v", port))
-       }
-       log.Print("keepclient created")
        if bogusClientToken {
-               arvadostest.ResetEnv()
+               arv.ApiToken = "bogus-token"
        }
-
-       {
-               os.Args = append(args, fmt.Sprintf("-listen=:%v", port))
-               listener = nil
-               go main()
+       kc := keepclient.New(&arv)
+       sr := map[string]string{
+               TestProxyUUID: "http://" + listener.Addr().String(),
        }
+       kc.SetServiceRoots(sr, sr, sr)
+       kc.Arvados.External = true
+       kc.Using_proxy = true
 
        return &kc
 }
 
 func (s *ServerRequiredSuite) TestPutAskGet(c *C) {
-       log.Print("TestPutAndGet start")
-
-       os.Args = []string{"keepproxy", "-listen=:29950"}
-       listener = nil
-       go main()
-       time.Sleep(100 * time.Millisecond)
-
-       setupProxyService()
-
-       os.Setenv("ARVADOS_EXTERNAL_CLIENT", "true")
-       arv, err := arvadosclient.MakeArvadosClient()
-       c.Assert(err, Equals, nil)
-       kc, err := keepclient.MakeKeepClient(&arv)
-       c.Assert(err, Equals, nil)
-       c.Check(kc.Arvados.External, Equals, true)
-       c.Check(kc.Using_proxy, Equals, true)
-       c.Check(len(kc.LocalRoots()), Equals, 1)
-       for _, root := range kc.LocalRoots() {
-               c.Check(root, Equals, "http://localhost:29950")
-       }
-       os.Setenv("ARVADOS_EXTERNAL_CLIENT", "")
-
-       waitForListener()
+       kc := runProxy(c, nil, false)
        defer closeListener()
 
        hash := fmt.Sprintf("%x", md5.Sum([]byte("foo")))
@@ -254,15 +179,10 @@ func (s *ServerRequiredSuite) TestPutAskGet(c *C) {
                c.Check(blocklen, Equals, int64(0))
                log.Print("Finished Get zero block")
        }
-
-       log.Print("TestPutAndGet done")
 }
 
 func (s *ServerRequiredSuite) TestPutAskGetForbidden(c *C) {
-       log.Print("TestPutAskGetForbidden start")
-
-       kc := runProxy(c, []string{"keepproxy"}, 29951, true)
-       waitForListener()
+       kc := runProxy(c, nil, true)
        defer closeListener()
 
        hash := fmt.Sprintf("%x", md5.Sum([]byte("bar")))
@@ -300,15 +220,10 @@ func (s *ServerRequiredSuite) TestPutAskGetForbidden(c *C) {
                c.Check(blocklen, Equals, int64(0))
                log.Print("Get")
        }
-
-       log.Print("TestPutAskGetForbidden done")
 }
 
 func (s *ServerRequiredSuite) TestGetDisabled(c *C) {
-       log.Print("TestGetDisabled start")
-
-       kc := runProxy(c, []string{"keepproxy", "-no-get"}, 29952, false)
-       waitForListener()
+       kc := runProxy(c, []string{"-no-get"}, false)
        defer closeListener()
 
        hash := fmt.Sprintf("%x", md5.Sum([]byte("baz")))
@@ -346,38 +261,26 @@ func (s *ServerRequiredSuite) TestGetDisabled(c *C) {
                c.Check(blocklen, Equals, int64(0))
                log.Print("Get")
        }
-
-       log.Print("TestGetDisabled done")
 }
 
 func (s *ServerRequiredSuite) TestPutDisabled(c *C) {
-       log.Print("TestPutDisabled start")
-
-       kc := runProxy(c, []string{"keepproxy", "-no-put"}, 29953, false)
-       waitForListener()
+       kc := runProxy(c, []string{"-no-put"}, false)
        defer closeListener()
 
-       {
-               hash2, rep, err := kc.PutB([]byte("quux"))
-               c.Check(hash2, Equals, "")
-               c.Check(rep, Equals, 0)
-               c.Check(err, Equals, keepclient.InsufficientReplicasError)
-               log.Print("PutB")
-       }
-
-       log.Print("TestPutDisabled done")
+       hash2, rep, err := kc.PutB([]byte("quux"))
+       c.Check(hash2, Equals, "")
+       c.Check(rep, Equals, 0)
+       c.Check(err, Equals, keepclient.InsufficientReplicasError)
 }
 
 func (s *ServerRequiredSuite) TestCorsHeaders(c *C) {
-       runProxy(c, []string{"keepproxy"}, 29954, false)
-       waitForListener()
+       runProxy(c, nil, false)
        defer closeListener()
 
        {
                client := http.Client{}
                req, err := http.NewRequest("OPTIONS",
-                       fmt.Sprintf("http://localhost:29954/%x+3",
-                               md5.Sum([]byte("foo"))),
+                       fmt.Sprintf("http://%s/%x+3", listener.Addr().String(), md5.Sum([]byte("foo"))),
                        nil)
                req.Header.Add("Access-Control-Request-Method", "PUT")
                req.Header.Add("Access-Control-Request-Headers", "Authorization, X-Keep-Desired-Replicas")
@@ -392,8 +295,7 @@ func (s *ServerRequiredSuite) TestCorsHeaders(c *C) {
 
        {
                resp, err := http.Get(
-                       fmt.Sprintf("http://localhost:29954/%x+3",
-                               md5.Sum([]byte("foo"))))
+                       fmt.Sprintf("http://%s/%x+3", listener.Addr().String(), md5.Sum([]byte("foo"))))
                c.Check(err, Equals, nil)
                c.Check(resp.Header.Get("Access-Control-Allow-Headers"), Equals, "Authorization, Content-Length, Content-Type, X-Keep-Desired-Replicas")
                c.Check(resp.Header.Get("Access-Control-Allow-Origin"), Equals, "*")
@@ -401,14 +303,13 @@ func (s *ServerRequiredSuite) TestCorsHeaders(c *C) {
 }
 
 func (s *ServerRequiredSuite) TestPostWithoutHash(c *C) {
-       runProxy(c, []string{"keepproxy"}, 29955, false)
-       waitForListener()
+       runProxy(c, nil, false)
        defer closeListener()
 
        {
                client := http.Client{}
                req, err := http.NewRequest("POST",
-                       "http://localhost:29955/",
+                       "http://"+listener.Addr().String()+"/",
                        strings.NewReader("qux"))
                req.Header.Add("Authorization", "OAuth2 4axaw8zxe0qm22wa6urpp5nskcne8z88cvbupv653y1njyi05h")
                req.Header.Add("Content-Type", "application/octet-stream")
@@ -444,8 +345,7 @@ func (s *ServerRequiredSuite) TestStripHint(c *C) {
 //   With a valid but non-existing prefix (expect "\n")
 //   With an invalid prefix (expect error)
 func (s *ServerRequiredSuite) TestGetIndex(c *C) {
-       kc := runProxy(c, []string{"keepproxy"}, 28852, false)
-       waitForListener()
+       kc := runProxy(c, nil, false)
        defer closeListener()
 
        // Put "index-data" blocks
@@ -477,7 +377,7 @@ func (s *ServerRequiredSuite) TestGetIndex(c *C) {
                {hash[:3], true, false},  // with matching prefix
                {"abcdef", false, false}, // with no such prefix
        } {
-               indexReader, err := kc.GetIndex("proxy", spec.prefix)
+               indexReader, err := kc.GetIndex(TestProxyUUID, spec.prefix)
                c.Assert(err, Equals, nil)
                indexResp, err := ioutil.ReadAll(indexReader)
                c.Assert(err, Equals, nil)
@@ -500,7 +400,7 @@ func (s *ServerRequiredSuite) TestGetIndex(c *C) {
        }
 
        // GetIndex with invalid prefix
-       _, err = kc.GetIndex("proxy", "xyz")
+       _, err = kc.GetIndex(TestProxyUUID, "xyz")
        c.Assert((err != nil), Equals, true)
 }