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

Add cluster login cmd #18

Merged
merged 1 commit into from Jun 12, 2019

Conversation

Projects
None yet
6 participants
@wanghaoran1988
Copy link
Contributor

commented Jun 6, 2019

This add cmd : uhc cluster login <cluster_id> to oc login to openshift cluster.
Should be helpful when you try to access the cluster by id.

@wanghaoran1988 wanghaoran1988 force-pushed the wanghaoran1988:add_cluster_login branch from a5ad712 to 90b40c0 Jun 6, 2019

@wanghaoran1988

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

This error didn't happen on my local env:

cmd/uhc/cluster/login/login.go:128: G204: Subprocess launched with variable (gosec)
	if err := syscall.Exec(path, append([]string{path, "login"}, url), os.Environ()); err != nil {
make: *** [lint] Error 1

My golint version:
golangci-lint --version
golangci-lint has version 1.15.0 built from 901cf25 on 2019-02-18T08:24:43Z

if len(url) == 0 {
fmt.Fprintf(os.Stderr, "cannot find the api url for cluster: %s", argv[0])
}
if err := syscall.Exec(path, append([]string{path, "login"}, url), os.Environ()); err != nil {

This comment has been minimized.

Copy link
@cblecker

cblecker Jun 6, 2019

Contributor

Is there a reason you're using syscall.Exec over os.Exec? Calling syscall directly is unsafe.

Something like should work without calling syscall directly:

import os/exec
cmd := exec.Command("oc", "login", url)
err := cmd.Run()
if err != nil {
  return "", err
}

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 7, 2019

Collaborator

I agree it is better to use os.Exec, please do it. However golangci-lint will still complain. To avoid that add a comment like this before the os.Exec line:

// #nosec G204
@aweiteka

This comment has been minimized.

Copy link

commented Jun 7, 2019

@aweiteka

This comment has been minimized.

Copy link

commented Jun 7, 2019

+1 to the use case. I was considering the same thing.

@jhernand
Copy link
Collaborator

left a comment

Very nice @wanghaoran1988, thanks! Please address the review comments and I will merge.

// Get the resource that manages the cluster that we want to display:
clusterResource := resource.Cluster(argv[0])

// Retrieve the collection of clusters:

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 7, 2019

Collaborator

This comment ^ isn't accurate, you are retrieving one individual cluster, not the collection.

}

cluster := response.Body()
// Get the cluster api for login

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 7, 2019

Collaborator

I try to keep a style of comments consistent across the project: they start with uppercase, end with punctuation and have a blank like before. So in this case it would be:

...
}

// Get cluster API for login:
url = cluster.API().URL()
...

Can you use the same style?

