Skip to content

feat: add dns-dedup E2E sample app#210

Merged
slayerjain merged 1 commit intomainfrom
add-dns-dedup
Mar 20, 2026
Merged

feat: add dns-dedup E2E sample app#210
slayerjain merged 1 commit intomainfrom
add-dns-dedup

Conversation

@slayerjain
Copy link
Copy Markdown
Member

Summary

  • HTTP server that triggers many DNS lookups for the same domain (default: sqs.us-east-1.amazonaws.com)
  • Endpoints: /health, /resolve, /resolve-many?n=30
  • Used by dns_dedup_test job in keploy/keploy#3950 to verify Keploy deduplicates DNS mocks when a domain returns rotating IPs

Test plan

  • Merge companion PR keploy/keploy#3950 (adds CI workflow + test script)
  • Verify dns_dedup E2E jobs pass

HTTP server that triggers many DNS lookups for the same domain,
testing that Keploy properly deduplicates DNS mocks when a domain
returns different IPs on each lookup (e.g., AWS SQS round-robin).
Copilot AI review requested due to automatic review settings March 20, 2026 15:50
@slayerjain slayerjain requested a review from khareyash05 as a code owner March 20, 2026 15:50
@slayerjain slayerjain merged commit 9d4d63b into main Mar 20, 2026
20 checks passed
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new standalone Go sample app (dns-dedup/) intended for Keploy E2E validation of DNS-mock deduplication when a single domain returns rotating IPs (e.g., AWS SQS).

Changes:

  • Introduces an HTTP server with /health, /resolve, and /resolve-many endpoints that perform repeated DNS lookups for a target domain.
  • Adds a minimal go.mod for the new sample.
  • Adds a curl.sh helper script to generate traffic against the endpoints.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
dns-dedup/main.go Implements the DNS lookup endpoints and the “many lookups” scenario used to validate dedup behavior.
dns-dedup/go.mod Defines a minimal Go module for building/running the sample app.
dns-dedup/curl.sh Provides a simple traffic generator for manual/CI exercising of the endpoints.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +36 to +40
d := r.URL.Query().Get("domain")
if d == "" {
d = domain
}
ips, err := net.LookupHost(d)
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

domain is taken directly from the request and used for DNS lookups. If this sample server is ever run in a non-local environment, it becomes an open DNS “oracle” that can be abused to enumerate internal/private DNS names. Consider restricting allowed domains by default (or gating arbitrary domains behind an explicit flag/env).

Copilot uses AI. Check for mistakes.
Comment on lines +55 to +58
d := r.URL.Query().Get("domain")
if d == "" {
d = domain
}
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

Same concern as /resolve: domain is fully user-controlled and used for DNS lookups, which can leak internal DNS information if this service is exposed. Consider an allowlist or a flag to enable arbitrary domains.

Copilot uses AI. Check for mistakes.
port := "8086"
fmt.Printf("DNS dedup test server starting on :%s\n", port)
if err := http.ListenAndServe(":"+port, nil); err != nil {
fmt.Fprintf(os.Stderr, "server error: %v\n", err)
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The startup error message server error: ... doesn’t provide an actionable next step. Consider including a hint such as checking whether the port is already in use or whether the process has permission to bind the port.

Suggested change
fmt.Fprintf(os.Stderr, "server error: %v\n", err)
fmt.Fprintf(os.Stderr, "server failed to start on :%s: %v\nHint: Check if port %s is already in use or if this process has permission to bind it.\n", port, err, port)

Copilot uses AI. Check for mistakes.

set -euo pipefail

BASE="http://localhost:8086"
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

For easier reuse in CI/local runs, consider allowing the base URL to be overridden via an environment variable (similar to other scripts in this repo), instead of hardcoding localhost:8086.

Suggested change
BASE="http://localhost:8086"
BASE="${BASE:-http://localhost:8086}"

Copilot uses AI. Check for mistakes.
Comment on lines +80 to +83
key := fmt.Sprintf("%v", ips)
isNew := !seen[key]
seen[key] = true
results = append(results, result{Iteration: i, IPs: ips, New: isNew})
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

In /resolve-many, the dedup key is derived from fmt.Sprintf("%v", ips), which is order-sensitive. If DNS responses return the same set of IPs in a different order, this will incorrectly count them as different “unique_ip_sets”. Consider normalizing before keying (e.g., sort the IP slice and join) and using map[string]struct{} for the seen set.

Copilot uses AI. Check for mistakes.
Comment on lines +59 to +64
n := 20
if ns := r.URL.Query().Get("n"); ns != "" {
if parsed, err := strconv.Atoi(ns); err == nil && parsed > 0 {
n = parsed
}
}
Copy link

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

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

The n query parameter is effectively unbounded; a large value can make this handler run for a very long time (and allocate a large results slice), which is risky for CI stability and for anyone accidentally exposing the service. Consider enforcing a reasonable max n (and/or returning 400 when exceeded).

Copilot uses AI. Check for mistakes.
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.

2 participants