Support Variable Depths and improve regex #118

pecigonzalo · 2018-08-29T09:52:28Z

Based on the work of #70 but improving the regex and adding a separate validator for / paths.

Given we have no tests, I have left some comments explaining how the validation is expected to work, but I can easily remove it if you believe its too much noise/ugly. (which it is, but I dont know how else to document this in this repo)

Relates to: #70

# Test Key Validation

## Works as expected
./dist/chamber-local-linux-amd64 write validKey foo secret
./dist/chamber-local-linux-amd64 write validKey foo.bar secret
./dist/chamber-local-linux-amd64 write validKey foo. secret
./dist/chamber-local-linux-amd64 write validKey .foo secret
## Fails as expected
./dist/chamber-local-linux-amd64 write invalidKey /foo secret
./dist/chamber-local-linux-amd64 write invalidKey foo//bar secret
./dist/chamber-local-linux-amd64 write invalidKey foo/bar secret
## Displays all keys
./dist/chamber-local-linux-amd64 list validkey
## Shows all envs correctly
./dist/chamber-local-linux-amd64 exec validkey -- env | grep FOO

# Test Service Validation CHAMBER_NO_PATHS=1
## Works as expected
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write validKey foo secret
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write valid.Key foo secret
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write valid.Key.Service foo secret
## Fails as expected
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write validKey. foo secret
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write .validKey foo secret
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write valid/Key foo secret
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 write valid..Key foo secret
## Displays all keys
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 list validKey
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 list valid.Key
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 list valid.Key.Service # Recurses to `valid.Key.Service`
## Shows all envs correctly
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 exec validkey -- env | grep FOO
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 exec valid.Key -- env | grep FOO
CHAMBER_NO_PATHS=1 ./dist/chamber-local-linux-amd64 exec valid.Key.Service -- env | grep FOO

