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

Fix panic on startup if the ExternalURL field in the ruler config is empty #5525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kralicky
Copy link
Contributor

If ExternalURL is "" in the ruler config, it will be unmarshaled into a nil *URL object, causing a nil pointer dereference when building the ruler config. The String() helper (which performs the correct nil-check) on the ExternalURL's wrapper type was likely intended to be used to obtain the url string, but the nested *URL struct's String() was called instead, causing a panic if it's nil.

What this PR does:

Which issue(s) this PR fixes:
Fixes #

Checklist

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

@alanprot
Copy link
Member

Nice catch.

Can you add a test for it? with a valid url as well?

@kralicky
Copy link
Contributor Author

Sure thing 👍

@kralicky
Copy link
Contributor Author

I'm not sure exactly where a test for this would go - since there wasn't anything wrong with the URL wrapper code itself, just the usage, maybe it could be added in an integration test somewhere? Could you point me in the right direction?

@alanprot
Copy link
Member

alanprot commented Sep 8, 2023

We can try to create a a config with a problematic entry and run the test (make sure we dont throw an error).

A simple test is enough, just create the bad config and call DefaultTenantManagerFactory

@kralicky kralicky force-pushed the fix-ruler-externalurl branch from bf45305 to 881330c Compare September 21, 2023 19:22
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.

2 participants