Move away from traditional whois to using rdap

mrheinen commented 2 months ago

User description

This fixes issue https://github.com/mrheinen/lophiid/issues/3

PR Type

Enhancement

Description

Replaced traditional WHOIS client with RDAP (Registration Data Access Protocol) client for more standardized and structured data retrieval.
Implemented RdapManager to handle RDAP queries and caching, replacing the previous WhoisManager.
Updated database schema to include new fields like Country for RDAP data.
Enhanced metrics to track RDAP query retries and failures.
Modified UI to display new RDAP information, including country data.
Updated tests to cover new RDAP functionality and metrics.
Adjusted Bazel build configurations to reflect the switch from WHOIS to RDAP.
Improved error handling and logging for RDAP queries.

Changes walkthrough 📝

Relevant files

Enhancement

8 files

api_server.go `Update JavaScript runner initialization` cmd/api/api_server.go Removed unnecessary comment Updated `NewGojaJavascriptRunner` function call with `dbc` parameter	+1/-2
backend_main.go `Replace whois client with RDAP client` cmd/backend/backend_main.go Replaced `lwhois` package with `rdap` package Updated whois client initialization to use RDAP client Changed `NewCachedWhoisManager` to `NewCachedRdapManager`	+5/-4
server.go `Add RDAP string to whois response` pkg/api/server.go Removed unnecessary comment Added `RdapString` field to whois response	+1/-1
backend.go `Update backend to use RDAP manager` pkg/backend/backend.go Changed `WhoisManager` to `RdapManager` Updated function signatures and type references	+2/-2
database.go `Enhance Whois struct with RDAP data` pkg/database/database.go Added `Country` field to `Whois` struct Added `RdapString` field for API server use	+9/-4
metrics.go `Add new RDAP lookup metrics` pkg/whois/metrics.go Added new metrics for whois retries and exceeded retries Updated metric registration	+15/-2
rdap.go `Implement RDAP manager and client` pkg/whois/rdap.go Renamed `WhoisManager` to `RdapManager` Implemented RDAP client and query logic Updated error handling and metrics	+41/-20
RequestView.vue `Enhance UI to display RDAP information` ui/src/components/container/RequestView.vue Added display for country information from RDAP data Updated whois data display to show RDAP string if available	+15/-1

Tests

1 files

rdap_test.go

Update and add tests for RDAP functionality

pkg/whois/rdap_test.go

Updated tests to use RDAP client instead of Whois

Added new tests for RDAP functionality

Implemented metric checks in tests

+71/-10

Configuration changes

2 files

BUILD.bazel `Update Bazel dependencies for RDAP` cmd/backend/BUILD.bazel - Replaced `likexian_whois` dependency with `openrdap_rdap`	+1/-1
BUILD.bazel `Update Bazel configuration for RDAP changes` pkg/whois/BUILD.bazel Updated source file names and dependencies Added new test dependencies	+5/-2

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns

No apparent security concerns were introduced in this PR. The switch from WHOIS to RDAP generally improves security as RDAP is a more standardized and secure protocol. However, it's important to ensure that the RDAP client is configured to use HTTPS for secure communication with RDAP servers.

⚡ Key issues to review

Error Handling
The error handling in the `DoWhoisWork` method could be improved. It currently logs warnings but doesn't handle retries or failures comprehensively. Data Processing
The RDAP response processing in `DoWhoisWork` method might need more robust parsing and error checking, especially when dealing with the `Country` field. Schema Change
The `Whois` struct has been significantly modified. Ensure that this change is compatible with existing data and that any necessary database migrations are implemented.

github-actions[bot] commented 2 months ago

Preparing PR description...

github-actions[bot] commented 2 months ago

Preparing review...

