[WIP] storage/remote: use YAML data instead of JSON for MD5 hash #6456

simonpasquier · 2019-12-12T13:35:37Z

The configuration structs don't have JSON field tags so there's no guarantee that different configurations produce different JSON data. This isn't the case for YAML.

This is similar to #6455.

brian-brazil · 2019-12-12T14:03:23Z

Huh? I don't see how that could be an issue.

That we're not considering secrets is an issue.

simonpasquier · 2019-12-12T15:23:10Z

Huh? I don't see how that could be an issue.

relabel.Regexp are always marshaled to {}.

That we're not considering secrets is an issue.

True. I've added a unit test to catch this.

csmarchbanks · 2019-12-12T17:42:41Z

storage/remote/write_test.go

+	testutil.Equals(t, 3, len(s.queues))
+
+	testutil.Assert(t, q0 == s.queues[0], "Pointer of unchanged queue should have remained the same")
+	testutil.Assert(t, q1 != s.queues[1], "If the index changed, the queue should be stopped and recreated.")


Shouldn't the message here be around changing the bearer token should restart the queue?

brian-brazil · 2019-12-12T18:18:40Z

relabel.Regexp are always marshaled to {}.

I don't see what that has to do with field tags.

cstyan

I don't have opinions on the yaml vs. json

cstyan · 2019-12-12T22:23:49Z

storage/remote/write_test.go

 	testutil.Equals(t, 3, len(s.queues))

-	testutil.Assert(t, q == s.queues[1], "Pointer of unchanged queue should have remained the same")
+	testutil.Assert(t, q0 != s.queues[0], "If the index changed, the queue should be stopped and recreated.")


The message here isn't helpful in my opinion. What index are you referring to? The index of the queue's config in the overall RemoteWriteConfigs array? Rearranging those should no longer result in queue restarts. So I think this (and similar lines) should mention which part of the config was changed, and that when changing that field we expect to see a queue restart.

storage/remote/write_test.go

johncming · 2019-12-13T05:37:49Z

@simonpasquier

the master branch has updated which use hash instead of index. #6043

simonpasquier · 2019-12-13T09:54:33Z

As @johncming pointed out, I need to rebase the change but from a quick look, the issues should still exist.

cstyan · 2019-12-17T23:52:08Z

As @johncming pointed out, I need to rebase the change but from a quick look, the issues should still exist.

My bad, I pointed out that the test failure messages should be updated but that PR had only been merged a few hours earlier. It had been open long enough that I just had it in my head that the index to hash change was already merged.

jutley · 2020-01-03T20:15:44Z

I've run into this bug in two ways, both with regards to write_relabel_configs:

When the only difference between two remote_write configs is a regex, they produce the same hash, and Prometheus refuses to start. This is new in 2.15.
When a remote_write is updated and the only change is in its regex, the new rules never take effect. This was an issue before 2.15.

I'd really like to see this PR merged. We can create hacky workarounds, but would prefer not to.

csmarchbanks · 2020-01-14T22:18:13Z

@simonpasquier Do you think you will have an opportunity to rebase this soon?

simonpasquier · 2020-01-15T16:50:37Z

I've rebased my change but it only includes the test that demonstrates the issue. I don't have an immediate solution though.

csmarchbanks · 2020-01-15T17:28:07Z

I thought just changing the hashes to use YAML instead of JSON fixed it? Did those changes disappear?

simonpasquier · 2020-01-21T10:53:00Z

It does fix the issue in a way but it breaks in another. More precisely secret fields (bearer tokens for instance) are marshaled as <secret> in YAML so this test won't pass if we switch from JSON to YAML.

jutley · 2020-01-31T23:25:08Z

Would it make sense to update the yaml marshaller to include a hash of the secret? That should fix the toHash function for regexes and secrets without exposing the secret. Not sure if it will break anything.

roidelapluie · 2020-02-01T09:11:28Z

I did think about it but I don't like it... That would make the hash user visible Le sam. 1 févr. 2020 à 00:25, Jake Utley <notifications@github.com> a écrit :

…

Would it make sense to update the yaml marshaller to include a hash of the secret? That should fix the toHash function for regexes and secrets without exposing the secret. Not sure if it will break anything. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6456?email_source=notifications&email_token=AACHHJSG7HTYZEWOTJ5HU73RASXNVA5CNFSM4JZ6TWZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKQKR2Q#issuecomment-580954346>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACHHJV7YWVTHCR6JNNXT33RASXNVANCNFSM4JZ6TWZQ> .

jutley · 2020-02-04T00:12:43Z

@roidelapluie To be clear, what would be the concern with a user-visible hash? I suppose it would allow a user to identify secret re-use. Are there any other concerns?

roidelapluie · 2020-02-04T06:08:56Z

Hash should not be used to hide secrets. They are not meant for security.

brian-brazil · 2020-02-04T08:23:32Z

It depends on the hash, but even with a proper cryptographic hash you could e.g. notice that two secrets were the same.

jutley · 2020-02-04T19:05:55Z

So, it seems like there are 2 requirements here:

