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

Remove /ui/ redirect #53766

Merged
merged 1 commit into from Feb 13, 2018

Conversation

@liggitt
Copy link
Member

liggitt commented Oct 12, 2017

The existing kube-apiserver hard-codes /ui to redirect to an optional add-on, which is not appropriate. It does not work in the following scenarios:

  • https-enabled dashboards
  • the dashboard is deployed to a different namespace or service name
  • the dashboard is not installed at all
  • authorization is enabled and does not allow access to /ui

This PR removes the hard-coded /ui redirect.

apiserver: the /ui kube-dashboard redirect has been removed. Follow instructions specific to your deployment to access kube-dashboard
@@ -270,7 +269,7 @@ func (cfg *Config) Complete(informers informers.SharedInformerFactory) Completed
}

// enable swagger UI only if general UI support is on

This comment has been minimized.

@dixudx

dixudx Oct 12, 2017

Member

Remove this comment?

@maciaszczykm

This comment has been minimized.

Copy link
Member

maciaszczykm commented Oct 12, 2017

I have opened #53046 to fix this issue without /ui removal. It is not merged yet because it is blocked by #53382. This will decrease Dashboard discoverability a lot.

@kubernetes/dashboard-maintainers

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Oct 12, 2017

I would also prefer to keep the redirect if we manage to fix it. As this is a public interface, we would have to deprecate that anyway, e.g. in the changelogs.

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Oct 12, 2017

I would also prefer to keep the redirect if we manage to fix it

I don't think the redirect is appropriate... the API server shouldn't assume the presence of a particular add-on like kube-dashboard.

As this is a public interface, we would have to deprecate that anyway, e.g. in the changelogs.

Release note added

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from 205fc9c to 9a4938e Oct 12, 2017

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Oct 13, 2017

I would also prefer to keep the redirect if we manage to fix it

We can easily fix the logic and make redirect smarter (similar to logic in this PR). Many people got used to it and are often using it to access Dashboard. We can see that based on number of issues that mention this redirect.

@maciaszczykm

This comment has been minimized.

Copy link
Member

maciaszczykm commented Oct 13, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Oct 13, 2017

We can easily fix the logic and make redirect smarter (similar to logic in this PR). Many people got used to it and are often using it to access Dashboard. We can see that based on number of issues that mention this redirect.

kube-dashboard is not part of core, it's an add-on. I don't think it is appropriate to hardcode UI-oriented redirects for optional add-ons into the API server

@ericchiang

This comment has been minimized.

Copy link
Member

ericchiang commented Oct 13, 2017

We've had customers notice issues like this in the past, e.g. kubectl top breaking because we didn't ship heapster by default. If a customer sees something that looks broken, that reflects badly on us, even if it was because we chose not to ship an optional add-on.

+1 to removing this. The fewer add-ons core assumes are running, the better.

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from 9a4938e to 638f00f Oct 13, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Oct 13, 2017

moved the cluster-info change to #53897 and limited this to the UI redirect change

@liggitt liggitt changed the title Remove /ui/ redirect, output port name and guessed scheme in cluster-info Remove /ui/ redirect Oct 13, 2017

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from 638f00f to f05766a Oct 17, 2017

@liggitt liggitt force-pushed the liggitt:ui-redirect branch 3 times, most recently from 72d60d7 to a86bd5c Oct 17, 2017

@derekwaynecarr

This comment has been minimized.

Copy link
Member

derekwaynecarr commented Oct 18, 2017

/test pull-kubernetes-unit

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from a86bd5c to 51896db Oct 18, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 27, 2017

Updating documentation to remove all /ui references is a prerequisite to this, I think. We can replace the /ui references with direct service proxy link.

There are no references to /ui in the kubernetes/website, kubernetes/kubernetes, or kubernetes/examples. The only documentation reference I found in the kubernetes org was in kubernetes/dashboard. Opened kubernetes/dashboard#2620 to update that.

@liggitt liggitt added this to the v1.10 milestone Nov 27, 2017

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from a6b2113 to 6892af3 Dec 14, 2017

@kubernetes kubernetes deleted a comment from k8s-github-robot Dec 14, 2017

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Dec 15, 2017

/retest

@cdenneen

This comment has been minimized.

Copy link

cdenneen commented Jan 18, 2018

Why can't the redirect just redirect to new URL?

@liggitt liggitt force-pushed the liggitt:ui-redirect branch from 6892af3 to f8e206e Feb 12, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Feb 12, 2018

@bryk @floreks updated to remove /ui/ redirect as discussed in #53046 (comment)
PTAL

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Feb 13, 2018

/lgtm

@floreks

This comment has been minimized.

Copy link
Member

floreks commented Feb 13, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, liggitt, sttts

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

The pull request process is described here

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

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 13, 2018

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 13, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 13, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 13, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit fd553ca into kubernetes:master Feb 13, 2018

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@liggitt liggitt deleted the liggitt:ui-redirect branch Feb 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.