-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Add basic auth for ecs #2026
Add basic auth for ecs #2026
Conversation
provider/ecs/ecs_test.go
Outdated
} | ||
|
||
for _, c := range cases { | ||
c := c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could rename c
to test
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we rename c
to test
the complete file will be not consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can rename all c
😃
docs/configuration/backends/ecs.md
Outdated
| `traefik.frontend.passHostHeader=true` | forward client `Host` header to the backend. | | ||
| `traefik.frontend.priority=10` | override default frontend priority | | ||
| `traefik.frontend.entryPoints=http,https` | assign this frontend to entry points `http` and `https`. Overrides `defaultEntryPoints`. | | ||
| `traefik.frontend.auth.basic=test:$apr1$H6uskkkW$IgXLP6ewTrSuBkTrqE8wj/,test2:$apr1$d9hr9HBB$4HxwgUir3HP4EsggP/QNo0` | Sets basic authentication for that frontend with the usernames and passwords test:test and test2:test2, respectively | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you replace by
| `traefik.frontend.auth.basic=EXPR` | Sets basic authentication for that frontend in CSV format: `User:Hash,User:Hash` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
9e5006d
to
924353a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
YEAH. Basic Auth for ECS. Thanks a lot for PR @mmatur :)
LGTM 👼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question and one suggestion.
Not sure why reviews for Marathon, Kubernetes, and Docker owners were requested. The PR only seems to touch the ECS provider.
provider/ecs/ecs_test.go
Outdated
} | ||
|
||
for _, test := range cases { | ||
c := test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test := test
would work as well. No need to change the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You right. A refactoring of ECS provider has been done in #2050. I will do the change to be consistent with this refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
func (p *Provider) getBasicAuth(i ecsInstance) []string { | ||
label := i.label(types.LabelFrontendAuthBasic) | ||
if label != "" { | ||
return strings.Split(label, ",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need/also want to check that each basic auth value adheres to the user:hash
? I think we should answer the question with yes if it would simplify reporting an error to the user somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldez This should have been answered prior to merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐮
[ecs] - rename all c to test [ecs] - improve documentation
439f558
to
6c18aa6
Compare
Description
Implements frontend basic auth for ecs provider.
This addresses the ecs part of #1465