Skip to content

Change platform selection logic#189

Merged
willmurphyscode merged 8 commits intomainfrom
revert-152
Jun 9, 2023
Merged

Change platform selection logic#189
willmurphyscode merged 8 commits intomainfrom
revert-152

Conversation

@willmurphyscode
Copy link
Copy Markdown
Contributor

This PR a couple things going on:

I wanted to point out a couple things for discussion:

  1. It's not 100% clear to me that Stereoscope pulls different images when using docker: vs registry: from multi-platform images  #149 represented incorrect behavior. I could see an argument that making the registry provider imitate docker's platform selection was the wrong call.
  2. This pull request could be 2 different ones, one to fix Platform selection in docker client has unnecessary error #188, and one to re-fix Stereoscope pulls different images when using docker: vs registry: from multi-platform images  #149.
  3. The function defaultPlatformIfNil intentionally swallows an error if it occurs. That's because we're trying to guess a default behavior here, and I don't think an attempt to infer the desired default should ever return an error.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
#152)"

This reverts commit d7551b7.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
This was previously fixed by PR 152, but that PR introduced another bug.
Re-implement platform defaulting after the revert of PR 152.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 9, 2023

Benchmark Test Results

Benchmark results from the latest changes vs base branch
latest: Pulling from library/ubuntu
tar: Option --mtime: Treating date 'UTC 2019-09-16' as 2019-09-16 00:00:00
goos: linux
goarch: amd64
pkg: github.com/anchore/stereoscope/pkg/file
cpu: Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz
docker: 
           │ ./.tmp/benchmark-f29f84a.txt │
           │            sec/op            │
TarIndex-2                   49.43µ ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

           │ ./.tmp/benchmark-f29f84a.txt │
           │             B/op             │
TarIndex-2                  5.561Ki ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

           │ ./.tmp/benchmark-f29f84a.txt │
           │          allocs/op           │
TarIndex-2                    93.00 ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

pkg: github.com/anchore/stereoscope/test/integration
                                      │ ./.tmp/benchmark-f29f84a.txt │
                                      │            sec/op            │
SimpleImage_GetImage/docker-archive-2                   1.968m ± ∞ ¹
SimpleImage_GetImage/oci-archive-2                      1.935m ± ∞ ¹
SimpleImage_GetImage/oci-dir-2                          1.400m ± ∞ ¹
geomean                                                 1.747m
¹ need >= 6 samples for confidence interval at level 0.95

                                      │ ./.tmp/benchmark-f29f84a.txt │
                                      │             B/op             │
SimpleImage_GetImage/docker-archive-2                  356.9Ki ± ∞ ¹
SimpleImage_GetImage/oci-archive-2                     646.3Ki ± ∞ ¹
SimpleImage_GetImage/oci-dir-2                         413.6Ki ± ∞ ¹
geomean                                                456.9Ki
¹ need >= 6 samples for confidence interval at level 0.95

                                      │ ./.tmp/benchmark-f29f84a.txt │
                                      │          allocs/op           │
SimpleImage_GetImage/docker-archive-2                   2.649k ± ∞ ¹
SimpleImage_GetImage/oci-archive-2                      1.567k ± ∞ ¹
SimpleImage_GetImage/oci-dir-2                          1.351k ± ∞ ¹
geomean                                                 1.777k
¹ need >= 6 samples for confidence interval at level 0.95

docker: Error response from daemon: Get "http://localhost/v2/": dial tcp [::1]:80: connect: connection refused.
                                                   │ ./.tmp/benchmark-f29f84a.txt │
                                                   │            sec/op            │
SimpleImage_FetchSquashedContents/docker-archive-2                   22.55µ ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

                                                   │ ./.tmp/benchmark-f29f84a.txt │
                                                   │             B/op             │
SimpleImage_FetchSquashedContents/docker-archive-2                  2.648Ki ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

                                                   │ ./.tmp/benchmark-f29f84a.txt │
                                                   │          allocs/op           │
SimpleImage_FetchSquashedContents/docker-archive-2                    21.00 ± ∞ ¹
¹ need >= 6 samples for confidence interval at level 0.95

Copy link
Copy Markdown
Contributor

@spiffcs spiffcs left a comment

Choose a reason for hiding this comment

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

Initial nit comments added - I fully agree with the changes/reverts done to selectImageProvider - I think this clears up the messaging and intent for users and puts it in a better spot than it is currently.

For discussion points:

  1. I agree and think in hindsight making registery imitate the docker platform selection is the wrong call. Maybe we can settle on some good language to add to some part of the documentation expressing why they're different and the intention behind the two prefix

  2. I think this is fine as a single PR because there is some overlap between the two and it's not and overly complex revert/change =)

  3. Oh no! I should have read this before I added my nit comment - This resonates with me and I'm good with the logic of if we're deciding a default then we commit and make a choice and never error. I'm fine with it being it's own function vs a method on the config just wanted to point out the other option since it's a mutation that doesn't return anything =)

Comment thread client.go
Comment thread client.go
Comment thread test/integration/platform_test.go
Comment thread test/integration/platform_test.go
Previously, the test would loop over the RepoDigest field in the image
manifest to prove that the expect digest was present. However, in some
cases, the RepoDigests field can be empty. Therefore, just use the ID
from the manfiest, since this seems consisent across image sources, and
is always present.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
Signed-off-by: Will Murphy <will.murphy@anchore.com>
@willmurphyscode willmurphyscode marked this pull request as ready for review June 9, 2023 18:37
@willmurphyscode willmurphyscode merged commit 5b5049b into main Jun 9, 2023
@willmurphyscode willmurphyscode deleted the revert-152 branch June 9, 2023 19:05
gnmahanth pushed a commit to deepfence/stereoscope that referenced this pull request Jun 15, 2023
This reverts commit d7551b7.

Defaulting to platform for all providers resulted in syft having unnecessary
when pulling an image that had a particular digest, if that digest didn't match
the architecture of the host running the pull. Revert commit
d7551b7, which introduced that error, but then
add platform defaulting logic back for the OCI Registry Provider, since
defaulting there has been specifically requested. Add integ tests to cover the
new behavior. Also, update integ tests to use the manifest ID for assertsion,
since the RepoDigests array can be empty.

Signed-off-by: Will Murphy <will.murphy@anchore.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants