OCM-3227: Add support for regionalized urls #876
Conversation
tylercreller
commented
Nov 30, 2023
tylercreller
commented
- Adds Regionalized URL support to ConnectionBuilder
| @@ -566,6 +578,7 @@ func (b *ConnectionBuilder) Load(source interface{}) *ConnectionBuilder { | |||
| URL *string `yaml:"url"` | |||
| AlternativeURLs map[string]string `yaml:"alternative_urls"` | |||
There was a problem hiding this comment.
any idea what use cases these alternativeURLs are used for? I can guess at it from their context, just want to make sure we've done some due diligence to make sure they dont also need to be region aware (I dont think they do since they seems like explicit overrides)
There was a problem hiding this comment.
Alternative URLs are used to set different URLs for different services. For example if you wanted to run the SDK against integration accounts mgmt and staging clusters service. URL uses Alternative URLs under the hood with an empty identifier (the default URL). Region utilizes this same precedent albeit with an explicit pattern to uphold.
connection.go
Outdated
| @@ -566,6 +578,7 @@ func (b *ConnectionBuilder) Load(source interface{}) *ConnectionBuilder { | |||
| URL *string `yaml:"url"` | |||
| AlternativeURLs map[string]string `yaml:"alternative_urls"` | |||
| TokenURL *string `yaml:"token_url"` | |||
| Region *string `yaml:"region"` | |||
There was a problem hiding this comment.
should region and url be mutually exclusive? they effectively are, with region winning since its set last. just wondering if would be better with an explicit check with a warning/error. or maybe that should be in the end client not the sdk.
bit nit picky, not too worried about it, use your best judgement.
There was a problem hiding this comment.
I was thinking the same thing. I don't believe the SDK enforces anything like this today and leaves these decisions up to the consumers.
There was a problem hiding this comment.
it's a valid point. because region is now meaningful, then --region=integration works exactly as you think. no region is commercial prod.
| connection, err := sdk.NewConnectionBuilder(). | ||
| Logger(logger). | ||
| Tokens(token). | ||
| URL(fmt.Sprintf("https://%s", regionURL)). // Apply the region URL |
There was a problem hiding this comment.
so you connect to "integration" as a Region, get ocm-shards.json, to extract the singapore region url and set it with URL rather than Region...i understand why its done like this as written, but it seems a bit...muddy. Like what real value is ConnectionBuilder.Region providing here besides a slightly different way of setting the url?
My initial though with adding region support to ocm-sdk was to reduce duplicate work in downstream consumers. We are basically implementing a service discovery mechanism. Point a client at a global OCM instance, ask it what regions is knows about (ocm-shards.json, which admittedly is a short term WIP concept which will probably change as we work with it in UI and cli tools), and select the right url based on a short human usable string (i.e. "rh-singapore"). We need to implement that same basic thing in both rosa installer and ocm cli.
Does this MR facilitate that workflow?
There was a problem hiding this comment.
Right, we've already chatted about this a bit but to bring it over here...
This is a byproduct given the current setup of ocm shards. Ideally during the discovery phase we would only care about the region ID and that region ID would directly correlate to the region URL. Such as api.rh-singapore.openshift.com. This would reduce some of the "mud" in this example.
This MR can facilitate the workflow you are talking about once we settle into our design of the region source-of-truth. Ultimately we are just overwriting the URL based on the desired region. IMO I don't think the SDK should be attempting to auto-discover regions when the Region flag is set, it should just attempt to connect and consumers should dictate how that region is discovered.
connection.go
Outdated
| @@ -566,6 +578,7 @@ func (b *ConnectionBuilder) Load(source interface{}) *ConnectionBuilder { | |||
| URL *string `yaml:"url"` | |||
| AlternativeURLs map[string]string `yaml:"alternative_urls"` | |||
| TokenURL *string `yaml:"token_url"` | |||
| Region *string `yaml:"region"` | |||
There was a problem hiding this comment.
it's a valid point. because region is now meaningful, then --region=integration works exactly as you think. no region is commercial prod.
