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.

Sign up for GitHub

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

Jump to bottom

Fixed reader to support both authentication and tenant-id #5719

Merged
merged 2 commits into from
May 18, 2022
Merged

Fixed reader to support both authentication and tenant-id #5719

merged 2 commits into from
May 18, 2022

Conversation

kovaxur
Copy link
Contributor

@kovaxur kovaxur commented Mar 28, 2022

What this PR does / why we need it:
Loki-canary will be able to use both basic-auth and x-scope-orgid headers.

Which issue(s) this PR fixes:
Fixes #5718

Special notes for your reviewer:

Checklist

  • Documentation added
  • [N/A] Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

@kovaxur kovaxur requested review from KMiller-Grafana and a team as code owners March 28, 2022 18:03
@CLAassistant
Copy link

CLAassistant commented Mar 28, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@kovaxur kovaxur force-pushed the canary-tenant-fix branch 2 times, most recently from ce786da to 9aeab3c Compare March 29, 2022 18:12
sandeepsukhani
sandeepsukhani reviewed Apr 7, 2022
View reviewed changes
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! May I please know what is the use case for supporting both?

pkg/canary/reader/reader.go Outdated Show resolved Hide resolved
@sandeepsukhani
Copy link
Contributor

It also won't work as is because the header setting you have changed is used just for live tailing. You also need to update QueryCountOverTime and Query functions below.

@kovaxur
Copy link
Contributor Author

kovaxur commented Apr 7, 2022

Hi,
Our Loki instance is running on a separate cluster, which is used only for monitoring. I would like to install a canary on each production cluster to test the full log path, however we have authentication enabled and we also use separate tenants for each cluster, so this would enable us to use loki-canary in our setup.

KMiller-Grafana
KMiller-Grafana suggested changes Apr 20, 2022
View reviewed changes
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

2 tiny doc changes.

docs/sources/operations/loki-canary.md Outdated Show resolved Hide resolved
cmd/loki-canary/main.go Outdated Show resolved Hide resolved
KMiller-Grafana
KMiller-Grafana approved these changes May 17, 2022
View reviewed changes
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

From a Docs perspective, I approve this PR.

sandeepsukhani
sandeepsukhani approved these changes May 18, 2022
View reviewed changes
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

LGTM

@sandeepsukhani sandeepsukhani merged commit e9d2a30 into grafana:main May 18, 2022
This was referenced Jun 28, 2022
Closed
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KMiller-Grafana KMiller-Grafana KMiller-Grafana approved these changes

@sandeepsukhani sandeepsukhani sandeepsukhani approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Loki-canary can't use both authentication and x-scope-orgid (tenant-id)
4 participants
@kovaxur @CLAassistant @sandeepsukhani @KMiller-Grafana