// Get the cluster api for login
url := cluster.API().URL()
if len(url) == 0 {
fmt.Fprintf(os.Stderr, "cannot find the api url for cluster: %s", argv[0])

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 7, 2019

Collaborator

Messages shown to the user should start with uppercase and end with new line, so this should be:

fmt.Fprintf(os.Stderr(os.Stderr, "Can't ... cluster: %s\n", argv[0])

Also you will probably want to os.Exit(1) when this happens.

if len(url) == 0 {
fmt.Fprintf(os.Stderr, "cannot find the api url for cluster: %s", argv[0])
}
if err := syscall.Exec(path, append([]string{path, "login"}, url), os.Environ()); err != nil {

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 7, 2019

Collaborator

I agree it is better to use os.Exec, please do it. However golangci-lint will still complain. To avoid that add a comment like this before the os.Exec line:

// #nosec G204
@jhernand

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

@wanghaoran1988 another thing to consider, but not for this patch, is that if you have the right permissions the API will give you access to the kubeadmin password and to the kubeconfig file of the cluster. Using that, when available, it would be possible to login to the cluster without having to type any user name or password.

@aweiteka

This comment has been minimized.

Copy link

commented Jun 7, 2019

if you have the right permissions the API will give you access to the kubeadmin password and to the kubeconfig file of the cluster. Using that, when available, it would be possible to login to the cluster without having to type any user name or password.

We want to discourage logging in via kubeadmin user. In fact, most SREs will not have access to it due to compliance requirements. Kubeadmin will be reserved for a "break glass" scenario.

@jhernand

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2019

We want to discourage logging in via kubeadmin user. In fact, most SREs will not have access to it due to compliance requirements. Kubeadmin will be reserved for a "break glass" scenario.

I understand, and agree.

@wanghaoran1988 wanghaoran1988 force-pushed the wanghaoran1988:add_cluster_login branch from 90b40c0 to b773e19 Jun 11, 2019

ocCmd.Stderr = os.Stderr
ocCmd.Stdin = os.Stdin
ocCmd.Stdout = os.Stdout
err = ocCmd.Run()

This comment has been minimized.

Copy link
@fahlmant

fahlmant Jun 11, 2019

Contributor

oc login $URL prompts you for a username and password. I haven't tested your code directly, but does this allow for interactive input?

This comment has been minimized.

Copy link
@wanghaoran1988

wanghaoran1988 Jun 11, 2019

Author Contributor

@fahlmant Yes, that's how the line 132,133 works.

@wanghaoran1988

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

@jhernand Comments addressed, could you take a look?

@cblecker

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Tested this locally, and it works as expected :)


func init() {
flags := Cmd.Flags()
flags.BoolVar(&args.debug, "debug", false, "Enable debug mode.")

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Pleae put each argument to these methods in a separate line, as we do in all the other commands:

flags.BoolBar(
    &args.debug,
    "debug",
    false,
    "Enable debug mode.",
)

This comment has been minimized.

Copy link
@wanghaoran1988

wanghaoran1988 Jun 12, 2019

Author Contributor

done

@wanghaoran1988 wanghaoran1988 force-pushed the wanghaoran1988:add_cluster_login branch from dcd2b1b to b7e89f4 Jun 12, 2019

@jhernand
Copy link
Collaborator

left a comment

Looks good @wanghaoran1988. My only relevant comment is that I think we should remove the password option, as passing passwords in the command line isn't safe, in general.

I have also other minor requests.

flags := Cmd.Flags()
flags.BoolVar(&args.debug, "debug", false, "Enable debug mode.")
flags.StringVarP(&args.user, "username", "u", "", "Username, will prompt if not provided")
flags.StringVarP(&args.pass, "password", "p", "", "Password, will prompt if not provided")

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Passing the password in the command line is a bad practice. You will probably be asked to remove it in an hypothetical security review. I have been asked to do that in the past, in other projects. Can you remove it?

This comment has been minimized.

Copy link
@wanghaoran1988

wanghaoran1988 Jun 12, 2019

Author Contributor

sure

flags.BoolVar(&args.debug, "debug", false, "Enable debug mode.")
flags.StringVarP(&args.user, "username", "u", "", "Username, will prompt if not provided")
flags.StringVarP(&args.pass, "password", "p", "", "Password, will prompt if not provided")
}

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Add a blank line here.

if err != nil {
fmt.Fprint(os.Stderr, "To run this, you need install openshfit oc first.\n")
os.Exit(1)
}

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Add a blank line here.

os.Exit(1)
}

// Create the connection:

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

This comment isn't accurate.

fmt.Fprintf(os.Stderr, "Can't retrieve clusters: %s", err)
os.Exit(1)
}

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Remove this blank line ...

os.Exit(1)
}

cluster := response.Body()

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

... and add it here.

}
ocArgs := []string{}
ocArgs = append(ocArgs, "login", url)
if len(args.user) != 0 {

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

I think that args != "" is clearer, consider using that instead of len(args.user) != 0.

ocCmd.Stdout = os.Stdout
err = ocCmd.Run()
if err != nil {
fmt.Fprintf(os.Stderr, "Failed to login to cluster: %s", err)

This comment has been minimized.

Copy link
@jhernand

jhernand Jun 12, 2019

Collaborator

Add os.Exit(1).

@wanghaoran1988 wanghaoran1988 force-pushed the wanghaoran1988:add_cluster_login branch from b7e89f4 to e2fc42d Jun 12, 2019

@wanghaoran1988

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@jhernand Comments addressed.

@jhernand

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

Looks good, thanks @wanghaoran1988. Before merging can you please rebase and squash to one commit?

@jhernand

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

[test]

1 similar comment
@jmelis

This comment has been minimized.

Copy link

commented Jun 12, 2019

[test]

@jhernand

This comment has been minimized.

Copy link
Collaborator

commented Jun 12, 2019

@wanghaoran1988 now that tests pass I will merge as soon as you squash to one commit. Then I will do a new release.

@wanghaoran1988 wanghaoran1988 force-pushed the wanghaoran1988:add_cluster_login branch from e2fc42d to b6a8f14 Jun 12, 2019

@wanghaoran1988

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

@jhernand squashed

@jhernand jhernand merged commit 1a4a6a0 into openshift-online:master Jun 12, 2019

1 check passed

ci.ext.devshift.net PR build
Details
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.