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

Version 0.5.1 #830

Merged
merged 4 commits into from
Oct 22, 2024
Merged

Version 0.5.1 #830

merged 4 commits into from
Oct 22, 2024

Conversation

paulgb
Copy link
Member

@paulgb paulgb commented Oct 22, 2024

Version 0.5.0 was not published to crates.io because it depended on the repo-local crate dynamic-proxy.

This bumps the version to 0.5.1, and renames dynamic-proxy to plane-dynamic-proxy with the intent of publishing it as its own crate. (At least for now, I like this better than incorporating it in Plane, because it has its own test suite that we would have to co-mingle with Plane's if we were to merge them)

Copy link

vercel bot commented Oct 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
plane ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 22, 2024 1:48pm

@paulgb paulgb marked this pull request as ready for review October 22, 2024 13:36
Copy link
Contributor

coderabbitai bot commented Oct 22, 2024

📝 Walkthrough

Walkthrough

The pull request involves modifications to the Cargo.toml files across three packages: dynamic-proxy, plane, and plane-tests. The dynamic-proxy package's name has been changed from "dynamic-proxy" to "plane-dynamic-proxy" and its version updated from "0.5.0" to "0.5.1". The edition remains as "2021" with no changes to dependencies. The plane package has also updated its version to "0.5.1" and replaced its dependency on dynamic-proxy with the newly named plane-dynamic-proxy, specifying the same version and path. Similarly, the plane-tests package has incremented its version to "0.5.1" and added the plane-dynamic-proxy dependency while removing the previous dynamic-proxy reference. All other dependencies in these packages remain unchanged. Additionally, various import statements across multiple test files have been updated to reflect the new package name.

Possibly related PRs

  • Refactor proxy #814: The changes in the main PR regarding the package name and versioning are directly related to the refactor of the proxy, which also involves the dynamic-proxy package.
  • Bump to v0.5.0 #821: This PR updates the version of the dynamic-proxy package, which is relevant to the changes made in the main PR that also involve versioning updates.
  • Fix proxy restart EAB issue #829: The modifications in this PR regarding the CertManager and its handling of ACME configurations may relate to the overall structure and organization of the dynamic-proxy, which is part of the changes in the main PR.

Suggested reviewers

  • rolyatmax
  • michaelsilver

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
plane/plane-tests/Cargo.toml (1)

17-17: Dependency change looks good, but consider adding a version specifier.

The dynamic-proxy dependency has been correctly replaced with plane-dynamic-proxy, which aligns with the PR objectives. The path is correct.

Consider adding a version specifier to the plane-dynamic-proxy dependency to ensure consistency with other packages. For example:

plane-dynamic-proxy = { path = "../../dynamic-proxy", version = "0.5.1" }

This would make it explicit that we're using version 0.5.1 of the renamed crate.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1c20be4 and abd718d.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • dynamic-proxy/Cargo.toml (1 hunks)
  • plane/Cargo.toml (2 hunks)
  • plane/plane-tests/Cargo.toml (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • dynamic-proxy/Cargo.toml
🧰 Additional context used
🔇 Additional comments (2)
plane/plane-tests/Cargo.toml (1)

3-3: Version update looks good.

The version has been correctly updated to 0.5.1, which is consistent with the PR objectives and the changes in other packages.

plane/Cargo.toml (1)

3-3: Version update looks good.

The package version has been correctly updated to 0.5.1, which aligns with the PR objectives.

plane/Cargo.toml Show resolved Hide resolved
@paulgb paulgb merged commit 8343db3 into main Oct 22, 2024
6 checks passed
@paulgb paulgb deleted the paulgb/v0.5.1 branch October 22, 2024 13:50
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (5)
dynamic-proxy/tests/test_proxy_websocket.rs (1)

Line range hint 41-51: Consider improving error handling and kudos on concurrent upgrade handling.

The changes to the call method have introduced some improvements, but there are areas that could be further enhanced:

  1. Error Handling: The use of unwrap() on client.request(request).await could lead to panics if the request fails. Consider using ? operator or explicit error handling to provide more graceful failure scenarios.

  2. Concurrent Upgrade Handling: Spawning the upgrade handler in a separate asynchronous task is a good practice. It allows the WebSocket connection to be handled concurrently without blocking the main execution flow.

Here's a suggested improvement for error handling:

Box::pin(async move {
    match client.request(request).await {
        Ok((res, Some(upgrade_handler))) => {
            tokio::spawn(async move {
                if let Err(e) = upgrade_handler.run().await {
                    eprintln!("Error in upgrade handler: {:?}", e);
                }
            });
            Ok(res)
        }
        Ok((res, None)) => Ok(res),
        Err(e) => Err(Box::new(e) as Box<dyn std::error::Error + Send + Sync>),
    }
})

This approach provides more robust error handling and logging, while maintaining the concurrent upgrade handler execution.

dynamic-proxy/tests/test_proxy_request.rs (1)

Line range hint 119-139: Consider creating an issue for the TODO comment.

The TODO comment indicates that there's pending work on re-implementing timeout handling. To ensure this doesn't get overlooked, it might be beneficial to create a GitHub issue to track this task.

Would you like me to create a GitHub issue for tracking the re-implementation of timeout handling?

plane/src/proxy/proxy_server.rs (3)

Line range hint 46-73: Implemented Root Path Redirection in call Method

The call method now handles requests to the root path ("/"). If a root_redirect_url is provided, it responds with a MOVED_PERMANENTLY (301) status and includes a Location header for redirection. If not, it returns a BAD_REQUEST (400) status. The logic is sound and correctly incorporated.

If the redirection is intended to be temporary, consider using StatusCode::FOUND (302) instead of MOVED_PERMANENTLY (301). Choose the status code that best reflects the desired client behavior.


Line range hint 76-78: Appropriate Handling of Missing Bearer Token

When the bearer token is missing after parsing the URI, the code returns a BAD_REQUEST (400) status. While this is acceptable, consider using UNAUTHORIZED (401) to indicate that authentication is required.

Returning UNAUTHORIZED (401) might more accurately reflect that the request lacks valid authentication credentials.


Line range hint 102-107: Error Handling in Proxy Request Execution

Errors encountered during the proxy request are logged, and an INTERNAL_SERVER_ERROR (500) status is returned to the client. This is appropriate; however, ensure that error logs do not expose sensitive information.

Review the error logging to ensure that no sensitive data is included in the logged errors.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between abd718d and a821bf5.

📒 Files selected for processing (18)
  • dynamic-proxy/tests/common/hello_world_service.rs (1 hunks)
  • dynamic-proxy/tests/common/simple_upgrade_service.rs (1 hunks)
  • dynamic-proxy/tests/graceful.rs (2 hunks)
  • dynamic-proxy/tests/graceful_https.rs (1 hunks)
  • dynamic-proxy/tests/hello_world_http.rs (1 hunks)
  • dynamic-proxy/tests/https_test.rs (1 hunks)
  • dynamic-proxy/tests/test_http_redirect.rs (1 hunks)
  • dynamic-proxy/tests/test_http_versions.rs (1 hunks)
  • dynamic-proxy/tests/test_proxy_request.rs (1 hunks)
  • dynamic-proxy/tests/test_proxy_websocket.rs (1 hunks)
  • dynamic-proxy/tests/test_upgrade.rs (1 hunks)
  • plane/plane-tests/tests/common/proxy_mock.rs (1 hunks)
  • plane/plane-tests/tests/common/test_env.rs (1 hunks)
  • plane/src/proxy/cert_manager.rs (1 hunks)
  • plane/src/proxy/cert_pair.rs (1 hunks)
  • plane/src/proxy/mod.rs (1 hunks)
  • plane/src/proxy/proxy_server.rs (1 hunks)
  • plane/src/proxy/request.rs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • plane/src/proxy/request.rs
🧰 Additional context used
📓 Learnings (1)
dynamic-proxy/tests/graceful.rs (2)
Learnt from: paulgb
PR: jamsocket/plane#814
File: dynamic-proxy/tests/graceful.rs:46-60
Timestamp: 2024-09-24T19:32:37.996Z
Learning: In the `test_graceful_shutdown()` test in `dynamic-proxy/tests/graceful.rs`, if the server has not shut down, the response will be returned immediately, failing the test, so adding a timeout is unnecessary.
Learnt from: paulgb
PR: jamsocket/plane#814
File: dynamic-proxy/tests/graceful.rs:46-60
Timestamp: 2024-10-08T15:32:21.105Z
Learning: In the `test_graceful_shutdown()` test in `dynamic-proxy/tests/graceful.rs`, if the server has not shut down, the response will be returned immediately, failing the test, so adding a timeout is unnecessary.
🔇 Additional comments (28)
dynamic-proxy/tests/hello_world_http.rs (1)

3-3: LGTM! Import statement updated correctly.

The import statement has been successfully updated to use the new crate name plane_dynamic_proxy. This change is consistent with the PR objectives of renaming the dynamic-proxy crate.

Let's verify that this change has been applied consistently across the codebase:

✅ Verification successful

Import renaming verified successfully.

All imports of the server module now consistently use the new crate name plane_dynamic_proxy. No remaining references to dynamic_proxy::server were found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of the server module use the new crate name

# Test: Search for any remaining imports from the old crate name
rg 'use dynamic_proxy::server'

# Test: Confirm that the new crate name is used consistently
rg 'use plane_dynamic_proxy::server'

Length of output: 1036

dynamic-proxy/tests/https_test.rs (2)

Line range hint 8-38: No changes required in the test function.

The test function test_https remains valid after the import change. The HttpsConfig and SimpleHttpServer types are correctly used, and no further modifications are necessary.


2-2: LGTM! Verify consistency across the codebase.

The import change from dynamic_proxy::server to plane_dynamic_proxy::server is correct and aligns with the PR objectives of renaming the crate.

Let's verify that this change is consistent across the codebase:

✅ Verification successful

Import renaming verified across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old crate name

# Test 1: Search for any remaining imports of the old crate name
echo "Searching for any remaining imports of 'dynamic_proxy'"
rg -i "use\s+dynamic_proxy"

# Test 2: Search for any remaining references to the old crate name in Cargo.toml files
echo "Searching for any remaining references to 'dynamic_proxy' in Cargo.toml files"
rg -i "dynamic_proxy" $(fd -g "Cargo.toml")

# Expected results:
# - Both searches should return no results, indicating that all references have been updated.

Length of output: 526


Script:

#!/bin/bash
# Description: Comprehensive search for any remaining references to 'dynamic_proxy'

# Test 1: Search for 'dynamic_proxy' in all Rust source files excluding build directories
echo "Searching for 'dynamic_proxy' in Rust source files"
rg -i "dynamic_proxy" --type rust --glob '!node_modules/*' --glob '!dist/*' --glob '!build/*'

# Test 2: Search for 'dynamic_proxy' in documentation files (README.md, docs/)
echo "Searching for 'dynamic_proxy' in documentation files"
rg -i "dynamic_proxy" --type md --type txt --glob 'docs/**/*'

# Test 3: Search for 'dynamic_proxy' in comments within Rust source files
echo "Searching for 'dynamic_proxy' in comments within Rust source files"
rg -i "//.*dynamic_proxy" --type rust

Length of output: 2651


Script:

#!/bin/bash
# Description: Accurate search for any remaining standalone references to 'dynamic_proxy'

# Test 1: Search for exact 'dynamic_proxy' in all Rust source files excluding build directories
echo "Searching for exact 'dynamic_proxy' in Rust source files"
rg -i "\bdynamic_proxy\b" --type rust --glob '!node_modules/*' --glob '!dist/*' --glob '!build/*'

# Test 2: Search for exact 'dynamic_proxy' in documentation files (README.md, docs/)
echo "Searching for exact 'dynamic_proxy' in documentation files"
rg -i "\bdynamic_proxy\b" --type md --type txt --glob 'docs/**/*'

# Test 3: Search for exact 'dynamic_proxy' in comments within Rust source files
echo "Searching for exact 'dynamic_proxy' in comments within Rust source files"
rg -i "//.*\bdynamic_proxy\b" --type rust

Length of output: 634

dynamic-proxy/tests/common/hello_world_service.rs (1)

4-4: LGTM! Verify consistency across the codebase.

The import statement has been correctly updated to use plane_dynamic_proxy::body instead of dynamic_proxy::body, which aligns with the PR objective of renaming the dynamic-proxy crate to plane-dynamic-proxy.

To ensure consistency across the codebase, please run the following script:

This script will help identify any missed import updates and confirm the widespread adoption of the new crate name.

✅ Verification successful

Verification Successful: All import statements have been updated to plane_dynamic_proxy::.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports from dynamic_proxy have been updated to plane_dynamic_proxy

# Test: Search for any remaining imports from dynamic_proxy. Expect: No results.
rg "use dynamic_proxy::" -g '!Cargo.toml'

# Test: Confirm that plane_dynamic_proxy is now used. Expect: Multiple results.
rg "use plane_dynamic_proxy::"

Length of output: 2031

dynamic-proxy/tests/test_http_versions.rs (1)

3-3: LGTM! Verify test execution after the rename.

The import statement has been correctly updated to use the new crate name plane_dynamic_proxy. This change aligns with the PR objectives of renaming the dynamic-proxy crate.

To ensure the tests are still functioning correctly after the rename, please run the following command:

dynamic-proxy/tests/graceful.rs (2)

3-4: LGTM: Import statements correctly updated

The import statements have been properly updated to reflect the renaming of the dynamic-proxy crate to plane-dynamic-proxy. This change is consistent with the PR objectives and ensures that the correct modules are imported.


14-14: LGTM: Function signature correctly updated

The return type in the slow_hello_world function signature has been properly updated to use plane_dynamic_proxy::body::SimpleBody instead of dynamic_proxy::body::SimpleBody. This change aligns with the crate renaming and ensures type consistency.

dynamic-proxy/tests/test_http_redirect.rs (1)

Line range hint 6-89: LGTM! Test logic remains intact and comprehensive.

The core functionality of the tests has not been altered, which is appropriate given that only the module name has changed. The tests continue to cover various scenarios for HTTP to HTTPS redirection, including:

  • Basic redirection
  • Redirection with slash path
  • Redirection with a specific path
  • Redirection with query parameters

This comprehensive test suite ensures that the redirection behavior remains correct after the module rename.

dynamic-proxy/tests/test_upgrade.rs (1)

9-9: LGTM! Import statement updated correctly.

The import statement has been updated to use the new crate name plane_dynamic_proxy, which is consistent with the PR objectives of renaming the dynamic-proxy crate.

Let's verify that this change is consistent across the entire codebase:

✅ Verification successful

Verified: Import statements updated consistently across the codebase.

All server module imports are using the new crate name plane_dynamic_proxy.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of the server module use the new crate name

# Test: Search for any remaining imports from the old crate name
old_imports=$(rg -i "use\s+dynamic_proxy::server")

if [ -n "$old_imports" ]; then
    echo "Found old imports that need to be updated:"
    echo "$old_imports"
else
    echo "All server module imports are using the new crate name."
fi

# Test: Confirm that the new import is used consistently
new_imports=$(rg -i "use\s+plane_dynamic_proxy::server")
echo "New imports found:"
echo "$new_imports"

Length of output: 3202

dynamic-proxy/tests/graceful_https.rs (2)

Line range hint 1-78: Confirmed: No further changes needed in the file.

The import changes are sufficient, and no modifications are required in the rest of the file. The test function test_graceful_shutdown_https continues to use SimpleHttpServer, HttpsConfig, and to_simple_body correctly with the updated imports.


4-6: LGTM: Import statements updated correctly.

The import statements have been successfully updated to use the new plane_dynamic_proxy module instead of dynamic_proxy. This change aligns with the PR objectives of renaming the dynamic-proxy crate to plane-dynamic-proxy.

To ensure consistency, let's verify that no other occurrences of dynamic_proxy remain in the file:

✅ Verification successful

LGTM: No remaining 'dynamic_proxy' occurrences found.

All instances of dynamic_proxy have been successfully updated to plane_dynamic_proxy in dynamic-proxy/tests/graceful_https.rs.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining occurrences of 'dynamic_proxy' in the file.

# Test: Search for 'dynamic_proxy' in the file. Expect: No matches.
rg 'dynamic_proxy' dynamic-proxy/tests/graceful_https.rs

Length of output: 200

dynamic-proxy/tests/common/simple_upgrade_service.rs (1)

12-12: LGTM! Verify consistent import updates across the codebase.

The import statement has been correctly updated to use the new plane_dynamic_proxy module name, which aligns with the PR objectives of renaming the dynamic-proxy crate. The change is isolated and doesn't affect the rest of the file's functionality.

To ensure consistency across the codebase, run the following script to verify that all relevant imports have been updated:

✅ Verification successful

All import statements successfully updated to plane_dynamic_proxy across the codebase.

No remaining instances of dynamic_proxy:: were found, and all imports now correctly reference plane_dynamic_proxy::.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of dynamic_proxy have been updated to plane_dynamic_proxy

# Test 1: Search for any remaining imports of dynamic_proxy
echo "Checking for any remaining imports of dynamic_proxy:"
rg "use dynamic_proxy::" -g '!Cargo.toml'

# Test 2: Verify that plane_dynamic_proxy is now being used
echo "Verifying usage of plane_dynamic_proxy:"
rg "use plane_dynamic_proxy::"

# Note: If Test 1 returns results or Test 2 doesn't, manual review may be needed.

Length of output: 2235

plane/plane-tests/tests/common/proxy_mock.rs (1)

6-6: LGTM! Verify consistent renaming across the project.

The import statement has been correctly updated to use the new crate name plane_dynamic_proxy. This change aligns with the PR objectives of renaming the 'dynamic-proxy' crate.

To ensure consistency across the project, run the following script to check for any remaining references to the old crate name:

✅ Verification successful

Consistent renaming confirmed across the project.
All references to dynamic_proxy have been successfully updated to plane_dynamic_proxy.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to 'dynamic_proxy' in the codebase

# Test: Search for 'dynamic_proxy' usage. Expect: No results or only in comments/documentation.
rg 'dynamic_proxy' --type rust

Length of output: 2099

dynamic-proxy/tests/test_proxy_websocket.rs (2)

5-9: LGTM: Import statements updated correctly.

The import statement has been updated to use plane_dynamic_proxy instead of dynamic_proxy, which is consistent with the package renaming mentioned in the PR objectives. The imported items remain the same, ensuring that the functionality is preserved.


Line range hint 1-109: Summary: Package renaming implemented correctly, with minor suggestions for improvement.

The changes in this file successfully implement the package renaming from "dynamic-proxy" to "plane-dynamic-proxy" as outlined in the PR objectives. The import statements have been updated correctly, and the SimpleProxyService implementation has been enhanced with concurrent upgrade handling.

The main test function test_websocket_echo remains unchanged, which is appropriate as its functionality should not be affected by the package renaming.

Overall, the changes are consistent and maintain the existing functionality. The suggested improvement for error handling in the SimpleProxyService::call method would further enhance the robustness of the code.

dynamic-proxy/tests/test_proxy_request.rs (1)

5-7: LGTM! Import statements updated correctly.

The changes to the import statements are consistent with the PR objectives. The dynamic-proxy crate has been correctly renamed to plane-dynamic-proxy, and the explicit imports of http and http_body_util improve code clarity.

To ensure consistency across the codebase, let's verify that all other test files have been updated similarly:

✅ Verification successful

Import statements successfully updated across all test files.

Verified that all test files in dynamic-proxy/tests now use plane_dynamic_proxy and no instances of dynamic_proxy remain.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all test files in the dynamic-proxy directory have been updated to use the new import.

# Test: Search for the new import in all test files
rg -t rust 'use plane_dynamic_proxy' dynamic-proxy/tests

# Test: Ensure no old imports remain
rg -t rust 'use dynamic_proxy' dynamic-proxy/tests

Length of output: 1396

plane/src/proxy/mod.rs (1)

6-8: LGTM! Verify consistency across the project.

The import statement has been correctly updated to use plane_dynamic_proxy::server instead of dynamic_proxy::server. This change aligns with the project's goal of renaming the dynamic-proxy crate to plane-dynamic-proxy.

To ensure consistency across the project, please run the following script:

This will help ensure that the renaming has been applied consistently throughout the Rust files in the project.

plane/src/proxy/cert_pair.rs (1)

Line range hint 1-138: Consistency confirmed: Changes align with PR objectives and AI summary.

The modifications in this file are limited to updating import paths, which is consistent with the PR objectives of renaming dynamic-proxy to plane-dynamic-proxy. The AI summary accurately captured these changes. No unexpected modifications or inconsistencies were found in the rest of the file.

plane/plane-tests/tests/common/test_env.rs (1)

27-27: LGTM: Import statement updated correctly.

The import statement has been updated to use the new crate name plane-dynamic-proxy, which is consistent with the PR objectives of renaming the dynamic-proxy crate.

To ensure consistency across the codebase, let's verify that all other occurrences of dynamic_proxy have been updated:

✅ Verification successful

LGTM: Import statement updated correctly.

The import statement has been updated to use the new crate name plane_dynamic_proxy, which is consistent with the PR objectives of renaming the dynamic-proxy crate. All other occurrences of dynamic_proxy have been successfully updated across the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining occurrences of 'dynamic_proxy' in the codebase

# Test: Search for 'dynamic_proxy' in all files. Expect: No occurrences outside of comments or strings.
rg -n 'dynamic_proxy' --type rust

Length of output: 2152

plane/src/proxy/proxy_server.rs (8)

Line range hint 8-13: Import Path Updated Correctly to plane_dynamic_proxy

The import path has been successfully updated to plane_dynamic_proxy, reflecting the renaming of the crate. This ensures that all references align with the new package name.


Line range hint 21-24: Added root_redirect_url Field to ProxyStateInner

The addition of the root_redirect_url: Option<String> field to the ProxyStateInner struct allows for optional root path redirection. This enhancement is properly implemented.


Line range hint 29-31: new Method Updated to Accept root_redirect_url

The new method now accepts a root_redirect_url parameter, aligning with the updated ProxyStateInner struct. The initialization correctly sets this value.


Line range hint 83-85: Correct Error Handling for URI Construction

The code checks if the reconstructed URI is valid and returns a BAD_REQUEST (400) status if not. This ensures that malformed requests are appropriately rejected.


Line range hint 123-138: Enhanced Host and Subdomain Validation in prepare_request

The prepare_request function now includes validation of the Host header against the expected cluster and subdomain. This strengthens security by ensuring that requests are properly scoped.


Line range hint 140-155: Removed Inbound x-verified-* Headers

The code removes any inbound headers starting with x-verified- to prevent header spoofing. This is a good security practice to ensure that these headers are set only by the proxy.


Line range hint 176-184: Applied General Response Headers Including CORS

The apply_general_headers function sets common headers for all responses, such as Access-Control-Allow-Origin, Access-Control-Allow-Methods, Access-Control-Allow-Headers, and the Server name. This implementation correctly ensures that clients can interact with the server without CORS issues.


Line range hint 157-169: Set Special Plane Headers for Verified Information

Special headers such as x-verified-username, x-verified-user-data, x-verified-path, x-verified-backend, and x-verified-secret are added to the request. While this facilitates passing verified information to the backend, exposing x-verified-secret may pose a security risk.

Please verify whether including x-verified-secret in the headers sent to the backend is necessary and safe. Ensure that this does not expose sensitive tokens that could be misused if intercepted.

Run the following script to identify where x-verified-secret is used in the codebase and assess its implications:

The script will help you audit the usage of this header to ensure it doesn't introduce security vulnerabilities.

plane/src/proxy/cert_manager.rs (1)

13-13: Verify that all import paths have been updated to plane_dynamic_proxy

The import path has been updated to plane_dynamic_proxy, which aligns with the renaming of the crate for publishing. Please ensure that all references to dynamic_proxy have been updated throughout the codebase to prevent any import errors.

Run the following script to check for remaining references to dynamic_proxy:

✅ Verification successful

All import paths successfully updated to plane_dynamic_proxy

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any occurrences of 'dynamic_proxy' in the codebase

# Search for 'dynamic_proxy' in all files, excluding 'Cargo.lock' and the 'target' directory
rg 'dynamic_proxy' --glob '!*Cargo.lock' --glob '!target/*'

Length of output: 2128

Comment on lines +1 to 5
use http::{header, StatusCode};
use plane_dynamic_proxy::{
https_redirect::HttpsRedirectService,
server::{HttpsConfig, SimpleHttpServer},
};
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remaining references to dynamic_proxy found.

The following files still contain references to dynamic_proxy that need to be updated:

  • dynamic-proxy/tests/graceful.rs
  • dynamic-proxy/tests/test_upgrade.rs
  • dynamic-proxy/tests/test_proxy_websocket.rs
  • dynamic-proxy/tests/test_proxy_request.rs
  • dynamic-proxy/tests/test_http_versions.rs
  • dynamic-proxy/tests/test_http_redirect.rs
  • dynamic-proxy/tests/https_test.rs
  • dynamic-proxy/tests/hello_world_http.rs
  • dynamic-proxy/tests/graceful_https.rs
  • dynamic-proxy/tests/common/simple_upgrade_service.rs
  • dynamic-proxy/tests/common/hello_world_service.rs
  • plane/src/proxy/request.rs
  • plane/src/proxy/cert_manager.rs
  • plane/src/proxy/proxy_server.rs
  • plane/src/proxy/mod.rs
  • plane/src/proxy/cert_pair.rs
  • plane/plane-tests/tests/common/proxy_mock.rs
  • plane/plane-tests/tests/common/test_env.rs
🔗 Analysis chain

LGTM! Import statements updated correctly.

The changes to the import statements are in line with the PR objectives. The dynamic_proxy module has been correctly renamed to plane_dynamic_proxy, and the necessary http module has been imported.

Let's verify the consistency of these changes across the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all imports of dynamic_proxy have been updated to plane_dynamic_proxy

# Test 1: Check for any remaining imports of dynamic_proxy
echo "Checking for any remaining imports of dynamic_proxy:"
rg "use dynamic_proxy"

# Test 2: Verify the new imports of plane_dynamic_proxy
echo "Verifying new imports of plane_dynamic_proxy:"
rg "use plane_dynamic_proxy"

# Test 3: Check for any missed renames in comments or strings
echo "Checking for any missed renames in comments or strings:"
rg "dynamic_proxy" -g "!Cargo.toml"

Length of output: 4452

Comment on lines +3 to +7
use pem::Pem;
use plane_dynamic_proxy::rustls::{
crypto::aws_lc_rs::sign::any_supported_type, pki_types::PrivateKeyDer,
};
use dynamic_proxy::tokio_rustls::rustls::sign::CertifiedKey;
use pem::Pem;
use plane_dynamic_proxy::tokio_rustls::rustls::sign::CertifiedKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused import: pem::Pem is imported but not used in plane/src/proxy/cert_pair.rs.

  • Consider eliminating the use pem::Pem; statement to clean up the codebase.
🔗 Analysis chain

LGTM: Import changes align with PR objectives.

The updated import paths for rustls and tokio_rustls from plane_dynamic_proxy are consistent with the PR objectives of renaming the dynamic-proxy crate to plane-dynamic-proxy. This change ensures that the code correctly references the new package structure.

To verify the usage of the newly imported pem::Pem, let's run the following script:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of pem::Pem in the file

# Test: Search for pem::Pem usage
rg 'pem::Pem' plane/src/proxy/cert_pair.rs

Length of output: 56


Script:

#!/bin/bash
# Description: Search for any usage of Pem in cert_pair.rs excluding the import line

# Search for 'Pem' as a whole word, excluding lines that import it
rg '\\bPem\\b' plane/src/proxy/cert_pair.rs | grep -v 'use pem::Pem;'

Length of output: 71

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