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

add proxy for the webconsole #17862

Merged
merged 2 commits into from
Jan 10, 2018
Merged

add proxy for the webconsole #17862

merged 2 commits into from
Jan 10, 2018

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Dec 18, 2017

Adds a proxy for the webconsole.

/assign @soltysh
/assign @liggitt
/assign @sttts

@jwforres @spadgett something for you to test against. When all the other parts are ready we can merge it.

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 18, 2017
@deads2k deads2k mentioned this pull request Dec 18, 2017
Closed
@openshift-merge-robot openshift-merge-robot added the vendor-update Touching vendor dir or related files label Dec 18, 2017
@deads2k deads2k force-pushed the webconsole-06-proxy branch from b9f8b33 to a7165a9 Compare December 18, 2017 19:51
@openshift-merge-robot openshift-merge-robot removed the vendor-update Touching vendor dir or related files label Dec 18, 2017
sttts
sttts reviewed Dec 19, 2017
View reviewed changes
pkg/cmd/server/origin/master.go
config.GenericConfig.AuditBackend = genericConfig.AuditBackend
config.GenericConfig.AuditPolicyChecker = genericConfig.AuditPolicyChecker
assetServer, err := config.Complete().New(apiserver.EmptyDelegate)
proxyHandler, err := NewServiceProxyHandler("webconsole", "openshift-web-console", aggregatorapiserver.NewClusterIPServiceResolver(c.ClientGoKubeInformers.Core().V1().Services().Lister()), caBundle)
Copy link
Contributor

Choose a reason for hiding this comment

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

in-cluster service 👍

sttts
sttts reviewed Dec 19, 2017
View reviewed changes
pkg/cmd/server/origin/master.go
@@ -307,9 +303,6 @@ func (c *MasterConfig) withConsoleRedirection(handler, assetServerHandler http.H
if assetConfig == nil {
return handler
}
if !c.WebConsoleEnabled() || c.WebConsoleStandalone() {
Copy link
Contributor

Choose a reason for hiding this comment

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

was it opt-in and is not anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was it opt-in and is not anymore?

Yeah. It's moving in 3.9. I don't think there's any plan to continue supporting the old path. They've even stopped pushing bindata updates.

sttts
sttts reviewed Dec 19, 2017
View reviewed changes
pkg/cmd/server/origin/service_proxy_handler.go

// TODO this should properly handle content type negotiation
// if the caller asked for protobuf and you write JSON bad things happen.
func (r *responder) Object(statusCode int, obj runtime.Object) {
Copy link
Contributor

Choose a reason for hiding this comment

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

where is this coming from? the responder of NewUpdateAwareHandler is a ErrorResponder which only has the Error method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

where is this coming from? the responder of NewUpdateAwareHandler is a ErrorResponder which only has the Error method.

Hmm.. Could be cruft. I copied from the aggregator which still had it. It's always hard to tell in golang. Is this function being used somewhere in the stack to cast as a particular kind of object and control behavior? Maybe. Worst language ever.

@deads2k
Copy link
Contributor Author

deads2k commented Dec 19, 2017

/retest

@spadgett
Copy link
Member

spadgett commented Jan 2, 2018

I've verified that this works with #17575.

@spadgett spadgett mentioned this pull request Jan 2, 2018
Merged
liggitt
liggitt reviewed Jan 3, 2018
View reviewed changes
pkg/cmd/server/origin/service_proxy_handler.go
// write a new location based on the existing request pointed at the target service
location := &url.URL{}
location.Scheme = "https"
rloc, err := r.serviceResolver.ResolveEndpoint(r.serviceNamespace, r.serviceName)
Copy link
Contributor

Choose a reason for hiding this comment

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

move the resolution into a dialer in the rest client config so we only do a lookup when establishing a new connection, not on every http request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

move the resolution into a dialer in the rest client config so we only do a lookup when establishing a new connection, not on every http request

I think I'll do that as a follow on. This unblocks the majority of the work and is technically correct.

@spadgett Are there tests that make sure the console is accessible via this proxy?

@deads2k deads2k changed the title [DO NOT MERGE] add proxy for the webconsole add proxy for the webconsole Jan 9, 2018
@deads2k deads2k force-pushed the webconsole-06-proxy branch from a7165a9 to dd05826 Compare January 9, 2018 21:52
deads2k
deads2k commented Jan 9, 2018
View reviewed changes
test/integration/web_console_access_test.go
@@ -237,39 +237,3 @@ func TestAccessOriginWebConsoleMultipleIdentityProviders(t *testing.T) {
tryAccessURL(t, url, exp.statusCode, exp.location, nil)
}
}

func TestAccessStandaloneOriginWebConsole(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this option no longer makes any sense

deads2k
deads2k commented Jan 9, 2018
View reviewed changes
test/integration/web_console_extensions_test.go
testserver "github.com/openshift/origin/test/util/server"
)

func TestWebConsoleExtensions(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spadgett you need to be sure you're testing this in the webconsole-server repo. Do you think you can port it yourself or do you need help?

Copy link
Member

Choose a reason for hiding this comment

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

These tests need to be reworked anyway since we're changing how extensions load. I can look at it in web-console-server.

openshift-merge-robot added a commit that referenced this pull request Jan 10, 2018
@openshift-merge-robot
Merge pull request #17575 from spadgett/console-cluster-up
a027e96 
Automatic merge from submit-queue.

Support web console image for cluster up

https://trello.com/c/9oaUh8xP

Install and run the web console server from a template during cluster up. Requires the proxy from #17862 to actually use the console server.

@deads2k @jwforres
@deads2k deads2k force-pushed the webconsole-06-proxy branch from dd05826 to f18a1a4 Compare January 10, 2018 13:10
@deads2k deads2k force-pushed the webconsole-06-proxy branch from 14178b5 to 53410d1 Compare January 10, 2018 13:16
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2018
@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 10, 2018

/retest

@deads2k deads2k force-pushed the webconsole-06-proxy branch from 53410d1 to c1c520d Compare January 10, 2018 15:02
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Jan 10, 2018

/retest

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Jan 10, 2018
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 17914, 18021, 18022, 17862, 18043).

@openshift-merge-robot openshift-merge-robot merged commit d17ed94 into openshift:master Jan 10, 2018
@deads2k deads2k deleted the webconsole-06-proxy branch January 24, 2018 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@liggitt liggitt liggitt left review comments

@spadgett spadgett spadgett left review comments

@juanvallejo juanvallejo Awaiting requested review from juanvallejo

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@soltysh soltysh

@sttts sttts

@liggitt liggitt

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@deads2k @spadgett @openshift-ci-robot @openshift-merge-robot @sttts @liggitt @soltysh