Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the surrounding loop is unconditionally terminated (SA4004) #1986

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Fix the surrounding loop is unconditionally terminated (SA4004) #1986

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 17 additions & 11 deletions drivers/overlay/overlay.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2532,6 +2532,19 @@ func nameWithSuffix(name string, number int) string {
return fmt.Sprintf("%s%d", name, number)
}

func validateOneAdditionalLayerPath(target string) error {
for _, p := range []string{
filepath.Join(target, "diff"),
filepath.Join(target, "info"),
filepath.Join(target, "blob"),
} {
if err := fileutils.Exists(p); err != nil {
return err
}
}
return nil
}

func (d *Driver) getAdditionalLayerPath(tocDigest digest.Digest, ref string) (string, error) {
refElem := base64.StdEncoding.EncodeToString([]byte(ref))
for _, ls := range d.options.layerStores {
Expand All @@ -2540,18 +2553,11 @@ func (d *Driver) getAdditionalLayerPath(tocDigest digest.Digest, ref string) (st
ref = refElem
}
target := path.Join(ls.path, ref, tocDigest.String())
// Check if all necessary files exist
for _, p := range []string{
filepath.Join(target, "diff"),
filepath.Join(target, "info"),
filepath.Join(target, "blob"),
} {
if err := fileutils.Exists(p); err != nil {
wrapped := fmt.Errorf("failed to stat additional layer %q: %w", p, err)
return "", fmt.Errorf("%v: %w", wrapped, graphdriver.ErrLayerUnknown)
}
err := validateOneAdditionalLayerPath(target)
if err == nil {
return target, nil
}
return target, nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

@giuseppe @ktock “Additional layer stores” support multiple stores, but apparently the code was only able to work with the first one, since the very start of the feature ( 64f0181 ).

What do you think about simplifying to only support one store? Admittedly that would not help simplify the codebase all that much, ALS is AFAICS only a feature within this driver and ~this one loop — and instead, we would need to add even more code to reject multiple stores when parsing options.

Either way, we seem not to have tests sufficiently covering the feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about simplifying to only support one store?

SGTM

logrus.Debugf("additional Layer Store %v failed to stat additional layer: %v", ls, err)
}

return "", fmt.Errorf("additional layer (%q, %q) not found: %w", tocDigest, ref, graphdriver.ErrLayerUnknown)
Expand Down