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

promtail: add multi-tenant support #1135

Merged

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Oct 9, 2019

What this PR does / why we need it:

Promtail currently doesn't allow to specify the tenant / org ID. In this PR I've introduced three main changes:

  1. Add tenant_id support to promtail client config
  2. Add multi-tenant capabilities to promtail client, to be able to override the tenant ID on a per log entry basis
  3. Add a tenant stage to easily override the tenant ID

Which issue(s) this PR fixes:

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

@pracucci pracucci changed the title Add multi-tenant support to promtail promtail: add multi-tenant support Oct 9, 2019
@pracucci pracucci force-pushed the add-multi-tenant-support-to-promtail branch 2 times, most recently from 0cf4393 to b64515f Compare October 18, 2019 12:30
@pracucci pracucci marked this pull request as ready for review October 18, 2019 12:32
@pracucci pracucci force-pushed the add-multi-tenant-support-to-promtail branch from b64515f to 1134b22 Compare October 22, 2019 06:58
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

If we find support for multiple tenants without tenant-specific authentication a valid use case, this certainly looks like it would work.

pkg/logentry/stages/tenant.go Outdated Show resolved Hide resolved
- template:
source: tenant_id
template: 'team-api'
- tenant:
Copy link
Member

Choose a reason for hiding this comment

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

This is convoluted example with constant tenant ID. If that is a useful use case, maybe tenant stage can support it directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This originates from a comment made by @cyriltovena, where he was suggesting to add this example too (at least this is what I understood). Cyril, is this example what you meant?

Copy link
Contributor

@cyriltovena cyriltovena Oct 24, 2019

Choose a reason for hiding this comment

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

Yes because I think that's mainly how it will be used. I don't think someone has a tenant id in its log, or at least this is not the most common.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can the tenant stage support also a const tenant ID ? I think that's a valid use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not, but it can. What do you think if we add a value field, which is mutually exclusive with source? For example:

# Here the value is hardcoded
- tenant:
    value: team-api

# Here the value is picked from tenant_id field
- tenant:
    source: tenant_id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cyriltovena Done! What's your take?

pkg/promtail/client/batch.go Show resolved Hide resolved
@pracucci pracucci force-pushed the add-multi-tenant-support-to-promtail branch from 1134b22 to 9336bf2 Compare October 23, 2019 07:46
@pracucci
Copy link
Contributor Author

Thanks @pstibrany for your feedback. I've addressed 2 out of 3 comments. May you check it out again, please?

@cyriltovena
Copy link
Contributor

cyriltovena commented Nov 5, 2019 via email

@pracucci pracucci force-pushed the add-multi-tenant-support-to-promtail branch from 9336bf2 to 34e55d2 Compare November 6, 2019 09:52
Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM, I don't think we need a new file for a single constant that's a nitpick.

@cyriltovena cyriltovena merged commit 04f58c8 into grafana:master Nov 7, 2019
@brancz
Copy link

brancz commented Nov 7, 2019

Nice! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

promtail client is not able to send orgID headers Allow promtail to set X-Scope-OrgID header dynamically
5 participants