From d0f8459cd3a1de395af7bbd4e8893dffc8b9ed8f Mon Sep 17 00:00:00 2001 From: Matthew Stratton Date: Mon, 12 Feb 2024 16:24:10 -0500 Subject: [PATCH] improving ocm login and ocm list rh-region url resolution to reuse the 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 | 45 +++++++++++---- cmd/ocm/login/cmd.go | 71 ++++++++++------------- pkg/urls/main_test.go | 13 +++++ pkg/urls/url_expander_test.go | 7 --- pkg/urls/well_known.go | 66 ++++++++++++++++++++++ pkg/urls/well_known_test.go | 103 ++++++++++++++++++++++++++++++++++ 6 files changed, 248 insertions(+), 57 deletions(-) create mode 100644 pkg/urls/main_test.go create mode 100644 pkg/urls/well_known_test.go diff --git a/cmd/ocm/list/rhRegion/cmd.go b/cmd/ocm/list/rhRegion/cmd.go index aee3d60a..7272efb5 100644 --- a/cmd/ocm/list/rhRegion/cmd.go +++ b/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 } diff --git a/cmd/ocm/login/cmd.go b/cmd/ocm/login/cmd.go index b5798fd4..49fabef1 100644 --- a/cmd/ocm/login/cmd.go +++ b/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) diff --git a/pkg/urls/main_test.go b/pkg/urls/main_test.go new file mode 100644 index 00000000..911b0123 --- /dev/null +++ b/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") +} diff --git a/pkg/urls/url_expander_test.go b/pkg/urls/url_expander_test.go index 8a3ca114..34b93caa 100644 --- a/pkg/urls/url_expander_test.go +++ b/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 diff --git a/pkg/urls/well_known.go b/pkg/urls/well_known.go index d3a2ddc2..85918b95 100644 --- a/pkg/urls/well_known.go +++ b/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 +} diff --git a/pkg/urls/well_known_test.go b/pkg/urls/well_known_test.go new file mode 100644 index 00000000..a5d434cc --- /dev/null +++ b/pkg/urls/well_known_test.go @@ -0,0 +1,103 @@ +package urls + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/openshift-online/ocm-cli/pkg/config" + sdk "github.com/openshift-online/ocm-sdk-go" +) + +var _ = Describe("Gateway URL Resolution", func() { + + var nilConfig *config.Config = nil + var emptyConfig *config.Config = &config.Config{} + var emptyURLConfig *config.Config = &config.Config{URL: ""} + var nonEmptyURLConfig *config.Config = &config.Config{URL: "https://api.example.com"} + validUrlOverrides := []string{ + "https://api.example.com", "http://api.example.com", "http://localhost", + "http://localhost:8080", "https://localhost:8080/", "unix://my.server.com/tmp/api.socket", + "unix+https://my.server.com/tmp/api.socket", "h2c://api.example.com", + "unix+h2c://my.server.com/tmp/api.socket", + } + invalidUrlOverrides := []string{ + //nolint:misspell // intentional misspellings + "productin", "PRod", //alias typo + "localhost", "192.168.1.1", "api.openshift.com", //ip address/hostname without protocol + } + + It("Prority 1 - cli arg valid url aliases", func() { + for alias, url := range OCMURLAliases { + resolved, err := ResolveGatewayURL(alias, nilConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(url)) + + resolved, err = ResolveGatewayURL(alias, emptyConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(url)) + + resolved, err = ResolveGatewayURL(alias, emptyURLConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(url)) + + resolved, err = ResolveGatewayURL(alias, nonEmptyURLConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(url)) + } + }) + + It("Priority 2 - cli arg valid url", func() { + for _, urlOverride := range validUrlOverrides { + resolved, err := ResolveGatewayURL(urlOverride, nilConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(urlOverride)) + + resolved, err = ResolveGatewayURL(urlOverride, emptyConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(urlOverride)) + + resolved, err = ResolveGatewayURL(urlOverride, emptyURLConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(urlOverride)) + + resolved, err = ResolveGatewayURL(urlOverride, nonEmptyURLConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(urlOverride)) + } + }) + + It("Priority 3 - valid config url", func() { + for _, urlOverride := range validUrlOverrides { + resolved, err := ResolveGatewayURL("", &config.Config{URL: urlOverride}) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(urlOverride)) + } + }) + + It("Priority 4 - api.openshift.com", func() { + resolved, err := ResolveGatewayURL("", nilConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(sdk.DefaultURL)) + + resolved, err = ResolveGatewayURL("", emptyConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(sdk.DefaultURL)) + + resolved, err = ResolveGatewayURL("", emptyURLConfig) + Expect(err).ToNot(HaveOccurred()) + Expect(resolved).To(Equal(sdk.DefaultURL)) + }) + + It("Invalid url alias throws an error", func() { + for _, urlOverride := range invalidUrlOverrides { + _, err := ResolveGatewayURL(urlOverride, nilConfig) + Expect(err).To(HaveOccurred()) + } + }) + + It("Invalid cfg.URL throws an error", func() { + for _, urlOverride := range invalidUrlOverrides { + _, err := ResolveGatewayURL("", &config.Config{URL: urlOverride}) + Expect(err).To(HaveOccurred()) + } + }) +})