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

Change Header used for impersonation scopes #18378

Merged
merged 1 commit into from
Feb 2, 2018
Merged

Change Header used for impersonation scopes #18378

merged 1 commit into from
Feb 2, 2018

Conversation

simo5
Copy link
Contributor

@simo5 simo5 commented Jan 31, 2018

Fixes #18374

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 31, 2018
@simo5 simo5 mentioned this pull request Feb 1, 2018
Closed
deads2k
deads2k reviewed Feb 1, 2018
View reviewed changes
pkg/cmd/server/origin/handlers.go Outdated
@@ -155,10 +154,12 @@ func (c *MasterConfig) versionSkewFilter(handler http.Handler, contextMapper api
})
}

const legacyImpersonateUserScopeHeader = "Impersonate-User-Scope"
Copy link
Contributor

Choose a reason for hiding this comment

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

Godoc explaining that older servers respected this. We switched to the new way in 1q 2017.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

deads2k
deads2k reviewed Feb 1, 2018
View reviewed changes
pkg/oauthserver/client/impersonate.go Outdated
@@ -27,16 +30,18 @@ func newImpersonatingRoundTripper(user user.Info, delegate http.RoundTripper) ht

func (rt *impersonatingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

you were going to remove this, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@simo5
Copy link
Contributor Author

simo5 commented Feb 1, 2018

@deads2k the last force push should be final, PTAL

@deads2k
Copy link
Contributor

deads2k commented Feb 1, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 1, 2018
knobunc
knobunc approved these changes Feb 1, 2018
View reviewed changes
Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

/lgtm

@simo5
Copy link
Contributor Author

simo5 commented Feb 1, 2018

/retest

@simo5
Copy link
Contributor Author

simo5 commented Feb 1, 2018

Flake #12072

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2018
@simo5
Use the Upstream header for scope impersonation
6ef9312 
Note: the upstream function does not forcibliy remove potential existing
impersonation headers, and it also uses the existing ones ignoring any
new headers we may be setting. This i ok, because we do not depend nor
probably want to allow multiple levels of impersonation.

Signed-off-by: Simo Sorce <[email protected]>
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 2, 2018
@simo5
Copy link
Contributor Author

simo5 commented Feb 2, 2018

Just rebased
/lgtm

@openshift-ci-robot
Copy link

@simo5: you cannot LGTM your own PR.

In response to this:

Just rebased
/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@deads2k
Copy link
Contributor

deads2k commented Feb 2, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, knobunc, simo5

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@simo5
Copy link
Contributor Author

simo5 commented Feb 2, 2018

Flake #18412
/retest

@simo5
Copy link
Contributor Author

simo5 commented Feb 2, 2018

Flake #18136
/retest

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 46c7fdd into openshift:master Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deads2k deads2k deads2k left review comments

@knobunc knobunc knobunc approved these changes

@enj enj Awaiting requested review from enj

Assignees

@deads2k deads2k

@knobunc knobunc

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@simo5 @deads2k @openshift-ci-robot @openshift-merge-robot @knobunc