Changed the ui redirect path from a constant to a flag #56074

konryd · 2017-11-20T16:58:08Z

What this PR does / why we need it: It changes the UI redirect path into a environment-settable flag. This makes it possible to serve the /ui requests for cases when the Kubernetes dashboard is replaced by custom app.

Release note:

NONE

k8s-ci-robot · 2017-11-20T16:58:11Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-github-robot · 2017-11-20T16:59:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: konryd
We suggest the following additional approvers: deads2k, jszczepkowski

Assign the PR to them by writing /assign @deads2k @jszczepkowski in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

konryd · 2017-11-20T17:02:31Z

/assign @mwielgus @sttts

lavalamp · 2017-11-20T17:45:08Z

cmd/kube-apiserver/app/options/options.go

@@ -71,6 +71,8 @@ type ServerRunOptions struct {

 	MasterCount            int
 	EndpointReconcilerType string
+
+	UIRedirectUrl string


s/Url/URL/g

lavalamp · 2017-11-20T17:47:07Z

pkg/routes/ui.go

 // UIRedirect redirects /ui to the kube-ui proxy path.
-type UIRedirect struct{}
+type UIRedirect struct {
+	DashboardPath string


DestinationURL

Actually destinationURL since it doesn't need to be public.

lavalamp · 2017-11-20T17:47:31Z

pkg/routes/ui.go

@@ -22,14 +22,18 @@ import (
 	"k8s.io/apiserver/pkg/server/mux"
 )

-const dashboardPath = "/api/v1/namespaces/kube-system/services/kubernetes-dashboard/proxy"
-
 // UIRedirect redirects /ui to the kube-ui proxy path.


to the given path

lavalamp · 2017-11-20T17:48:28Z

pkg/routes/ui.go

+	DashboardPath string
+}
+
+func NewUIRedirect(dashboardPath string) UIRedirect {


Change the name here, too (it's not necessarily just a path). Add a godoc comment.

lavalamp · 2017-11-20T17:48:47Z

pkg/routes/ui.go

+}
+
+func NewUIRedirect(dashboardPath string) UIRedirect {
+	return UIRedirect{DashboardPath: dashboardPath}


are all destinations equally valid?

Not sure I understand the question. I've manually validated an http redirect to work.

lavalamp · 2017-11-20T17:50:55Z

It's a bit late but I'm inclined to take this, since hard coding this path is kinda dumb anyway.

liggitt · 2017-11-20T17:51:31Z

I'd rather see the apiserver become less entangled with UIs, not more ("why does my API server have a UI-related flag?"). Do we really want /ui to become an API?

lavalamp · 2017-11-20T17:54:46Z

cmd/kube-apiserver/app/options/options.go

@@ -188,6 +191,9 @@ func (s *ServerRunOptions) AddFlags(fs *pflag.FlagSet) {
 		"A port range to reserve for services with NodePort visibility. "+
 		"Example: '30000-32767'. Inclusive at both ends of the range.")

+	fs.StringVar(&s.UIRedirectUrl, "ui-redirect-url", s.UIRedirectUrl, ""+
+		"URL of the application serving the UI for the cluster.")


Suggestion: provide a way to disable this. Setting it to the empty string?

lavalamp · 2017-11-20T17:56:27Z

pkg/master/master.go

@@ -302,7 +303,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
 	}

 	if c.ExtraConfig.EnableUISupport {


Is EnableUISupport currently exposed in a flag?

no, and access to /ui is not allowed as part of standard policy, either

Didn't see it anywhere.

bryk · 2017-11-20T18:00:11Z

@liggitt

I'd rather see the apiserver become less entangled with UIs, not more ("why does my API server have a UI-related flag?"). Do we really want /ui to become an API?

I think it it already an API. There's a lot of blog posts, documentation, links out there that link to /ui. That's why we're having this PR.

liggitt · 2017-11-20T18:05:48Z

I think it it already an API. There's a lot of blog posts, documentation, links out there that link to /ui. That's why we're having this PR.

What is the contract of /ui? Is it required to exist? Is it required to redirect to kube-dashboard, or any UI?

Making the apiserver aware of a UI running against the apiserver seems like a mistake we should be moving away from, not doubling down on.

konryd · 2017-11-20T18:28:33Z

@liggitt

Should /ui get deprecated, a redirect to the replacement would be a way to introduce users to the change in a non-disruptive manner.

liggitt · 2017-11-20T18:32:26Z

Should /ui get deprecated, a redirect to the replacement would be a way to introduce users to the change in a non-disruptive manner.

/ui is based on assumptions that do not hold for many API servers (see description of #53766). Rather than trying to re-introduce it at the cost of re-entangling the apiserver with a UI layer, I'd prefer to remove the incorrect coupling and allow UI's to deploy on top of the API.

bryk · 2017-11-20T18:35:40Z

@liggitt
I believe the contract for /ui is "it will open Kubernetes UI", though it was never formalized. I'd love to get rid of it, but there is plenty of references to /ui all over internet and even books like "Kubernetes: Up and Running".

Just removing /ui would break flows of beginner users who follow some tutorial.

I think it'd be good to remove /ui endpoint long term. I don't believe we can do this for 1.9, though.

bryk · 2017-11-20T18:36:10Z

@kubernetes/dashboard-maintainers

bryk · 2017-11-20T18:39:01Z

@liggitt what do you believe would be needed to remove /ui endpoint? In terms of work items.

liggitt · 2017-11-20T18:45:00Z

I think it'd be good to remove /ui endpoint long term

I agree. Let's not add flags, config, and encourage new use of something we think should be removed.

@liggitt what do you believe would be needed to remove /ui endpoint? In terms of work items.

Announce deprecation
Merge Remove /ui/ redirect #53766 after deprecation period

k8s-ci-robot · 2017-11-20T19:13:41Z

@konryd: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel-test	`a753d55`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-e2e-gce	`a753d55`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

konryd · 2017-11-20T20:38:27Z

@liggitt @bryk @lavalamp

It looks like there's a general agreement on the need to deprecate the /ui endpoint, but we seem to disagree on the exact way of handling it.

Unless preceded with community outreach, cutting off the /ui will get perceived as abrupt, no matter how long the feature is deprecated. Setting this to a new location will let us move away from it while not surprising users.

liggitt · 2017-11-20T21:01:10Z

Setting this to a new location will let us move away from it while not surprising users.

Adding config/flags and encouraging new use of it is the opposite of deprecating, and will make it harder to move away from.

lavalamp · 2017-11-21T00:52:27Z

I think I prefer #53766 to this PR. @liggitt can you make sure a note about deprecation ends up in the 1.9 release notes, and put some comments in the code about when it is safe to remove this feature?

(Points that swayed me: /ui isn't safe for https, and a flag is the wrong way to configure something that all apiservers have to be in sync on.)

@konryd I am sorry I don't have a work around for you. You can investigate installing something in the place of the service which does a redirection. I can't promise it will work, though. It is too late in the release process to provide a real solution.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 20, 2017

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 20, 2017

k8s-github-robot assigned liggitt and jszczepkowski Nov 20, 2017

k8s-ci-robot assigned mwielgus and sttts Nov 20, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 20, 2017

lavalamp reviewed Nov 20, 2017

View reviewed changes

Changed the ui redirect path from a constant to a flag

a753d55

lavalamp closed this Nov 21, 2017

liggitt mentioned this pull request Nov 27, 2017

Update Dashboard addon to version 1.8.0 and align /ui redirect with it #53046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed the ui redirect path from a constant to a flag #56074

Changed the ui redirect path from a constant to a flag #56074

konryd commented Nov 20, 2017

k8s-ci-robot commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

konryd commented Nov 20, 2017

lavalamp Nov 20, 2017

lavalamp Nov 20, 2017

lavalamp Nov 20, 2017

konryd Nov 20, 2017

lavalamp Nov 20, 2017

konryd Nov 20, 2017

lavalamp Nov 20, 2017

konryd Nov 20, 2017

lavalamp Nov 20, 2017

konryd Nov 20, 2017

lavalamp commented Nov 20, 2017

liggitt commented Nov 20, 2017 •

edited

lavalamp Nov 20, 2017

lavalamp Nov 20, 2017

liggitt Nov 20, 2017

konryd Nov 20, 2017

bryk commented Nov 20, 2017

liggitt commented Nov 20, 2017 •

edited

konryd commented Nov 20, 2017 •

edited

liggitt commented Nov 20, 2017

bryk commented Nov 20, 2017

bryk commented Nov 20, 2017

bryk commented Nov 20, 2017

liggitt commented Nov 20, 2017

k8s-ci-robot commented Nov 20, 2017

konryd commented Nov 20, 2017

liggitt commented Nov 20, 2017

lavalamp commented Nov 21, 2017

		@@ -302,7 +303,7 @@ func (c completedConfig) New(delegationTarget genericapiserver.DelegationTarget)
		}

		if c.ExtraConfig.EnableUISupport {

Changed the ui redirect path from a constant to a flag #56074

Changed the ui redirect path from a constant to a flag #56074

Conversation

konryd commented Nov 20, 2017

k8s-ci-robot commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

konryd commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Nov 20, 2017

liggitt commented Nov 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryk commented Nov 20, 2017

liggitt commented Nov 20, 2017 • edited

konryd commented Nov 20, 2017 • edited

liggitt commented Nov 20, 2017

bryk commented Nov 20, 2017

bryk commented Nov 20, 2017

bryk commented Nov 20, 2017

liggitt commented Nov 20, 2017

k8s-ci-robot commented Nov 20, 2017

konryd commented Nov 20, 2017

liggitt commented Nov 20, 2017

lavalamp commented Nov 21, 2017

liggitt commented Nov 20, 2017 •

edited

liggitt commented Nov 20, 2017 •

edited

konryd commented Nov 20, 2017 •

edited