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

Adds a local service for decorating IPs with annotations. #23

Merged
merged 4 commits into from
Mar 4, 2020

Conversation

pboothe
Copy link
Contributor

@pboothe pboothe commented Mar 3, 2020

Does not (yet) hook it up to main.


This change is Reviewable

@coveralls
Copy link
Collaborator

coveralls commented Mar 3, 2020

Pull Request Test Coverage Report for Build 160

  • 124 of 126 (98.41%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.3%) to 99.707%

Changes Missing Coverage Covered Lines Changed/Added Lines %
geoannotator/ipannotator.go 16 18 88.89%
Totals Coverage Status
Change from base Build 152: -0.3%
Covered Lines: 681
Relevant Lines: 683

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @pboothe and @stephen-soltesz)


ipservice/ipservice.go, line 8 at r1 (raw file):

// SocketFilename is a flag to allow both clients and servers to use the same command-line flag.
var SocketFilename = flag.String(
	"ipservice.SocketFilename",

Why mixed case?

How about? -annotator.sock?


ipservice/server.go, line 38 at r1 (raw file):

	a := &annotator.ClientAnnotations{}
	if h.asn != nil {
		a.Network = h.asn.AnnotateIP(ipstring) // Should this error be ignored?

What error?


ipservice/server.go, line 41 at r1 (raw file):

	}
	if h.geo != nil {
		h.geo.AnnotateIP(ip, &a.Geo) // Should this error be ignored?

Maybe just logging it for now?


ipservice/server.go, line 100 at r1 (raw file):

	mux := http.NewServeMux()
	mux.Handle("/ip", h)

Please add a version prefix to the path. i.e. /v1/annotate -- I suggest a different resource name so that the HTTP requests look like: /v1/annotate?ip=1.2.3.4.

Also, all external methods now respect locking in asnannotator and
geoannotator.
TODO: consider moving these methods to a wider scope if we need them
elsewhere.
Copy link
Contributor Author

@pboothe pboothe left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @stephen-soltesz)


ipservice/ipservice.go, line 8 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Why mixed case?

How about? -annotator.sock?

It should be named after the package. ipservice.sock is good, though.


ipservice/server.go, line 38 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

What error?

This returns nil on error.


ipservice/server.go, line 41 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Maybe just logging it for now?

Will add a log.


ipservice/server.go, line 100 at r1 (raw file):

Previously, stephen-soltesz (Stephen Soltesz) wrote…

Please add a version prefix to the path. i.e. /v1/annotate -- I suggest a different resource name so that the HTTP requests look like: /v1/annotate?ip=1.2.3.4.

Changed to:

/v1/annotate/ip?ip=1.2.3.4

I think the type of annotation desired should be in the URL, not simply an argument.

@pboothe pboothe merged commit decbcdf into master Mar 4, 2020
@pboothe pboothe deleted the local-service branch March 4, 2020 21:01
Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Still :lgtm: -- thank you for the Geo.Missing field.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained


ipservice/server.go, line 100 at r1 (raw file):

Previously, pboothe (Peter Boothe) wrote…

Changed to:

/v1/annotate/ip?ip=1.2.3.4

I think the type of annotation desired should be in the URL, not simply an argument.

My thinking is that "ip" is not the annotation type. The type of object returned is a client annotation. IP is the input to generating that object. I'm satisfied with the version prefix.

@pboothe
Copy link
Contributor Author

pboothe commented Mar 5, 2020

annotate is the service, ip is the annotation function being called. When I imaging extending this code to annotating new things, I think I would prefer to write a different handler with different input and output handling and types to handle annotating different input datatypes. So I wanted the input datatype to be part of the base URL to allow the http.Muxer to delegate to the right handler instead of having to put that logic in there myself in the future.

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