Skip to content

Queriers: Fix external search config when using RBAC with Google Cloud Run#2722

Merged
joe-elliott merged 1 commit intografana:mainfrom
modulitos:fix-querier-config
Jul 28, 2023
Merged

Queriers: Fix external search config when using RBAC with Google Cloud Run#2722
joe-elliott merged 1 commit intografana:mainfrom
modulitos:fix-querier-config

Conversation

@modulitos
Copy link
Copy Markdown
Contributor

@modulitos modulitos commented Jul 28, 2023

What this PR does:

I implemented a feature enabling RBAC on Google Cloud Run endpoints, which was merged last week: #2593

But there is a bug with the configuration, as described below. This PR fixes that bug.

Repro:

Run Tempo with the following config:

querier:
  search:
    external_backend: google_cloud_run
    google_cloud_run:
      external_endpoints: ["https://my-cloud-run-endpoint.run.app"]

Open the Grafana Tempo explore panel, and search for traces.
We'll see traces in our results, but they should be coming from our external Cloud Run endpoint, not an internal block search.

Expected:

Block queries are sent to the external Cloud Run endpoint.

Actual:

An internal search is performed instead of an external search. This is because the querier.search.external_endpoints list is unset, so it defaults to an empty list, and is caught by this conditional:

if len(q.cfg.Search.ExternalEndpoints) == 0 {
return q.internalSearchBlock(ctx, req)
}

We can work around it by defining a non-empty querier.search.external_endpoints list:

querier:
  search:
    external_endpoints: ["dummy-value"]
    external_backend: google_cloud_run
    google_cloud_run:
      external_endpoints: ["https://my-cloud-run-endpoint.run.app"]

but clearly that's not desirable. This PR fixes the conditional and adds a unit test to cover this code path.

Checklist

  • Tests updated
  • Documentation added
    • No documentation changes required
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
    • Since this is a bug on a pre-release feature, I figured it's not worth updating the changelog. Happy to update it though if requested.

Signed-off-by: modulitos <hi@modulitos.com>
@modulitos modulitos force-pushed the fix-querier-config branch from c44ed2c to 1cd0cfa Compare July 28, 2023 08:49
@modulitos modulitos changed the title Queriers: Fix external search config for RBAC queries Queriers: Fix external search config when using RBAC on Google Cloud Run Jul 28, 2023
@modulitos modulitos changed the title Queriers: Fix external search config when using RBAC on Google Cloud Run Queriers: Fix external search config when using RBAC with Google Cloud Run Jul 28, 2023
Copy link
Copy Markdown
Contributor

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM.

cc/ @joe-elliott, bugfix to include in v2.2

func (q *Querier) SearchBlock(ctx context.Context, req *tempopb.SearchBlockRequest) (*tempopb.SearchResponse, error) {
// if we have no external configuration always search in the querier
if len(q.cfg.Search.ExternalEndpoints) == 0 {
if q.cfg.Search.ExternalBackend == "" && len(q.cfg.Search.ExternalEndpoints) == 0 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i think this can be a cleaner check in the future, but i'm fine with it for now.

@joe-elliott joe-elliott added type/bug Something isn't working backport release-v2.2 labels Jul 28, 2023
@joe-elliott joe-elliott merged commit 2cb5a7b into grafana:main Jul 28, 2023
github-actions Bot pushed a commit that referenced this pull request Jul 28, 2023
Signed-off-by: modulitos <hi@modulitos.com>
(cherry picked from commit 2cb5a7b)
joe-elliott pushed a commit that referenced this pull request Jul 28, 2023
Signed-off-by: modulitos <hi@modulitos.com>
(cherry picked from commit 2cb5a7b)

Co-authored-by: Lucas Swart <hi@modulitos.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport release-v2.2 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants