Skip to content

Set additional header with user guid #2167

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
Merged
sethboyles merged 2 commits into from
Apr 1, 2021
Merged

Set additional header with user guid #2167

sethboyles merged 2 commits into from
Apr 1, 2021

Conversation

@johha
Copy link
Contributor

@johha johha commented Mar 25, 2021 •
edited
Loading

  • A short explanation of the proposed change:
    Set user guid as additional header. This can be used for example in the nginx access logs.
    There will be a related capi PR which ensures that the guid is not forwarded to the client/user

  • Links to any other associated PRs

  • I have reviewed the contributing guide

  • I have viewed, signed, and submitted the Contributor License Agreement

  • I have made this pull request to the main branch

  • I have run all the unit tests using bundle exec rake

  • I have run CF Acceptance Tests

@cf-gitbot
Copy link

cf-gitbot commented Mar 25, 2021

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/177492022

The labels on this github issue will be updated when the story is started.

@johha johha mentioned this pull request Mar 25, 2021
Merged
3 tasks
@johha johha force-pushed the set-user-guid-header branch 3 times, most recently from 871fa27 to e318ff8 Compare March 25, 2021 14:48
@sweinstein22
Copy link
Contributor

sweinstein22 commented Mar 30, 2021

Hi @johha ,

Thanks so much for the PR! One minor question, it seems like we've eased an assertion in moving from eq to an include in the it 'forwards the response from the rate limiter' test. It feels like it would be beneficial to keep at least one equality test, to guard against any potential accidental header inclusions in the future. What would you think about either moving back to an eq and including the X-USER-GUID header in that test, or changing the it 'sets the X-USER-GUID header' assertion to say it 'adds the X-USER-GUID header' and moving to an eq in that case?

Thanks,
Sarah & @MarcPaquette

@johha johha force-pushed the set-user-guid-header branch 2 times, most recently from c5e1be5 to f340e65 Compare March 31, 2021 09:08
johha and others added 2 commits March 31, 2021 13:42
@johha @philippthun
Set new header with user guid
3a954ab 
* Can be used in nginx access log

Co-authored-by: Philipp Thun <philippthun@users.noreply.github.com>
@johha @philippthun
Ensure that there is only one header
626bd4f 
Co-authored-by: Philipp Thun <philippthun@users.noreply.github.com>
@johha johha force-pushed the set-user-guid-header branch from f340e65 to 626bd4f Compare March 31, 2021 11:42
@johha
Copy link
Contributor Author

johha commented Mar 31, 2021

Hi @sweinstein22 @MarcPaquette

thanks for your suggestions. I adjusted the tests but now the CI jobs are failing. Locally all tests are green. Could you have a look?

Thanks,
Johannes

@sethboyles
Copy link
Member

sethboyles commented Apr 1, 2021

The failing tests are unrelated flakes, so I am going to merge this in. Thanks for the PR!

@sethboyles sethboyles merged commit fcf59a6 into cloudfoundry:main Apr 1, 2021
@johha johha mentioned this pull request May 12, 2021
Closed
@johha johha mentioned this pull request Jun 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@johha @cf-gitbot @sweinstein22 @sethboyles