S3 repository: Add named configurations #22762

rjernst · 2017-01-24T07:53:17Z

This change implements named configurations for s3 repository as
proposed in #22520. The access/secret key secure settings which were
added in #22479 are reverted, and the only secure settings are those
with the new named configs. All other previously used settings for the
connection are deprecated.

closes #22520

This change implements named configurations for s3 repository as proposed in elastic#22520. The access/secret key secure settings which were added in elastic#22479 are reverted, and the only secure settings are those with the new named configs. All other previously used settings for the connection are deprecated. closes elastic#22520

dadoonet

I gave a first quick look and before do a real review I'd like to discuss directly with you some of the points I raised. Let's sync later online.

dadoonet · 2017-01-24T07:57:19Z

core/src/main/java/org/elasticsearch/common/settings/SecureString.java

+     * NOTE: This is not actually secure, since the provided String cannot be deallocated, but
+     * this constructor allows for easy compatibility between new and old apis.
+     */
+    public SecureString(String s) {


Is this something we should mark as Deprecated so we know that this will go away and that we need to remove old APIs which are using that?

I added this in f711224.

dadoonet · 2017-01-24T07:58:31Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java

     */
-    SecureSetting<SecureString> KEY_SETTING = SecureSetting.secureString("cloud.aws.access_key", null, true, Property.Shared);
-
+    Setting<SecureString> LEGACY_KEY_SETTING = new Setting<>("cloud.aws.access_key", "", SecureString::new,


Can we mark it as Deprecated in Java as well so we need we have to remove it at some point?
Just writing it for this setting but this also applies to other settings.

I don't think we have use java @deprecated for Settings anywhere else, and I'm not sure what advantage it would bring. It's completely internal, and the deprecated property signals to the user (through automatic deprecation logging on use) that the setting is going away.

I did at some point but may be no one uses that anymore. I'm fine not adding Deprecated.

dadoonet · 2017-01-24T08:03:28Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java

         */
-        SecureSetting<SecureString> KEY_SETTING = SecureSetting.secureString("cloud.aws.s3.access_key", AwsS3Service.KEY_SETTING, true);
+        Setting<SecureString> KEY_SETTING =
+            new Setting<>("cloud.aws.s3.access_key", AwsS3Service.LEGACY_KEY_SETTING, SecureString::new,


While we are at it, I'd really like to move our repository-s3 settings under a unique namespace repositories.s3 instead.

cloud.aws prefix was nice when we were sharing same credentials and when we are using cloud.enabled: true but I'm sure we don't need that namespace anymore.

Can you do that in the same PR or open a new issue so we track it?

Same for package names, we should move all what we can under a unique package name. So debugging will be much easier (I mean setting log levels on a unique package instead of many).

I created #22773.

dadoonet · 2017-01-24T08:12:16Z

plugins/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Repository.java

    public static final String TYPE = "s3";

+    // prefix for aws client settings
+    private static final String PREFIX = "aws.config.";


I think we should change this to repositories.s3.config. or something like this.
Unless you want to share aws.config between ec2 and s3?

I was assuming we still wanted to share. If that is not the case, I am happy to use a repository-s3 specific prefix.

What about something like s3.client.? I think anything regarding the client configuration should stay under this named config, and anything else should be repository specific (eg the bucket). I actually don't think we need to have fallback settings like the other bucket or server_side_encryption settings. The client settings (ie the ones I have moved in this PR) should be under named configs, and bucket, etc should be in the repository settings. Users just aren't setting these up that often, I don't see a real need for multiple levels of configuration (and the complication in code and testing that comes with it).

So if I understand what you are proposing here we would just support repository settings per repository (like bucket) but no more with global settings (like repositories.s3.bucket). Is that what you meant?

I looked at S3 repository specific settings we have today and I think it makes sense to have that in a single place, per repository.

If so, it would mean that we don't use anymore any repositories.s3.* settings. In that case, I'd suggest using repositories.s3.client to define the client settings for the repository-s3 plugin and move all our classes under org.elasticsearch.repositories.s3 package name.

S3 Client would go in org.elasticsearch.repositories.s3.client package.

WDYT?

So if I understand what you are proposing here we would just support repository settings per repository (like bucket) but no more with global settings (like repositories.s3.bucket). Is that what you meant?

Yes that is what I mean.

I'd suggest using repositories.s3.client

I was trying to have something not as verbose. The length of our setting names is quite annoying...

I'd suggest using repositories.s3.client to define the client settings for the repository-s3 plugin and move all our classes under org.elasticsearch.repositories.s3 package name

I don't think the package names need to correspond in any way with the settings, and I don't think it is necessary to have more than a single java package for a plugin.

I don't think the package names need to correspond in any way with the settings

Well. That has been our policy since the beginning. We even advertised that in our trainings IIRC. But I know it makes less sense now that logging is using the full package name. Still I always found super nice to know that if I want to debug a component which defines x.y.z setting, I can easily add DEBUG level to package named x.y before 5.0 and from 5.0 package org.elasticsearch.x.y.

@clintongormley Do you have any opinion on that? Consistent settings vs short setting names?

I don't think it is necessary to have more than a single java package for a plugin.

I agree.

That has been our policy since the beginning

I remember that long ago, prior to 2.0. There used to be crazy support for this in setting names, even reflecting on the package to come up with setting names IIRC. But that was all removed a long time ago.

The link between the settings names and the logging class was never much use to me - I'd prefer meaningful, understandable names

Thanks. So s3.client looks the best name here.

dadoonet

I just reviewed your changes and it's really great.
Can you just update the final settings name?

When everything is ready I can test for real your PR if you wish.

dadoonet · 2017-01-25T10:49:31Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java

+        TimeValue.timeValueMillis(ClientConfiguration.DEFAULT_SOCKET_TIMEOUT), Property.NodeScope, Property.Deprecated, Property.Shared);

    /**
     * Defines specific s3 settings starting with cloud.aws.s3.


Can we also add

// Legacy AWS S3 settings

dadoonet · 2017-01-25T10:51:19Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/AwsS3Service.java

+         * @see AwsS3Service#LEGACY_KEY_SETTING
         */
-        SecureSetting<SecureString> KEY_SETTING = SecureSetting.secureString("cloud.aws.s3.access_key", AwsS3Service.KEY_SETTING, true);
+        Setting<SecureString> KEY_SETTING =


And also rename them as you did for the cloud.aws global settings?

So here s/KEY_SETTING/LEGACY_KEY_SETTING

And same applies for the other cloud.aws.s3 settings.

I actually reverted the rename to LEGACY, I don't think it helps with much, it is confusing either way. The settings are going away in master in a followup anyways.

dadoonet · 2017-01-25T10:53:53Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

 import java.util.HashMap;
 import java.util.Map;
+import java.util.function.Function;
 import java.util.function.Supplier;


Not needed anymore.

dadoonet · 2017-01-25T10:53:58Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

+import java.util.function.Function;
 import java.util.function.Supplier;

 import static org.elasticsearch.repositories.s3.S3Repository.getValue;


Not needed anymore.

It is used (Function.identity())

I commented on line:

import static org.elasticsearch.repositories.s3.S3Repository.getValue;

dadoonet · 2017-01-25T15:11:16Z

plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java

        MockSecureSettings secureSettings = new MockSecureSettings();
-        secureSettings.setString(AwsS3Service.KEY_SETTING.getKey(), "aws_key");
-        secureSettings.setString(AwsS3Service.SECRET_SETTING.getKey(), "aws_secret");
+        secureSettings.setString("aws.config.default.access_key", "aws_key");


Don't forget to update to s3.client. here. But i'm sure you'll think about it when you will run the tests :)
I don't comment on other lines in this test but same applies.

dadoonet · 2017-01-25T15:13:19Z

plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java

-        launchAWSEndpointTest(generateRepositorySettings("repository_key", "repository_secret", null, "repository.endpoint", null),
+    public void testEndpointSetting() {
+        Settings settings = Settings.builder()
+            .put("aws.config.default.endpoint", "ec2.endpoint")


can it be s3.endpoint? I think I badly named it initially.

dadoonet · 2017-01-25T15:18:59Z

plugins/repository-s3/src/test/java/org/elasticsearch/cloud/aws/AwsS3ServiceImplTests.java

+        String configName = InternalAwsS3Service.CONFIG_NAME.get(repositorySettings);
+        String foundEndpoint = InternalAwsS3Service.findEndpoint(logger, repositorySettings, settings, configName);
        assertThat(foundEndpoint, is(expectedEndpoint));
    }


Adding a comment unrelated to your change but generateRepositorySettings() method always gets null for the maxRetries parameter. May be we can remove it? Or really test it?

I added tests.

rjernst · 2017-01-25T23:52:09Z

@dadoonet I pushed 28c1e5e to address all your feedback. If you want to manually test with a real s3 account, that would be great.

rjernst · 2017-01-26T16:48:21Z

retest this please

dadoonet · 2017-01-26T18:01:56Z

@rjernst I tried to do a first test today.

I built your branch, unzip elasticsearch-6.0.0-alpha1-SNAPSHOT, installed repository-s3 plugin:

bin/elasticsearch-plugin install file:///path/to/repository-s3-6.0.0-alpha1-SNAPSHOT.zip

Then I edited my elasticsearch.yml config file as is:

s3.client.default.access_key: "KEY_HERE"
s3.client.default.secret_key: "SECRET_HERE"

Then I started bin/elasticsearch and I'm getting:

[2017-01-26T18:56:34,106][WARN ][o.e.b.ElasticsearchUncaughtExceptionHandler] [] uncaught exception in thread [main]
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: unknown setting [s3.client.default.access_key] please check that any required plugins are installed, or check the breaking changes documentation for removed settings
	at org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:127) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.bootstrap.Elasticsearch.execute(Elasticsearch.java:114) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:58) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:122) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.cli.Command.main(Command.java:88) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:91) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]
	at org.elasticsearch.bootstrap.Elasticsearch.main(Elasticsearch.java:84) ~[elasticsearch-6.0.0-alpha1-SNAPSHOT.jar:6.0.0-alpha1-SNAPSHOT]

