This is an automated email from the ASF dual-hosted git repository. dubeejw pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-openwhisk.git
The following commit(s) were added to refs/heads/master by this push: new ac99eed Don't assume apihost is https for sdk and action urls (#2748) ac99eed is described below commit ac99eed848e2f300e30ff7b0ff7d11f3aeb1852b Author: Ben Browning <ben...@gmail.com> AuthorDate: Wed Sep 20 15:21:51 2017 -0400 Don't assume apihost is https for sdk and action urls (#2748) * Don't assume apihost is https for sdk and action urls Reuse the getURLBase utility method when computing the URL for sdk downloads and action URLs. This fixes #2720 and fixes #2719. * Cleanup some trailing whitespace I missed * Missed this import in last-second rebase * Update debug messages to match `GetURLBase` method name --- .../src/test/scala/system/basic/WskSdkTests.scala | 19 +++++++++- .../whisk/core/cli/test/WskBasicUsageTests.scala | 42 ++++++++++++++++++---- tools/cli/go-whisk-cli/commands/action.go | 8 ++++- tools/cli/go-whisk-cli/commands/commands.go | 4 +-- tools/cli/go-whisk-cli/commands/property.go | 8 ++--- tools/cli/go-whisk-cli/commands/util.go | 24 ------------- tools/cli/go-whisk/whisk/action.go | 20 +++++++---- tools/cli/go-whisk/whisk/sdk.go | 6 +++- tools/cli/go-whisk/whisk/util.go | 24 +++++++++++++ 9 files changed, 108 insertions(+), 47 deletions(-) diff --git a/tests/src/test/scala/system/basic/WskSdkTests.scala b/tests/src/test/scala/system/basic/WskSdkTests.scala index 62c8d60..df550b8 100644 --- a/tests/src/test/scala/system/basic/WskSdkTests.scala +++ b/tests/src/test/scala/system/basic/WskSdkTests.scala @@ -24,8 +24,8 @@ import scala.collection.JavaConversions.asScalaBuffer import org.apache.commons.io.FileUtils import org.junit.runner.RunWith import org.scalatest.junit.JUnitRunner - import common.TestHelpers +import common.TestUtils.ERROR_EXIT import common.TestUtils.SUCCESS_EXIT import common.WhiskProperties import common.Wsk @@ -40,6 +40,23 @@ class WskSdkTests extends TestHelpers with WskTestHelpers { behavior of "Wsk SDK" + it should "prefix https to apihost if no scheme given" in { + val result = wsk.cli(Seq("--apihost", "localhost:54321", "sdk", "install", "docker"), expectedExitCode = ERROR_EXIT) + result.stderr should include regex ("""(?i)Get https://localhost:54321/""") + } + + it should "not prefix https to http apihost" in { + val result = + wsk.cli(Seq("--apihost", "http://localhost:54321", "sdk", "install", "docker"), expectedExitCode = ERROR_EXIT) + result.stderr should include regex ("""(?i)Get http://localhost:54321/""") + } + + it should "not double prefix https to https apihost" in { + val result = + wsk.cli(Seq("--apihost", "https://localhost:54321", "sdk", "install", "docker"), expectedExitCode = ERROR_EXIT) + result.stderr should include regex ("""(?i)Get https://localhost:54321/""") + } + it should "download docker action sdk" in { val dir = File.createTempFile("wskinstall", ".tmp") dir.delete() diff --git a/tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala b/tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala index c411705..c635959 100644 --- a/tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala +++ b/tests/src/test/scala/whisk/core/cli/test/WskBasicUsageTests.scala @@ -652,9 +652,10 @@ class WskBasicUsageTests extends TestHelpers with WskTestHelpers { val encodedPackageName = URLEncoder.encode(packageName, StandardCharsets.UTF_8.name).replace("+", "%20") val encodedWebActionName = URLEncoder.encode(webActionName, StandardCharsets.UTF_8.name).replace("+", "%20") val encodedNamespace = URLEncoder.encode(namespace, StandardCharsets.UTF_8.name).replace("+", "%20") - val actionPath = "https://%s/api/%s/namespaces/%s/actions/%s" + val scheme = "https" + val actionPath = "%s://%s/api/%s/namespaces/%s/actions/%s" val packagedActionPath = s"$actionPath/%s" - val webActionPath = "https://%s/api/%s/web/%s/%s/%s" + val webActionPath = "%s://%s/api/%s/web/%s/%s/%s" assetHelper.withCleaner(wsk.action, actionName) { (action, _) => action.create(actionName, defaultAction) @@ -677,25 +678,52 @@ class WskBasicUsageTests extends TestHelpers with WskTestHelpers { } wsk.action.get(actionName, url = Some(true)).stdout should include( - actionPath.format(wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedActionName)) + actionPath.format(scheme, wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedActionName)) // Ensure url flag works when a field filter and summary flag are specified wsk.action.get(actionName, url = Some(true), fieldFilter = Some("field"), summary = true).stdout should include( - actionPath.format(wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedActionName)) + actionPath.format(scheme, wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedActionName)) wsk.action.get(webActionName, url = Some(true)).stdout should include( webActionPath - .format(wskprops.apihost, wskprops.apiversion, encodedNamespace, defaultPackageName, encodedWebActionName)) + .format( + scheme, + wskprops.apihost, + wskprops.apiversion, + encodedNamespace, + defaultPackageName, + encodedWebActionName)) wsk.action.get(packagedAction, url = Some(true)).stdout should include( packagedActionPath - .format(wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedPackageName, encodedActionName)) + .format(scheme, wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedPackageName, encodedActionName)) wsk.action.get(packagedWebAction, url = Some(true)).stdout should include( webActionPath - .format(wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedPackageName, encodedWebActionName)) + .format( + scheme, + wskprops.apihost, + wskprops.apiversion, + encodedNamespace, + encodedPackageName, + encodedWebActionName)) wsk.action.get(nonExistentActionName, url = Some(true), expectedExitCode = NOT_FOUND) + + val httpsProps = WskProps(apihost = "https://" + wskprops.apihost) + wsk.action.get(actionName, url = Some(true))(httpsProps).stdout should include( + actionPath + .format("https", wskprops.apihost, wskprops.apiversion, encodedNamespace, encodedActionName)) + wsk.action.get(webActionName, url = Some(true))(httpsProps).stdout should include( + webActionPath + .format( + "https", + wskprops.apihost, + wskprops.apiversion, + encodedNamespace, + defaultPackageName, + encodedWebActionName)) + } it should "limit length of HTTP request and response bodies for --verbose" in withAssetCleaner(wskprops) { (wp, assetHelper) => diff --git a/tools/cli/go-whisk-cli/commands/action.go b/tools/cli/go-whisk-cli/commands/action.go index bfa6294..86b427b 100644 --- a/tools/cli/go-whisk-cli/commands/action.go +++ b/tools/cli/go-whisk-cli/commands/action.go @@ -229,10 +229,16 @@ var actionGetCmd = &cobra.Command{ } if flags.action.url { - actionURL := action.ActionURL(Properties.APIHost, + actionURL, err := action.ActionURL(Properties.APIHost, DefaultOpenWhiskApiPath, Properties.APIVersion, qualifiedName.GetPackageName()) + if err != nil { + errStr := wski18n.T("Invalid host address '{{.host}}': {{.err}}", + map[string]interface{}{"host": Properties.APIHost, "err": err}) + werr := whisk.MakeWskError(errors.New(errStr), whisk.EXIT_CODE_ERR_GENERAL, whisk.DISPLAY_MSG, whisk.NO_DISPLAY_USAGE) + return werr + } printActionGetWithURL(qualifiedName.GetEntity(), actionURL) } else if flags.common.summary { printSummary(action) diff --git a/tools/cli/go-whisk-cli/commands/commands.go b/tools/cli/go-whisk-cli/commands/commands.go index 6a93ee6..63ee37a 100644 --- a/tools/cli/go-whisk-cli/commands/commands.go +++ b/tools/cli/go-whisk-cli/commands/commands.go @@ -33,7 +33,7 @@ const DefaultOpenWhiskApiPath string = "/api" var UserAgent string = "OpenWhisk-CLI" func setupClientConfig(cmd *cobra.Command, args []string) (error){ - baseURL, err := getURLBase(Properties.APIHost, DefaultOpenWhiskApiPath) + baseURL, err := whisk.GetURLBase(Properties.APIHost, DefaultOpenWhiskApiPath) // Determine if the parent command will require the API host to be set apiHostRequired := (cmd.Parent().Name() == "property" && cmd.Name() == "get" && (flags.property.auth || @@ -45,7 +45,7 @@ func setupClientConfig(cmd *cobra.Command, args []string) (error){ // Display an error if the parent command requires an API host to be set, and the current API host is not valid if err != nil && !apiHostRequired { - whisk.Debug(whisk.DbgError, "getURLBase(%s, %s) error: %s\n", Properties.APIHost, DefaultOpenWhiskApiPath, err) + whisk.Debug(whisk.DbgError, "whisk.GetURLBase(%s, %s) error: %s\n", Properties.APIHost, DefaultOpenWhiskApiPath, err) errMsg := wski18n.T("The API host is not valid: {{.err}}", map[string]interface{}{"err": err}) whiskErr := whisk.MakeWskErrorFromWskError(errors.New(errMsg), err, whisk.EXIT_CODE_ERR_GENERAL, whisk.DISPLAY_MSG, whisk.NO_DISPLAY_USAGE) diff --git a/tools/cli/go-whisk-cli/commands/property.go b/tools/cli/go-whisk-cli/commands/property.go index 27ad367..4a6f9b9 100644 --- a/tools/cli/go-whisk-cli/commands/property.go +++ b/tools/cli/go-whisk-cli/commands/property.go @@ -106,11 +106,11 @@ var propertySetCmd = &cobra.Command{ } if apiHost := flags.property.apihostSet; len(apiHost) > 0 { - baseURL, err := getURLBase(apiHost, DefaultOpenWhiskApiPath) + baseURL, err := whisk.GetURLBase(apiHost, DefaultOpenWhiskApiPath) if err != nil { // Not aborting now. Subsequent commands will result in error - whisk.Debug(whisk.DbgError, "getURLBase(%s, %s) error: %s", apiHost, DefaultOpenWhiskApiPath, err) + whisk.Debug(whisk.DbgError, "whisk.GetURLBase(%s, %s) error: %s", apiHost, DefaultOpenWhiskApiPath, err) errStr := fmt.Sprintf( wski18n.T("Unable to set API host value; the API host value '{{.apihost}}' is invalid: {{.err}}", map[string]interface{}{"apihost": apiHost, "err": err})) @@ -537,10 +537,10 @@ func parseConfigFlags(cmd *cobra.Command, args []string) error { if client != nil { client.Config.Host = apiHost - baseURL, err := getURLBase(apiHost, DefaultOpenWhiskApiPath) + baseURL, err := whisk.GetURLBase(apiHost, DefaultOpenWhiskApiPath) if err != nil { - whisk.Debug(whisk.DbgError, "getURLBase(%s, %s) failed: %s\n", apiHost, DefaultOpenWhiskApiPath, err) + whisk.Debug(whisk.DbgError, "whisk.GetURLBase(%s, %s) failed: %s\n", apiHost, DefaultOpenWhiskApiPath, err) errStr := wski18n.T("Invalid host address '{{.host}}': {{.err}}", map[string]interface{}{"host": Properties.APIHost, "err": err}) werr := whisk.MakeWskError(errors.New(errStr), whisk.EXIT_CODE_ERR_GENERAL, whisk.DISPLAY_MSG, whisk.NO_DISPLAY_USAGE) diff --git a/tools/cli/go-whisk-cli/commands/util.go b/tools/cli/go-whisk-cli/commands/util.go index e8c422f..fa16168 100644 --- a/tools/cli/go-whisk-cli/commands/util.go +++ b/tools/cli/go-whisk-cli/commands/util.go @@ -34,7 +34,6 @@ import ( "compress/gzip" "archive/zip" "encoding/json" - "net/url" "io/ioutil" "sort" "reflect" @@ -815,29 +814,6 @@ func checkArgs(args []string, minimumArgNumber int, maximumArgNumber int, comman } } -func getURLBase(host string, path string) (*url.URL, error) { - if len(host) == 0 { - errMsg := wski18n.T("An API host must be provided.") - whiskErr := whisk.MakeWskError(errors.New(errMsg), whisk.EXIT_CODE_ERR_GENERAL, - whisk.DISPLAY_MSG, whisk.DISPLAY_USAGE) - return nil, whiskErr - } - - if !strings.HasPrefix(host, "http") { - host = "https://" + host - } - - urlBase := fmt.Sprintf("%s%s", host, path) - url, err := url.Parse(urlBase) - - if len(url.Scheme) == 0 || len(url.Host) == 0 { - urlBase = fmt.Sprintf("https://%s%s", host, path) - url, err = url.Parse(urlBase) - } - - return url, err -} - func normalizeNamespace(namespace string) (string) { if (namespace == "_") { namespace = wski18n.T("default") diff --git a/tools/cli/go-whisk/whisk/action.go b/tools/cli/go-whisk/whisk/action.go index 8b17d23..7284ae6 100644 --- a/tools/cli/go-whisk/whisk/action.go +++ b/tools/cli/go-whisk/whisk/action.go @@ -122,33 +122,39 @@ func (action Action) WebAction() (webExportValue bool) { Returns the URL of an action as a string. A valid API host, path and version must be passed. A package that contains the action must be passed as well. An empty string must be passed if the action is not packaged. */ -func (action Action) ActionURL(apiHost string, apiPath string, apiVersion string, pkg string) (actionURL string) { - webActionPath := "https://%s%s/%s/web/%s/%s/%s" - actionPath := "https://%s%s/%s/namespaces/%s/actions/%s" +func (action Action) ActionURL(apiHost string, apiPath string, apiVersion string, pkg string) (string, error) { + baseURL, err := GetURLBase(apiHost, apiPath) + if err != nil { + Debug(DbgError, "GetURLBase(%s, %s) failed: %s\n", apiHost, apiPath, err) + return "", err + } + webActionPath := "%s/%s/web/%s/%s/%s" + actionPath := "%s/%s/namespaces/%s/actions/%s" packagedActionPath := actionPath + "/%s" namespace := strings.Split(action.Namespace, "/")[0] namespace = strings.Replace(url.QueryEscape(namespace), "+", "%20", -1) name := strings.Replace(url.QueryEscape(action.Name), "+", "%20", -1) pkg = strings.Replace(url.QueryEscape(pkg), "+", "%20", -1) + var actionURL string if action.WebAction() { if len(pkg) == 0 { pkg = "default" } - actionURL = fmt.Sprintf(webActionPath, apiHost, apiPath, apiVersion, namespace, pkg, name) + actionURL = fmt.Sprintf(webActionPath, baseURL, apiVersion, namespace, pkg, name) Debug(DbgInfo, "Web action URL: %s\n", actionURL) } else { if len(pkg) == 0 { - actionURL = fmt.Sprintf(actionPath, apiHost, apiPath, apiVersion, namespace, name) + actionURL = fmt.Sprintf(actionPath, baseURL, apiVersion, namespace, name) Debug(DbgInfo, "Packaged action URL: %s\n", actionURL) } else { - actionURL = fmt.Sprintf(packagedActionPath, apiHost, apiPath, apiVersion, namespace, pkg, name) + actionURL = fmt.Sprintf(packagedActionPath, baseURL, apiVersion, namespace, pkg, name) Debug(DbgInfo, "Action URL: %s\n", actionURL) } } - return actionURL + return actionURL, nil } //////////////////// diff --git a/tools/cli/go-whisk/whisk/sdk.go b/tools/cli/go-whisk/whisk/sdk.go index 01e4f3b..df3178a 100644 --- a/tools/cli/go-whisk/whisk/sdk.go +++ b/tools/cli/go-whisk/whisk/sdk.go @@ -40,7 +40,11 @@ type SdkRequest struct { // Install artifact {component = docker || swift || iOS} func (s *SdkService) Install(relFileUrl string) (*http.Response, error) { - urlStr := fmt.Sprintf("https://%s/%s", s.client.Config.BaseURL.Host, relFileUrl) + baseURL := s.client.Config.BaseURL + // Remove everything but the scheme, host, and port + baseURL.Path, baseURL.RawQuery, baseURL.Fragment = "", "", "" + + urlStr := fmt.Sprintf("%s/%s", baseURL, relFileUrl) req, err := http.NewRequest("GET", urlStr, nil) if err != nil { diff --git a/tools/cli/go-whisk/whisk/util.go b/tools/cli/go-whisk/whisk/util.go index f29f1e0..cf576d8 100644 --- a/tools/cli/go-whisk/whisk/util.go +++ b/tools/cli/go-whisk/whisk/util.go @@ -22,6 +22,7 @@ import ( "fmt" "net/url" "reflect" + "strings" "github.com/fatih/color" "github.com/google/go-querystring/query" @@ -80,3 +81,26 @@ func PrintJSON(v interface{}) { output, _ := prettyjson.Marshal(v) fmt.Fprintln(color.Output, string(output)) } + +func GetURLBase(host string, path string) (*url.URL, error) { + if len(host) == 0 { + errMsg := wski18n.T("An API host must be provided.") + whiskErr := MakeWskError(errors.New(errMsg), EXIT_CODE_ERR_GENERAL, + DISPLAY_MSG, DISPLAY_USAGE) + return nil, whiskErr + } + + if !strings.HasPrefix(host, "http") { + host = "https://" + host + } + + urlBase := fmt.Sprintf("%s%s", host, path) + url, err := url.Parse(urlBase) + + if len(url.Scheme) == 0 || len(url.Host) == 0 { + urlBase = fmt.Sprintf("https://%s%s", host, path) + url, err = url.Parse(urlBase) + } + + return url, err +} -- To stop receiving notification emails like this one, please contact ['"commits@openwhisk.apache.org" <commits@openwhisk.apache.org>'].