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.

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

Wrap oauth/login requests to clear in-memory session #8435

Merged
merged 1 commit into from
Apr 11, 2016
Merged

Wrap oauth/login requests to clear in-memory session #8435

merged 1 commit into from
Apr 11, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Apr 8, 2016

Adds wrappers around OAuth-specific endpoints to ensure we release session store memory when requests are complete

@liggitt liggitt changed the title Wrap login requests to clear in-memory session Wrap oauth/login requests to clear in-memory session Apr 8, 2016
@liggitt
Copy link
Contributor Author

liggitt commented Apr 8, 2016

@smarterclayton
[test]

@liggitt
Copy link
Contributor Author

liggitt commented Apr 8, 2016

in addition to automated tests, I manually tested the following scenarios:

  • console disabled
  • session disabled
  • non-password-login paths (github/google, etc)
  • grant prompting enabled
  • login failures (login page redisplay)
  • multiple login methods

@liggitt
Copy link
Contributor Author

liggitt commented Apr 9, 2016

one pass, one flake (#8436), one known failure on conformance
[test]

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2016

two clean runs, two known failures on conformance

}

return ret, nil
}

func BuildSessionAuth(secure bool, config *configapi.SessionConfig) (*session.Authenticator, error) {
func BuildSessionAuth(secure bool, config *configapi.SessionConfig) (*session.Authenticator, handlerWrapper, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Returning private type feels slightly wierd on public function. Fine here though for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wasn't used elsewhere, made the function private

@smarterclayton
Copy link
Contributor

Approved, Lgtm within the risk profile we are willing to accept. Merge when ready.

@liggitt
Copy link
Contributor Author

liggitt commented Apr 10, 2016

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 852ca25

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2873/)

@liggitt
Copy link
Contributor Author

liggitt commented Apr 11, 2016

merge flaked on #8436

@liggitt
Copy link
Contributor Author

liggitt commented Apr 11, 2016

[merge]

@liggitt
Copy link
Contributor Author

liggitt commented Apr 11, 2016

flake #8444
re[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5559/) (Image: devenv-rhel7_3948)

@liggitt
Copy link
Contributor Author

liggitt commented Apr 11, 2016

merge failed on #8403 which should be fixed in master now, re[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 852ca25

@openshift-bot openshift-bot merged commit a89b776 into openshift:master Apr 11, 2016
@liggitt liggitt deleted the session-memory-leak branch April 14, 2016 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants