feat: add scope name to proxy path to have unique address #590
Conversation
Signed-off-by: Smuu <[email protected]>
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/instance/proxy.go (2)
30-30: LGTM, but needs input validationThe addition of scope to the prefix will help ensure unique addresses as intended. However, the scope value should be validated to prevent potential issues.
Consider adding validation before the prefix construction:
+ if n.instance.Scope == "" { + return "", ErrInvalidScope.WithParams("scope cannot be empty") + } + // Prevent special characters that could cause URL parsing issues + if strings.Contains(n.instance.Scope, "-") { + return "", ErrInvalidScope.WithParams("scope cannot contain hyphens") + } prefix := fmt.Sprintf("%s-%s-%d", n.instance.Scope, serviceName, port)
Line range hint
30-36: Consider adding logging for debuggingGiven this is a significant change in proxy addressing, adding debug logging would help track any issues during rollout.
prefix := fmt.Sprintf("%s-%s-%d", n.instance.Scope, serviceName, port) + log.Debugf("Generated proxy prefix %s for service %s (scope: %s, port: %d)", + prefix, serviceName, n.instance.Scope, port) if err := n.instance.Proxy.AddHost(ctx, serviceName, prefix, port); err != nil {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/instance/proxy.go(1 hunks)
🔇 Additional comments (1)
pkg/instance/proxy.go (1)
Line range hint 30-36: Verify impact on existing proxy paths
The change in prefix format could affect existing service discovery. We should verify how this change impacts running systems and whether a migration strategy is needed.
✅ Verification successful
Let me gather more information about the proxy implementation and usage patterns.
Let me try one more search to find the proxy implementation and its usage patterns.
Change in prefix format is safe and backwards compatible
The verification shows that the proxy implementation (Traefik) handles prefixes independently for each service registration. The prefix is used only within the scope of a single AddHost call and doesn't affect existing services:
- The prefix is generated uniquely for each service using instance scope, service name, and port
- Traefik creates isolated resources (middleware, ingress routes) for each prefix
- No shared state or dependencies between different service registrations were found
- The prefix format change only affects new service registrations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing proxy URL usages to assess impact
rg -l 'URL\(ctx,.*\)' | while read -r file; do
echo "=== $file ==="
rg -A 2 'URL\(ctx,.*\)' "$file"
done
# Look for any direct prefix construction patterns that might need updating
ast-grep --pattern 'fmt.Sprintf("%s-%d"'
Length of output: 1101
Script:
#!/bin/bash
# Search for proxy-related implementations and usages
echo "=== Searching for Proxy interface and implementation ==="
ast-grep --pattern 'type Proxy interface'
echo -e "\n=== Looking for AddHost implementations ==="
ast-grep --pattern 'func $_($_ context.Context, $_ string, $_ string, $_ int)'
echo -e "\n=== Searching for prefix construction patterns ==="
rg -A 2 'Sprintf.*%s.*%s.*%d'
echo -e "\n=== Looking for proxy-related tests ==="
fd -e go -x rg -l 'Proxy' {} \; | while read -r file; do
if [[ $file == *"_test.go" ]]; then
echo "=== $file ==="
rg -A 2 'AddHost|URL\(' "$file"
fi
done
Length of output: 2879
Script:
#!/bin/bash
# Search for proxy-related code
echo "=== Searching for proxy implementation files ==="
fd -e go -x rg -l 'type.*Proxy.*struct|type.*Proxy.*interface' {} \;
echo -e "\n=== Looking for proxy usage patterns ==="
rg -A 3 'instance\.Proxy\.'
echo -e "\n=== Checking for existing prefix usage ==="
rg -A 3 'AddHost.*prefix'
echo -e "\n=== Looking for service discovery related code ==="
rg -A 3 'serviceName.*port'
Length of output: 2571
Overview
Summary by CodeRabbit
New Features
Bug Fixes