Make the importer utilities rely on TSDB-GW for authentication and org-association by replay · Pull Request #1335 · grafana/metrictank

replay · 2019-05-28T23:11:34Z

This change makes the importer rely on tsdb-gw to authenticate the incoming requests and associate them with an org-id based on the user the authenticated as.
It also creates a new mdata/importer package which contains all the importer data conversion logic and the related structs for sending the data over the wire.

fixes #1334

woodsaj · 2019-05-29T06:11:56Z

 	uriPath         = flag.String("uri-path", "/chunks", "the URI on which we expect chunks to get posted")
 	numPartitions   = flag.Int("num-partitions", 1, "Number of Partitions")
 	logLevel        = flag.String("log-level", "info", "log level. panic|fatal|error|warning|info|debug")
+	orgId           = flag.Int("org-id", -1, "the id of the org we import to")


I think we should set the default to '0', which should mean dont do org-id check.

replay · 2019-05-31T03:04:09Z

I pushed quite a different implementation, as discussed in #1334
It's not very well tested yet, will do that tomorrow. And I'll still need to fix the unit tests.

replay · 2019-05-31T15:50:00Z

Just like last time, Once this is accepted I'll create a PR to raintank/schema for those changes and once it's merged there I'll update the vendored raintank/schema here.

Dieterbe · 2019-06-07T10:22:08Z

+	listenPort      = flag.Int("listen-port", 8080, "The port to listen on")
 	ttlsStr         = flag.String("ttls", "35d", "list of ttl strings used by MT separated by ','")
 	partitionScheme = flag.String("partition-scheme", "bySeries", "method used for partitioning metrics. This should match the settings of tsdb-gw. (byOrg|bySeries)")
 	uriPath         = flag.String("uri-path", "/chunks", "the URI on which we expect chunks to get posted")


uriPath needs updating to "/metrics/import"

Dieterbe · 2019-06-07T11:31:28Z

+	return ChunkWriteRequest{ChunkWriteRequestPayload{ttl, t0, data, ts}, callback, key}
+}
+
+// ChunkWriteRequestWithoutOrg is used by the importer utility to send cwrs over the network


if you need to document a structure in mdata that it's [only] used by a specific tool, then we should just move that structure to a package for that tool.
e.g. create a package importer and create a ChunkWriteRequest type in there?

all of cmd/mt-whisper-importer-reader/conversion.go should go into an importer package as well, cmd is meant for the main program code.

good idea. where would you suggest i put that importer package? should that live in mdata since it is all kind of related to data handling & conversion?

sounds good, if you don't run into circular import problems

Dieterbe · 2019-06-07T11:34:52Z

 	exitOnError     = flag.Bool("exit-on-error", false, "Exit with a message when there's an error")
-	httpEndpoint    = flag.String("http-endpoint", "127.0.0.1:8080", "The http endpoint to listen on")
+	listenAddress   = flag.String("listen-address", "127.0.0.1", "The address to listen on")
+	listenPort      = flag.Int("listen-port", 8080, "The port to listen on")


why are you changing this? the commit message "importer transfers cwrs without org" doesn't give any clues..
why not keep this consistent with http.listen? (the config option used for MT to establish its listen address and port)

right, i think this temporarily made sense during some older revision, but now not anymore. changing it back

Dieterbe · 2019-06-10T15:49:17Z

