WIP: Add support for validating cluster verification label in HTTP/gRPC requests #641
Conversation
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
duricanikolic
force-pushed
the
arve/validate-cluster-name
branch
2 times, most recently
from
ad74ad7 to
eca96aa
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
duricanikolic
force-pushed
the
arve/validate-cluster-name
branch
2 times, most recently
from
6c26bcf to
890ab6c
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
duricanikolic
force-pushed
the
arve/validate-cluster-name
branch
2 times, most recently
from
78f96ff to
2447c47
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
This reverts commit 2f94ffc.
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
aknuds1
force-pushed
the
arve/validate-cluster-name
branch
from
84927aa to
b464a96
Compare
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
Signed-off-by: Arve Knudsen <[email protected]>
| // Allow for a potential path prefix being configured. | ||
| reB.WriteString(".*/(metrics|debug/pprof.*|ready") |
There was a problem hiding this comment.
I think this MUST be configurable. Each downstream will have some paths that may not need the cluster verification, for example the admin frontend for Mimir.
There was a problem hiding this comment.
Maybe that's what auxPaths is? I think this method needs more docs.
There was a problem hiding this comment.
auxPaths is for additional paths, yes. I kept a base set of hard-coded paths here since they are common (/metrics, /debug/pprof, /ready). That decision is of course up for discussion.
| reB.WriteString("|" + regexp.QuoteMeta(p)) | ||
| } | ||
| reB.WriteString(")") | ||
| reAuxPath := regexp.MustCompile(reB.String()) |
There was a problem hiding this comment.
Note that regular expression aren't anchored to start/end of line by default, so you probably need to do that here.
Right now this matches the endpoint /api/v1/metrics/query, but I don't think that's intended.
There was a problem hiding this comment.
Yep, I missed the anchoring. This is a WIP after all :)
Co-authored-by: Oleg Zaytsev <[email protected]>
middleware/cluster.go
Outdated
| "cluster_verification_label", cluster, "request_cluster_verification_label", reqCluster, | ||
| "header", clusterutil.ClusterVerificationLabelHeader, "url", r.URL, "path", r.URL.Path) | ||
| if invalidClusters != nil { | ||
| invalidClusters.WithLabelValues("http", r.URL.Path, reqCluster).Inc() |
There was a problem hiding this comment.
I think this is very fragile, the's no contract on which labels invalidClusters has, and if those change, this will panic when it should fail the request. I would suggest passing here a closure that would increment the correct metric (and let the caller pass an empty closure, not nil), so the responsibility of declaring labels and using them would stay on the initialization side.
Shameless plug, this is why I wrote cabify/gotoprom 5 years ago :D
There was a problem hiding this comment.
Interesting, will look into this when I have more time.
…rors Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
duricanikolic
force-pushed
the
arve/validate-cluster-name
branch
from
f339af6 to
22ac17e
Compare
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
Signed-off-by: Yuri Nikolic <[email protected]>
What this PR does:
Which issue(s) this PR fixes:
TODO:
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]