Make S3Overrides accessible via E-Var or option AND tests #40

mrmarcsmith · 2017-01-26T20:21:38Z

Background: when using parse server with a config file there is no way to specify S3Override options.
-This change doesn't affect the current method of passing s3overrides = backwards compatible.
-s3overrides would now be accessible by via parse-server config file like this:
"filesAdapter": {
"module":"parse-server-s3-adapter",
"options":{
"s3overrides": { "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"","pathname":"/","path":"/","href":"https:///"},
"s3BucketEndpoint": false,
ANY OTHER S3OVERRIDES GO HERE... },
"baseUrl": "https://",
"directAccess": true,
"accessKey": "",
"secretKey": "",
"bucket": "",
"region": ""
}
}

-OR in theory (I haven't tested) by Environmental variable like so:

S3_OVERRIDES="{ "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"$FILEURL","pathname":"/","path":"/","href":"https://$FILEURL/"},
"s3BucketEndpoint": false,
ANY OTHER S3OVERRIDES GO HERE... }"

Background: when using parse server with a config file there is no way to specify S3Override options. -This change doesn't affect the current method of passing s3overrides = backwards compatible. -s3overrides would now be accessible by via parse-server config file like this: "filesAdapter": { "module":"parse-server-s3-adapter", "options":{ "s3overrides": { "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"<your NOT s3 url like minio>","pathname":"/","path":"/","href":"https://<your NOT s3 url like minio>/"}, "s3BucketEndpoint": false, ANY OTHER S3OVERRIDES GO HERE... }, "baseUrl": "https://<your url>", "directAccess": true, "accessKey": "<your access key>", "secretKey": "<your secret>", "bucket": "<your bucket>", "region": "" } } -OR in theory (I haven't tested) by Environmental variable like so: S3_OVERRIDES="{ "endpoint": {"protocol":"https:","host":"$FILEURL","port":443,"hostname":"$FILEURL","pathname":"/","path":"/","href":"https://$FILEURL/"}, "s3BucketEndpoint": false, ANY OTHER S3OVERRIDES GO HERE... }"

Make S3Overrides accessible via E-Var or option.

deleted data to see if that helps error

changed Key to see if this is the issue

isolating ACL

removed isolations variables

deleting extra space

added error handling to create bucket

bucket change

only mark bucket as yes if it exists

caps

added logging of final override options

this ends up being a breaking change. reverting

mrmarcsmith · 2017-01-26T20:54:55Z

@acinader Nope, I ran into a discussion topic, given we have a config JSON as follow

var confObj = { 
bucketPrefix: 'test/', 
bucket: 'bucket-1', 
secretKey: 'secret-1', 
accessKey: 'key-1' ,
s3overrides: {
       secretAccessKey: 'secret-2', 
       accessKeyId: 'key-2', 
       params: { 
              Bucket: 'bucket-2' 
       }
   } 
};

which should take priority bucket-1 or bucket-2?
or should we throw an error so avoid confusion?

S3Overrides yielding to params

acinader · 2017-01-26T21:03:48Z

what do you think?

My $.02 in a situation like this would be: ideally throw an error, as it is probably a mistake.
But simply going with the last value works for me too.

Ok, the one in overrides should take precedent. that's what override means, right?

acinader · 2017-01-26T21:07:37Z

spec/test.spec.js

@@ -123,6 +123,27 @@ describe('S3Adapter tests', () => {
      expect(options.bucketPrefix).toEqual('test/');
    });

