Bind PackageServer to ephemeral port to avoid port conflicts #227

translatenix · 2024-02-22T01:01:22Z

Here is my take on how to best solve the "flaky PackageServer tests" problem.
It is ready to be merged after #217 .
See commit message for details.
If you prefer a different solution, perhaps this can serve as inspiration.

translatenix · 2024-03-07T22:59:26Z

@bioball Rebased and ready for review.

bioball

Overall, this is looking great! I have some minor comments.

bioball · 2024-03-07T23:11:35Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt

+      .single()
+      .int()
+      .default(-1)
+      .validate { it in 1024..65535 }


Doesn't seem very useful to validate given it's a test flag, and it's also provisioned by the OS. Do we know it's going to be in this range?

Suggested change

.validate { it in 1024..65535 }

Not terribly important to have this validated, but ephemeral ports are guaranteed to be high ports (1024-65535).
https://en.wikipedia.org/wiki/Ephemeral_port#Range

Gotcha, thanks for the link. Let's still remove this, though; it adds some unnecessary brittleness to our tests.

bioball · 2024-03-07T23:12:46Z

pkl-core/src/main/java/org/pkl/core/http/HttpClient.java

+     *
+     * <p>If set, requests that specify port 12110 are rewritten to use the given port. This enables
+     * users of this client to connect to a test server without having to know its (ephemeral)
+     * listening port.


Suggested change

* listening port.

* listening port.

*

* <p>This is an internal option used for testing purposes only.

Calling this option "internal" felt strange because HttpClient is a public API and we don't have a way to hide testPort. It's also harmless. Should replace 12210 with 0 in a follow-up PR.

It's the same with org.pkl.commons.cli.CliBaseOptions; both are public APIs but expose flags that are only meant for test purposes (e.g. testMode, testPort added by this PR) and we call them "internal" in the doc comments.

bioball · 2024-03-07T23:16:02Z

pkl-core/src/main/java/org/pkl/core/util/HttpUtils.java