Indeed, we don't seem to register in the plugin class those new settings. Or I'm missing something.

BTW do you intend to update the documentation with this PR?

dadoonet · 2017-01-26T18:05:04Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

 public class InternalAwsS3Service extends AbstractLifecycleComponent implements AwsS3Service {

+    // pkg private for tests
+    static final Setting<String> CLIENT_NAME = new Setting<>("client", "default", Function.identity());


Should we create here a public static String for "default" like:

public static final String CLIENT_DEFAULT_NAME = "default";

Then reuse that here and in our tests?

@rjernst Ping on that cosmetic thing. Not sure it is worth changing though. Up to you :)

I don't think it is worthwhile. It will make the tests hard to read (with lots of constants, instead of being able to simply see what setting is there).

rjernst · 2017-01-26T18:19:49Z

You can't put access key and secret key in yml. They need to be in the keystore.

dadoonet · 2017-01-26T18:51:23Z

Thanks. I was unsure it was merged or not.

So here is what I did:

bin/elasticsearch-keystore create
bin/elasticsearch-keystore add s3.client.default.access_key
bin/elasticsearch-keystore add s3.client.default.secret_key

In config/elasticsearch.yml, I have:

repositories.s3.bucket: "test.eu-west-1.elasticsearch.org"
repositories.s3.region: "eu-west-1"

Then I create a repo like this:

curl -XPOST 'http://localhost:9200/_snapshot/backups?pretty=true' -d '{ "type":"s3" }'

It fails with:

{
  "error" : {
    "root_cause" : [
      {
        "type" : "repository_verification_exception",
        "reason" : "[backups] path  is not accessible on master node"
      }
    ],
    "type" : "repository_verification_exception",
    "reason" : "[backups] path  is not accessible on master node",
    "caused_by" : {
      "type" : "i_o_exception",
      "reason" : "Unable to upload object tests-IjZGnVKCR5C3d0j--wHNOQ/master.dat-temp",
      "caused_by" : {
        "type" : "amazon_s3_exception",
        "reason" : "The AWS Access Key Id you provided does not exist in our records. (Service: Amazon S3; Status Code: 403; Error Code: InvalidAccessKeyId; Request ID: 7FF16CD338F67C02)"
      }
    }
  },
  "status" : 500
}

I'm 99% sure I tested recently the same credentials when I tested your PR about using a keystore for s3.
As it remains 1%, I'll do more test tomorrow morning. :)

rjernst · 2017-01-26T23:08:32Z

@dadoonet I found a bug in the keystore infra (the secure settings were not passed along correctly). However, there is a larger issue I also found, in that the entire reason I needed named configs was so that we can eagerly read these secure settings during node construction (ie ctor of InternalAwsS3Service), since we close the SecureSettings after the node is ready. I will need a bit of time to rework this.

rjernst · 2017-01-26T23:16:37Z

Actually, this PR is already too large, I would like to get it in and do the eager client config parsing as a followup. I've pushed a fix for the issues I mentioned (with the latter being not closing the secure settings when the node is constructed, for now, with a TODO I will remove in the followup). With this code I was able to create an s3 repository.