codiumai-pr-agent-pro[bot] commented 2 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Ensure proper initialization order of components ___ Consider initializing `jRunner` with `dbc` before creating `as`. This ensures that `jRunner` has access to the database client when it's created. [cmd/api/api_server.go [117-119]](https://github.com/mrheinen/lophiid/pull/8/files#diff-9bfccb4639f2f956c54c30543b7cdbd9fa139a3a768eb254245febfaa970514bR117-R119) ```diff +dbc := database.NewKSQLClient(&db) +jRunner := javascript.NewGojaJavascriptRunner(dbc, javascript.CreateGoJaMetrics(reg)) +as := api.NewApiServer(dbc, jRunner, id.String()) - ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: Initializing jRunner with dbc before creating as ensures proper dependency injection and prevents potential issues with uninitialized components.	8
Error handling	Handle potential errors from printing RDAP response ___ Consider handling the error returned by `rdapPrinter.Print(resNetwork)` to ensure any printing errors are caught and logged. [pkg/whois/rdap.go [126-129]](https://github.com/mrheinen/lophiid/pull/8/files#diff-e28100c5c0bcf097b7a414bde2c3d8f1b0fa075281de07b908ace5e022520848R126-R129) ```diff -rdapPrinter.Print(resNetwork) +if err := rdapPrinter.Print(resNetwork); err != nil { + slog.Warn("Failed to print RDAP response", slog.String("error", err.Error())) +} if _, err := c.dbClient.Insert( ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Error handling for rdapPrinter.Print() improves robustness and helps in identifying and logging potential issues during RDAP response printing.	7
Enhancement	Add context parameter for better control over RDAP queries ___ Consider adding a context parameter to the `LookupIP` method to allow for cancellation and timeout control of the RDAP query. [pkg/whois/rdap.go [156]](https://github.com/mrheinen/lophiid/pull/8/files#diff-e28100c5c0bcf097b7a414bde2c3d8f1b0fa075281de07b908ace5e022520848R156-R156) ```diff -func (c CachedRdapManager) LookupIP(ip string) error { +func (c CachedRdapManager) LookupIP(ctx context.Context, ip string) error { ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a context parameter allows for better control over RDAP queries, enabling timeouts and cancellations, but it's not a critical change for functionality.	6
Testing	Improve test assertions for more precise metric checking ___ Consider adding more specific assertions for the metrics values in the test cases. Instead of just checking if the metric is not zero, assert the exact expected value. [pkg/whois/rdap_test.go [172-175]](https://github.com/mrheinen/lophiid/pull/8/files#diff-d8fca80ecd2f638442d11e2e4bd8a8e55a78e942b83fb0eea8dc609a9ce66049R172-R175) ```diff metric := testutil.ToFloat64(metrics.whoisRetriesExceededCount) -if int(metric) != 1 { - t.Errorf("expected 1, got %f", metric) +expectedMetricValue := float64(1) +if metric != expectedMetricValue { + t.Errorf("expected %.2f, got %.2f", expectedMetricValue, metric) } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 5 Why: More specific assertions in tests can improve the precision of metric checking, but the current implementation is already functional and this change is mainly for test quality improvement.	5

codiumai-pr-agent-pro[bot] commented 2 months ago

CI Failure Feedback 🧐

**Action:** build

**Failed stage:** [Build](https://github.com/mrheinen/lophiid/actions/runs/10370128601/job/28707350436) [❌]

**Failure summary:** The action failed due to compilation errors in the Go code:

Multiple undefined references to whois.FakeWhoisManager in the file pkg/backend/backend_test.go.

The errors occur on lines 184, 220, 270, 360, 393, 510, 611, 702, 738, and 791 of backend_test.go.

The compiler stopped after encountering too many errors (10 in total).

These errors suggest that the FakeWhoisManager type or function is not properly imported or defined
in the whois package.

Relevant error logs:

```yaml 1: ##[group]Operating System 2: Ubuntu ... 603: [224 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 3s linux-sandbox ... (2 actions, 1 running) 604: [231 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 4s linux-sandbox ... (2 actions, 1 running) 605: [234 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 5s linux-sandbox ... (2 actions running) 606: [241 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 6s linux-sandbox ... (2 actions, 1 running) 607: [242 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 7s linux-sandbox ... (2 actions, 1 running) 608: [243 / 291] GoCompilePkg external/com_github_dop251_goja/goja.a; 9s linux-sandbox ... (2 actions running) 609: [257 / 291] GoCompilePkg backend_service/backend_service.a; 0s linux-sandbox ... (2 actions running) 610: [267 / 291] GoLink pkg/vt/vt_test_/vt_test; 0s linux-sandbox ... (2 actions, 1 running) 611: ERROR: /home/runner/work/lophiid/lophiid/pkg/backend/BUILD.bazel:40:8: GoCompilePkg pkg/backend/backend_test.internal.a failed: (Exit 1): builder failed: error executing GoCompilePkg command (from target //pkg/backend:backend_test) bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/go_sdk/builder_reset/builder compilepkg -sdk external/go_sdk -installsuffix linux_amd64 -src pkg/backend/backend.go -src pkg/backend/client.go -src ... (remaining 83 arguments skipped) 612: Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging 613: ##[error]pkg/backend/backend_test.go:184:26: undefined: whois.FakeWhoisManager 614: ##[error]pkg/backend/backend_test.go:220:24: undefined: whois.FakeWhoisManager 615: ##[error]pkg/backend/backend_test.go:270:24: undefined: whois.FakeWhoisManager 616: ##[error]pkg/backend/backend_test.go:360:26: undefined: whois.FakeWhoisManager 617: ##[error]pkg/backend/backend_test.go:393:24: undefined: whois.FakeWhoisManager 618: ##[error]pkg/backend/backend_test.go:510:24: undefined: whois.FakeWhoisManager 619: ##[error]pkg/backend/backend_test.go:611:24: undefined: whois.FakeWhoisManager 620: ##[error]pkg/backend/backend_test.go:702:26: undefined: whois.FakeWhoisManager 621: ##[error]pkg/backend/backend_test.go:738:24: undefined: whois.FakeWhoisManager 622: ##[error]pkg/backend/backend_test.go:791:24: undefined: whois.FakeWhoisManager 623: ##[error]pkg/backend/backend_test.go:791:24: too many errors 624: compilepkg: error running subcommand external/go_sdk/pkg/tool/linux_amd64/compile: exit status 2 625: Use --verbose_failures to see the command lines of failed build steps. 626: INFO: Elapsed time: 125.235s, Critical Path: 83.14s 627: INFO: 271 processes: 45 internal, 226 linux-sandbox. 628: ERROR: Build did NOT complete successfully 629: ##[error]Process completed with exit code 1. ```

✨ CI feedback usage guide:

The CI feedback tool (`/checks)` automatically triggers when a PR has a failed check. The tool analyzes the failed checks and provides several feedbacks: - Failed stage - Failed test name - Failure summary - Relevant error logs In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR: ``` /checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}" ``` where `{repo_name}` is the name of the repository, `{run_number}` is the run number of the failed check, and `{job_number}` is the job number of the failed check. #### Configuration options - `enable_auto_checks_feedback` - if set to true, the tool will automatically provide feedback when a check is failed. Default is true. - `excluded_checks_list` - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list. - `enable_help_text` - if set to true, the tool will provide a help message with the feedback. Default is true. - `persistent_comment` - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true. - `final_update_message` - if `persistent_comment` is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true. See more information about the `checks` tool in the [docs](https://pr-agent-docs.codium.ai/tools/ci_feedback/).

mrheinen commented 2 months ago

So overall the PR code suggestions are not good. The only good this to consider is to refactor DoWhoisWork to return an error but that requires some design changes.

mrheinen / lophiid