+          uri.getQuery(),
+          uri.getFragment());
+    } catch (URISyntaxException e) {
+      throw new AssertionError(e); // only port changed


Suggested change

throw new AssertionError(e); // only port changed

throw PklBugException.unreachableCode(); // only port changed

Done. org.pkl.core.util depending on org.pkl.core seems problematic. But since PklBugException is already used in org.pkl.core.util, it's not making things worse.

bioball · 2024-03-07T23:18:06Z

pkl-executor/src/main/java/org/pkl/executor/ExecutorOptions2.java

@@ -49,6 +50,7 @@ public class ExecutorOptions2 extends ExecutorOptions {
   * @param projectDir API equivalent of the {@code --project-dir} CLI option.
   * @param certificateFiles API equivalent of the {@code --ca-certificates} CLI option
   * @param certificateUris API equivalent of the {@code --ca-certificates} CLI option
+   * @param testPort API equivalent of the {@code --test-port} CLI option


Suggested change

* @param testPort API equivalent of the {@code --test-port} CLI option

* @param testPort Internal option used for testing purposes.

Removed this param from the public API.

bioball · 2024-03-07T23:19:29Z

pkl-executor/src/main/java/org/pkl/executor/ExecutorOptions2.java

@@ -62,7 +64,8 @@ public ExecutorOptions2(
      /* @Nullable */ Path moduleCacheDir,
      /* @Nullable */ Path projectDir,
      List<Path> certificateFiles,
-      List<URI> certificateUris) {
+      List<URI> certificateUris,
+      int testPort) {


Since testPort is an internal test-only option, we should have another constructor that doesn't have it as an option, and sets testPort to -1.

Should probably address the root cause and provide a builder for ExecutorOptions2. For example, almost nobody will want to pass certificates either.

I thought about modernizing the pkl-executor API to look more like Evaluator and language bindings. But then I realized that my time is better spent on Java language bindings, which have the potential to make pkl-executor unnecessary.

Added a builder and removed testPort from public API. See commit message for details.

translatenix · 2024-03-08T04:13:59Z

@bioball Ready from my side. See latest commit message for details. Quite happy with the cleanup.

bioball

Just a couple minor comments, but this is pretty close!

bioball · 2024-03-12T20:26:45Z

pkl-executor/src/main/java/org/pkl/executor/ExecutorOptions.java

+  public static Builder builder() {
+    return new Builder();
+  }
+


Good idea with the builder!

We have an existing pattern of having unconfigured() and preconfigured() builders; see EvaluatorBuilder, ConfigEvaluatorBuilder, ValueMapperBuilder, etc. I think let's follow that.

Suggested change

public static Builder builder() {

return new Builder();

}

// keep in sync with SecurityManagers.defaultAllowedModules

/// Returns the allowed module patterns that the CLI uses by default.

public static final List<String> defaultAllowedModules =

List.of(

"repl:",

"file:",

"jar:file:",

"modulepath:",

"https:",

"pkl:",

"package:",

"projectpackage:"

);

// keep in sync with SecurityManagers.defaultAllowedResources

/// Returns the allowed resources patterns that the CLI uses by default.

public static final List<String> defaultAllowedResources =

List.of(

"prop:",

"env:",

"file:",

"modulepath:",

"package:",

"projectpackage:",

"https:");

/**

* Creates a builder that has no options set.

*/

public static Builder unconfigured() {

return new Builder();

}

/**

* Creates a builder configured with:

*

* <ul>

* <li>{@link #defaultAllowedModules}</li>

* <li>{@link #defaultAllowedResources}</li>

* <li>System environment variables</li>

* <li>System properties</li>

* </ul>

*/

public static Builder preconfigured() {

//noinspection unchecked,rawtypes

return unconfigured()

.allowedModules(defaultAllowedModules)

.allowedResources(defaultAllowedResources)

.environmentVariables(System.getenv())

.externalProperties((Map) System.getProperties());

}

bioball · 2024-03-12T20:28:49Z

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt

+      .single()
+      .int()
+      .default(-1)
+      .validate { it in 1024..65535 }


Gotcha, thanks for the link. Let's still remove this, though; it adds some unnecessary brittleness to our tests.

bioball · 2024-03-12T20:33:00Z

pkl-core/src/main/java/org/pkl/core/http/HttpClient.java

+     *
+     * <p>If set, requests that specify port 12110 are rewritten to use the given port. This enables
+     * users of this client to connect to a test server without having to know its (ephemeral)
+     * listening port.


It's the same with org.pkl.commons.cli.CliBaseOptions; both are public APIs but expose flags that are only meant for test purposes (e.g. testMode, testPort added by this PR) and we call them "internal" in the doc comments.

translatenix · 2024-03-12T23:47:58Z

We have an existing pattern of having unconfigured() and preconfigured() builders; see EvaluatorBuilder, ConfigEvaluatorBuilder, ValueMapperBuilder, etc. I think let's follow that.

According to the Javadoc, Executor "evaluates Pkl modules in a sandbox".
Does preconfiguring with System.getenv() and System.getProperties() make sense here?
And shouldn't granting file system and network access be an explicit decision?

Apart from adding unconfigured()/preconfigured(),
we could add builder().grantFileSystemAccess(rootDir).grantNetworkAccess().
Or we could stick to a basic builder for now.
Let me know how to proceed.

Let's still remove this, though; it adds some unnecessary brittleness to our tests.

Removed.
Just to clarify, this validation can't possibly cause brittleness. It prevents brittleness by catching the mistake of picking a well-known port as test port. Operating systems would never pick a well-known port as ephemeral port.

both are public APIs but expose flags that are only meant for test purposes (e.g. testMode, testPort added by this PR) and we call them "internal" in the doc comments.

There is a big difference: testMode is only useful for testing Pkl itself, whereas testPort is equally useful for testing third-party code that uses CliBaseOptions or HttpClient. But I won't fight over this one.

bioball · 2024-03-13T00:01:58Z

According to the Javadoc, Executor "evaluates Pkl modules in a sandbox".
Does preconfiguring with System.getenv() and System.getProperties() make sense here?
And shouldn't granting file system and network access be an explicit decision?

I don't see any issues with the set of options provided by preconfigured(). This is an opt-in behavior, and users are expected to know what preconfigured() means. It's actually not that different from using our evaluator in pkl-core (which is also a sandbox).

translatenix · 2024-03-13T01:54:40Z

I don't see any issues with the set of options provided by preconfigured(). This is an opt-in behavior, and users are expected to know what preconfigured() means. It's actually not that different from using our evaluator in pkl-core (which is also a sandbox).

What is Executor used for at Apple? It seems to be intended for different use cases than (say) ConfigEvaluator, where exposing process env vars is a sensible default. What also worries me about preconfigured()/unconfigured() is that it's all or nothing, and that without adding many additional methods, preconfigured() values are impossible to customize. On the other hand, a method such as grantNetworkAccess() is naturally incremental.

This is a comprehensive solution to the "flaky PackageServer tests" problem. It rules out port conflicts and imposes no limits on test parallelism. The same solution can be used for other test servers in the future. Major changes: - Turn `PackageServer` from a singleton into a class that is instantiated per test class or test method. - Start the server the first time its `port` property is read. Bind the server to an ephemeral port instead of port 12110. - For every test that uses `PackageServer`, pass the server port to `--test-port`, `HttpClient.Builder.setTestPort`, the `CliBaseOptions` or `ExecutorOptions` constructor, or the Gradle plugin's `testPort` property. Wire all of these to `RequestRewritingClient`'s `testPort` constructor parameter. - Enhance `RequestRewritingClient` to replace port 12110 with `testPort` in request URIs unless `testPort` is -1 (its default). - Introduce `ExecutorOptions.Builder`. This makes executor options more comfortable to create and allows to hide options such as `testPort`. - Deprecate the `ExecutorOptions` constructor to steer users towards the builder. - Get rid of `ExecutorOptions2`, which is no longer needed. - Clean up `EmbeddedExecutorTest` with the help of the builder.

translatenix · 2024-03-13T04:44:12Z

I've made 2 out of 3 requested changes and have squashed my commits. I'll leave any builder changes to you.

I've also rebased my Gradle 8.6 PR, which I'd love to land by the end of the week.

bioball · 2024-03-13T17:39:12Z

The executor API is typically meant for services that need to evaluate 3rd party Pkl code. For instance, if you are a service that accepts Pkl files as configuration. And, like you said, in those cases, you wouldn't want to pass your own env vars or system properties to Pkl.

The core thing that the executor provides that ConfigEvaluator (or just Evaluator) don't is the ability to use multiple different Pkl versions for evaluation. The rest of the sandboxing is already part of Evaluator/ConfigEvaluator (e.g. allowed modules, allowed resources).

Thought about this a little more--yeah, there's probably not much of a use-case for ExecutorOptions#preconfigured(). I'm okay merging this as-is.

translatenix force-pushed the package-server branch 12 times, most recently from 9bc511f to 17b25d9 Compare February 23, 2024 23:36

translatenix mentioned this pull request Feb 24, 2024

Update to Gradle 8.6 #245

Merged

translatenix force-pushed the package-server branch 2 times, most recently from e8b8074 to e210fe9 Compare February 26, 2024 18:58

translatenix mentioned this pull request Feb 26, 2024

Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection #217

Merged

translatenix force-pushed the package-server branch 7 times, most recently from 46c6edd to a370e41 Compare March 1, 2024 03:08

translatenix force-pushed the package-server branch 3 times, most recently from 99efc41 to d06dfb7 Compare March 7, 2024 22:58

bioball requested changes Mar 7, 2024

View reviewed changes

bioball requested changes Mar 12, 2024

View reviewed changes

translatenix force-pushed the package-server branch 2 times, most recently from d32d02c to 0cb0030 Compare March 13, 2024 04:37

translatenix force-pushed the package-server branch from 0cb0030 to d61e6ee Compare March 13, 2024 04:39

bioball approved these changes Mar 13, 2024

View reviewed changes

bioball merged commit 014b3a8 into apple:main Mar 13, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bind PackageServer to ephemeral port to avoid port conflicts #227

Bind PackageServer to ephemeral port to avoid port conflicts #227

translatenix commented Feb 22, 2024 •

edited

Loading

translatenix commented Mar 7, 2024

bioball left a comment

bioball Mar 7, 2024

translatenix Mar 8, 2024

bioball Mar 12, 2024

bioball Mar 7, 2024

translatenix Mar 8, 2024 •

edited

Loading

bioball Mar 12, 2024

bioball Mar 7, 2024

translatenix Mar 8, 2024 •

edited

Loading

bioball Mar 7, 2024

translatenix Mar 8, 2024

bioball Mar 7, 2024

translatenix Mar 8, 2024 •

edited

Loading

translatenix Mar 8, 2024 •

edited

Loading

translatenix commented Mar 8, 2024

bioball left a comment

bioball Mar 12, 2024

bioball Mar 12, 2024

bioball Mar 12, 2024

translatenix commented Mar 12, 2024

bioball commented Mar 13, 2024

translatenix commented Mar 13, 2024 •

edited

Loading

translatenix commented Mar 13, 2024 •

edited

Loading

bioball commented Mar 13, 2024 •

edited

Loading

	throw new AssertionError(e); // only port changed
	throw PklBugException.unreachableCode(); // only port changed

	* @param testPort API equivalent of the {@code --test-port} CLI option
	* @param testPort Internal option used for testing purposes.

-  public static Builder builder() {
-    return new Builder();
-  }
+  // keep in sync with SecurityManagers.defaultAllowedModules
+  /// Returns the allowed module patterns that the CLI uses by default.
+  public static final List<String> defaultAllowedModules =
+    List.of(
+      "repl:",
+      "file:",
+      "jar:file:",
+      "modulepath:",
+      "https:",
+      "pkl:",
+      "package:",
+      "projectpackage:"
+    );
+  // keep in sync with SecurityManagers.defaultAllowedResources
+  /// Returns the allowed resources patterns that the CLI uses by default.
+  public static final List<String> defaultAllowedResources =
+    List.of(
+      "prop:",
+      "env:",
+      "file:",
+      "modulepath:",
+      "package:",
+      "projectpackage:",
+      "https:");
+  /**
+   * Creates a builder that has no options set.
+   */
+  public static Builder unconfigured() {
+    return new Builder();
+  }
+  /**
+   * Creates a builder configured with:
+   *
+   * <ul>
+   *   <li>{@link #defaultAllowedModules}</li>
+   *   <li>{@link #defaultAllowedResources}</li>
+   *   <li>System environment variables</li>
+   *   <li>System properties</li>
+   * </ul>
+   */
+  public static Builder preconfigured() {
+    //noinspection unchecked,rawtypes
+    return unconfigured()
+      .allowedModules(defaultAllowedModules)
+      .allowedResources(defaultAllowedResources)
+      .environmentVariables(System.getenv())
+      .externalProperties((Map) System.getProperties());
+  }

Bind PackageServer to ephemeral port to avoid port conflicts #227

Bind PackageServer to ephemeral port to avoid port conflicts #227

Conversation

translatenix commented Feb 22, 2024 • edited Loading

translatenix commented Mar 7, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

translatenix Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

translatenix commented Mar 8, 2024

bioball left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

translatenix commented Mar 12, 2024

bioball commented Mar 13, 2024

translatenix commented Mar 13, 2024 • edited Loading

translatenix commented Mar 13, 2024 • edited Loading

bioball commented Mar 13, 2024 • edited Loading

translatenix commented Feb 22, 2024 •

edited

Loading

translatenix Mar 8, 2024 •

edited

Loading

translatenix Mar 8, 2024 •

edited

Loading

translatenix Mar 8, 2024 •

edited

Loading

translatenix Mar 8, 2024 •

edited

Loading

translatenix commented Mar 13, 2024 •

edited

Loading

translatenix commented Mar 13, 2024 •

edited

Loading

bioball commented Mar 13, 2024 •

edited

Loading