Also, don't close secure settings (for now).

dadoonet · 2017-01-27T06:03:10Z

Works for me. I'd just suggest marking the follow up issue as a blocker.

dadoonet · 2017-01-27T08:57:00Z

So I see what is happening. I just tried with your latest efef46b.

I do have a system var AWS_SECRET_ACCESS_KEY and AWS_ACCESS_KEY_ID which are used here because InternalAwsS3Service#getConfigValue is called with an empty list of secured settings. I can see that in debug mode.

    private static <T> T getConfigValue(Settings repositorySettings, Settings globalSettings, String configName,
                                        Setting.AffixSetting<T> configSetting, Setting<T> repositorySetting, Setting<T> globalSetting) {
        Setting<T> concreteSetting = configSetting.getConcreteSettingForNamespace(configName);
        if (concreteSetting.exists(globalSettings)) {
            return concreteSetting.get(globalSettings);
        } else if (repositorySetting == null) {
            // no repository setting, just use global setting
            return globalSetting.get(globalSettings);
        } else {
            return S3Repository.getValue(repositorySettings, globalSettings, repositorySetting, globalSetting);
        }
    }

So this returns an empty String for the key and the secret, so buildCredentials is falling back to sysprops credentials and such.

I wonder why Secured settings is coming back empty. May I missed something?

rjernst · 2017-01-27T15:21:25Z

@dadoonet My apologies. This is the exact issue that I had fixed. However, I had tried cleaning up the commit (removing stuff unrelated while starting to try and work the ctor for S3Repostiory), and had inadvertently removed the most important bugfix (6cbe091).

dadoonet · 2017-01-27T16:57:12Z

Fantastic!

$ curl -XPOST 'http://localhost:9200/_snapshot/backups?pretty=true' -d '{ "type":"s3" }'

{
  "acknowledged" : true
}

I'm doing so more tests and will then approve the PR.

dadoonet

I left minor comments but I think we are all good now.

I did some tests:

using default, using a named client, then removing default and using the named client.

All good.

dadoonet · 2017-01-27T17:07:00Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

+        String foundEndpoint = findEndpoint(logger, repositorySettings, settings, configName);

-        AWSCredentialsProvider credentials = buildCredentials(logger, deprecationLogger, settings, repositorySettings);
+        AWSCredentialsProvider credentials = buildCredentials(logger, deprecationLogger, settings, repositorySettings, configName);


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:07:41Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

+    public synchronized AmazonS3 client(Settings repositorySettings, Integer maxRetries,
+                                              boolean useThrottleRetries, Boolean pathStyleAccess) {
+        String configName = CLIENT_NAME.get(repositorySettings);
+        String foundEndpoint = findEndpoint(logger, repositorySettings, settings, configName);


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:07:50Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

        client = new AmazonS3Client(
            credentials,
-            buildConfiguration(logger, settings, protocol, maxRetries, foundEndpoint, useThrottleRetries));
+            buildConfiguration(logger, repositorySettings, settings, configName, maxRetries, foundEndpoint, useThrottleRetries));


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:07:58Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

