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

Ruler Object Storage Base64 Encoding #2646

Merged
merged 1 commit into from May 29, 2020

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented May 28, 2020

What this PR does:

This PR incorporates a small aspect of the #2543 that ensures rule group names and namespaces are base64 encoded when incorporated into the object key and written to disk. This fixes an issue where illegal characters such as / can be set in a rule group that affects the proper operation of the system. This can be seen by running the current integration test present in this PR against a current version of Cortex.

Base64 Encoding was chosen since it is a universally accepted format and fulfills all the naming requirements of common object storage backends:

*Note: This is a breaking change for anyone with the -experimental.ruler.enable-api enabled and object storage configured. I am working on a small migration tool w/ docs for anyone in this predicament.

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
@gotjosh
Copy link
Contributor

gotjosh commented May 28, 2020

The code looks good so I have approved, but there's more to consider here.

Depending on how you look at it, this could be considered a breaking change and/or a bugfix. I think we might need to support both lookup methods (whilst all new writing should be done under the new method) in the meantime and deprecate eventually on Cortex 1.4 (I have seen that version number used around, so I think there are some cadence rules around deprecation/breaking changes).

With that said, I understand there's migration tooling in the work to aid Cortex users on migrating to the new format. Perhaps that could help accelerate the deprecation process?

We probably need @pracucci or @pstibrany input here.

@jtlisi
Copy link
Contributor Author

jtlisi commented May 28, 2020

You're are correct that it's both a bug fix and a breaking change. However, the ruler API and the underlying object storage implementation is experimental. Our V1 Guarantees are very clear this feature is subject to breaking changes. IMO, the best we can do is communicate this change clearly in the Change Log and we can provide resources for a migration. I don't think it makes sense to support both methods since anyone who uses the old method will be using an implementation that is broken by design.

Currently the number of users exposed to this feature is relatively small and it is better to make the transition now rather than wait until more users are affected.

@gotjosh
Copy link
Contributor

gotjosh commented May 28, 2020

@jtlisi You're 100% correct. It is clearly listed under the experimental features - I take my comment back.

I'm good with this 👍

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I understand the need of doing this fix. LGTM 👍

Please remind to explain the change in the CHANGELOG.

*Note: This is a breaking change for anyone with the -experimental.ruler.enable-api enabled and object storage configured. I am working on a small migration tool w/ docs for anyone in this predicament.

That would be great, but not strictly required IMO, considering it's an experimental feature (unless you will build it anyway for other reasons, like your own usage). But at least should be clearly stated in the CHANGELOG.

@jtlisi jtlisi marked this pull request as ready for review May 29, 2020 14:42
@jtlisi jtlisi merged commit 44d2724 into cortexproject:master May 29, 2020
jtlisi added a commit that referenced this pull request May 29, 2020
jtlisi added a commit that referenced this pull request Jun 1, 2020
* add changelog entry

Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

* Update CHANGELOG.md

Co-authored-by: gotjosh <josue@grafana.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>

Co-authored-by: gotjosh <josue@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants