Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,12 @@ public static class StatusConfig {
@JsonProperty("resource")
private String resource = "/status";

@JsonProperty("min_delay_seconds")
private Integer minDelaySeconds;

@JsonProperty("max_delay_seconds")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Defaulting maxDelaySeconds to a non-null 30 makes the "unset max → 2 * min" fallback in StatusPlugin.start() unreachable through config, and turns a min-only configuration into a validation error.

Jackson only invokes a setter for keys physically present in the config, so an omitted max_delay_seconds retains this 30 default rather than becoming null. As a result:

  • maxDelaySeconds = minDelaySeconds * 2 in start() can only fire if a caller explicitly passes null (as the test does) — never via normal YAML/JSON — contradicting the PR description's "if max_delay_seconds is unset, it now defaults to 2 * min_delay_seconds."
  • A user who sets only min_delay_seconds: 60 (max omitted) keeps max = 30, so validate() fails with min_delay_seconds (60) cannot be greater than max_delay_seconds (30) at startup — even though they never configured a max.

DecisionLogsConfig sidesteps this with split defaults (300/600), and ReportingConfig uses null/null. Consider defaulting maxDelaySeconds (and probably minDelaySeconds) to null here and applying the 30 default in start(), so the documented min-only fallback actually works.

private Integer maxDelaySeconds;

public Boolean getConsole() {
return console;
}
Expand Down Expand Up @@ -482,6 +488,24 @@ public StatusConfig setResource(String resource) {
return this;
}

public Integer getMinDelaySeconds() {
return minDelaySeconds;
}

public StatusConfig setMinDelaySeconds(Integer minDelaySeconds) {
this.minDelaySeconds = minDelaySeconds;
return this;
}

public Integer getMaxDelaySeconds() {
return maxDelaySeconds;
}

public StatusConfig setMaxDelaySeconds(Integer maxDelaySeconds) {
this.maxDelaySeconds = maxDelaySeconds;
return this;
}

@Override
public String toString() {
return "StatusConfig{"
Expand All @@ -493,6 +517,10 @@ public String toString() {
+ ", resource='"
+ resource
+ '\''
+ ", minDelaySeconds="
+ minDelaySeconds
+ ", maxDelaySeconds="
+ maxDelaySeconds
+ '}';
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.Executors;
import java.util.concurrent.RejectedExecutionException;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;
import io.github.open_policy_agent.opa.bundle.Bundle;
import io.github.open_policy_agent.opa.config.Config;
Expand Down Expand Up @@ -48,22 +49,42 @@ public Set<String> validate(PluginManager manager) {
}
}

// Validate delay settings (mirrors BundleDownloader.validatePolling)
Integer min = statusConfig.getMinDelaySeconds();
Integer max = statusConfig.getMaxDelaySeconds();
if (min != null && min < 0) {
errors.add("Status min_delay_seconds must be >= 0");
}
if (max != null && max < 0) {
errors.add("Status max_delay_seconds must be >= 0");
}
if (min != null && max != null && min >= 0 && max >= 0 && min > max) {
errors.add(
"Status min_delay_seconds ("
+ min
+ ") cannot be greater than max_delay_seconds ("
+ max
+ ")");
}

return errors;
}

@Override
public Plugin initialize(PluginManager manager) {
StatusPlugin plugin = new StatusPlugin();
plugin.manager = manager;
plugin.scheduler = Executors.newScheduledThreadPool(1);
plugin.scheduler = BundleDownloader.newPollScheduler("opa-status-scheduler");

Config.StatusConfig statusConfig = manager.getConfig().getStatus();
if (statusConfig != null) {
plugin.status =
new Status(manager, manager.getLogger())
.setConsole(statusConfig.getConsole())
.setService(statusConfig.getService())
.setResource(statusConfig.getResource());
.setResource(statusConfig.getResource())
.setMinDelaySeconds(statusConfig.getMinDelaySeconds())
.setMaxDelaySeconds(statusConfig.getMaxDelaySeconds());
}

return plugin;
Expand All @@ -76,12 +97,70 @@ public void start() {
return;
}

// Report status every 30 seconds (matches OPA default)
scheduler.scheduleAtFixedRate(() -> status.reportStatus(), 0, 30, TimeUnit.SECONDS);
// Get report interval bounds (default: 30 seconds, matching OPA's previous fixed interval;
// OPA Go's status plugin has no standalone min/max delay of its own today - reports are
// triggered by the bundle/discovery plugin's polling - so there's no upstream number to
// mirror here beyond the interval this SDK already used).
// - only min set → max defaults to 2 * min
// - only max set → min defaults to min(30, max) so a max below 30 is not silently ignored
// - neither set → min=30, max=60
Integer configuredMin = status.getMinDelaySeconds();
Integer configuredMax = status.getMaxDelaySeconds();
int minDelaySeconds;
int maxDelaySeconds;
if (configuredMin != null && configuredMax != null) {
minDelaySeconds = configuredMin;
maxDelaySeconds = configuredMax;
} else if (configuredMin != null) {
minDelaySeconds = configuredMin;
maxDelaySeconds = configuredMin * 2;
} else if (configuredMax != null) {
maxDelaySeconds = configuredMax;
minDelaySeconds = Math.min(30, configuredMax);
} else {
minDelaySeconds = 30;
maxDelaySeconds = 60;
}

// Report immediately on startup (matches previous behavior), then continue with a jittered
// chained schedule for subsequent reports - mirrors BundleDownloader.startPolling(), which
// downloads immediately before starting its own chained poll.
scheduler.schedule(() -> status.reportStatus(), 0, TimeUnit.SECONDS);
scheduleNextReport(minDelaySeconds, maxDelaySeconds);

manager.updatePluginStatus("status", PluginManager.Status.OK);
}

// Re-schedules the next status report with a uniformly random delay in [minDelay, maxDelay],
// mirroring BundleDownloader.scheduleNextPoll and DecisionLogPlugin.scheduleNextFlush.
// ScheduledExecutorService has no built-in jitter, so the task chains itself.
// RejectedExecutionException after a shutdown breaks the chain cleanly.
private void scheduleNextReport(int minDelay, int maxDelay) {
long delay =
minDelay >= maxDelay
? minDelay
: ThreadLocalRandom.current().nextLong(minDelay, (long) maxDelay + 1);
try {
scheduler.schedule(
() -> {
try {
status.reportStatus();
} catch (Exception e) {
// reportStatus() handles its own logging; swallow so the chain keeps reporting.
// Only Exception is caught here — Errors (OOM, etc.) propagate and let the
// executor's uncaught-exception handler tear down the pool, which is the right
// outcome for unrecoverable conditions.
} finally {
scheduleNextReport(minDelay, maxDelay);
}
},
delay,
TimeUnit.SECONDS);
} catch (RejectedExecutionException stopped) {
// Scheduler was shut down; let the chain end.
}
}

@Override
public void stop() {
if (scheduler != null) {
Expand Down Expand Up @@ -109,6 +188,8 @@ public static class Status {
private Boolean console;
private String service;
private String resource;
private Integer minDelaySeconds;
private Integer maxDelaySeconds;

private Status(PluginManager manager, Logger logger) {
this.manager = manager;
Expand Down Expand Up @@ -142,6 +223,24 @@ public Status setResource(String resource) {
return this;
}

public Integer getMinDelaySeconds() {
return minDelaySeconds;
}

public Status setMinDelaySeconds(Integer minDelaySeconds) {
this.minDelaySeconds = minDelaySeconds;
return this;
}

public Integer getMaxDelaySeconds() {
return maxDelaySeconds;
}

public Status setMaxDelaySeconds(Integer maxDelaySeconds) {
this.maxDelaySeconds = maxDelaySeconds;
return this;
}

/** Collect and report current status. */
void reportStatus() {
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,8 @@ void config_statusDefaults() {

assertFalse(status.getConsole());
assertEquals("/status", status.getResource());
assertNull(status.getMinDelaySeconds());
assertNull(status.getMaxDelaySeconds());
}

@Test
Expand Down
Loading