Allow authorization with Google application default credentials #213

jeremywadsack · 2016-11-18T01:33:32Z

This adds support for reading the application default
credentials installed on a system by Google. For example, on a
development system where gcloud is configured or on a GCP
instance.

I think this is a better solution to #210.

simon3z · 2016-11-18T12:38:05Z

@moolitayer can you review this? Especially the googleauth dependency? (It may create dependency issues in other proejcts).

jeremywadsack · 2016-11-18T18:40:38Z

Hmm.. the travis failure is because it's running on a system that does not have any default application credentials configured. So I need to figure out how to test around that. I didn't see any stubbing in the existing tests except with webmock. I'll look into how to stub that with minitest's native stubbing.

moolitayer · 2016-11-21T06:54:13Z

lib/kubeclient/common.rb

@@ -70,6 +72,9 @@ def initialize_client(
      elsif auth_options[:bearer_token_file]
        validate_bearer_token_file
        bearer_token(File.read(@auth_options[:bearer_token_file]))
+      elsif auth_options[:default_credentials]


Please initialize this variable (line 23) with nil

You will also need to add it to the input validation in line 442

moolitayer · 2016-11-21T06:58:19Z

README.md

+
+```ruby
+auth_options = {
+  default_credentials: true


I would name this use_default since its boolean. It seems more consistent with the other auth keys that all contain string values. Also maybe use_default_gce or use_gce is better since it's currently only gce?
Naming is hard 🐏

Naming is hard. And often having more people looking at it helps. :)

How about use_default_gcp because this applies to GKE as well.

moolitayer · 2016-11-21T07:01:36Z

lib/kubeclient/common.rb

+
+    def default_credentials_token
+      scopes =  ['https://www.googleapis.com/auth/cloud-platform']
+      authorization = Google::Auth.get_application_default(scopes)


Google::Auth.get_application_default(scopes).apply({}).access_token

#access_token is a method on the object returned by #get_application_default, not on the #apply method result (it's not chainable). I could use #tap if you don't want the interim local variable but I tend to find that harder to read.

moolitayer · 2016-11-21T07:04:53Z

test/test_kubeclient.rb

+      .to_return(body: open_test_file('pod_list.json'),
+                 status: 200)
+
+    client = Kubeclient::Client.new 'http://localhost:8080/api/',


Parenthesis around parameter list please

moolitayer · 2016-11-21T07:05:08Z

test/test_kubeclient.rb

+                               default_credentials: true
+                             }
+    end
+    assert_equal expected_msg, exception.message


Parenthesis around parameter list please

moolitayer · 2016-11-21T07:05:47Z

test/test_kubeclient.rb

+      Kubeclient::Client.new 'http://localhost:8080',
+                             auth_options: {
+                               default_credentials: true
+                             }


Parenthesis around parameter list please

moolitayer · 2016-11-21T07:17:14Z

I think for consistency we would need to catch Signet::AuthorizationError and throw a KubeException.

This seems like a good direction. Thanks for the patch @jeremywadsack 👏

moolitayer · 2016-11-21T07:17:33Z

@moolitayer can you review this? Especially the googleauth dependency? (It may create dependency issues in other proejcts).

@simon3z about the dependency honestly I hope it won't break anything like the dependency problems we have been having lately. Maybe if we encounter problems in the future we can define such dependencies as optional to mitigate the problem.

jeremywadsack · 2016-11-21T20:05:27Z

@moolitayer I made most of the changes you requested (except can't inline Google::Auth.get_application_default(scopes).apply({}).access_token).

Also:

Fixed the tests to build on a systems without default credentials (like Travis).
Updated use of parentheses for all calls in the test file for consistency. Note that there are other files that are not using that (including the README).
Updated the two tests for auth exceptions that were not testing the JSON message parsing so that they would (because I extracted that parsing code).

I also thought about making the dependency optional, especially for those not on Google platforms. Let me know if you want to to take a stab at that.

I can squash and rebase if you want once you are done reviewing.

moolitayer · 2016-11-22T09:01:33Z

test/test_kubeclient.rb

+    auth = Minitest::Mock.new
+    auth.expect(:apply, nil, [{}])
+    auth.expect(:access_token, 'valid_token')
+    Google::Auth.stub(:get_application_default, auth) do


moolitayer · 2016-11-22T09:03:12Z

Updated use of parentheses for all calls in the test file for consistency. Note that there are other files that are not using that (including the README).

That's good but it makes this change harder to review. If you will submit that in a separate pr we can get it merged fast. You can wait for that or you can use the old style here for consistency.
(I usually don't look at all the file when reviewing so sorry for not noticing that, but if your pr matches the style of the file that is actually preferable)

I also thought about making the dependency optional, especially for those not on Google platforms. Let me know if you want to to take a stab at that.

Cool, don't invest time in that just yet. Go ahead and squash your changes

moolitayer · 2016-11-22T09:03:33Z

test/test_kubeclient.rb

@@ -389,16 +400,18 @@ def test_api_bearer_token_success
  def test_api_bearer_token_failure
    error_message = '"/api/v1" is forbidden because ' \
                    'system:anonymous cannot list on pods in'
-    response = OpenStruct.new(code: 401, message: error_message)
+    response = RestClientResponseStub.new('', 403)


Can you explain the change in expectation?
Also is there a functional need in using a class extending string instead of OpenStruct.?

The previous code was raising a KubeException in the request, which meant that #handle_exception was just passing through the exception. So all the expectations in this test were just testing the OpenStruct, not testing the code within #handle_exception that retains the status code and response body.

Instead of raising a KubeException this raises the RestClient::Exception which is what #handle_exception is rescuing and parsing. The response is required by RestClient::Exception. The stub extends String because the RestClient::Response object extends String and is expected to behave the same way (that is, the response is the body).

jeremywadsack · 2016-12-02T19:11:00Z

@moolitayer I was on holiday for a week and just now catching up.

I'll try to push the style changes as a separate PR today. Rolling back is harder. Once that's merged I can squash and rebase this on top of that.

moolitayer · 2016-12-04T12:22:16Z

LGTM +1 once squashed

@simon3z please review. Also please comment if you would like us to explore the path of optional dependencies. I didn't find any built in solution in gemspec but ad hock ones. I saw Keenan comment on those issues so he might be able to help us down that path.

Some stats: without googleauth kubeclinet depends on 29 gems. Adding it takes us up to 39. Those dependencies are (I'm using 2.3):

faraday-0.10.0
googleauth-0.5.1
jwt-1.5.6
little-plugger-1.1.4
logging-2.1.0
memoist-0.15.0
multi_json-1.12.1
multipart-post-2.0.0
os-0.9.6
signet-0.7.3

jeremywadsack · 2016-12-04T20:17:51Z

One solution for optional dependency is to add a note in the Readme that says, "to use the default Google account with GCP include googleauth gem in your Gemfile as well." And then we just require with a rescue.

On Sun, Dec 4, 2016 at 4:22 AM Mooli Tayer ***@***.***> wrote: LGTM +1 once squashed @simon3z <https://github.com/simon3z> please review. Also please comment if you would like us to explore the path of optional dependencies. I didn't find any built in solution in gemspec but ad hock ones. I saw Keenan comment on those issues so he might be able to help us down that path. Some stats: without googleauth kubeclinet depends on 29 gems. Adding it takes us up to 39. Those dependencies are (I'm using 2.3): faraday-0.10.0 googleauth-0.5.1 jwt-1.5.6 little-plugger-1.1.4 logging-2.1.0 memoist-0.15.0 multi_json-1.12.1 multipart-post-2.0.0 os-0.9.6 signet-0.7.3 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#213 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOurIq-s1y_xr6jjbUWCl4sAKwO_PNiks5rErB5gaJpZM4K2BcX> .

-- Jeremy Wadsack

simon3z · 2016-12-05T14:34:30Z

lib/kubeclient/common.rb

-        {}
-      end
-      err_message = json_error_msg['message'] || e.message
+      err_message = json_error_message(e.response) || e.message


@jeremywadsack is this change related? Why is it needed?

@simon3z Because I used the same code for parsing the error message from the body of a Signet::AuthorizationError, so I encapsulated the code in a method.

simon3z · 2016-12-05T14:34:53Z

@jeremywadsack I am wondering... isn't this something that could wrap kubeclient instead of being part of it?

jeremywadsack · 2016-12-05T17:34:32Z

@simon3z I could certainly create a gem that adds this. Let me know which way you prefer to go.

jeremywadsack · 2016-12-05T17:40:13Z

@simon3z My goal here was to resolve the fact that kubeclient doesn't work on systems authorized for Google Cloud (#210).

jeremywadsack · 2016-12-06T19:02:32Z

@simon3z @moolitayer Rebased and squashed.

xldenis · 2017-03-14T20:12:54Z

What's the status of these changes? Is there a chance they will get merged in the near future?

xldenis · 2017-04-28T16:52:27Z

@jeremywadsack Do you have time to work on this? Or should someone else take over?

jeremywadsack · 2017-04-28T17:13:54Z

@xldenis: From @simon3z's last comment I didn't think this was something that they wanted in this gem. I don't have time at the moment to look into building a wrapper gem and injecting all the functionality.

xldenis · 2017-04-28T17:29:41Z

That's very disappointing, it would be useful to support GCE/GKE. Thanks for the update though.

xldenis · 2017-05-11T19:53:16Z

That's totally possible. If you'd like I'll whip up a patch merging my config wrapper with the main one and conditionally guarding the behaviour around a gem presence check.

jeremywadsack · 2017-05-11T22:12:21Z

@xldenis Thank you for picking this up. I would also love to have this supported natively in the gem for the same reason: GKE is a major platform for Kubernetes.

As I posted before we can just add a README entry that says that add the googleauth gem for GCE/GKE support. @bglusman's suggestion of checking for defined? constant is smart.

simon3z · 2017-05-12T08:20:36Z

That's totally possible. If you'd like I'll whip up a patch merging my config wrapper with the main one and conditionally guarding the behaviour around a gem presence check.

@xldenis sounds good to me. And I'm OK with the dependency in dev.

cben

Hi, I'd like to somehow move this along.
I'm again questioning whether obtaining the token belongs in kubeclient :-)
Please don't read this as objection — I see there was consensus on "optional dependency" approach, and I'm good with that!
I'm just trying to understand whether it's the most convenient thing for GCP users, or maybe obtaining the token yourself and passing it to Client gives much more flexibility... (I don't know anything about GCP)

P.S. thanks everyone for #210, #211 and Shopify/krane#88, the multiple implementations are very helpful for understand the problem space 🙇‍♂️

cben · 2018-02-01T15:07:21Z

test/test_kubeclient.rb

@@ -509,22 +524,22 @@ def test_init_username_and_bearer_token
    assert_equal(expected_msg, exception.message)
  end

-  def test_init_user_and_bearer_token
+  def test_init_username_and_bearer_token_file


At first I wanted to ask why we need both bearer_token and bearer_token_file but then noticed it's already supported and documented, thanks for adding tests! 👏

cben · 2018-02-01T15:17:32Z

lib/kubeclient/common.rb

@@ -70,6 +73,9 @@ def initialize_client(
      elsif auth_options[:bearer_token_file]
        validate_bearer_token_file
        bearer_token(File.read(@auth_options[:bearer_token_file]))
+      elsif auth_options[:use_default_gcp]


I wonder if this should be an option for the Client.
It's resolved to a token on initialization, and after that the Client doesn't care.
Could it be responsibility of Kubeclient::Config instead? (like in Shopify/krane#88)
Well, it's also useful for people who don't use kubeconfig.
But maybe expose a class method gcp_auth_options or something like that, also used by Kubeclient::Config?

README recommends calling config.context several times in Client instantiation, and it's silly if that will result in obtaining many tokens :-(

Seems solvable by moving it from fetch_user_auth_options now called during config.context to only the call context.auth_options.

It should be possible to use a single Config instance for many Client instantiations, obtaining a new token for each one.

Conversely it should be possible to reuse one token for many Client instances (e.g. it's currently common to create several instances for each API group/version you use).

cben · 2018-02-01T16:06:56Z

lib/kubeclient/common.rb

+
+      authorization.apply({})
+
+      authorization.access_token


This is not a lot of code, so I'm wondering if kubeclient needs to include it at all.

Does this cover most uses with GCP, or will people need other variations?

Add google cloud configuration object for kubeclient Shopify/krane#88 says it's valid for 1 hour. Is that tunable? Should a long-lived Client object be able to renew it?

The alternative I'm thinking about is:

Document example code in README.

Those directly passing auth_options: just write this or similar code and pass the token.

For those reading a config file, Kubeclient::Config could have a hook to plug your own handling of 'auth-provider'. E.g. auth-provider: gcp would call fetch_auth_provider_options_gcp method one could override in a subclass.

See also my other comment about one Config / many context calls / many Client instances / one or many tokens. Perhaps for gcp you better take matters into your own hands and not call config.context.auth_options at all? (for that, Config should let user access the raw auth-provider portion of config)

I'm sure there are other cases with GCP but I can think of the two:

User has authorized gcloud for their GCP project (and by extension kubectl). In that case, the token is only good for an hour as I pointed out in Authentication from "gcp" provider #210. I tried to address this in Enable "gcp" authentication structure for bearer token in .kube/config #211 but I couldn't get it to work for more than an hour and I'm not sure if we can.

This is on a GCP server (GKE, GCE, App Engine, Cloud Functions), where Google has installed "default credentials". That's what this PR addresses. (You can always install default credentials on your local dev machine as "long-running" credentials.)

I see your point that this could just be documented. It would be really nice if kubeclient just worked out-of-the-box for gcloud-configured machines (e.g. development machine) and GCE/GKE machines. I can see that sometimes users are going to want to modify the scope and may eventually want to use specific keys for role-based-access (rather than authorizing the default credentials on a machine for this permission). So maybe there's a place for basic "out-of-the-box" support and anything beyond that is documented.

It does make more sense to use Config than Client. However, the shopify solution depends on the presence of auth-provider.name in the configuration file but there is no such file in a GCP server. So it wouldn't solve for that case.

cben · 2018-02-01T16:09:17Z

lib/kubeclient/common.rb

+      authorization.apply({})
+
+      authorization.access_token
+    rescue Signet::AuthorizationError => e


Is this wrapping helping? It was suggested "for consistency" but perhaps it's nicer to be able to rescue original Signet exceptions rather than a kubeclient exception?
Plus, we rebranded our main exception as Kubeclient::HttpError this makes less sense.

cben · 2018-02-01T16:53:48Z

A lot of my questions above are around Config vs tokens vs Client lifespans, which are not one-size-fits-all because tokens expire.
@garethr mentions on #210 (comment) that gcloud can produce a fixed non-expiring client cert. Can googleauth gem do that?

cben · 2018-02-04T09:24:33Z

However, the shopify solution depends on the presence of auth-provider.name in the configuration file but there is no such file in a GCP server. So it wouldn't solve for that case.

OK, so how do you detect/activate GCP support for use case 2? Or do you mean you manually specify `use_default_gcp: true`?

jeremywadsack · 2018-02-05T19:20:54Z

so how do you detect/activate GCP support for use case 2?

@cben I think either have a configuration option that tells the config/client to look for the default Google authorization (and require googleauth) or have kubeclient detect if googleauth is installed and attempt to authenticate via default credentials if no other authorization option is specified.

From the comments here, my guess is this team is partial to an explicit selection.

fw42 · 2018-04-12T10:46:25Z

Any idea if this can be merged soon? Anyone still actively working on it? Anyone need help with anything?

cben · 2018-04-12T14:08:25Z

@moolitayer what do you think? You've LGTM'd this in the past.
I've asked lots of questions, got a good answer and I'm ok with this.
(I mildly prefer Signet::AuthorizationError to just raise unwrapped but won't insist.)
Are we good if this gets rebased?

jeremywadsack · 2018-04-12T19:06:48Z

@cben It's been so long since I looked at this that it's hard to recall where my head was at the time. As it happens, I'm just looking into this again (or into the code that we built that depends on this) and so it's a good time to rethink.

In production in our GKE cluster, we're using this:

          Kubeclient::Config::Context.new(
              "https://kubernetes",
              "v1",
              {ca_file: "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"},
              bearer_token_file: "/var/run/secrets/kubernetes.io/serviceaccount/token"
          )

That's basically what's described in the README for "using kubeclient inside a Kubernetes cluster" (with the addition of a well-known location for the API host and CA file). Given that it's documented, I don't know that we need to do anything to "make this work" in GKE. Although adding additional details for GKE might be helpful. (Incidentally, it's documented in Kubernetes as the recommended way to access the API server.)

I have no experience running kubeclient from GCE, App Engine, or Cloud Functions, but I believe that those are all enabled with Google's "Application Default Credentials". A gcloud-configured kuebclient machine may have Default Application Credentials.*

Here's a proposal, let me know what you think:

1. I'll open a new PR with documentation details about how to configure the client for a GKE system.
2. I'll rebase this, move the code into Kubeclient::Config as suggested, and make the dependecy optional.
3. I'll consider a PR separately, after Application Default Credentials support is in, to adapt the #read('.kube/config') method to work on gcloud-configured machines (essentially the Add google cloud configuration object for kubeclient Shopify/krane#88 solution).*

Does that sound good?

*Using gcloud to authorize kubectl for a GKE cluster does not generate Application Default Credentials. It used to but now it says this:

WARNING: `gcloud auth login` no longer writes application default credentials.
If you need to use ADC, see:
   gcloud auth application-default --help

cben · 2018-04-20T08:13:24Z

@jeremywadsack Sorry for delay, I was hoping @moolitayer would respond too (nag nag 😉)...
Your proposal sounds great.
Honestly, you have much better picture of the Google stuff than we do, I trust whatever you decide is right for kubeclient.

jeremywadsack · 2018-04-21T20:51:52Z

@cben @moolitayer: I've rewritten this and I think it should make more sense and be simpler.

First, note that the "application default credentials" aren't related to having kubectl installed. So I didn't think the Kubeclient::Config was the place to put the code. As you had wanted this to not pollute the Kubeclient itself, I opted for a new "token provider" that will generate the token and added documentation on how to use that.

I think this is a much simpler and more useful approach.

I removed the dependency on googleauth from the gem file, but retained if for testing. Because the code that calls Google::Auth isn't part of Kubeclient it won't raise any errors without googleauth installed unless you explicitly call Kubeclient::ApplicationDefaultCredentials.token. My README notes say that you need to have googleauth in your bundle and show that you need to require 'googleauth' as well to use the token provider.

Let me know if your thoughts and any changes you'd like. If this looks good, I can squash commits.

cben

nice! few last minor questions.

cben · 2018-04-23T10:36:07Z

lib/kubeclient/application_default_credentials.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+require 'googleauth'


With this file required in kubeclient.rb, I think this means you can't require kubeclient without having googleauth gem. Can you move this into token method so it's only done if you use it?

Good catch. Fixed in 774177b.

cben · 2018-04-23T10:37:20Z

lib/kubeclient/application_default_credentials.rb

+
+module Kubeclient
+  # Get a bearer token from the Google's application default credentials.
+  class ApplicationDefaultCredentials


This name sounds pretty generic, perhaps should somehow include "Google".

Done 24145e9. I thought that the name was already pretty long... ;) But I agree.

cben · 2018-04-23T10:43:29Z

README.md

+`kubeclient` dependencies so you should add it to your bundle.
+
+```ruby
+require 'googleauth'


this is not strictly neccessary here, just to emphasize the dependency on the gem, right?

Originally I think I was thinking of not having a require in the library code at all, in which case this would be necessary. As it is now it's not necessary, but it follows the example below this for Celluloid::IO.

However, when the googleauth gem is not installed, and you try to use get a token, it raises a load error:

2.3.1 :002 > Kubeclient::ApplicationDefaultCredentials.token LoadError: cannot load such file -- googleauth

I think this is obscure (even though it's documented in the README). What do you think about adding a message here that tells what to do. One of these options:

Do not include the require (because presumably it's already been done by bundler) and rescue NameError around Google::Auth.get_application_default and provide a message.

Retain the require (because it is a dependency) and rescue the LoadError around the require and provide a message.

Message would be something like this: "GoogleApplicationDefaultCredentials requires the googleauth gem. Please make sure this is included in your bundle."

I'd rather not assume bundler does require. Some people practice require: false in Gemfile. And I suppose some people [citation needed ;-] use gem install outside bundler.
=> if we don't require in the code, keep this require in README.

Personally I'd do require on demand inside the token method. Because it is a run time dependency of this code, only not a bundler dependency. So user is responsible to install it, but if installed we can use it.
But I don't care much, your call.

IMHO LoadError is clear & actionable enough, tells you what gem you need.
(But an uncatched NameError would be too obscure to my taste.)
If you want a rescue for a friendlier message, that's OK too...

Sounds good. I'll leave it as is. It accomplishes the job and I don't want to add more code.

cben

LGTM 👍

Just to be sure, I confirmed manually that kubeclient works with this PR without googleauth in the bundle. (Not checked by Travis, due to googleauth dev dependency. I think that's OK.)

@moolitayer can we merge?

moolitayer · 2018-04-25T06:35:38Z

lib/kubeclient.rb

@@ -1,6 +1,7 @@
 require 'json'
 require 'rest-client'

+require 'kubeclient/google_application_default_credentials'


nit: please keep this list sorted

I meant to do that. Then I renamed the file. I've fixed that and will squash and rebase (to address merge conflicts in README).

moolitayer · 2018-04-25T06:39:21Z

Nice and clean, thanks!

This adds support for reading the application default credentials installed on a system by Google. For example, on a development system where gcloud is configured or on a GCP instance.

cben

rebase looks good, merging.

Thanks for the perseverance! 🍰

jeremywadsack mentioned this pull request Nov 18, 2016

Enable "gcp" authentication structure for bearer token in .kube/config #211

Closed

jeremywadsack mentioned this pull request Nov 20, 2016

Authentication from "gcp" provider #210

Closed

moolitayer reviewed Nov 21, 2016

View reviewed changes

moolitayer reviewed Nov 22, 2016

View reviewed changes

jeremywadsack mentioned this pull request Dec 2, 2016

Code style: add parentheses around methods for test file #215

Merged

simon3z reviewed Dec 5, 2016

View reviewed changes

jeremywadsack force-pushed the auth_default_account branch from 1a840ef to 4f5f5ed Compare December 6, 2016 18:58

moolitayer mentioned this pull request Sep 7, 2017

Process JSON Body of ListObjects using streaming json parser #254

Closed

cben reviewed Feb 1, 2018

View reviewed changes

jeremywadsack mentioned this pull request Apr 12, 2018

Use Google Default Application Credentials when authorization fails keylimetoolbox/resque-kubernetes#9

Merged

jeremywadsack mentioned this pull request Apr 21, 2018

Update README directions for authenticating within a cluster #316

Merged

jeremywadsack force-pushed the auth_default_account branch from 4f5f5ed to 4c672a4 Compare April 21, 2018 20:36

cben reviewed Apr 23, 2018

View reviewed changes

cben approved these changes Apr 24, 2018

View reviewed changes

moolitayer reviewed Apr 25, 2018

View reviewed changes

cben assigned moolitayer Apr 25, 2018

cben added the enhancement label Apr 25, 2018

Allow authorization with Google application default credentials.

e6e10d0

This adds support for reading the application default credentials installed on a system by Google. For example, on a development system where gcloud is configured or on a GCP instance.

jeremywadsack force-pushed the auth_default_account branch from 4737898 to e6e10d0 Compare April 25, 2018 16:59

cben approved these changes Apr 27, 2018

View reviewed changes

cben merged commit 09e72a6 into ManageIQ:master Apr 27, 2018

jeremywadsack deleted the auth_default_account branch April 27, 2018 15:44

cben mentioned this pull request May 31, 2018

GoogleFriendlyConfig.read: Use YAML.safe_load to avoid arbitrary Ruby class construction Shopify/krane#295

Merged

cben mentioned this pull request Nov 20, 2018

Adds support for exec credential plugin #363

Merged

timothysmith0609 mentioned this pull request Feb 15, 2019

Automatically support GCP ApplicationDefaultCredentials #392

Closed

		@@ -0,0 +1,17 @@
		# frozen_string_literal: true

		require 'googleauth'

Allow authorization with Google application default credentials #213

Allow authorization with Google application default credentials #213

Conversation

jeremywadsack commented Nov 18, 2016

simon3z commented Nov 18, 2016

jeremywadsack commented Nov 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Nov 21, 2016

moolitayer commented Nov 21, 2016

jeremywadsack commented Nov 21, 2016

Choose a reason for hiding this comment

moolitayer commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremywadsack commented Dec 2, 2016

moolitayer commented Dec 4, 2016

jeremywadsack commented Dec 4, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Dec 5, 2016

jeremywadsack commented Dec 5, 2016

jeremywadsack commented Dec 5, 2016

jeremywadsack commented Dec 6, 2016

xldenis commented Mar 14, 2017

xldenis commented Apr 28, 2017

jeremywadsack commented Apr 28, 2017

xldenis commented Apr 28, 2017

xldenis commented May 11, 2017

jeremywadsack commented May 11, 2017

simon3z commented May 12, 2017

cben left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben commented Feb 1, 2018 • edited Loading

cben commented Feb 4, 2018 via email • edited Loading

jeremywadsack commented Feb 5, 2018

fw42 commented Apr 12, 2018

cben commented Apr 12, 2018

jeremywadsack commented Apr 12, 2018 • edited Loading

cben commented Apr 20, 2018

jeremywadsack commented Apr 21, 2018

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cben left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moolitayer commented Apr 25, 2018

cben left a comment

Choose a reason for hiding this comment

cben left a comment •

edited

Loading

cben commented Feb 1, 2018 •

edited

Loading

cben commented Feb 4, 2018 via email •

edited

Loading

jeremywadsack commented Apr 12, 2018 •

edited

Loading