-                                                         String endpoint, boolean useThrottleRetries) {
+    // pkg private for tests
+    static ClientConfiguration buildConfiguration(Logger logger, Settings repositorySettings, Settings settings,
+                                                         String configName, Integer maxRetries, String endpoint,


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:08:16Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

-    protected static String findEndpoint(Logger logger, Settings settings, String endpoint, String region) {
+    // pkg private for tests
+    /** Returns the endpoint the client should use, based on the available endpoint settings found. */
+    static String findEndpoint(Logger logger, Settings repositorySettings, Settings settings, String configName) {


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:08:33Z

plugins/repository-s3/src/main/java/org/elasticsearch/cloud/aws/InternalAwsS3Service.java

+     * Find the setting value, trying first with named configs,
+     * then falling back to repository and global repositories settings.
+     */
+    private static <T> T getConfigValue(Settings repositorySettings, Settings globalSettings, String configName,


Can we rename configName to clientName?

dadoonet · 2017-01-27T17:55:54Z

+1 to merge! :D

dadoonet · 2017-01-27T17:58:03Z

In #22762 (comment), I asked:

BTW do you intend to update the documentation with this PR?

If you don't have time to do it now, I can work on it later if you wish.

rjernst · 2017-01-27T18:23:23Z

BTW do you intend to update the documentation with this PR?

Sorry I missed this. I will do it in a followup.

This change implements named configurations for s3 repository as proposed in #22520. The access/secret key secure settings which were added in #22479 are reverted, and the only secure settings are those with the new named configs. All other previously used settings for the connection are deprecated. closes #22520

In elastic#22762, settings preparation during bootstrap was changed slightly to account for SecureSettings, by starting with a fresh settings builder after reading the initial configuration. However, this the defaults from system properties were never re-read. This change fixes that bug (which was never released). closes elastic#22861

In #22762, settings preparation during bootstrap was changed slightly to account for SecureSettings, by starting with a fresh settings builder after reading the initial configuration. However, this the defaults from system properties were never re-read. This change fixes that bug (which was never released). closes #22861

…tory We should have the same behavior for Azure repositories as we have for S3 (see elastic#22762). Instead of: ```yml cloud: azure: storage: my_account1: account: your_azure_storage_account1 key: your_azure_storage_key1 default: true my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Support something like: ``` azure.client: default: account: your_azure_storage_account1 key: your_azure_storage_key1 my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Then instead of: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "account": "my_account2" } } ``` Use: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "config": "my_account2" } } ``` If someone uses: ``` PUT _snapshot/my_backup3 { "type": "azure" } ``` It will use the `default` azure repository settings. And mark as deprecated old settings. Closes elastic#22763.

…tory We should have the same behavior for Azure repositories as we have for S3 (see #22762). Instead of: ```yml cloud: azure: storage: my_account1: account: your_azure_storage_account1 key: your_azure_storage_key1 default: true my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Support something like: ``` azure.client: default: account: your_azure_storage_account1 key: your_azure_storage_key1 my_account2: account: your_azure_storage_account2 key: your_azure_storage_key2 ``` Then instead of: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "account": "my_account2" } } ``` Use: ``` PUT _snapshot/my_backup3 { "type": "azure", "settings": { "config": "my_account2" } } ``` If someone uses: ``` PUT _snapshot/my_backup3 { "type": "azure" } ``` It will use the `default` azure repository settings. And mark as deprecated old settings. Backport of #23405 in 6.x branch

rjernst added :Plugin Repository S3 >enhancement v5.3.0 v6.0.0-alpha1 labels Jan 24, 2017

dadoonet reviewed Jan 24, 2017

View reviewed changes

dadoonet mentioned this pull request Jan 24, 2017

Azure repository: Move to named configurations as we do for S3 repository #22763

Closed

rjernst mentioned this pull request Jan 24, 2017

Move shared settings from aws (ec2 and s3 plugins) to plugin specific settings #22773

Closed

Add deprecation for compatibility SecureString ctor

f711224

dadoonet reviewed Jan 25, 2017

View reviewed changes

PR feedback

28c1e5e

dadoonet mentioned this pull request Jan 26, 2017

Get rid of repositories.impl settings totally #22800

Closed

6 tasks

dadoonet reviewed Jan 26, 2017

View reviewed changes

Fix bug in environment preparation not passing secure settings along.

efef46b

Also, don't close secure settings (for now).

rjernst force-pushed the keystore4 branch from 2231896 to efef46b Compare January 26, 2017 23:21

Pass through secure settings when builder receives settings object.

6cbe091

dadoonet approved these changes Jan 27, 2017

View reviewed changes

Rename configName to clientName

1590fea

rjernst merged commit aad51d4 into elastic:master Jan 27, 2017

rjernst deleted the keystore4 branch January 27, 2017 18:42

rjernst mentioned this pull request Jan 30, 2017

Settings: Fix settings reading to account for defaults #22871

Merged

dadoonet mentioned this pull request Feb 20, 2017

[WIP] Remove repositories.s3 settings and deprecated settings #23276

Closed

dadoonet mentioned this pull request Feb 28, 2017

Azure repository: Move to named configurations as we do for S3 repository and secure settings #23405

Merged

clintongormley added :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs and removed :Plugin Repository S3 labels Feb 14, 2018

S3 repository: Add named configurations #22762

S3 repository: Add named configurations #22762

Uh oh!

Conversation

rjernst commented Jan 24, 2017

Uh oh!

dadoonet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dadoonet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jan 25, 2017

Uh oh!

rjernst commented Jan 26, 2017

Uh oh!

dadoonet commented Jan 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst Jan 24, 2017 •

edited

Loading

dadoonet Jan 24, 2017 •

edited

Loading