Add feature flag to enable shared cache#39
Conversation
github-project-automation
bot
moved this to Milestone 1
in Scalable Cloud Tools Project
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds build and dependency remote-caching configuration: new operator arguments and accessors, propagates them into template replacement placeholders (including TLS/trust bundle placeholders), and updates deployment templates with cache-related env vars, optional trust-bundle volume/mount, and Gradle/CA envs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as Operator CLI
participant Operator as TheiaCloudOperator
participant Replacer as DefaultDeploymentTemplateReplacements
participant K8s as Kubernetes API
participant Container as App Container
CLI->>Operator: start with args (enableBuildCaching, buildCacheUrl, enableBuildCachePush, enableDependencyCaching, dependencyCacheUrl)
Operator->>Replacer: provide AppDefinition + caching flags/URLs
Replacer->>Replacer: set placeholders (BUILD_/DEPENDENCY_* envs, GRADLE_OPTS, NODE_EXTRA_CA_CERTS, trust bundle mount/config if HTTPS)
Replacer->>K8s: submit generated Deployment manifest
K8s->>Container: container started with cache envs and optional trust-bundle mounted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
KevinGruber2001
moved this from Milestone 1
to In Progress
in Scalable Cloud Tools Project
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/AddedHandlerUtil.java`:
- Around line 57-61: Remove the unused imports in AddedHandlerUtil: delete the
import lines for ConfigMapVolumeSource, ObjectMeta, OwnerReference, Volume, and
VolumeMount since they are not referenced anywhere in the AddedHandlerUtil
class; update the import block accordingly so only used classes remain and run a
quick compile or IDE organize-imports to ensure no other unused imports remain.
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/session/EagerSessionHandler.java`:
- Around line 165-175: The comment and warning in EagerSessionHandler around the
try block misleadingly state that a per-session gradle.properties/init config is
created and mounted, but AddedHandlerUtil.configureGradleCaching only injects
environment variables; update the comment and the LOGGER.warn message to
accurately state that configureGradleCaching injects env vars into the
deployment (refer to AddedHandlerUtil.configureGradleCaching and
deploymentName), or if mounting gradle.properties was intended implement the
mount logic instead of calling configureGradleCaching; adjust the text in the
try-block comment and the formatLogMessage call so logs reflect the actual
behavior.
🧹 Nitpick comments (3)
java/operator/.idea/workspace.xml (1)
1-24: IDE configuration file should not be committed.The
.idea/workspace.xmlfile contains user-specific and machine-specific IntelliJ IDEA settings that should not be tracked in version control. These files can cause merge conflicts and clutter the repository.♻️ Recommended fix
Remove this file from the PR:
git rm --cached java/operator/.idea/workspace.xmlEnsure
.idea/workspace.xmlis in your.gitignore:+**/workspace.xmlNote: Project-level
.ideafiles (likecodeStyles,runConfigurations) can be shared, butworkspace.xmlcontains user-specific settings and should always be excluded.java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorArguments.java (1)
125-131: Consider defaultingenableCachingtofalsefor safer rollout.Enabling caching by default (
enableCaching = true) changes behavior for all existing deployments upon upgrade. This could cause unexpected issues if the cache service isn't deployed. A safer approach would be to default tofalseand require explicit opt-in.Additionally, the default
cacheUrluses HTTP. While this may be acceptable for internal cluster communication, the PR description mentions "switching the default cache URL to use TLS." If HTTPS is intended, update the default accordingly.Suggested changes
`@Option`(names = { "--enableCaching" }, description = "Whether to enable caching of Theia application containers.", required = false) -private boolean enableCaching = true; +private boolean enableCaching = false; `@Option`(names = { - "--cacheUrl" }, description = "The URL of the remote cache server.", required = false, defaultValue = "http://theia-cloud-combined-cache:8080/cache/") + "--cacheUrl" }, description = "The URL of the remote cache server.", required = false, defaultValue = "https://theia-cloud-combined-cache:8080/cache/") private String cacheUrl;java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/AddedHandlerUtil.java (1)
260-317: Implementation looks correct, with a few considerations.The method properly configures Gradle caching environment variables. A few observations:
Duplicate env vars risk: If this method is called multiple times on the same deployment (e.g., during retries), it will add duplicate environment variables. Consider checking for existing env vars before adding or clearing them first.
Hardcoded push permission:
GRADLE_REMOTE_CACHE_PUSHis hardcoded to"true". The comment at line 306 acknowledges this could be made configurable. For a read-only cache scenario, this would need to befalse.Optional: Prevent duplicate env vars
+ private static void setOrReplaceEnvVar(Container container, String name, String value) { + container.getEnv().removeIf(env -> name.equals(env.getName())); + EnvVar envVar = new EnvVar(); + envVar.setName(name); + envVar.setValue(value); + container.getEnv().add(envVar); + } + public static void configureGradleCaching(String correlationId, Deployment deployment, AppDefinition appDefinition, TheiaCloudOperatorArguments arguments) { // ... existing code to find container and init env list ... - // Always set the ENABLED variable for explicit control - EnvVar cacheEnabledEnv = new EnvVar(); - cacheEnabledEnv.setName("GRADLE_REMOTE_CACHE_ENABLED"); - cacheEnabledEnv.setValue(String.valueOf(cachingEnabled)); - container.getEnv().add(cacheEnabledEnv); + setOrReplaceEnvVar(container, "GRADLE_REMOTE_CACHE_ENABLED", String.valueOf(cachingEnabled)); if (cachingEnabled) { - // Set cache URL - EnvVar cacheUrlEnv = new EnvVar(); - cacheUrlEnv.setName("GRADLE_REMOTE_CACHE_URL"); - cacheUrlEnv.setValue(arguments.getCacheUrl().trim()); - container.getEnv().add(cacheUrlEnv); - - // Set push permission (default: true, could be made configurable) - EnvVar cachePushEnv = new EnvVar(); - cachePushEnv.setName("GRADLE_REMOTE_CACHE_PUSH"); - cachePushEnv.setValue("true"); - container.getEnv().add(cachePushEnv); + setOrReplaceEnvVar(container, "GRADLE_REMOTE_CACHE_URL", arguments.getCacheUrl().trim()); + setOrReplaceEnvVar(container, "GRADLE_REMOTE_CACHE_PUSH", "true"); // ... } }
....cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/AddedHandlerUtil.java
Outdated
Show resolved
Hide resolved
...ator/src/main/java/org/eclipse/theia/cloud/operator/handler/session/EagerSessionHandler.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/AddedHandlerUtil.java`:
- Around line 253-295: The configureRemoteCaching method currently always
appends REMOTE_CACHE_* EnvVar entries causing duplicates and leaving stale
REMOTE_CACHE_URL/REMOTE_CACHE_PUSH when caching is turned off; update
configureRemoteCaching to upsert env vars by name in the container's env list
(use findContainerIdxInDeployment to locate the container and container.getEnv()
to operate on the list), i.e. if an EnvVar with name
"REMOTE_CACHE_ENABLED"/"REMOTE_CACHE_URL"/"REMOTE_CACHE_PUSH" exists update its
value, otherwise add it; when cachingEnabled is false ensure you remove any
existing "REMOTE_CACHE_URL" and "REMOTE_CACHE_PUSH" entries (and only
set/override "REMOTE_CACHE_ENABLED" to "false"), avoiding duplicate entries on
redeploys.
🧹 Nitpick comments (1)
java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/session/LazySessionHandler.java (1)
472-476: Fix indentation to match the surrounding lambda body.The new code has inconsistent indentation compared to the rest of the lambda body (lines 459-470 and 478-481). This should be aligned to match the existing code style within the lambda.
Additionally, the comment on line 475 is slightly misleading—
configureRemoteCachingis called unconditionally and always setsREMOTE_CACHE_ENABLED(to either"true"or"false"). Consider rewording to clarify that configuration is always applied:Suggested fix for indentation and comment
- AddedHandlerUtil.addCustomEnvVarsToDeploymentFromSession(correlationId, deployment, session, - appDefinition); - - // If caching is enabled, configure remote build cache via environment variables - AddedHandlerUtil.configureRemoteCaching(correlationId, deployment, appDefinition, arguments); + AddedHandlerUtil.addCustomEnvVarsToDeploymentFromSession(correlationId, deployment, session, + appDefinition); + + // Configure remote build cache environment variables (enabled/disabled based on arguments) + AddedHandlerUtil.configureRemoteCaching(correlationId, deployment, appDefinition, arguments);
....cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/handler/AddedHandlerUtil.java
Outdated
Show resolved
Hide resolved
lukaskratzel
left a comment
lukaskratzel
left a comment
There was a problem hiding this comment.
I think you could handle this in a simpler way by setting the environment variables directly in the deployment template, which I have done so far for all my changes touching on environment variables. You can find the relevant template file here: java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeployment.yaml
The replacements for the templates are handled through this file: java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/util/TheiaCloudHandlerUtil.java.
But if that doesn't work for you we can also try this approach.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/replacements/DefaultDeploymentTemplateReplacements.java`:
- Around line 162-186: The cache URL is inconsistently handled: trim the value
once and reuse it rather than calling arguments.getCacheUrl() twice; change both
checks to use a local String (e.g., cacheUrl = arguments.getCacheUrl() == null ?
"" : arguments.getCacheUrl().trim()) and then use cacheUrl.isEmpty() and
cacheUrl.startsWith("https://") for the HTTPS branch; update the conditional
expressions that set PLACEHOLDER_ENV_CACHE_* and the trust bundle block in
DefaultDeploymentTemplateReplacements to reference this trimmed cacheUrl and the
stricter startsWith("https://") check to avoid mismatches and duplication.
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeployment.yaml`:
- Around line 100-107: The template currently always emits a volumeMount for
name "trust-bundle" with mountPath set to the placeholder
"placeholder-trust-bundle-mount-path", which can become an empty string and
produce an invalid pod spec; update the templateDeployment.yaml to omit the
entire volumeMount entry when the trust bundle is disabled (i.e., when the
placeholder resolves to empty). Specifically, make the volumeMount conditional
(or filter out entries) so that the "trust-bundle" volumeMount is only included
when the NODE_EXTRA_CA_CERTS/GRADLE_OPTS trust configuration provides a
non-empty mount path, or implement a post-render step that removes volumeMounts
with an empty mountPath before creating the Deployment.
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeploymentWithoutOAuthProxy.yaml`:
- Around line 73-78: The REMOTE_CACHE_PUSH environment variable in
templateDeploymentWithoutOAuthProxy.yaml allows anonymous cache push which can
enable untrusted clients to inject poisoned cache entries; change the default
placeholder for REMOTE_CACHE_PUSH to "false" (disable push by default) in the
Deployment template (the REMOTE_CACHE_PUSH env entry) and add a prominent note
in the README/PR description indicating that enabling push requires
authenticated remote-cache access and must not be enabled without proper auth;
ensure any rollout/upgrade logic that reads REMOTE_CACHE_PUSH treats
missing/placeholder values as false.
- Around line 85-92: The template renders empty trust-bundle env vars and a
volumeMount even when caching/remote cache is disabled, producing invalid
Kubernetes YAML (empty NODE_EXTRA_CA_CERTS, GRADLE_OPTS, and mountPath). Update
the templateDeploymentWithoutOAuthProxy.yaml to conditionally omit the
NODE_EXTRA_CA_CERTS and GRADLE_OPTS env entries and the trust-bundle volumeMount
(name "trust-bundle" / mountPath) unless remote caching is enabled (or the
resolved PLACEHOLDER_TRUST_BUNDLE_MOUNT_PATH is non-empty); alternatively move
the guard into the code that replaces PLACEHOLDER_CA_BUNDLE_PEM_PATH,
PLACEHOLDER_GRADLE_TRUST_OPTS, and PLACEHOLDER_TRUST_BUNDLE_MOUNT_PATH so they
are not set to empty strings but removed when REMOTE_CACHE_ENABLED is
false—ensure the volumeMount and env vars are only rendered when the mountPath
and trust options are present.
🧹 Nitpick comments (2)
java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/replacements/DefaultDeploymentTemplateReplacements.java (1)
176-180: Hardcoded paths, configmap name, and truststore password.The trust bundle mount path, PEM path, JKS path, configmap name (
theia-internal-trust), and truststore password (changeit) are all hardcoded inline. Consider:
- Extracting these as named constants at minimum for maintainability and grep-ability.
- Making the configmap name configurable via operator arguments, since cluster operators may already have a trust bundle configmap under a different name.
- The
changeitpassword is Java's well-known default, but embedding credentials in source is a code smell. If this JKS is generated at runtime, document that assumption; otherwise consider making the password configurable.java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeployment.yaml (1)
86-91: Cache and trust-bundle configuration is added unconditionally to all deployments.The remote cache env vars (lines 86–91), trust-bundle volumeMount (lines 104–107), and trust-bundle volume (lines 123–126) are baked into the template regardless of whether the
enableCachingflag is set. This means every Theia deployment will carry these entries even when caching is not used.This is acceptable if the placeholder replacement logic substitutes benign defaults (e.g., empty strings,
"false") when caching is disabled. However, it adds noise to every deployment manifest. Consider whether conditional injection at the Java level (similar to howplaceholder-monitor-portappears to be handled) would be cleaner for keeping non-caching deployments uncluttered.Also applies to: 123-126
| if (arguments.isEnableCaching() && arguments.getCacheUrl() != null | ||
| && !arguments.getCacheUrl().trim().isEmpty()) { | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "true"); | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, arguments.getCacheUrl().trim()); | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "true"); | ||
| } else { | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "false"); | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, ""); | ||
| environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "false"); | ||
| } | ||
|
|
||
| if (arguments.isEnableCaching() && arguments.getCacheUrl() != null | ||
| && arguments.getCacheUrl().startsWith("https")) { | ||
| // Only set trust config when using HTTPS for the cache | ||
| environmentVariables.put(PLACEHOLDER_CA_BUNDLE_PEM_PATH, "/etc/ssl/theia-trust/trust-bundle.pem"); | ||
| environmentVariables.put(PLACEHOLDER_GRADLE_TRUST_OPTS, | ||
| "-Djavax.net.ssl.trustStore=/etc/ssl/theia-trust/truststore.jks -Djavax.net.ssl.trustStorePassword=changeit"); | ||
| environmentVariables.put(PLACEHOLDER_TRUST_BUNDLE_MOUNT_PATH, "/etc/ssl/theia-trust"); | ||
| environmentVariables.put(PLACEHOLDER_TRUST_BUNDLE_CONFIGMAP, "theia-internal-trust"); | ||
| } else { | ||
| environmentVariables.put(PLACEHOLDER_CA_BUNDLE_PEM_PATH, ""); | ||
| environmentVariables.put(PLACEHOLDER_GRADLE_TRUST_OPTS, ""); | ||
| environmentVariables.put(PLACEHOLDER_TRUST_BUNDLE_MOUNT_PATH, ""); | ||
| environmentVariables.put(PLACEHOLDER_TRUST_BUNDLE_CONFIGMAP, ""); | ||
| } |
There was a problem hiding this comment.
Inconsistent URL handling between the two caching blocks.
Two issues:
-
Missing
trim()in the HTTPS check (Line 174): The first block (Line 165) trims the URL before use, but the second block checksarguments.getCacheUrl().startsWith("https")on the raw value. A URL with leading whitespace (e.g.," https://...") would pass the first block but fail the HTTPS detection. -
startsWith("https")is too loose (Line 174): This would also match nonsensical values like"httpsnotaurl". UsestartsWith("https://")instead.
Consider extracting the trimmed URL and the shared condition to reduce duplication:
Proposed fix
- if (arguments.isEnableCaching() && arguments.getCacheUrl() != null
- && !arguments.getCacheUrl().trim().isEmpty()) {
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "true");
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, arguments.getCacheUrl().trim());
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "true");
- } else {
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "false");
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, "");
- environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "false");
- }
-
- if (arguments.isEnableCaching() && arguments.getCacheUrl() != null
- && arguments.getCacheUrl().startsWith("https")) {
+ String cacheUrl = arguments.getCacheUrl() != null ? arguments.getCacheUrl().trim() : "";
+ boolean cachingEnabled = arguments.isEnableCaching() && !cacheUrl.isEmpty();
+
+ if (cachingEnabled) {
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "true");
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, cacheUrl);
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "true");
+ } else {
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_ENABLED, "false");
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_URL, "");
+ environmentVariables.put(PLACEHOLDER_ENV_CACHE_PUSH, "false");
+ }
+
+ if (cachingEnabled && cacheUrl.startsWith("https://")) {🤖 Prompt for AI Agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/replacements/DefaultDeploymentTemplateReplacements.java`
around lines 162 - 186, The cache URL is inconsistently handled: trim the value
once and reuse it rather than calling arguments.getCacheUrl() twice; change both
checks to use a local String (e.g., cacheUrl = arguments.getCacheUrl() == null ?
"" : arguments.getCacheUrl().trim()) and then use cacheUrl.isEmpty() and
cacheUrl.startsWith("https://") for the HTTPS branch; update the conditional
expressions that set PLACEHOLDER_ENV_CACHE_* and the trust bundle block in
DefaultDeploymentTemplateReplacements to reference this trimmed cacheUrl and the
stricter startsWith("https://") check to avoid mismatches and duplication.
java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeployment.yaml
Show resolved
Hide resolved
| - name: REMOTE_CACHE_ENABLED | ||
| value: placeholder-cache-enabled | ||
| - name: REMOTE_CACHE_URL | ||
| value: placeholder-cache-url | ||
| - name: REMOTE_CACHE_PUSH | ||
| value: placeholder-cache-push |
There was a problem hiding this comment.
Security note: anonymous cache push without authorization.
The PR description acknowledges that no authorization is configured, requiring anonymous access on the remote cache. However, REMOTE_CACHE_PUSH being enabled with anonymous access means any client (not just trusted builds) could push poisoned cache entries. Consider documenting this risk prominently or defaulting REMOTE_CACHE_PUSH to false until authentication is implemented.
🤖 Prompt for AI Agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/resources/templateDeploymentWithoutOAuthProxy.yaml`
around lines 73 - 78, The REMOTE_CACHE_PUSH environment variable in
templateDeploymentWithoutOAuthProxy.yaml allows anonymous cache push which can
enable untrusted clients to inject poisoned cache entries; change the default
placeholder for REMOTE_CACHE_PUSH to "false" (disable push by default) in the
Deployment template (the REMOTE_CACHE_PUSH env entry) and add a prominent note
in the README/PR description indicating that enabling push requires
authenticated remote-cache access and must not be enabled without proper auth;
ensure any rollout/upgrade logic that reads REMOTE_CACHE_PUSH treats
missing/placeholder values as false.
...org.eclipse.theia.cloud.operator/src/main/resources/templateDeploymentWithoutOAuthProxy.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@java/operator/org.eclipse.theia.cloud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorArguments.java`:
- Around line 129-145: Add a validation step that throws when a caching flag is
enabled but its URL is missing: implement a public validate() method on
TheiaCloudOperatorArguments that checks enableBuildCaching implies buildCacheUrl
is non-null/non-blank and enableDependencyCaching implies dependencyCacheUrl is
non-null/non-blank, and throw IllegalArgumentException with a clear message
(e.g., "--buildCacheUrl is required when --enableBuildCaching is set") when the
checks fail; ensure callers (CLI bootstrap) invoke
TheiaCloudOperatorArguments.validate() after parsing arguments.
...ava/org/eclipse/theia/cloud/operator/replacements/DefaultDeploymentTemplateReplacements.java
Show resolved
Hide resolved
...oud.operator/src/main/java/org/eclipse/theia/cloud/operator/TheiaCloudOperatorArguments.java
Show resolved
Hide resolved
KevinGruber2001
moved this from In Progress
to In Review
in Scalable Cloud Tools Project
2 participants
In this PR a feature flag is added to the Theia Cloud Operator, to mount a gradle configureation file into the Thea IDEs to enable them to access the remote cache.
At the moment, no authorization is configured. Which means that the Remote Gradle Cache needs to allow anonymouse access
Summary by CodeRabbit