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

s3_secret_access_key does not allow slashes #1607

Open
PabloCastellano opened this issue Jan 29, 2020 · 9 comments · May be fixed by grafana/dskit#265
Open

s3_secret_access_key does not allow slashes #1607

PabloCastellano opened this issue Jan 29, 2020 · 9 comments · May be fixed by grafana/dskit#265
Assignees
@owen-d
Labels
keepalive An issue or PR that will be kept alive and never marked as stale.

Comments

@PabloCastellano
Copy link
Contributor

Describe the bug
Loki fails to start when an s3 bucket is configured as storage and the secret key contains a slash. I have generated a different access key pair whose secret key doesn't contain a slash character and it works fine.

To Reproduce
Steps to reproduce the behavior:

  1. Configure s3 storage with a secret key that contains a slash. E.g.
storage_config:
  boltdb:
    directory: /tmp/loki/index
  filesystem:
    directory: /tmp/loki/chunks
  aws:
    s3: s3://ASDFGHJIQWETTYUI:Jkasduahdkjh213kj1h31+lkjaflkjzvKASDOasofhjafaKFAF/GoQd@region/bucket_name
  1. Start Loki (master-765b34d)

Expected behavior
Loki should correctly parse the secret key and don't interpret it as a port

Environment:

  • Infrastructure: Docker running in a VM
  • Deployment tool: Docker-compose

Screenshots, Promtail config, or terminal output

failed parsing config: /etc/loki/local-config.yaml: parse s3://ASDFGHJIQWETTYUI:Jkasduahdkjh213kj1h31+lkjaflkjzvKASDOasofhjafaKFAF/GoQd@region/bucket_name invalid port ":Jkasduahdkjh213kj1h31+lkjaflkjzvKASDOasofhjafaKFAF" after 
host
@adityacs
Copy link
Contributor

adityacs commented Feb 4, 2020 •
edited
Loading

@PabloCastellano you should encode the URL. Try Like below
s3://ASDFGHJIQWETTYUI:Jkasduahdkjh213kj1h31+lkjaflkjzvKASDOasofhjafaKFAF%2FGoQd@region/bucket_name

/ is replaced with %2F

FYR: Check the comment here https://github.com/weaveworks/common/blob/master/aws/config.go#L16

@owen-d
Copy link
Member

owen-d commented Feb 4, 2020

Nice digging @adityacs. I do think we should have nicer error handling for this (or transparently handle this case)

@PabloCastellano
Copy link
Contributor Author

I can confirm that URL encoding the character works. In my opinion there should be better error handling or at least the documentation should warn you. I can provide a PR for documentation if you think this is useful

@owen-d
Copy link
Member

owen-d commented Feb 6, 2020

Definitely! Feel free to stop by #loki-dev on grafana.slack.com if you have any questions.

@stale
Copy link

stale bot commented Mar 7, 2020

This issue has been automatically marked as stale because it has not had any activity in the past 30 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale A stale issue or PR that will automatically be closed. label Mar 7, 2020
@owen-d owen-d added the keepalive An issue or PR that will be kept alive and never marked as stale. label Mar 8, 2020
@stale stale bot removed the stale A stale issue or PR that will automatically be closed. label Mar 8, 2020
@slim-bean
Copy link
Collaborator

@PabloCastellano are you still available to add some documentation around this, that would be very helpful!

@twix14
Copy link

twix14 commented Feb 1, 2022

Still active in 2.4.2

@jphx
Copy link

jphx commented Nov 17, 2022

I just ran across this myself. I ended up calling a URL-encoding function from here in order to set my secret key to s3_secret_access_key. I think it would be better if the Loki jsonnet does that than to ask the user to provide an already URL-encoded string.

kavirajk added a commit to grafana/dskit that referenced this issue Feb 7, 2023
@kavirajk
flagext: Add new type URLEscaped
6b8c28b 
Sometimes we need to store just escaped URL in the config like S3 URL that contains credentials which can include characters like
`:` and `/`. Currently we cannot use normal `flagext.URLValue` as it just uses unescaped URL.

Please check issue
grafana/loki#1607
@kavirajk kavirajk linked a pull request Feb 7, 2023 that will close this issue
Draft
2 tasks
kavirajk added a commit that referenced this issue Feb 7, 2023
@kavirajk
fix(s3config): Accept S3 URL as url escaped.
0012f89 
Fixes: #1607

Introduced this type in another PR
grafana/dskit#265
@kavirajk kavirajk mentioned this issue Feb 7, 2023
Closed
5 tasks
@cstyan
Copy link
Contributor

cstyan commented Nov 8, 2023

@kavirajk do you want to reopen your PR for fixing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@owen-d owen-d

Labels
keepalive An issue or PR that will be kept alive and never marked as stale.
Projects
None yet
Milestone
No milestone
7 participants
@PabloCastellano @cstyan @adityacs @owen-d @slim-bean @jphx @twix14