+
+// SerializableChunkWriteRequest is used by the importer utility to send cwrs over the network
+// It does not contain the org id because this is assumed to be defined by the auth mechanism
+type SerializableChunkWriteRequest struct {


"serializable" is not helpful because pretty much all datastructures are serializable .
I would just stick with the name ChunkWriteRequest, the fact that this is in the importer package already gives a solid clue that this CWR type is specific to the importer.
You can see the same pattern in stdlib, for example https://golang.org/pkg/html/template/#Template and https://golang.org/pkg/text/template/#Template

For example the .Callback property of the mdata.ChunkWriteRequest is not serializable because that's a memory address. But I'm fine either way, I'll rename it back to ChunkWriteRequest

replay · 2019-06-11T02:06:08Z

I've moved all the importer-related logic into the importer package, please take another look

replay · 2019-06-12T17:03:49Z

FYI I've done another test import with the latest version of the writer & reader, once into BigTable and once into Cassandra, and they both look as expected.

Dieterbe · 2019-06-12T20:57:52Z

#!/bin/bash -eo pipefail
scripts/qa/docs.sh
checking tools
709c709
<     	The http endpoint to listen on (default "127.0.0.1:8080")
---
>     	The http endpoint to listen on (default "0.0.0.0:8080")
docs/tools.md does not match output of scripts/dev/tools-to-doc.sh
Exited with code 2

these two changes were in reaction to PR comments

also move all the related tests keep incResolution() and decResolution() as stand-alone functions because that way they're way more testable than if they were an integrated method of the conversion type

the instantiation of the ArchiveRequest struct should be located together with the struct definition, this moves it there

replay · 2019-06-12T21:19:15Z

sorry about that, fixed the docs

Dieterbe

LGTM. i didn't look very thoroughly, but you've tested this fairly well right? then i think this is good to go

replay · 2019-06-18T22:24:28Z

Cool, thx. Yeah, I've done multiple test imports

replay · 2019-06-18T23:44:47Z

I didn't update the vendored raintank/schema yet (#1335 (comment)). I'll make a PR for that, because otherwise the build is broken

replay force-pushed the limit_writer_org branch 2 times, most recently from e20989d to b9ecb6a Compare May 28, 2019 23:35

woodsaj reviewed May 29, 2019

View reviewed changes

replay force-pushed the limit_writer_org branch 3 times, most recently from 25f1457 to 20ffe51 Compare May 30, 2019 21:38

replay mentioned this pull request May 31, 2019

We need to be able to restrict the whisper importer utility to a single org #1334

Closed

replay force-pushed the limit_writer_org branch from 2b56cb0 to 072a028 Compare May 31, 2019 15:40

replay force-pushed the limit_writer_org branch 2 times, most recently from 61673e0 to f8743a2 Compare June 3, 2019 14:25

importer transfers cwrs without org

aeb7734

replay force-pushed the limit_writer_org branch from f8743a2 to aeb7734 Compare June 3, 2019 17:28

replay mentioned this pull request Jun 6, 2019

remove unused serialization functions, add one raintank/schema#29

Merged

Dieterbe reviewed Jun 7, 2019

View reviewed changes

Dieterbe reviewed Jun 10, 2019

View reviewed changes

replay force-pushed the limit_writer_org branch from a4cfd9f to 61827c4 Compare June 10, 2019 18:28

replay changed the title ~~limit importer-writer to specific org id~~ Make the importer utilities rely on TSDB-GW for authentication and org-association Jun 10, 2019

replay force-pushed the limit_writer_org branch 5 times, most recently from c43e9fe to ebf5284 Compare June 11, 2019 01:21

replay force-pushed the limit_writer_org branch 2 times, most recently from 0053fd1 to 8b99e7f Compare June 12, 2019 17:00

replay added 5 commits June 12, 2019 21:18

change default uri path & listen address syntax

47bd3cb

these two changes were in reaction to PR comments

move cwr into its own "importer" package

301e767

move conversion and chunk encoding to importer pkg

24def99

also move all the related tests keep incResolution() and decResolution() as stand-alone functions because that way they're way more testable than if they were an integrated method of the conversion type

move more converter logic into importer package

f0b5305

move ArchiveRequest instantiation into importer package

d966645

the instantiation of the ArchiveRequest struct should be located together with the struct definition, this moves it there

replay force-pushed the limit_writer_org branch from 8b99e7f to d966645 Compare June 12, 2019 21:19

replay mentioned this pull request Jun 14, 2019

Reject invalid tags #1348

Merged

Dieterbe approved these changes Jun 17, 2019

View reviewed changes

Dieterbe merged commit a4bc9e1 into master Jun 17, 2019

Dieterbe deleted the limit_writer_org branch June 17, 2019 21:08

Conversation

replay commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

replay commented May 31, 2019

Uh oh!

replay commented May 31, 2019

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!

Dieterbe Jun 7, 2019 • 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!

replay commented Jun 11, 2019

Uh oh!

replay commented Jun 12, 2019

Uh oh!

Dieterbe commented Jun 12, 2019

Uh oh!

replay commented Jun 12, 2019

Uh oh!

Dieterbe left a comment

Choose a reason for hiding this comment

Uh oh!

replay commented Jun 18, 2019

Uh oh!

replay commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

replay commented May 28, 2019 •

edited

Loading

Dieterbe Jun 7, 2019 •

edited

Loading