Configuration marshaling exposes nothing about a secret's value
Configuration hashing reflects changes to any field (including regexes and secrets)

In order for these to not conflict, I think we must either 1) salt and hash the secrets when marshaling, or 2) not implement hashing using YAML marshaling.

I already implemented the second option with my original solution in #6546, which uses the JSON marshaler and provides an implementation for Regexp. However, this really feels like an abuse of the fact that there are multiple marshalers available, especially given that the JSON marshalers are never intentionally implemented in this project.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

But it doesn't work either because of secret fields. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

csmarchbanks · 2020-03-02T22:06:19Z

Would it be reasonable to keep the yaml marshaling for the hash, thus having redacted secrets, and specifically update a queue whenever one of the secrets changes? Or else have a way to update the http client on a remote.Client so we wouldn't even have to restart the whole queue if the auth changes?

roidelapluie · 2020-03-02T22:10:46Z

What prevents us to use reflects.Deepequal?

csmarchbanks · 2020-04-16T22:38:10Z

Closing as I took this over in #7073

simonpasquier requested review from cstyan and csmarchbanks December 12, 2019 13:35

simonpasquier mentioned this pull request Dec 12, 2019

*: avoid missed Alertmanager targets #6455

Merged

simonpasquier force-pushed the fix-md5-remote-write branch 2 times, most recently from 3896e4a to b99d365 Compare December 12, 2019 15:15

csmarchbanks reviewed Dec 12, 2019

View reviewed changes

cstyan reviewed Dec 12, 2019

View reviewed changes

csmarchbanks mentioned this pull request Jan 3, 2020

Hashing for remote storage queues is broken #6545

Closed

csmarchbanks mentioned this pull request Jan 3, 2020

Add test case and fix for regexp json marshalling #6546

Closed

simonpasquier force-pushed the fix-md5-remote-write branch from b99d365 to d67dbce Compare January 15, 2020 16:49

simonpasquier changed the title ~~storage/remote: use YAML data instead of JSON for MD5 hash~~ [WIP] storage/remote: use YAML data instead of JSON for MD5 hash Jan 15, 2020

simonpasquier added 2 commits February 5, 2020 14:56

storage/remote: add tests for config changes

2fff5db

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Hash YAML instead of JSON

b446c4e

But it doesn't work either because of secret fields. Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the fix-md5-remote-write branch from d67dbce to b446c4e Compare February 5, 2020 14:01

csmarchbanks mentioned this pull request Mar 31, 2020

Fix remote write not updating when relabel configs change #7073

Merged

csmarchbanks closed this Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] storage/remote: use YAML data instead of JSON for MD5 hash #6456

[WIP] storage/remote: use YAML data instead of JSON for MD5 hash #6456

simonpasquier commented Dec 12, 2019

brian-brazil commented Dec 12, 2019

simonpasquier commented Dec 12, 2019

csmarchbanks Dec 12, 2019

brian-brazil commented Dec 12, 2019

cstyan left a comment

cstyan Dec 12, 2019

johncming commented Dec 13, 2019

simonpasquier commented Dec 13, 2019

cstyan commented Dec 17, 2019

jutley commented Jan 3, 2020

csmarchbanks commented Jan 14, 2020

simonpasquier commented Jan 15, 2020

csmarchbanks commented Jan 15, 2020

simonpasquier commented Jan 21, 2020

jutley commented Jan 31, 2020

roidelapluie commented Feb 1, 2020 via email

jutley commented Feb 4, 2020

roidelapluie commented Feb 4, 2020

brian-brazil commented Feb 4, 2020

jutley commented Feb 4, 2020

csmarchbanks commented Mar 2, 2020

roidelapluie commented Mar 2, 2020

csmarchbanks commented Apr 16, 2020

[WIP] storage/remote: use YAML data instead of JSON for MD5 hash #6456

[WIP] storage/remote: use YAML data instead of JSON for MD5 hash #6456

Conversation

simonpasquier commented Dec 12, 2019

brian-brazil commented Dec 12, 2019

simonpasquier commented Dec 12, 2019

csmarchbanks Dec 12, 2019

Choose a reason for hiding this comment

brian-brazil commented Dec 12, 2019

cstyan left a comment

Choose a reason for hiding this comment

cstyan Dec 12, 2019

Choose a reason for hiding this comment

johncming commented Dec 13, 2019

simonpasquier commented Dec 13, 2019

cstyan commented Dec 17, 2019

jutley commented Jan 3, 2020

csmarchbanks commented Jan 14, 2020

simonpasquier commented Jan 15, 2020

csmarchbanks commented Jan 15, 2020

simonpasquier commented Jan 21, 2020

jutley commented Jan 31, 2020

roidelapluie commented Feb 1, 2020 via email

jutley commented Feb 4, 2020

roidelapluie commented Feb 4, 2020

brian-brazil commented Feb 4, 2020

jutley commented Feb 4, 2020

csmarchbanks commented Mar 2, 2020

roidelapluie commented Mar 2, 2020

csmarchbanks commented Apr 16, 2020