feat: Enhance Smokescreen integration tests #249
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change significantly improves the integration test suite for Smokescreen,
adding coverage for several ACL features and enhancing validation capabilities.
Key improvements include:
1. **New Test Cases Added:**
* Wildcard domain globs (e.g., `*.example.com`).
* MitmDomains functionality:
* `add_headers`: Verified using a custom HTTP handler that reflects headers.
* `detailed_http_logs`: Validated by checking for the presence of
MITM-added headers in Smokescreen's log output.
* ExternalProxyGlobs: Tested ACL evaluation of the `X-Upstream-Https-Proxy`
header.
* GlobalDenyList and GlobalAllowList: Added tests for their behavior
and precedence.
* ACL Configuration Validation: New tests (`TestInvalidACLConfigs`) verify
Smokescreen's startup behavior with invalid ACLs (bad globs,
non-normalized domains).
2. **Enhanced `sample_config.yaml`**: The test ACL configuration was
extended with new services and global lists to support the new test scenarios.
3. **Improved Log Validation Logic**:
* The `TestCase` struct was augmented with `ExpectedLogReason` and
`ExpectedLogProject` fields.
* The `validateProxyResponse` function in `cmd/integration_test.go` was
updated to assert these fields, allowing for more precise validation of
ACL decisions and logged project names.
* An initial set of common test cases were updated to use these new
log assertion fields.
**Further Work Required (Manual Update):**
Due to limitations I encountered with applying extensive, fine-grained changes,
a number of existing test cases within `TestSmokescreenIntegration` (specifically
in `wildcardTestCases`, `mitmTestCases`, `externalProxyTestCases`, and
`globalListTestCases`) still need their `ExpectedLogReason` and
`ExpectedLogProject` fields populated. The infrastructure for this validation
is in place, but the data for each specific test case needs to be manually
filled in.
The `DisabledPolicies` ACL feature was investigated but I found it to be
untestable via the current YAML/CLI-based integration test setup, as it
requires programmatic configuration.
This commit introduces new integration tests for Smokescreen, focusing on
redirect handling, request path/query forwarding, and setup for testing
operational endpoints like healthcheck and stats.
**Key Changes and Test Additions:**
1. **Redirect Following (GET Requests)**:
* Added `TestRedirectFollowing` to `cmd/integration_test.go`.
* Tests scenarios where Smokescreen follows GET redirects:
* Allow initial, allow redirect: Verifies final destination is reached and query parameters are preserved.
* Allow initial, deny redirect: Verifies ACL on the redirected URL is enforced.
* Deny initial: Verifies initial ACL denial.
* Updated `sample_config.yaml` with roles (`role-redirect-allow-all`, `role-redirect-allow-deny`, `role-redirect-deny-initial`) to support these scenarios using fixed ports for test servers.
* Enhanced `TestCase` and `validateProxyResponse` to support body content and final query parameter assertions.
2. **Request Path and Query Parameter Forwarding**:
* Added `TestPathQueryForwarding` to `cmd/integration_test.go`.
* Introduced an `echoServerHandler` that reflects path, query, host, method, and body.
* Tests verify that paths (including URL-encoded parts) and query parameters are correctly forwarded through Smokescreen for both standard HTTP proxy and CONNECT-tunneled HTTPS requests.
* Added `role-echo-test` to `sample_config.yaml`.
3. **Setup for Healthcheck Endpoint Testing**:
* Modified `startSmokescreen` in `cmd/integration_test.go` to:
* Inject a default `conf.Healthcheck` handler.
* Wrap the main proxy handler with `smokescreen.HealthcheckMiddleware`.
* This prepares the test environment for healthcheck endpoint tests.
4. **Refactor for Stats Server Testing**:
* Made `newServer` in `pkg/smokescreen/stats_server.go` public as `NewServerTesting` to facilitate direct testing of the stats server logic.
**Attempted Changes & Blockers (Further Work Required):**
* **POST Redirect Tests**:
* *Goal*: Test that Smokescreen returns 307/308 redirects to you for POST requests, rather than following them.
* *Status*: Foundational changes to `TestCase` (for method/body) and `sample_config.yaml` (new role `role-redirect-post-allow`) were made. However, I encountered difficulties when trying to add the specific test server handlers and test case logic to `cmd/integration_test.go`. The intended test logic has been documented in development history.
* **Healthcheck Endpoint Test Function**:
* *Goal*: Add `TestHealthcheckEndpoint` to verify the `/healthcheck` path.
* *Status*: I was blocked by the same difficulties when trying to add the new test function to `cmd/integration_test.go`. The setup in `startSmokescreen` is complete.
* **Stats Server Handler Test Function**:
* *Goal*: Add `TestStatsServerHandler` to unit-test the stats server's `ServeHTTP` method using the now-public `NewServerTesting`.
* *Status*: I was also blocked by difficulties when attempting to add the new test function.
**Summary of Issue:**
I encountered a recurring issue when attempting to add new, moderately sized test functions or apply numerous small changes across existing arrays of test cases in the large `cmd/integration_test.go` file. This prevented the full implementation of several planned test scenarios. The changes that were successfully applied were typically smaller, more isolated modifications or additions to `sample_config.yaml`.
Despite these limitations, the successfully implemented tests provide valuable new coverage for redirect and path/query forwarding behaviors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change significantly improves the integration test suite for Smokescreen, adding coverage for several ACL features and enhancing validation capabilities.
Key improvements include:
New Test Cases Added:
*.example.com).add_headers: Verified using a custom HTTP handler that reflects headers.detailed_http_logs: Validated by checking for the presence ofMITM-added headers in Smokescreen's log output.
X-Upstream-Https-Proxyheader.
and precedence.
TestInvalidACLConfigs) verifySmokescreen's startup behavior with invalid ACLs (bad globs,
non-normalized domains).
Enhanced
sample_config.yaml: The test ACL configuration was extended with new services and global lists to support the new test scenarios.Improved Log Validation Logic:
TestCasestruct was augmented withExpectedLogReasonandExpectedLogProjectfields.validateProxyResponsefunction incmd/integration_test.gowasupdated to assert these fields, allowing for more precise validation of
ACL decisions and logged project names.
log assertion fields.
Further Work Required (Manual Update):
Due to limitations I encountered with applying extensive, fine-grained changes, a number of existing test cases within
TestSmokescreenIntegration(specifically inwildcardTestCases,mitmTestCases,externalProxyTestCases, andglobalListTestCases) still need theirExpectedLogReasonandExpectedLogProjectfields populated. The infrastructure for this validation is in place, but the data for each specific test case needs to be manually filled in.The
DisabledPoliciesACL feature was investigated but I found it to be untestable via the current YAML/CLI-based integration test setup, as it requires programmatic configuration.