+    it('should accept options and overrides as an option in args', () => {
+      var confObj = { bucketPrefix: 'test/', bucket: 'bucket-1', secretKey: 'secret-1', accessKey: 'key-1' ,s3overrides: { secretAccessKey: 'secret-2', accessKeyId: 'key-2', params: { Bucket: 'bucket-2' }} };


nit: add a space after the comma

it should be
, s3over....
not
,s3over...

acinader · 2017-01-26T21:17:25Z

lib/optionsFromArguments.js

  options[key] = options[key] || process.env[env] || defaultValue;
+
+  //If we used the overrides,


nit. add space after //

so // If

acinader · 2017-01-26T21:26:38Z

two suggestions:

figure out how to configure editor to strip trailing white space.
run npm run lint before pushing commits. if you look at srcipts section of package.json you'll see the if your run npm test lint is also run first.

mrmarcsmith · 2017-01-26T21:30:30Z

@acinader Sorry about flooding your feed with commits. I thought this would be a quick fix so I've been editing on the GitHub website. I'll switch over.

acinader · 2017-01-26T21:31:49Z

haha well that explains that!

acinader · 2017-01-26T21:34:56Z

spec/test.spec.js

@@ -133,7 +133,7 @@ describe('S3Adapter tests', () => {
    });

    it('should accept options and overrides as an Enviromental Variable', () => {
-      var confObj = { bucketPrefix: 'test/', bucket: 'bucket-1', secretKey: 'secret-1', accessKey: 'key-1' };
+      var confObj = { bucketPrefix: 'test/', bucket: 'bucket-1', secretKey: 'secret-1', accessKey: 'key-1'};


why'd you take out that trailing space? for consistency, there should be a space between the 1' };

that's how all the other object spacing is done in the file. we just want to be consistent, when possible we put those consistency rules into lint so we don't have to think about it......

ghost · 2017-04-11T16:06:04Z

Hi
What is the state of this pr?

acinader · 2017-04-11T16:12:26Z

@aaronschmied unfortunately the unit tests are failing as a direct result of this change. I haven't looked into why that is.

ghost · 2017-04-11T16:32:42Z

For everyone who's looking for a quick and dirty fix:
I installed @acinader's repo using the following command in the Dockerfile:

RUN npm install && \
    npm install --save git+https://github.com/mrmarcsmith/parse-server-s3-adapter && \
    npm run build

After this I was able to configure my custom s3 endpoint in the parse server's config.json

...
        "cloud": "/parse-server/cloud/main.js",
        "filesAdapter": {
                "module": "parse-server-s3-adapter",
                "options": {
                        "bucket": "bucketname",
                        "accessKey": "accesskey",
                        "secretKey": "secretkey",
                        "region": "region",
                        "directAccess": true,
                        "baseUrl": "https://my.custom.s3/bucketname",
                        "s3overrides": {
                                "endpoint": "my.custom.s3",
                                "s3ForcePathStyle": true
                        }
                }
        }
}

acinader · 2017-04-11T16:34:12Z

it's actually @mrmarcsmith's repo, not mine ;).

would be nice to get the unit test fixed and have this merged in....

ghost · 2017-04-11T16:37:46Z

Oh, missed that 😆
Thanks for your work @mrmarcsmith

mrmarcsmith · 2017-04-11T19:11:04Z

@aaronschmied no problem! Let me see if I have time today or tomorrow to look into the failing unit test so we can get this merged.

flovilmart · 2017-06-17T21:44:28Z

@mrmarcsmith any news on that?

- fix typos in unit tests - remove ability to configure s3overrides via environment variables -- can't store objects in env - add a pretty test reporter so i can read output.

acinader

I pulled this branch so I could figure out what was going on with the tests to determine what to do with this pr. Now that I have a thorough understanding, I'd like to close this pr un-merged.

The idea of passing an object in the environment just does't work. Env vars need to be strings. The unit test that attempts to pass the s3Overrides is failing because the values are not in the env, just the string "[object, object]"
the two other tests are failing because this pr doesn't attempt to reconcile the existing difference in names for the AWS Secret. Its secretKey in one place but secretAccessKey in an another (see it in action right here).

While I could attempt to figure out some sensible way to deal with this, I don't want to bother cause a) Its probable that there will be some backward compatibility issue, but more importantly, I'm about to deprecate passing keys and secrets anyway.

@mrmarcsmith if you want help me understand so I can fix, I'm all ears, but given how old this is and that we shouldn't be monkeying with keys and secrets in our code any way since AWS has a super awesome way to deal with it that relieves even having to thinking about how to handle these things securely.

acinader · 2017-07-01T21:11:05Z

@mrmarcsmith we can reopen, but i'm going to close it pending further discussion with you.

mrmarcsmith · 2017-07-05T18:08:21Z

@acinader thank you for taking the time to look at this PR. That makes a lot of sense about the Env Vars but that is only half of the PR and I would gladly get rid of the Env Var part in favor of being able to pass the override options as a dictionary in the parse server config file. This is a setup (not Env Vars) I am currently using in production because I couldn't find any other way to get it to work before Parse shut down. If I remove the Env Vars part would this PR be considered?

mrmarcsmith · 2017-07-05T19:28:38Z

Since we have removed ENV VAR, I have created a new PR #50 to avoid any confusion.

mrmarcsmith added 16 commits December 29, 2016 21:38

Merge pull request #1 from mrmarcsmith/mrmarcsmith-s3override-access

4abb443

Make S3Overrides accessible via E-Var or option.

Update index.js

fd7ba2b

deleted data to see if that helps error

Update index.js

87e3b2a

changed Key to see if this is the issue

Update index.js

7986b61

isolating ACL

Update index.js

03fd287

removed isolations variables

Update index.js

4ab310e

deleting extra space

Update index.js

e18efe8

added error handling to create bucket

Update index.js

eae68ec

bucket change

Update index.js

311a44d

only mark bucket as yes if it exists

Update index.js

bcc7ef4

caps

Update index.js

0ee6e5e

added logging of final override options

Update index.js

5690044

this ends up being a breaking change. reverting

Removed debugging Logs

2b9fe8f

added tests for SOverrides as Env or an option

078a342

moved function so auto merge can happen

344cfb3

mrmarcsmith mentioned this pull request Jan 26, 2017

Update test.spec.js #39

Closed

mrmarcsmith added 2 commits January 26, 2017 13:24

removed trailing spaces

d4a3881

Debuggin CI Error

a4bce1e

mrmarcsmith added 3 commits January 26, 2017 13:55

Fix

a19d31d

S3Overrides yielding to params

removed trailing space

803523a

revert CI debug

747a17e

acinader reviewed Jan 26, 2017

View reviewed changes

options now yield to overrides

ddc96f6

acinader reviewed Jan 26, 2017

View reviewed changes

mrmarcsmith added 2 commits January 26, 2017 14:18

Added tests to ensure overrides take priority

8725059

removed trailing spaces

fcf3d39

acinader reviewed Jan 26, 2017

View reviewed changes

acinader mentioned this pull request Jun 20, 2017

Don't accept Key and Secret as constructor arguments #20

Open

acinader mentioned this pull request Jul 1, 2017

Make S3Overrides accessible via E-Var or option. #37

Closed

acinader suggested changes Jul 1, 2017

View reviewed changes

acinader closed this Jul 1, 2017

acinader mentioned this pull request Jul 1, 2017

Pass S3Override as environmental variable #36

Closed

mrmarcsmith mentioned this pull request Jul 5, 2017

S3 overrides as option #50

Merged

		options[key] = options[key] \|\| process.env[env] \|\| defaultValue;

		//If we used the overrides,

Uh oh!

Make S3Overrides accessible via E-Var or option AND tests #40

Make S3Overrides accessible via E-Var or option AND tests #40

Uh oh!

Conversation

mrmarcsmith commented Jan 26, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

acinader Jan 26, 2017

Choose a reason for hiding this comment

Uh oh!

acinader Jan 26, 2017

Choose a reason for hiding this comment

Uh oh!

acinader commented Jan 26, 2017

Uh oh!

mrmarcsmith commented Jan 26, 2017

Uh oh!

acinader commented Jan 26, 2017

Uh oh!

acinader Jan 26, 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!

ghost commented Apr 11, 2017

Uh oh!

acinader commented Apr 11, 2017

Uh oh!

ghost commented Apr 11, 2017 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acinader commented Apr 11, 2017

Uh oh!

ghost commented Apr 11, 2017

Uh oh!

mrmarcsmith commented Apr 11, 2017

Uh oh!

flovilmart commented Jun 17, 2017

Uh oh!

acinader left a comment • 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!

acinader commented Jul 1, 2017

Uh oh!

mrmarcsmith commented Jul 5, 2017

Uh oh!

mrmarcsmith commented Jul 5, 2017

Uh oh!

Uh oh!

mrmarcsmith commented Jan 26, 2017 •

edited

Loading

acinader commented Jan 26, 2017 •

edited

Loading

acinader Jan 26, 2017 •

edited

Loading

ghost commented Apr 11, 2017 •

edited by ghost

Loading

acinader left a comment •

edited

Loading