Use api subdomain for REST and GQL clients when host is tenant

[williammartin]

Description

When targeting the API of tenants we should be using the subdomain rather than the path prefix.

Implementation Choices

Right now, I took the straightest path to implementing this, which included copying functions from the auth package. It was an intentional choice not to DRY this out because I want to derisk this work and then review it with confidence that the black box behaviour is correct.

How To Test

The easiest way to black box validate these changes is to use the PR in cli/cli that pins to the commit sha.

Alternatively, replace the dependency in cli/cli like so:

go mod edit -replace github.com/cli/go-gh/v2=/path/to/go-gh

Then build and run the CLI with commands that use these clients such as:

GH_DEBUG=api GH_HOST=tenant.ghe.com ./bin/gh secret list --repo whatever/whatever # REST
GH_DEBUG=api GH_HOST=tenant.ghe.com ./bin/gh repo list # GQL

In both cases the HTTP requests should demonstrate the API subdomain being used instead of the path prefix.

[andyfeller]

I'm torn regarding some of the duplication of this logic within cli/go-gh as well as cli/cli. My knee jerk would be cli/go-gh is the source of truth for this logic and cli/cli leveraging the logic for its needs, which I realize is outside of the scope of the original issue and likely would be deferred.

Beyond that, I wonder how we go about testing these changes in an extension other than gh to ensure they work as expected. Perhaps this is a great opportunity to pair on testing this with gh copilot?

[andyfeller]

Are there any concerns about this being seemingly redundant to the logic in auth?

I did a double take when I first saw this because I thought the enterprise check was already taking tenancy into account, when I realized I was thinking about:

go-gh/pkg/auth/auth.go

Lines 152 to 167 in 25db6b9

	// TenancyHost is the domain name of a tenancy GitHub instance.
	const tenancyHost = "ghe.com"
	// IsEnterprise determines if a provided host is a GitHub Enterprise Server instance,
	// rather than GitHub.com or a tenancy GitHub instance.
	func IsEnterprise(host string) bool {
	normalizedHost := normalizeHostname(host)
	return normalizedHost != github && normalizedHost != localhost && !IsTenancy(normalizedHost)
	}
	// IsTenancy determines if a provided host is a tenancy GitHub instance,
	// rather than GitHub.com or a GitHub Enterprise Server instance.
	func IsTenancy(host string) bool {
	normalizedHost := normalizeHostname(host)
	return strings.HasSuffix(normalizedHost, "."+tenancyHost)
	}

My concern is managing 2 sets of similar / overlapping functionality in 2 places and introducing drift.

[williammartin]

See "implementation choices" section in PR description.

I think we should do work here or in a follow up to address this but I'm not going through tearing things up until we know this is works as expected and covers the right use cases.

[andyfeller]

Similar to my comment above, it seems we now have 3 copies of this logic across cli/cli and 2 places within cli/go-gh and that makes me a little concerned:

go-gh/pkg/auth/auth.go

Lines 169 to 186 in 25db6b9

	func normalizeHostname(host string) string {
	hostname := strings.ToLower(host)
	if strings.HasSuffix(hostname, "."+github) {
	return github
	}
	if strings.HasSuffix(hostname, "."+localhost) {
	return localhost
	}
	// This has been copied over from the cli/cli NormalizeHostname function
	// to ensure compatible behaviour but we don't fully understand when or
	// why it would be useful here. We can't see what harm will come of
	// duplicating the logic.
	if before, found := cutSuffix(hostname, "."+tenancyHost); found {
	idx := strings.LastIndex(before, ".")
	return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)
	}
	return hostname
	}

[williammartin]

Same as my response above.

[williammartin]

Beyond that, I wonder how we go about testing these changes in an extension other than gh to ensure they work as expected. Perhaps this is a great opportunity to pair on testing this with gh copilot?

I'm not sure what the difference between gh and extensions are though, they both consume this module in the same way. However, it's probably worth doing anyway to derisk.

Best way would be the same way I suggested in the PR description for gh.

[andyfeller]

LGTM