# Test Service Validation (using / and PATHS)
## Works as expected
./dist/chamber-local-linux-amd64 write validKey foo secret
./dist/chamber-local-linux-amd64 write valid/Key foo secret
./dist/chamber-local-linux-amd64 write valid/Key/Service foo secret
## Fails as expected
./dist/chamber-local-linux-amd64 write validKey/ foo secret
./dist/chamber-local-linux-amd64 write /validKey foo secret
./dist/chamber-local-linux-amd64 write valid//Key foo secret
./dist/chamber-local-linux-amd64 write valid.Key foo secret
## Displays all keys
./dist/chamber-local-linux-amd64 list validKey
./dist/chamber-local-linux-amd64 list valid/Key # Does not recurse to `valid/Key/Service`
./dist/chamber-local-linux-amd64 list valid/Key/Service
## Shows all envs correctly
./dist/chamber-local-linux-amd64 exec validkey -- env | grep FOO
./dist/chamber-local-linux-amd64 exec valid/Key -- env | grep FOO
./dist/chamber-local-linux-amd64 exec valid/Key/Service -- env | grep FOO```

nickatsegment

Thanks for your contribution! Maybe the tests could go into store/ssmstore_test.go? It's true we don't run any tests in the Makefile; they can be run with go test ./...

pecigonzalo · 2018-08-29T17:02:24Z

Oh I did not notice the test there, my bad ill add them

pecigonzalo · 2018-08-29T17:29:07Z

@nickatsegment I noticed that store/ssmstore_test.go is not actually doing validation of the regexs, and that fields are also not validated on write for the SSMStore only on list which makes the testing a bit more complex.
I think its out of the scope of this PR, but probably it should also validate on write

EDIT:
Found a simple way to do it, working on it.

nickatsegment · 2018-08-29T18:10:27Z

Oh I did not notice the test there, my bad ill add them

No worries. :)

I noticed that store/ssmstore_test.go is not actually doing validation of the regexs, and that fields are also not validated on write for the SSMStore only on list which makes the testing a bit more complex.

I was thinking just simple unit tests for validateService and validateKey against your valid/invalid lists. Though testing that write/list/etc. validate in the same way could be valuable, but sort of duplication.

I think its out of the scope of this PR, but probably it should also validate on write

Probably :) Wanna cut a bug for that?

pecigonzalo · 2018-08-29T18:16:27Z

@nickatsegment Added the tests, which found some small issues (which I believe would not happen anyway because of how the rest of the components work) and fixed it.

Added tests for ssm_store and also to root, let me know if that is ok.

nickatsegment · 2018-08-29T18:26:53Z

Added tests for ssm_store and also to root, let me know if that is ok.

Cool. Out of scope, but at some point it'd be nice to centralize this validation.

Tests pass on my machine, so LGTM. I'm going to loop in some of the other maintainers before merging: I'm new :)

pecigonzalo · 2018-08-29T18:34:51Z

I agree. I mentioned on the linked #119 that we should do that as well, not only for the test but I believe it is probably for the best that both validations are the same.

AndreyMarchuk

add dash support into regexps
update tests with dash case

AndreyMarchuk · 2018-09-07T22:52:29Z

cmd/root.go

-	validServiceFormat = regexp.MustCompile(`^[A-Za-z0-9-_]+$`)
+	validKeyFormat         = regexp.MustCompile(`^[\w\.]+$`)
+	validServiceFormat     = regexp.MustCompile(`^[\w]+(\.[\w]+)*$`)
+	validServicePathFormat = regexp.MustCompile(`^[\w]+(\/[\w\.]+)*$`)


does not support dashes

^[\w\.-]+$ ^[\w-]+(\.[\w-]+)*$ ^[\w-]+(\/[\w\.-]+)*$

@AndreyMarchuk Good catches, I updated the test cases and the regex both for SSM and for the CMD. I will put it as a fixup, so you can see the changes, once we are ready to merge I can squash them.

One slight comment, I left some of the old functionality where a /./this path/key is allowed, which I believe should be fine and it leaves on the user's hands using a proper naming convention.

Awesome, thanks! I like that we are doing a quick progress.

This should be ready to review now.

maartenvanderhoef · 2018-09-07T23:28:15Z

Hi everyone, I'm late in the game here, big thumbs up for everyone contributing! I see quite some effort was made already into giving the service a different path which is just what I was looking for 👍

My idea (#128) however is different from implementation and would like to see comments on it.. I suggested having a SSM_PATH_NAMESPACE env var to be prepended in front of the service.

This would keep the Dockerfile cleaner and I think a bit more context agnostic. If the same docker is deployed in the same region using a different path (think of migrations) with the current PR, it would look like:

CMD chamber exec ${PREPEND}/Service

Having chamber interpolating SSM_PATH_NAMESPACE as ENV var, it would look like this:

CMD chamber exec service

The other advantage of having a single depth service with a prepend is that this leaves the possibility for a later moment to create variables with variable depth .. One could do a replace on / with _ and you can then have

/service/monitoring/logging_this_app_key     MONITORING_LOGGING_THIS_APP_KEY
/service/monitoring/data_api_key             MONITORING_DATA_API_KEY

Thanks!

pecigonzalo · 2018-09-08T02:33:47Z

@maartenvanderhoef I think your requirement it covered by this PR, we could also add the ENV VAR, but Isn't it easier to just interpolate your prefix on your CMD line?

pecigonzalo · 2018-09-08T03:16:36Z

Not that fixup! commits are there just for visibility of the change right now, ill squash them once this is ready to merge.

maartenvanderhoef · 2018-09-08T08:06:26Z

Hi Gonzalo, that's right, it's covered 👍. I think it would be great if an ENV VAR could work as well.. I think that it is more clean looking compared to Dockerfiles which have to 'interpolate' a variable in the entrypoint or cmd.

pecigonzalo · 2018-09-08T10:02:30Z

I would keep that as a separate PR as it is a different feature, and it can be done sort of easily once this is merged.

systemizer

Looks good to me! Also tested it locally as a sanity check. Thanks for adding this feature.

pecigonzalo · 2018-09-10T16:24:07Z

Excellent, I'll squash for merge

…

On Mon, Sep 10, 2018, 6:01 PM Rob McQueen ***@***.***> wrote: ***@***.**** approved this pull request. Looks good to me! Also tested it locally as a sanity check. Thanks for adding this feature. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#118 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJcviSztF1L2IHD5ZSXaVZTwwJ2QxdbCks5uZozBgaJpZM4WRNOG> .

pecigonzalo · 2018-09-10T18:41:41Z

@systemizer @AndreyMarchuk @nickatsegment given the approves so far, I squashed the fixups, so this is ready to merge.

nickatsegment · 2018-09-10T19:35:16Z

@pecigonzalo Thanks! Merged. FYI we almost always do a squash and merge

imjoshholloway added 3 commits January 19, 2018 13:56

Make the service path variable depth

c8909bb

Update validation to support .

ca0ee69

Merge branch 'master' into master

0817116

pecigonzalo force-pushed the feature/variable-depth branch 3 times, most recently from 234fac1 to 5b5bbe3 Compare August 29, 2018 10:20

nickatsegment suggested changes Aug 29, 2018

View reviewed changes

pecigonzalo force-pushed the feature/variable-depth branch from 5b5bbe3 to d001a2c Compare August 29, 2018 18:15

nickatsegment approved these changes Aug 29, 2018

View reviewed changes

nickatsegment requested review from dfuentes, boggsboggs and systemizer August 29, 2018 18:27

pecigonzalo mentioned this pull request Aug 29, 2018

SSMStore should validate on write as well #119

Closed

AndreyMarchuk mentioned this pull request Sep 7, 2018

feature request: Having a namespace in front of the ssm /service/ path. #128

Open

AndreyMarchuk suggested changes Sep 7, 2018

View reviewed changes

pecigonzalo force-pushed the feature/variable-depth branch from 31f5973 to 3413da6 Compare September 8, 2018 03:09

systemizer approved these changes Sep 10, 2018

View reviewed changes

Support different validation for CHAMBER_NO_PATHS

9cebf19

Add tests for validations

75ff80b

pecigonzalo force-pushed the feature/variable-depth branch from ae4c1f4 to 75ff80b Compare September 10, 2018 18:40

nickatsegment merged commit d9c77ea into segmentio:master Sep 10, 2018

pecigonzalo deleted the feature/variable-depth branch September 10, 2018 19:40

pecigonzalo restored the feature/variable-depth branch September 10, 2018 19:40

mechanical-fish mentioned this pull request Oct 13, 2018

Make the service path support variable depths #70

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Variable Depths and improve regex #118

Support Variable Depths and improve regex #118

pecigonzalo commented Aug 29, 2018 •

edited

Loading

nickatsegment left a comment

pecigonzalo commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018 •

edited

Loading

nickatsegment commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018

nickatsegment commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018

AndreyMarchuk left a comment •

edited

Loading

AndreyMarchuk Sep 7, 2018 •

edited

Loading

AndreyMarchuk Sep 7, 2018

pecigonzalo Sep 8, 2018

pecigonzalo Sep 8, 2018

AndreyMarchuk Sep 8, 2018

pecigonzalo Sep 10, 2018

maartenvanderhoef commented Sep 7, 2018 •

edited

Loading

pecigonzalo commented Sep 8, 2018

pecigonzalo commented Sep 8, 2018

maartenvanderhoef commented Sep 8, 2018

pecigonzalo commented Sep 8, 2018

systemizer left a comment

pecigonzalo commented Sep 10, 2018 via email

pecigonzalo commented Sep 10, 2018

nickatsegment commented Sep 10, 2018

Support Variable Depths and improve regex #118

Support Variable Depths and improve regex #118

Conversation

pecigonzalo commented Aug 29, 2018 • edited Loading

nickatsegment left a comment

Choose a reason for hiding this comment

pecigonzalo commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018 • edited Loading

nickatsegment commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018

nickatsegment commented Aug 29, 2018

pecigonzalo commented Aug 29, 2018

AndreyMarchuk left a comment • edited Loading

Choose a reason for hiding this comment

AndreyMarchuk Sep 7, 2018 • edited Loading

Choose a reason for hiding this comment

AndreyMarchuk Sep 7, 2018

Choose a reason for hiding this comment

pecigonzalo Sep 8, 2018

Choose a reason for hiding this comment

pecigonzalo Sep 8, 2018

Choose a reason for hiding this comment

AndreyMarchuk Sep 8, 2018

Choose a reason for hiding this comment

pecigonzalo Sep 10, 2018

Choose a reason for hiding this comment

maartenvanderhoef commented Sep 7, 2018 • edited Loading

pecigonzalo commented Sep 8, 2018

pecigonzalo commented Sep 8, 2018

maartenvanderhoef commented Sep 8, 2018

pecigonzalo commented Sep 8, 2018

systemizer left a comment

Choose a reason for hiding this comment

pecigonzalo commented Sep 10, 2018 via email

pecigonzalo commented Sep 10, 2018

nickatsegment commented Sep 10, 2018

pecigonzalo commented Aug 29, 2018 •

edited

Loading

pecigonzalo commented Aug 29, 2018 •

edited

Loading

AndreyMarchuk left a comment •

edited

Loading

AndreyMarchuk Sep 7, 2018 •

edited

Loading

maartenvanderhoef commented Sep 7, 2018 •

edited

Loading