Add assigner support to Blimp

blimp/client/session/assignment_source.cc

Index: blimp/client/session/assignment_source.cc
diff --git a/blimp/client/session/assignment_source.cc b/blimp/client/session/assignment_source.cc
index 271569cb04b2916017a905b2910cfb6dbb55ec35..b6136e8615d7453cfe01facb1bf534efeeaac480 100644
--- a/blimp/client/session/assignment_source.cc
+++ b/blimp/client/session/assignment_source.cc
@@ -6,22 +6,52 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/location.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_number_conversions.h"
+#include "base/values.h"
 #include "blimp/client/app/blimp_client_switches.h"
+#include "blimp/client/session/blimp_client_url_request_context_getter.h"
+#include "blimp/common/protocol_version.h"
 #include "net/base/ip_address.h"
 #include "net/base/ip_endpoint.h"
+#include "net/base/load_flags.h"
+#include "net/base/url_util.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_fetcher.h"
 
 namespace blimp {
 namespace {
 
 // TODO(kmarshall): Take values from configuration data.
 const char kDummyClientToken[] = "MyVoiceIsMyPassport";
-const std::string kDefaultBlimpletIPAddress = "127.0.0.1";
+const char kDefaultBlimpletIPAddress[] = "127.0.0.1";
 const uint16_t kDefaultBlimpletTCPPort = 25467;
 
-net::IPAddress GetBlimpletIPAddress() {
+// Potential assigner URLs.
+const char kProdBlimpAssignerURL[] =

[Kevin M, 2016/02/12 19:45:26]

kDefaultAssignerURL?

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+    "https://blimp-pa.googleapis.com/v1/assignment";
+
+// Assignment request JSON keys.
+const char kProtocolVersionKey[] = "protocol_version";
+
+// Assignment response JSON keys.
+const char kClientTokenKey[] = "clientToken";
+const char kHostKey[] = "host";
+const char kPortKey[] = "port";
+const char kCertificateFingerprintKey[] = "certificateFingerprint";
+const char kCertificateKey[] = "certificate";
+
+bool HasCustomBlimpletEndpoint() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kBlimpletHost) &&
+         base::CommandLine::ForCurrentProcess()->HasSwitch(
+             switches::kBlimpletTCPPort);
+}
+
+net::IPAddress GetCustomBlimpletIPAddress() {
   std::string host;
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kBlimpletHost)) {
@@ -36,7 +66,7 @@ net::IPAddress GetBlimpletIPAddress() {
   return ip_address;
 }
 
-uint16_t GetBlimpletTCPPort() {
+uint16_t GetCustomBlimpletTCPPort() {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kBlimpletTCPPort)) {
     std::string port_str =
@@ -53,23 +83,166 @@ uint16_t GetBlimpletTCPPort() {
   }
 }
 
+GURL GetBlimpAssignerURL() {
+  // TODO(dtrainor): Add a way to specify another assigner.
+  return GURL(kProdBlimpAssignerURL);
+}
+
 }  // namespace
 
 namespace client {
 
 AssignmentSource::AssignmentSource(
-    const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner)
-    : main_task_runner_(main_task_runner) {}
+    const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner,
+    const scoped_refptr<base::SingleThreadTaskRunner>& io_task_runner)
+    : main_task_runner_(main_task_runner),
+      url_request_context_(
+          new BlimpClientURLRequestContextGetter(io_task_runner)) {}
 
 AssignmentSource::~AssignmentSource() {}
 
-void AssignmentSource::GetAssignment(const AssignmentCallback& callback) {
+void AssignmentSource::GetAssignment(const std::string& token,
+                                     const AssignmentCallback& callback) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
-  Assignment assignment;
-  assignment.ip_endpoint =
-      net::IPEndPoint(GetBlimpletIPAddress(), GetBlimpletTCPPort());
-  assignment.client_token = kDummyClientToken;
-  main_task_runner_->PostTask(FROM_HERE, base::Bind(callback, assignment));
+
+  // Cancel any outstanding callback.
+  if (callback_)

[Kevin M, 2016/02/12 19:45:26]

Use !callback_.is_null() and base::ResetAndReturn if you aren't using scoped ptr

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+    RespondWithError(ASSIGNMENT_SOURCE_RESULT_SERVER_INTERRUPTED);
+  callback_.reset(new AssignmentCallback(callback));

[Kevin M, 2016/02/12 19:45:26]

You can just use =

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+
+  // Attempt to build a custom assignment from command line arguments.
+  if (HasCustomBlimpletEndpoint()) {

[Kevin M, 2016/02/12 19:45:26]

Can you turn HasCustom...() into GetCustom...() and move all the parsing into
there? GetCustomBlimpletEndpoint can return return an empty assignment (testable
with an is_null() method) if there is no custom endpoint provided.

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+    Assignment assignment;
+    assignment.ip_endpoint = net::IPEndPoint(GetCustomBlimpletIPAddress(),
+                                             GetCustomBlimpletTCPPort());
+    assignment.client_token = kDummyClientToken;
+
+    // Post the result so that the behavior of this function is consistent.
+    main_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(*callback_, ASSIGNMENT_SOURCE_RESULT_OKAY, assignment));
+    callback_.reset();
+    return;
+  }
+
+  // Call out to the network for a real assignment.  Build the network request
+  // to hit the assigner.
+  url_fetcher_ = net::URLFetcher::Create(GetBlimpAssignerURL(),
+                                         net::URLFetcher::POST, this);
+  url_fetcher_->SetRequestContext(url_request_context_.get());
+  url_fetcher_->SetAutomaticallyRetryOn5xx(false);
+  url_fetcher_->SetAutomaticallyRetryOnNetworkChanges(0);
+  url_fetcher_->SetLoadFlags(net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DO_NOT_SEND_COOKIES);
+  url_fetcher_->AddExtraRequestHeader("Authorization: Bearer " + token);
+
+  // Write the JSON for the request data.
+  base::DictionaryValue dictionary;
+  dictionary.SetString(kProtocolVersionKey, blimp::kEngineVersion);
+  std::string json;
+  base::JSONWriter::Write(dictionary, &json);
+  url_fetcher_->SetUploadData("application/json", json);
+
+  url_fetcher_->Start();
+}
+
+void AssignmentSource::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(callback_.get());

