Skip to content
Draft
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 @@ -73,6 +73,10 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import org.apache.beam.sdk.util.BackOff;
import org.apache.beam.sdk.util.BackOffUtils;
import org.apache.beam.sdk.util.FluentBackoff;
import org.apache.beam.sdk.util.Sleeper;
Comment on lines +76 to +79
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

These imports are no longer needed if the manual retry loop in GenerateSBOMRunnable is removed as suggested. Keeping unused imports can lead to confusion and slightly increases the overhead of the class.

import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.maven.artifact.DependencyResolutionRequiredException;
Expand Down Expand Up @@ -1777,15 +1781,36 @@ public GenerateSBOMRunnable(String imagePathTag) {
@Override
public void run() throws Throwable {
LOG.info("Generating system SBOM for {}...", imagePathTag);
String output;
try {
output =
runCommandCapturesOutput(
new String[] {"gcloud", "artifacts", "sbom", "export", "--uri", imagePathTag},
null);
} catch (Exception e) {
throw new RuntimeException("Error generating SBOM.", e);
String output = null;

BackOff backOff =
FluentBackoff.DEFAULT
.withInitialBackoff(org.joda.time.Duration.standardSeconds(5))
.withMaxBackoff(org.joda.time.Duration.standardSeconds(30))
.withMaxRetries(5)
.backoff();
Sleeper sleeper = Sleeper.DEFAULT;
Exception lastException;
do {
try {
output =
runCommandCapturesOutput(
new String[] {"gcloud", "artifacts", "sbom", "export", "--uri", imagePathTag},
null);
lastException = null;
break;
} catch (Exception e) {
LOG.error(
"Error generating SBOM. Retrying after {} seconds...",
backOff.nextBackOffMillis() / 1000,
e);
lastException = e;
}
} while (BackOffUtils.next(sleeper, backOff));
if (lastException != null) {
throw new RuntimeException("Error generating SBOM.", lastException);
}
Comment on lines +1784 to 1812
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The manual retry loop introduced here is redundant and contains a logic bug.

  1. Redundancy: This run() method is already wrapped in a Failsafe retry policy at line 611 (using sbomRetryPolicy()). Adding a second layer of retries here results in nested retry logic (e.g., 5 retries inside 5 retries), which is likely unintended and significantly increases the total execution time on failure.
  2. Logic Bug: Calling backOff.nextBackOffMillis() inside the log statement (line 1805) and then again in BackOffUtils.next() (line 1809) advances the state of the BackOff object twice per iteration. This causes the loop to skip backoff intervals and terminate after fewer attempts than configured.

It is recommended to remove this manual loop and instead update the parameters in sbomRetryPolicy() (lines 1822-1831) if the retry behavior needs adjustment (e.g., to match the 5s-30s backoff used here).

Suggested change
String output = null;
BackOff backOff =
FluentBackoff.DEFAULT
.withInitialBackoff(org.joda.time.Duration.standardSeconds(5))
.withMaxBackoff(org.joda.time.Duration.standardSeconds(30))
.withMaxRetries(5)
.backoff();
Sleeper sleeper = Sleeper.DEFAULT;
Exception lastException;
do {
try {
output =
runCommandCapturesOutput(
new String[] {"gcloud", "artifacts", "sbom", "export", "--uri", imagePathTag},
null);
lastException = null;
break;
} catch (Exception e) {
LOG.error(
"Error generating SBOM. Retrying after {} seconds...",
backOff.nextBackOffMillis() / 1000,
e);
lastException = e;
}
} while (BackOffUtils.next(sleeper, backOff));
if (lastException != null) {
throw new RuntimeException("Error generating SBOM.", lastException);
}
String output;
try {
output =
runCommandCapturesOutput(
new String[] {"gcloud", "artifacts", "sbom", "export", "--uri", imagePathTag},
null);
} catch (Exception e) {
throw new RuntimeException("Error generating SBOM.", e);
}

Copy link
Copy Markdown
Contributor

@Abacn Abacn May 15, 2026

Choose a reason for hiding this comment

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

Yeah it's true that it's already scanned. Checked logs:

[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud
[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud
[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud
[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud
[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud
[INFO] Generating system SBOM for us.gcr.io/dataflow-templates/yaml-template:2026-05-05-00_RC00...
[INFO] Running: gcloud

Error is

ERROR: (gcloud.artifacts.sbom.export) NOT_FOUND: unable to find discovery occurrence for https://us.gcr.io/dataflow-templates/yaml-template@sha256:1d71b93f588eba08cceaa5ec3057439497bf85bbf7189684b47955beab10251e. Please first scan the artifact.

actually it's due to previously docker build gcb job timed out:

ERROR: (gcloud.builds.submit) build a96c34f7-12ae-4bc0-b149-6cae07ceef5b completed with status "TIMEOUT"

but the docker file was pushed, so pushed container existence check, but likely it's actually incomplete and gcloud artfiacts sbom export command would fail.


Matcher matcher = IMAGE_WITH_SBOM_DIGEST.matcher(output);
if (!matcher.find()) {
throw new RuntimeException(
Expand Down
Loading