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

azure: Support using connection string authentication #51

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

nholik
Copy link
Contributor

@nholik nholik commented Apr 28, 2023

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Added an additional option storage_connection_string to Azure config to use a connection string rather than a key. This is needed in limited privilege scenarios where only a SAS token is available and not full access key. When provided, the value is forwarded on to the Azure SDK method for creating a client that accepts a connection string.

Existing consumers will be unaffected as using this new path must be opted into with a non-empty value for the new config option.

Verification

Using a connection string with a SAS token creates a client successfully. Further verified that running a patched version of loki consuming this library passing through SAS tokens as the authentication method works.

@nholik nholik force-pushed the allow-az-conn-strings branch from d09908f to af059d5 Compare April 28, 2023 00:51
@nholik nholik changed the title azure - Support using connection string authentication azure: Support using connection string authentication Apr 28, 2023
@nholik nholik marked this pull request as draft April 28, 2023 17:19
@nholik nholik marked this pull request as ready for review April 28, 2023 22:38
Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

LGTM.

Needs a rebase, though.

Signed-off-by: Nick Holik <nholik@gmail.com>
@nholik nholik force-pushed the allow-az-conn-strings branch from 5c0b716 to ea9928f Compare August 4, 2023 11:38
@nholik
Copy link
Contributor Author

nholik commented Aug 4, 2023

LGTM.

Needs a rebase, though.
Thanks @kakkoyun - rebase done

nholik and others added 2 commits August 24, 2023 19:25
Signed-off-by: Kemal Akkoyun <kakkoyun@users.noreply.github.com>
@kakkoyun kakkoyun enabled auto-merge (squash) August 29, 2023 15:17
@kakkoyun kakkoyun merged commit 1b257a3 into thanos-io:main Aug 29, 2023
@nholik nholik deleted the allow-az-conn-strings branch September 1, 2023 01:07
MichelHollands pushed a commit to grafana/loki that referenced this pull request Sep 21, 2023
**What this PR does / why we need it**:

Adds a connection-string option to the azure blob storage config. So the
[Azurite](https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite)
emulator can be used in local test environments (my use case). But also
to support the use of a SAS token. See the (future) addition of this
option to the Thanos object storage client:
thanos-io/objstore#51

**Which issue(s) this PR fixes**:
n/a

**Special notes for your reviewer**:

thanos-io/objstore#51 isn't merged yet. But I'm
already creating this PR to see what others think.

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [x] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [x] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](d10549e)
rhnasc pushed a commit to inloco/loki that referenced this pull request Apr 12, 2024
**What this PR does / why we need it**:

Adds a connection-string option to the azure blob storage config. So the
[Azurite](https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite)
emulator can be used in local test environments (my use case). But also
to support the use of a SAS token. See the (future) addition of this
option to the Thanos object storage client:
thanos-io/objstore#51

**Which issue(s) this PR fixes**:
n/a

**Special notes for your reviewer**:

thanos-io/objstore#51 isn't merged yet. But I'm
already creating this PR to see what others think.

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [x] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [x] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@d10549e)
{
name: "Valid User Assigned and Connection String set",
config: []byte(`storage_account: "myAccount"
storage_account_key: ""

Choose a reason for hiding this comment

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

how is this valid since storage_account_key and storage_connection_string cannot both be set?

mraboosk pushed a commit to mraboosk/loki that referenced this pull request Oct 7, 2024
**What this PR does / why we need it**:

Adds a connection-string option to the azure blob storage config. So the
[Azurite](https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite)
emulator can be used in local test environments (my use case). But also
to support the use of a SAS token. See the (future) addition of this
option to the Thanos object storage client:
thanos-io/objstore#51

**Which issue(s) this PR fixes**:
n/a

**Special notes for your reviewer**:

thanos-io/objstore#51 isn't merged yet. But I'm
already creating this PR to see what others think.

**Checklist**
- [x] Reviewed the
[`CONTRIBUTING.md`](https://github.com/grafana/loki/blob/main/CONTRIBUTING.md)
guide (**required**)
- [x] Documentation added
- [x] Tests updated
- [x] `CHANGELOG.md` updated
- [ ] If the change is worth mentioning in the release notes, add
`add-to-release-notes` label
- [ ] Changes that require user attention or interaction to upgrade are
documented in `docs/sources/setup/upgrade/_index.md`
- [x] For Helm chart changes bump the Helm chart version in
`production/helm/loki/Chart.yaml` and update
`production/helm/loki/CHANGELOG.md` and
`production/helm/loki/README.md`. [Example
PR](grafana@2cef71e)
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.

3 participants