Enforce tenant labels on rule storage PUT request #219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jessicalins
reviewed
Jan 25, 2022
There was a problem hiding this comment.
cool! 📐 I think after we merge #198 we'd need to update the tests for the write cases too
| var rawRules rules.Rules | ||
| if err := yaml.Unmarshal(body, &rawRules); err != nil { | ||
| level.Error(rh.logger).Log("msg", "could not unmarshal rules", "err", err.Error()) | ||
| http.Error(w, "error unmarshaling rules", http.StatusInternalServerError) |
There was a problem hiding this comment.
Suggested change
| http.Error(w, "error unmarshaling rules", http.StatusInternalServerError) | |
| http.Error(w, "error unmarshalling rules", http.StatusInternalServerError) |
api/metrics/v1/rules.go
Outdated
|
|
||
| body, err = yaml.Marshal(rawRules) | ||
| if err != nil { | ||
| level.Error(rh.logger).Log("msg", "could not marshal YAML", "err", err.Error()) |
There was a problem hiding this comment.
Suggested change
| level.Error(rh.logger).Log("msg", "could not marshal YAML", "err", err.Error()) | |
| level.Error(rh.logger).Log("msg", "could not marshal rules YAML", "err", err.Error()) |
api/metrics/v1/rules.go
Outdated
| body, err = yaml.Marshal(rawRules) | ||
| if err != nil { | ||
| level.Error(rh.logger).Log("msg", "could not marshal YAML", "err", err.Error()) | ||
| http.Error(w, "error marshaling YAML", http.StatusInternalServerError) |
There was a problem hiding this comment.
Suggested change
| http.Error(w, "error marshaling YAML", http.StatusInternalServerError) | |
| http.Error(w, "error marshaling rules YAML", http.StatusInternalServerError) |
There was a problem hiding this comment.
I think since we have the same marshaling logic also in the get method maybe we could extract this to a separate function and reuse it in both get/put endpoints? same for the auth logic
There was a problem hiding this comment.
Yes, I think we can do it for both marshaling and unmarshalling rules!
matej-g
reviewed
Jan 26, 2022
There was a problem hiding this comment.
Looking good and great PR description @saswatamcode! Few questions, mostly just curious 😁
squat
requested changes
Jan 26, 2022
squat
reviewed
Jan 26, 2022
squat
reviewed
Jan 26, 2022
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
|
Let me also update this with the test cases from #198. |
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
saswatamcode
force-pushed
the
write-enforce
branch
from
618ab5b
to
3f7eaac
Compare
saswatamcode
commented
Jan 27, 2022
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
Signed-off-by: Saswata Mukherjee <saswataminsta@yahoo.com>
matej-g
approved these changes
Jan 27, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we enforce tenant labels on rules by adding the labels to them when Observatorium API gets it from the rules storage client i.e, the rules will only have the tenant labels when we get it from the API
rules/rawendpoint.With the recent changes in thanos-rule-syncer and rules-objstore, this means,
listallrulesendpoint, thanos-rule-syncer will try to get all rules by hitting rules-objstore directly (so enforcing labels won’t work as we’re no longer relying on the API read path, but read from rules-objstore directly)This PR fixes the above issue, by ensuring that the API adds tenant labels while making the PUT request to the rules storage client and validating the labels when a GET request is made. This way tenant labels are propagated to the rules storage client and then eventually to thanos-rule-syncer and Thanos Ruler.
For more diagrammatic context see MON-2169.