[Kevin M, 2016/02/12 19:45:26]

Could there be two concurrent URL fetches underway at once? When the first one
completes, the callback is reset. The second one will fail this dcheck.

If URLFetcher took a CancelableCallback then this wouldn't be an issue. :\

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Luckily the URLFetcher won't notify it's delegate if it gets destroyed.  When
url_fetcher_ gets rebuilt the previous one will die.  But yeah
CancelableCallback would be convenient as well!

+
+  if (!source->GetStatus().is_success()) {
+    RespondWithError(ASSIGNMENT_SOURCE_RESULT_NETWORK_FAILURE);
+    return;
+  }
+
+  switch (source->GetResponseCode()) {
+    case net::HTTP_OK: {
+      // Grab the response from the assigner request.
+      std::string response;
+      if (!source->GetResponseAsString(&response)) {

[Kevin M, 2016/02/12 19:45:26]

Since the outcome is the same for all these cases, how about stringing them
along in the same if statement with boolean-ANDs?

Unless we plan on adding some logging?

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

I like idea I'll try it out.  If we have unit tests for this kind of parsing we
probably don't need detailed logs here.  I'll add some tests.

+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      // Attempt to interpret the response as JSON and treat it as a dictionary.
+      scoped_ptr<base::Value> json = base::JSONReader::Read(response);
+      if (!json) {
+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      const base::DictionaryValue* dict;
+      if (!json->GetAsDictionary(&dict)) {
+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      // Validate that all the expected fields are present.
+      std::string client_token;
+      std::string host;
+      int port;
+      std::string cert_fingerprint;
+      std::string cert;
+      if (!dict->GetString(kClientTokenKey, &client_token) ||

[Kevin M, 2016/02/12 19:45:26]

!(x && y && z...)?

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+          !dict->GetString(kHostKey, &host) ||
+          !dict->GetInteger(kPortKey, &port) ||
+          !dict->GetString(kCertificateFingerprintKey, &cert_fingerprint) ||
+          !dict->GetString(kCertificateKey, &cert)) {
+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      net::IPAddress ip_address;
+      if (!net::IPAddress::FromIPLiteral(host, &ip_address)) {

[Kevin M, 2016/02/12 19:45:26]

Rebase; this method was just renamed!

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      if (!base::IsValueInRangeForNumericType<uint16_t>(port)) {
+        RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+        return;
+      }
+
+      Assignment assignment;
+      assignment.ip_endpoint = net::IPEndPoint(ip_address, port);
+      assignment.client_token = client_token;
+      assignment.certificate = cert;
+      assignment.certificate_fingerprint = cert_fingerprint;
+
+      RespondWithAssignment(assignment);
+    } break;

[Kevin M, 2016/02/12 19:45:26]

This is a huge switch case. I think it muddles the readability of the switch
block. Please refactor it into a function

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+    case net::HTTP_BAD_REQUEST:
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_REQUEST);
+      break;
+    case net::HTTP_UNAUTHORIZED:
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_EXPIRED_ACCESS_TOKEN);
+      break;
+    case net::HTTP_FORBIDDEN:
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_USER_INVALID);
+      break;
+    case 429:  // Too Many Requests
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_OUT_OF_VMS);
+      break;
+    case net::HTTP_INTERNAL_SERVER_ERROR:
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_SERVER_ERROR);
+      break;
+    default:
+      RespondWithError(ASSIGNMENT_SOURCE_RESULT_BAD_RESPONSE);
+      break;
+  }
+}
+
+void AssignmentSource::RespondWithAssignment(const Assignment& assignment) {

[Kevin M, 2016/02/12 19:45:27]

Only one call site; is it necessary to put this in a function?

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+  DCHECK(callback_.get());
+  callback_->Run(ASSIGNMENT_SOURCE_RESULT_OKAY, assignment);

[Kevin M, 2016/02/12 19:45:26]

ASSIGNMENT_SOURCE_OK for consistency with the spelling of other OKs in the
codebase

[Kevin M, 2016/02/12 19:45:26]

Use base::ResetAndReturn().Run() instead of Run and reset()

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+  callback_.reset();
+}
+
+void AssignmentSource::RespondWithError(AssignmentSourceResult result) {

[Kevin M, 2016/02/12 19:45:26]

I think the logic here is trivial enough to warrant just inlining
ResetAndReturn+Run on the call sites.

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+  DCHECK(result != ASSIGNMENT_SOURCE_RESULT_OKAY);

[Kevin M, 2016/02/12 19:45:26]

DCHECK_NE

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Acknowledged.

+  DCHECK(callback_.get());
+  callback_->Run(result, Assignment());

[Kevin M, 2016/02/12 19:45:26]

Ditto for ResetAndReturn

[David Trainor- moved to gerrit, 2016/02/17 21:09:47]

Done.

+  callback_.reset();
 }
 
 }  // namespace client