Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Project refactor #13

Merged

Conversation

tremblay-stripe
Copy link
Contributor

r?: @rlk-stripe

This PR kills some globals and parameterizes parts of Smokescreen.

Copy link
Contributor

@rlk-stripe rlk-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion on the overall shape of this, as I don't have a good sense of what's idiomatic for golang, but it seems reasonable enough. Some minor issues mentioned inline.

project: usersec
action: enforce
allowed_domains:
- '$example1\.com$' # Matches all Lyft+subdomains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bogus comment

@@ -0,0 +1,18 @@
broken:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should probably have comments explaining why it's broken.

project: usersec
action: enforce
allowed_domains:
- '^example1\.com$' # Matches all Lyft+subdomains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bogus comment

project: usersec
action: enforce
allowed_domains:
- 'example1\.com$' # Matches all Lyft+subdomains
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bogus comment


app := cli.NewApp()
app.Name = "smokescreen"
app.Usage = "A simple HTTP proxy that fogs over naughty URLs"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "...that prevents SSRF and can restrict destinations" or something else less cutesy and more practical?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against that. I just took that from the repo description on github.com/stripe/smokescreen.

"regexp"
)

type EgressAclYamlEntry struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class naming is kind of weird here-- EgressAclConfiguration maps directly to the YAML configuration file, but EgressAclYaml and EgressAclYamlEntry are the internal config format that doesn't map directly to YAML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was a bit lenient with this one since it's just an implementation detail behind an interface. I hear you though!

@tremblay-stripe
Copy link
Contributor Author

ptal @rlk-stripe


import "github.com/stretchr/testify/assert"
import "github.com/stripe/smokescreen/pkg/egressacl/decision"
import "testing"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is gofmt'd correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did!

@rlk-stripe
Copy link
Contributor

lgtm

@tremblay-stripe tremblay-stripe merged commit a8082af into stripe:master Aug 8, 2018
@tremblay-stripe tremblay-stripe deleted the tremblay-project-refactor branch August 8, 2018 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants