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

Enable to switch context in kubeconfig #4534

Closed
wants to merge 1 commit into from

Conversation

shu-mutou
Copy link
Contributor

@shu-mutou shu-mutou commented Nov 13, 2019

When user is logging in with kubeconfig that have multiple contexts, add buttons for the contexts into user menu and enable to switch contexts.

This solution assumes that dashboard-metrics-scraper is deployed on each cluster.
To get metrics from dashboard-metrics-scraper in each cluster, add sidecarHost: [endpoint URL for scraper] into cluster in kubeconfig like follow:

apiVersion: v1
clusters:
- cluster:
    certificate-authority-data: xxxxxxxxxxxxx
    server: http://192.168.100.13:8001
    sidecar-host: http://192.168.100.13:30080
  name: minikube
- cluster:
    certificate-authority-data: xxxxxxxxxxxxx
    server: http://192.168.100.14:8001
    sidecar-host: http://192.168.100.14:30080
  name: minikube2
contexts:
- context:
    cluster: minikube2
    user: minikube2-admin
  name: minikube2-admin@minikube2
- context:
    cluster: minikube
    user: minikube-admin
  name: minikube-admin@minikube
current-context: minikube-admin@minikube
kind: Config
preferences: {}
users:
- name: minikube-admin
  user:
    client-certificate-data: xxxxxxxxxxxxx
    client-key-data: xxxxxxxxxxxxx
    token: xxxxxxxxxxxxx
- name: minikube2-admin
  user:
    client-certificate-data: xxxxxxxxxxxxx
    client-key-data: xxxxxxxxxxxxx
    token: xxxxxxxxxxxxx

Endpoints for server and sidecarHost would be passed as cookies and clients for them would be created dynamically in server side.

As first step, this implements basic architecture. Config editor and etc. would be implemented in future PR.

Note: To test this with npm run start*, enlarge maximum http header size with export NODE_OPTIONS=--max-http-header-size=102400.

Partial implements #4522

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 13, 2019
@k8s-ci-robot k8s-ci-robot added language/fr Updates or issues for French translations. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. labels Nov 13, 2019
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2019
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #4534 into master will decrease coverage by 0.49%.
The diff coverage is 28.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4534      +/-   ##
==========================================
- Coverage   45.45%   44.96%   -0.50%     
==========================================
  Files         214      216       +2     
  Lines       10208    10449     +241     
  Branches      108      131      +23     
==========================================
+ Hits         4640     4698      +58     
- Misses       5293     5471     +178     
- Partials      275      280       +5     
Impacted Files Coverage Δ
src/app/backend/auth/api/types.go 33.33% <ø> (ø)
src/app/backend/auth/basic.go 0.00% <0.00%> (ø)
src/app/backend/auth/handler.go 35.21% <0.00%> (-7.90%) ⬇️
src/app/backend/handler/apihandler.go 27.74% <0.00%> (ø)
src/app/frontend/index.config.ts 100.00% <ø> (ø)
.../frontend/common/services/global/authentication.ts 32.94% <8.00%> (-5.95%) ⬇️
.../app/frontend/common/services/global/kubeconfig.ts 19.52% <19.52%> (ø)
src/app/backend/integration/metric/manager.go 44.44% <20.00%> (-5.56%) ⬇️
src/app/backend/client/manager.go 54.68% <40.00%> (-1.33%) ⬇️
src/app/frontend/login/component.ts 82.14% <66.66%> (+0.89%) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a83047e...5987cd4. Read the comment docs.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 29, 2019
@shu-mutou shu-mutou changed the title [WIP] Enable to switch context on kubeconfig [WIP] Enable to switch context in kubeconfig Nov 29, 2019
@shu-mutou shu-mutou requested review from floreks, maciaszczykm and jeefy and removed request for atoato88 and feloy January 8, 2020 06:32
@shu-mutou shu-mutou changed the title [WIP] Enable to switch context in kubeconfig Enable to switch context in kubeconfig Jan 8, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 8, 2020
@maciaszczykm
Copy link
Member

We will go back to this after the release, please ping if I will forget.

const clusters = conf.clusters.filter(cluster => {
return cluster.name === name;
});
if (!clusters.length) {
Copy link
Member

Choose a reason for hiding this comment

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

Please use !_.isEmpty() to check for clusters list emptiness. This is lodash utility function that can be imported with following line import * as _ from 'lodash';.

return ctx.name === context;
})
) {
conf['current-context'] = context;
Copy link
Member

Choose a reason for hiding this comment

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

As before, on known types please use dot to access object params.

apiVersion: string;
clusters: Cluster[];
contexts: Context[];
'current-context': string;
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to escape it. Can we change it to camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that js-yaml can not convert keys to camelcase. Instead I found a library camelcase-keys but it need to use require in import. Do you know better library or is it acceptable, i.e. using require?

Copy link
Member

Choose a reason for hiding this comment

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

You can use require.

@maciaszczykm
Copy link
Member

@floreks What do you think about the usability and approach here? I have done initial code review so far but we should also consider if that will not impact our current setup.

import {KdError} from '@api/frontendapi';
import {Config, CONFIG_DI_TOKEN} from '../../../index.config';

type Cluster = {
Copy link
Member

Choose a reason for hiding this comment

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

Those are backend api types. Move it to the backendapi.ts.

type User = {
name: string;
user: {
token: string;
Copy link
Member

Choose a reason for hiding this comment

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

We can't store token here as it is already part of our login mechanism and is stored in an encrypted cookie. This would be extremely insecure. Also, we do not support client-certificate-data as a way of logging in anyway, so this is obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we store all the tokens contained in kubeconfig in cookies, I'm worried that all HTTP requests will get too big and impact performance.
I'm considering to encrypt tokens in jwe and store them into cookie before login sequence. But do you have any other ideas?

@maciaszczykm
Copy link
Member

I have tested it locally using npm run start:prod command and the context are visible after logging with kubeconfig. However, after I try to switch context I see an error in the console (POST https://localhost:8080/api/v1/login 500) and I am redirected to the login view.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2020
@shu-mutou
Copy link
Contributor Author

Thank you very much for your reviews! I will continue considering pointed things, check and modify this next week.

@shu-mutou
Copy link
Contributor Author

I have tested it locally using npm run start:prod command and the context are visible after logging with kubeconfig. However, after I try to switch context I see an error in the console (POST https://localhost:8080/api/v1/login 500) and I am redirected to the login view.

I checked npm run start:prod and it works in my environment. Could you check your kubeconfig?

@maciaszczykm
Copy link
Member

I will try to verify again tomorrow but it seemed okay as I was able to login with it. Is some additional info required?

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2020
@shu-mutou
Copy link
Contributor Author

@maciaszczykm I improved my codes, but I'm considering about encrypting tokens with jwe and store them into cookie yet. But I'm happy if you try this UX.

@shu-mutou
Copy link
Contributor Author

/hold
need more investigations.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2020
When user is logged in with kubeconfig, add selections for contexts included in kubeconfig
into user menu and enable to switch contexts included in kubeconfig.

This commit implements basic functionalities for switching contexts,
so to set dashboard-metrics-scraper host for each `cluster`, you need to describe it
as `sidecarHost` in `cluster` directive in kubeconfig.

Note: To test this with `npm run start*`, enlarge maximum http header size with
`export NODE_OPTIONS=--max-http-header-size=102400`.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shu-mutou

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 files:

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

@shu-mutou
Copy link
Contributor Author

/hold cancel
All tokens are stored in cookie as jweTokens now.
Instead, please note that set env NODE_OPTIONS=--max-http-header-size=1024000 when run npm run start*.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
@floreks
Copy link
Member

floreks commented Jul 9, 2020

The solution that requires us to raise limits for HTTP header size is not really acceptable. We should try to limit data passed between browser and server as much as possible. Most proxies set their limits to 8KB and many people are using that. This change would break many configurations.

Processing all tokens at all times is also not good. Only one should be used at the time and others should naturally expire over time.

Another issue is encrypting all auth information to all servers with a single encryption key that is stored in the main cluster. We should not do that. It decreases security and makes all servers vulnerable in case of a single cluster breach.

There are a couple of more issues, but I think the general approach should be reconsidered. First, we should have some architectural concepts prepared. Possibly something like that should be handled in the user settings and we should have an option to add additional servers that we want to access. We could store some data in secrets/config maps and use multiple encryption tokens for actual auth info.

@shu-mutou
Copy link
Contributor Author

As I expected, the HTTP header size exceeded the limit.
Let's redesign token handling.
If we store sensitive informations into secrets, do you have any idea how to clear expired secrets?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2020
@k8s-ci-robot
Copy link
Contributor

@shu-mutou: PR needs rebase.

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.

@floreks
Copy link
Member

floreks commented Nov 12, 2020

Closing as stale. New PR can be opened as it requires major changes.

@floreks floreks closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/de Updates or issues for German translations. language/fr Updates or issues for French translations. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants