Skip to content

Conversation

@iamolegga
Copy link
Contributor

What does this PR do?

adds new field authResponseHeadersRegex to forwardAuth middleware

Motivation

fixes #6823

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

update of outdated pr #6828

@iamolegga
Copy link
Contributor Author

iamolegga commented Oct 22, 2020

That's pretty strange for me why not related test has failed, maybe we should just trigger check again 🤔

@jbdoumenjou
Copy link
Collaborator

Hi @iamolegga ,

Thanks for (re)opening the PR. 😉
Don't worry about the CI, it is related to another issue that will be fixed with #7436 .

Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

Hello @iamolegga,

Thanks for your work 👍
I have made several comments.
Also, could you:

  • add the new field in the dynamic references in the doc contents (file.toml, file.yml, marathon-labels.json and docker-labels.yml) and generate afterward the KV references, trough a go generate ?
  • add the new field to the middleware CRD ?

@iamolegga
Copy link
Contributor Author

@rtribotte I've updated PR, I hope I didn't miss anything 😀

@iamolegga iamolegga requested a review from rtribotte October 22, 2020 15:49
@iamolegga
Copy link
Contributor Author

@rtribotte sorry for disturbing, is it correct now?

@rtribotte
Copy link
Member

Hello @iamolegga,
Thanks for your modifications !
Sorry for the delay, i was awaiting for the update of the documentation references, but it was definitely not clear enough by stating it in my comment.
The idea is to complete the documentation :

  • add the new field in the dynamic references in the doc contents (file.toml, file.yml, marathon-labels.json and docker-labels.yml)
  • generate afterward the KV references, trough a go generate
  • add the new field to the middleware CRD

Don't hesitate if something is not clear ! ;-)

@iamolegga
Copy link
Contributor Author

@rtribotte thanks, finally understood what need to be done. updated 🙂

@iamolegga iamolegga requested a review from rtribotte October 29, 2020 12:42
Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator

@jbdoumenjou jbdoumenjou left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

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

LGTM

@traefiker traefiker merged commit 49cdb67 into traefik:master Oct 29, 2020
@iamolegga iamolegga deleted the auth-response-headers-regex branch October 29, 2020 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ForwardAuth authResponseHeaders accept regexp or wildcard

6 participants