Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: avoid assuming api.konghq in URI and support private link global api endpoint #165

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

harshadixit12
Copy link
Contributor

@harshadixit12 harshadixit12 commented Jan 16, 2025

fix: avoid assuming api.konghq in URI and support private link global api endpoint

Summary

IAM for konnect is global, and we call the global endpoint during login.
This breaks for konnect over privatelink since the base URL does not contain the separator we are looking for.

How have we fixed it?
While creating the baseURL for global endpoint, we are checking for a new separator first for privatelink, since it is unlikely to change.

Why did we choose this approach?
We cannot use the Konnect address provided as-is since IAM is global, and need to switch region in subdomain to global

More details:
https://konghq.atlassian.net/wiki/spaces/~712020bb1b1f365f32471bbc7f249f5d9df3ef/pages/4300111923/Konnect+URL+invalid+when+api.konghq+is+not+in+passed+url

How have I tested this?

  • Manually ran the gateway ping command with different --konnect-addr flags and verified it works
  • Shared binary with the reporter and tested

Issues resolved

Fix #1488 in Deck
Kong/deck#1488

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

@CLAassistant
Copy link

CLAassistant commented Jan 16, 2025

CLA assistant check
All committers have signed the CLA.

@harshadixit12 harshadixit12 force-pushed the fix/allow-private-link-in-konnect-addr branch 2 times, most recently from 366e570 to 8d1ef82 Compare January 20, 2025 08:28
@codecov-commenter
Copy link

codecov-commenter commented Jan 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 27.95%. Comparing base (0180dfb) to head (019c23b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   27.90%   27.95%   +0.04%     
==========================================
  Files         106      106              
  Lines       16347    16358      +11     
==========================================
+ Hits         4562     4573      +11     
  Misses      11303    11303              
  Partials      482      482              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@harshadixit12 harshadixit12 marked this pull request as ready for review January 20, 2025 08:54
@Prashansa-K
Copy link
Collaborator

@harshadixit12 Let's add a unit test for the changes made in this PR.

@harshadixit12
Copy link
Contributor Author

@harshadixit12 Let's add a unit test for the changes made in this PR.

@Prashansa-K I see the repo currently has integration tests - should I create new unit test and update the github pipeline workflow to run the unit test as well?

@Prashansa-K
Copy link
Collaborator

@harshadixit12 Let's add a unit test for the changes made in this PR.

@Prashansa-K I see the repo currently has integration tests - should I create new unit test and update the github pipeline workflow to run the unit test as well?

@harshadixit12 There are unit tests too in this repo that run in the CI Test workflow (.github/workflows/test.yaml).
For this change, the necessary tests should go to login_service_test.go file.

@harshadixit12
Copy link
Contributor Author

@harshadixit12 Let's add a unit test for the changes made in this PR.

@Prashansa-K I see the repo currently has integration tests - should I create new unit test and update the github pipeline workflow to run the unit test as well?

@harshadixit12 There are unit tests too in this repo that run in the CI Test workflow (.github/workflows/test.yaml). For this change, the necessary tests should go to login_service_test.go file.

Ah right, my bad, was looking in the wrong dir for unit tests.

@harshadixit12 harshadixit12 force-pushed the fix/allow-private-link-in-konnect-addr branch from 8d1ef82 to 683a9d3 Compare January 20, 2025 11:38
@@ -47,6 +47,13 @@ func (s *AuthService) Login(ctx context.Context, email,

// getGlobalEndpoint returns the global endpoint for a given base Konnect URL.
func getGlobalEndpoint(baseURL string) string {
// If svc.konghq is present in string, continue with global endpoint private link
if strings.Contains(baseURL, "svc.konghq") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"svc.konghq" can be a constant.


return baseEndpointPrivateLink + parts[len(parts)-1]
}

parts := strings.Split(baseURL, "api.konghq")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string can be a const too.

@harshadixit12 harshadixit12 merged commit 866d1bc into main Jan 22, 2025
23 checks passed
@harshadixit12 harshadixit12 deleted the fix/allow-private-link-in-konnect-addr branch January 22, 2025 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants