Merge pull request #2387 from fabianofranz/issues_2358

Merged by openshift-bot

hack/test-cmd.sh

@@ -171,17 +171,27 @@ if [[ "${API_SCHEME}" == "https" ]]; then
 fi
 
 # login and logout tests
+# --token and --username are mutually exclusive
 [ "$(osc login ${KUBERNETES_MASTER} -u test-user --token=tmp --insecure-skip-tls-verify 2>&1 | grep 'mutually exclusive')" ]
+# must only accept one arg (server)
 [ "$(osc login https://server1 https://server2.com 2>&1 | grep 'Only the server URL may be specified')" ]
+# logs in with a valid certificate authority
 osc login ${KUBERNETES_MASTER} --certificate-authority="${MASTER_CONFIG_DIR}/ca.crt" -u test-user -p anything
 osc logout
+# logs in skipping certificate check
 osc login ${KUBERNETES_MASTER} --insecure-skip-tls-verify -u test-user -p anything
+# logs in by an existing and valid token
 temp_token=$(osc config view -o template --template='{{range .users}}{{ index .user.token }}{{end}}')
 [ "$(osc login --token=${temp_token} 2>&1 | grep 'using the token provided')" ]
 osc logout
-osc login --server=${KUBERNETES_MASTER} --certificate-authority="${MASTER_CONFIG_DIR}/ca.crt" -u test-user -p anything
+# properly parse server port
+[ "$(osc login https://server1:844333 2>&1 | grep 'Not a valid port')" ]
+# properly handle trailing slash
+osc login --server=${KUBERNETES_MASTER}/ --certificate-authority="${MASTER_CONFIG_DIR}/ca.crt" -u test-user -p anything
+# create a new project
 osc new-project project-foo --display-name="my project" --description="boring project description"
 [ "$(osc project | grep 'Using project "project-foo"')" ]
+# denies access after logging out
 osc logout
 [ -z "$(osc get pods | grep 'system:anonymous')" ]
 

pkg/cmd/cli/cmd/login.go

@@ -142,11 +142,15 @@ func (o *LoginOptions) Complete(f *osclientcmd.Factory, cmd *cobra.Command, args
  o.StartingKubeConfig = kclientcmdapi.NewConfig()
  }
 
+ addr := flagtypes.Addr{Value: "localhost:8443", DefaultScheme: "https", DefaultPort: 8443, AllowPrefix: true}.Default()
+
  if serverFlag := kcmdutil.GetFlagString(cmd, "server"); len(serverFlag) > 0 {
- o.Server = serverFlag
+ if err := addr.Set(serverFlag); err != nil {
+ return err
+ }
+ o.Server = addr.String()
 
  } else if len(args) == 1 {
- addr := flagtypes.Addr{Value: "localhost:8443", DefaultScheme: "https", DefaultPort: 8443, AllowPrefix: true}.Default()
  if err := addr.Set(args[0]); err != nil {
  return err
  }
@@ -158,7 +162,6 @@ func (o *LoginOptions) Complete(f *osclientcmd.Factory, cmd *cobra.Command, args
  o.Server = cluster.Server
  }
  }
-
  }
 
  if certFile := kcmdutil.GetFlagString(cmd, "client-certificate"); len(certFile) > 0 {

pkg/cmd/cli/cmd/loginoptions.go

@@ -46,23 +46,26 @@ func (o *LoginOptions) getClientConfig() (*kclient.Config, error) {
 
  clientConfig := &kclient.Config{}
 
- // if someone specified a server, use it as the default
- if len(o.Server) > 0 {
- clientConfig.Host = o.Server
-
- } else {
+ if len(o.Server) == 0 {
  // we need to have a server to talk to
  if cmdutil.IsTerminal(o.Reader) {
  for !o.serverProvided() {
  defaultServer := defaultClusterURL
  promptMsg := fmt.Sprintf("OpenShift server [%s]: ", defaultServer)
 
  o.Server = cmdutil.PromptForStringWithDefault(o.Reader, defaultServer, promptMsg)
- clientConfig.Host = o.Server
  }
  }
  }
 
+ // normalize the provided server to a format expected by config
+ serverNormalized, err := config.NormalizeServerURL(o.Server)
+ if err != nil {
+ return nil, err
+ }
+ o.Server = serverNormalized
+ clientConfig.Host = o.Server
+
  if len(o.CAFile) > 0 {
  clientConfig.CAFile = o.CAFile
 

pkg/cmd/cli/config/helpers.go

@@ -1,11 +1,17 @@
 package config
 
 import (
+ "fmt"
+ "net"
+ "net/url"
+ "strconv"
+ "strings"
+
  clientcmdapi "github.com/GoogleCloudPlatform/kubernetes/pkg/client/clientcmd/api"
  "github.com/openshift/origin/pkg/cmd/util"
 )
 
-// TODO should me moved upstream
+// TODO should be moved upstream
 func RelativizeClientConfigPaths(cfg *clientcmdapi.Config, base string) (err error) {
  for k, cluster := range cfg.Clusters {
  if len(cluster.CertificateAuthority) > 0 {
@@ -39,3 +45,59 @@ func RelativizeClientConfigPaths(cfg *clientcmdapi.Config, base string) (err err
  }
  return nil
 }
+
+var validURLSchemes = []string{"https://", "http://", "tcp://"}
+
+// Opinionated normalization of a string that represents a URL. Returns the URL provided matching the format
+// expected when storing a URL in a config. Sets a scheme and port if not present, removes unnecessary trailing
+// slashes, etc. Can be used to normalize a URL provided by user input.
+func NormalizeServerURL(s string) (string, error) {
+ // normalize scheme
+ if !hasScheme(s) {
+ s = validURLSchemes[0] + s
+ }
+
+ addr, err := url.Parse(s)
+ if err != nil {
+ return "", fmt.Errorf("Not a valid URL: %v.", err)
+ }
+
+ // normalize host:port
+ if strings.Contains(addr.Host, ":") {
+ _, port, err := net.SplitHostPort(addr.Host)
+ if err != nil {
+ return "", fmt.Errorf("Not a valid host:port: %v.", err)
+ }
+ _, err = strconv.ParseUint(port, 10, 16)
+ if err != nil {
+ return "", fmt.Errorf("Not a valid port: %v. Port numbers must be between 0 and 65535.", port)
+ }
+ } else {
+ port := 0
+ switch addr.Scheme {
+ case "http":
+ port = 80
+ case "https":
+ port = 443
+ default:
+ return "", fmt.Errorf("No port specified.")
+ }
+ addr.Host = net.JoinHostPort(addr.Host, strconv.FormatInt(int64(port), 10))
+ }
+
+ // remove trailing slash if that's the only path we have
+ if addr.Path == "/" {
+ addr.Path = ""
+ }
+
+ return addr.String(), nil
+}
+
+func hasScheme(s string) bool {
+ for _, p := range validURLSchemes {
+ if strings.HasPrefix(s, p) {
+ return true
+ }
+ }
+ return false
+}