improving ocm login and ocm list rh-region url resolution to reuse th…

…e url saved in config before falling back to api.openshift.com

refactoring gateway url resolution to a common function

creating tests for the urls.ResolveGatewayURL

ResolveGatewayURL now as a final step, tries to parse the resolved URL using url.ParseRequestURI and return an error if that fails, improved test coverage

tweak output for clarity

fixing lint errors

cmd/ocm/list/rhRegion/cmd.go

@@ -2,12 +2,21 @@ package rhRegion
 
 import (
 	"fmt"
+	"os"
+	"strings"
+	"text/tabwriter"
+
 	sdk "github.com/openshift-online/ocm-sdk-go"
 
 	"github.com/openshift-online/ocm-cli/pkg/config"
+	"github.com/openshift-online/ocm-cli/pkg/urls"
 	"github.com/spf13/cobra"
 )
 
+var args struct {
+	discoveryURL string
+}
+
 var Cmd = &cobra.Command{
 	Use:   "rh-regions",
 	Short: "List available OCM regions",
@@ -18,24 +27,40 @@ ocm list rh-regions`,
 	Hidden: true,
 }
 
+func init() {
+	flags := Cmd.Flags()
+	flags.StringVar(
+		&args.discoveryURL,
+		"discovery-url",
+		"",
+		"URL of the OCM API gateway. If not provided, will reuse the URL from the configuration "+
+			"file or "+sdk.DefaultURL+" as a last resort. The value should be a complete URL "+
+			"or a valid URL alias: "+strings.Join(urls.ValidOCMUrlAliases(), ", "),
+	)
+}
+
 func run(cmd *cobra.Command, argv []string) error {
-	// Load the configuration file:
-	cfg, err := config.Load()
+
+	cfg, _ := config.Load()
+
+	gatewayURL, err := urls.ResolveGatewayURL(args.discoveryURL, cfg)
 	if err != nil {
-		return fmt.Errorf("Can't load config file: %v", err)
-	}
-	if cfg == nil {
-		return fmt.Errorf("Not logged in, run the 'login' command")
+		return err
 	}
 
-	regions, err := sdk.GetRhRegions(cfg.URL)
+	fmt.Fprintf(os.Stdout, "Discovery URL: %s\n\n", gatewayURL)
+	regions, err := sdk.GetRhRegions(gatewayURL)
 	if err != nil {
 		return fmt.Errorf("Failed to get OCM regions: %w", err)
 	}
 
-	for regionName := range regions {
-		fmt.Println(regionName)
+	writer := tabwriter.NewWriter(os.Stdout, 0, 0, 2, ' ', tabwriter.TabIndent)
+	fmt.Fprintf(writer, "RH Region\t\tGateway URL\n")
+	for regionName, region := range regions {
+		fmt.Fprintf(writer, "%s\t\t%v\n", regionName, region.URL)
 	}
-	return nil
+
+	err = writer.Flush()
+	return err
 
 }

cmd/ocm/login/cmd.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/openshift-online/ocm-cli/pkg/config"
@@ -31,25 +32,9 @@ import (
 )
 
 const (
-	productionURL  = "https://api.openshift.com"
-	stagingURL     = "https://api.stage.openshift.com"
-	integrationURL = "https://api.integration.openshift.com"
-	oauthClientID  = "ocm-cli"
+	oauthClientID = "ocm-cli"
 )
 
-// When the value of the `--url` option is one of the keys of this map it will be replaced by the
-// corresponding value.
-var urlAliases = map[string]string{
-	"production":  productionURL,
-	"prod":        productionURL,
-	"prd":         productionURL,
-	"staging":     stagingURL,
-	"stage":       stagingURL,
-	"stg":         stagingURL,
-	"integration": integrationURL,
-	"int":         integrationURL,
-}
-
 var args struct {
 	tokenURL      string
 	clientID      string
@@ -112,15 +97,18 @@ func init() {
 	flags.StringVar(
 		&args.url,
 		"url",
-		sdk.DefaultURL,
-		"URL of the API gateway. The value can be the complete URL or an alias. The "+
-			"valid aliases are 'production', 'staging', 'integration' and their shorthands.",
+		"",
+		"URL of the OCM API gateway. If not provided, will reuse the URL from the configuration "+
+			"file or "+sdk.DefaultURL+" as a last resort. The value should be a complete URL "+
+			"or a valid URL alias: "+strings.Join(urls.ValidOCMUrlAliases(), ", "),
 	)
 	flags.StringVar(
 		&args.rhRegion,
 		"rh-region",
 		"",
-		"OCM region identifier. Takes precedence over the --url flag",
+		"OCM data sovereignty region identifier. --url will be used to initiate a service discovery "+
+			"request to find the region URL matching the provided identifier. Use `ocm list rh-regions` "+
+			"to see available regions.",
 	)
 	flags.MarkHidden("rh-region")
 	flags.StringVar(
@@ -181,11 +169,6 @@ func run(cmd *cobra.Command, argv []string) error {
 
 	var err error
 
-	// Check mandatory options:
-	if args.url == "" {
-		return fmt.Errorf("Option '--url' is mandatory")
-	}
-
 	if args.useAuthCode {
 		fmt.Println("You will now be redirected to Red Hat SSO login")
 		// Short wait for a less jarring experience
@@ -295,11 +278,28 @@ func run(cmd *cobra.Command, argv []string) error {
 		clientID = args.clientID
 	}
 
-	// If the value of the `--url` is any of the aliases then replace it with the corresponding
-	// real URL:
-	gatewayURL, ok := urlAliases[args.url]
-	if !ok {
-		gatewayURL = args.url
+	gatewayURL, err := urls.ResolveGatewayURL(args.url, cfg)
+	if err != nil {
+		return err
+	}
+
+	// If an --rh-region is provided, --url is resolved as above and then used to initiate
+	// service discovery for the environment --url is a part of, but the gatewayURL (and
+	// ultimately the cfg.URL) is then updated to the URL of the matching --rh-region:
+	//   1. resolve the gatewayURL as above
+	//   2. fetch a well-known file from sdk.GetRhRegion
+	//   3. update the gatewayURL to the region URL matching args.rhRegion
+	//
+	// So `--url=https://api.stage.openshift.com --rh-region=singapore` might result in
+	// gatewayURL/cfg.URL being mutated to "https://api.singapore.stage.openshift.com"
+	//
+	// See ocm-sdk-go/rh_region.go for full details on how service discovery works.
+	if args.rhRegion != "" {
+		regValue, err := sdk.GetRhRegion(gatewayURL, args.rhRegion)
+		if err != nil {
+			return fmt.Errorf("Can't find region: %w", err)
+		}
+		gatewayURL = fmt.Sprintf("https://%s", regValue.URL)
 	}
 
 	// Update the configuration with the values given in the command line:
@@ -331,15 +331,6 @@ func run(cmd *cobra.Command, argv []string) error {
 		cfg.Password = ""
 	}
 
-	// If an OCM region is provided, update the config URL with the SDK generated URL
-	if args.rhRegion != "" {
-		regValue, err := sdk.GetRhRegion(args.url, args.rhRegion)
-		if err != nil {
-			return fmt.Errorf("Can't find region: %w", err)
-		}
-		cfg.URL = fmt.Sprintf("https://%s", regValue.URL)
-	}
-
 	err = config.Save(cfg)
 	if err != nil {
 		return fmt.Errorf("Can't save config file: %v", err)

pkg/urls/main_test.go

@@ -0,0 +1,13 @@
+package urls
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestURLs(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "URLs")
+}

pkg/urls/url_expander_test.go

@@ -17,17 +17,10 @@ limitations under the License.
 package urls
 
 import (
-	"testing"
-
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 )
 
-func TestURLExpander(t *testing.T) {
-	RegisterFailHandler(Fail)
-	RunSpecs(t, "URL expander")
-}
-
 type urlExpanderTest struct {
 	params      []string
 	expectError bool

pkg/urls/well_known.go

@@ -18,5 +18,71 @@ limitations under the License.
 
 package urls
 
+import (
+	"fmt"
+	"net/url"
+	"strings"
+
+	"github.com/openshift-online/ocm-cli/pkg/config"
+	sdk "github.com/openshift-online/ocm-sdk-go"
+)
+
 // OfflineTokenPage is the URL of the page used to generate offline access tokens.
 const OfflineTokenPage = "https://console.redhat.com/openshift/token" // #nosec G101
+
+const (
+	OCMProductionURL  = "https://api.openshift.com"
+	OCMStagingURL     = "https://api.stage.openshift.com"
+	OCMIntegrationURL = "https://api.integration.openshift.com"
+)
+
+var OCMURLAliases = map[string]string{
+	"production":  OCMProductionURL,
+	"prod":        OCMProductionURL,
+	"prd":         OCMProductionURL,
+	"staging":     OCMStagingURL,
+	"stage":       OCMStagingURL,
+	"stg":         OCMStagingURL,
+	"integration": OCMIntegrationURL,
+	"int":         OCMIntegrationURL,
+}
+
+func ValidOCMUrlAliases() []string {
+	keys := make([]string, 0, len(OCMURLAliases))
+	for k := range OCMURLAliases {
+		keys = append(keys, k)
+	}
+	return keys
+}
+
+// URL Precedent (from highest priority to lowest priority):
+//  1. runtime `--url` cli arg (key found in `urlAliases`)
+//  2. runtime `--url` cli arg (non-empty string)
+//  3. config file `URL` value (non-empty string)
+//  4. sdk.DefaultURL
+//
+// Finally, it will try to url.ParseRequestURI the resolved URL to make sure it's a valid URL.
+func ResolveGatewayURL(optionalParsedCliFlagValue string, optionalParsedConfig *config.Config) (string, error) {
+	gatewayURL := sdk.DefaultURL
+	source := "default"
+	if optionalParsedCliFlagValue != "" {
+		gatewayURL = optionalParsedCliFlagValue
+		source = "flag"
+		if _, ok := OCMURLAliases[optionalParsedCliFlagValue]; ok {
+			gatewayURL = OCMURLAliases[optionalParsedCliFlagValue]
+		}
+	} else if optionalParsedConfig != nil && optionalParsedConfig.URL != "" {
+		// re-use the URL from the config file
+		gatewayURL = optionalParsedConfig.URL
+		source = "config"
+	}
+
+	url, err := url.ParseRequestURI(gatewayURL)
+	if err != nil {
+		return "", fmt.Errorf(
+			"%w\n\nURL Source: %s\nExpected an absolute URI/path (e.g. %s) or a case-sensitive alias, one of: [%s]",
+			err, source, sdk.DefaultURL, strings.Join(ValidOCMUrlAliases(), ", "))
+	}
+
+	return url.String(), nil
+}