pranavshuklaa commented on code in PR #1091:
URL: https://github.com/apache/flink-agents/pull/1091#discussion_r4075937393
##########
runtime/src/main/java/org/apache/flink/agents/runtime/skill/repository/SkillMaterializer.java:
##########
@@ -58,6 +61,104 @@ public final class SkillMaterializer {
private static final int JAR_URL_PREFIX_LEN = "jar:".length();
+ // --- Size caps for download and extraction (issue #1072) ---
+
+ /**
+ * All four resource limits for a single materializer operation, grouped
so tests can inject
+ * small thresholds without touching production defaults and so future
config wiring has a
+ * single object to populate from {@code SkillMaterializerOptions}.
+ *
+ * <p>Production callers use {@link #DEFAULT}; tests construct a small
instance and pass it to
+ * the overloads of {@link SkillMaterializer#downloadToTempFile} and {@link
+ * SkillMaterializer#extractZipSafely} that accept a {@code Limits}
argument.
+ */
+ public static final class Limits {
+ /** Maximum number of bytes accepted from a single HTTP download. */
+ public final long maxDownloadBytes;
+
+ /**
+ * Maximum uncompressed size of any single entry during zip
extraction. Enforced against
+ * actual bytes written, not the attacker-controlled declared size.
+ */
+ public final long maxExtractEntryBytes;
+
+ /**
+ * Maximum cumulative uncompressed bytes written across all entries
during a single zip
+ * extraction. Enforced against actual bytes written.
+ */
+ public final long maxExtractTotalBytes;
+
+ /** Maximum number of entries permitted in a single zip archive. */
+ public final int maxExtractEntries;
+
+ /**
+ * Conservative production defaults. These are intentionally lower
than the original issue
+ * #1072 values to limit disk consumption per materialization,
especially when multiple
+ * skills are materialized concurrently. Deployments that need larger
archives should raise
+ * them explicitly via a {@code Limits} instance.
+ *
+ * <p>TODO: wire these through {@code AgentConfigOptions} / {@code
SkillMaterializerOptions}
+ * so deployments can override them from the YAML config without code
changes (follow-up
+ * PR).
+ */
+ public static final Limits DEFAULT =
+ new Limits(
+ 64L * 1024 * 1024, // 64 MiB download
+ 64L * 1024 * 1024, // 64 MiB per entry
+ 256L * 1024 * 1024, // 256 MiB total extraction
+ 1_000); // entries
+
+ /**
+ * Construct a {@code Limits} instance. All values must be strictly
positive.
+ *
+ * @throws IllegalArgumentException if any value is not strictly
positive.
+ */
+ public Limits(
+ long maxDownloadBytes,
+ long maxExtractEntryBytes,
+ long maxExtractTotalBytes,
+ int maxExtractEntries) {
+ if (maxDownloadBytes <= 0
+ || maxExtractEntryBytes <= 0
+ || maxExtractTotalBytes <= 0
+ || maxExtractEntries <= 0) {
+ throw new IllegalArgumentException("All Limits values must be
strictly positive");
+ }
+ this.maxDownloadBytes = maxDownloadBytes;
+ this.maxExtractEntryBytes = maxExtractEntryBytes;
+ this.maxExtractTotalBytes = maxExtractTotalBytes;
+ this.maxExtractEntries = maxExtractEntries;
+ }
+ }
+
+ /**
+ * Backward-compatible alias for {@link Limits#DEFAULT#maxDownloadBytes}.
+ *
+ * @deprecated Use {@link Limits#DEFAULT} or inject a {@link Limits}
instance.
+ */
+ public static final long MAX_DOWNLOAD_BYTES =
Limits.DEFAULT.maxDownloadBytes;
Review Comment:
That makes sense, thanks for the detailed suggestion! the four limits are
now exposed in AgentConfigOptions (Java) and AgentConfigOptions in
core_options.py, with YAML keys skill.source.url.max-download-bytes,
skill.source.url.max-extract-entry-bytes,
skill.source.url.max-extract-total-bytes, and
skill.source.url.max-extract-entries. All four values must be strictly positive
there is no unlimited setting. The new options are documented in
docs/content/docs/operations/configuration.md under a new "URL Skill Source
Options" section. Removed the MAX_* constants entirely.
Limits.fromConfig(ReadableConfiguration) / limits_from_config(config) build
a Limits / MaterializerLimits from the fourConfigOption values, falling back to
the conservative defaults when a key is absent or config is null. The limits
are now threaded through the full production path in both runtimes:
`ActionExecutionOperator `→ `ResourceCache `→ `ResourceContextImpl `→
`SkillManager `→ `SkillSourceRegistry `→ `URLSkillRepository `/
`FileSystemSkillRepository `→ Limits.fromConfig(config). Tests for fromConfig
are added in both `SkillMaterializerTest.java`
(limitsFromConfigReadsAllFourKeys, limitsFromConfigNullReturnsDefault) and
`test_materialize.py` (TestLimitsFromConfig). Pls suggest if there is an
alternative which would be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
