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.

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 directory resolver to consider CWD and root path input correctly #1840

Merged
merged 10 commits into from
May 25, 2023

Conversation

wagoodman
Copy link
Contributor

@wagoodman wagoodman commented May 23, 2023

There are two issues with the current directory resolver logic:

  • we are blindly cleaning the CWD path in the constructor, but this does not appear to be needed and is causing issues when the CWD is within the root and the root path contains a symlink
  • sometimes we are adding nodes to the filetree during indexing that have symlinks in one of their ancestor paths (the filetree should never have this)

This PR adds several test cases to account for these issues, notably adding a new dimension of changing CWD during testing.

There is probably room for improvement after this PR, but this is a good start.

Similar issues were found in the unindexed directory resolver, however, that has not been fixed in this PR. Only tests added with TODO notes where there are discrepancies (the correct answers are commented out).

@wagoodman wagoodman added the bug Something isn't working label May 23, 2023
Signed-off-by: Alex Goodman <[email protected]>
@github-actions
Copy link

github-actions bot commented May 23, 2023

Benchmark Test Results

Benchmark results from the latest changes vs base branch
goos: linux%0Agoarch: amd64%0Apkg: github.com/anchore/syft/test/integration%0Acpu: Intel(R) Xeon(R) Platinum 8272CL CPU @ 2.60GHz%0A                                                          │ ./.tmp/benchmark-fe89907.txt │%0A                                                          │            sec/op            │%0AImagePackageCatalogers/alpmdb-cataloger-2                                   12.79m ±  2%25%0AImagePackageCatalogers/apkdb-cataloger-2                                    801.3µ ±  7%25%0AImagePackageCatalogers/binary-cataloger-2                                   221.9µ ±  8%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                   640.6µ ±  3%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                              1.322m ±  3%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                         103.6µ ±  3%25%0AImagePackageCatalogers/java-cataloger-2                                     14.69m ±  1%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                     101.5µ ±  1%25%0AImagePackageCatalogers/javascript-package-cataloger-2                       458.6µ ±  2%25%0AImagePackageCatalogers/nix-store-cataloger-2                                306.3µ ±  2%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                   862.4µ ±  3%25%0AImagePackageCatalogers/portage-cataloger-2                                  523.4µ ±  3%25%0AImagePackageCatalogers/python-package-cataloger-2                           3.547m ±  4%25%0AImagePackageCatalogers/r-package-cataloger-2                                232.8µ ± 10%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                   594.7µ ±  3%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                             962.1µ ±  2%25%0AImagePackageCatalogers/sbom-cataloger-2                                     125.0µ ±  2%25%0Ageomean                                                                     670.4µ%0A%0A                                                          │ ./.tmp/benchmark-fe89907.txt │%0A                                                          │             B/op             │%0AImagePackageCatalogers/alpmdb-cataloger-2                                   5.126Mi ± 0%25%0AImagePackageCatalogers/apkdb-cataloger-2                                    205.3Ki ± 0%25%0AImagePackageCatalogers/binary-cataloger-2                                   30.18Ki ± 0%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                   168.9Ki ± 0%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                              405.1Ki ± 0%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                         9.906Ki ± 0%25%0AImagePackageCatalogers/java-cataloger-2                                     2.824Mi ± 0%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                     8.594Ki ± 0%25%0AImagePackageCatalogers/javascript-package-cataloger-2                       100.9Ki ± 0%25%0AImagePackageCatalogers/nix-store-cataloger-2                                49.16Ki ± 0%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                   186.5Ki ± 0%25%0AImagePackageCatalogers/portage-cataloger-2                                  119.9Ki ± 0%25%0AImagePackageCatalogers/python-package-cataloger-2                           1.003Mi ± 0%25%0AImagePackageCatalogers/r-package-cataloger-2                                53.29Ki ± 0%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                   180.9Ki ± 0%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                             144.0Ki ± 0%25%0AImagePackageCatalogers/sbom-cataloger-2                                     14.20Ki ± 0%25%0Ageomean                                                                     132.7Ki%0A%0A                                                          │ ./.tmp/benchmark-fe89907.txt │%0A                                                          │          allocs/op           │%0AImagePackageCatalogers/alpmdb-cataloger-2                                    87.75k ± 0%25%0AImagePackageCatalogers/apkdb-cataloger-2                                     4.182k ± 0%25%0AImagePackageCatalogers/binary-cataloger-2                                     830.0 ± 0%25%0AImagePackageCatalogers/dpkgdb-cataloger-2                                    3.000k ± 0%25%0AImagePackageCatalogers/dotnet-deps-cataloger-2                               6.338k ± 0%25%0AImagePackageCatalogers/go-module-binary-cataloger-2                           281.0 ± 0%25%0AImagePackageCatalogers/java-cataloger-2                                      39.81k ± 0%25%0AImagePackageCatalogers/graalvm-native-image-cataloger-2                       228.0 ± 0%25%0AImagePackageCatalogers/javascript-package-cataloger-2                        1.404k ± 0%25%0AImagePackageCatalogers/nix-store-cataloger-2                                  895.0 ± 0%25%0AImagePackageCatalogers/php-composer-installed-cataloger-2                    4.079k ± 0%25%0AImagePackageCatalogers/portage-cataloger-2                                   2.269k ± 0%25%0AImagePackageCatalogers/python-package-cataloger-2                            16.44k ± 0%25%0AImagePackageCatalogers/r-package-cataloger-2                                  929.0 ± 0%25%0AImagePackageCatalogers/rpm-db-cataloger-2                                    3.989k ± 0%25%0AImagePackageCatalogers/ruby-gemspec-cataloger-2                              2.447k ± 0%25%0AImagePackageCatalogers/sbom-cataloger-2                                       394.0 ± 0%25%0Ageomean                                                                      2.582k

Signed-off-by: Alex Goodman <[email protected]>
@wagoodman wagoodman marked this pull request as ready for review May 24, 2023 15:48
@wagoodman wagoodman enabled auto-merge (squash) May 24, 2023 21:15
@wagoodman wagoodman merged commit 6afbffc into main May 25, 2023
@wagoodman wagoodman deleted the dir-resolver-cwd-symlink-fix branch May 25, 2023 13:41
@wagoodman wagoodman assigned wagoodman and unassigned wagoodman Jun 1, 2023
spiffcs added a commit that referenced this pull request Jun 5, 2023
* main: (21 commits)
  chore(deps): bump github.com/sirupsen/logrus from 1.9.2 to 1.9.3 (#1862)
  chore(deps): bump modernc.org/sqlite from 1.22.1 to 1.23.0 (#1863)
  feat: source-version flag (#1859)
  chore(deps): bump github.com/spf13/viper from 1.15.0 to 1.16.0 (#1851)
  accept main.version ldflags even without vcs (#1855)
  feat: add scope to pom properties (#1779)
  chore(deps): bump github.com/stretchr/testify from 1.8.3 to 1.8.4 (#1852)
  chore(deps): bump github.com/docker/docker (#1849)
  Add test to ensure package metadata is represented in the JSON schema (#1841)
  Fix directory resolver to consider CWD and root path input correctly (#1840)
  Migrate location-related structs to the file package (#1751)
  chore(deps): bump github.com/go-git/go-git/v5 from 5.6.1 to 5.7.0 (#1843)
  fix: add panic recovery for license parse (#1839)
  chore: return both failures when failed to retrieve an image with a scheme (#1801)
  Extract go module versions from ldflags for binaries built by go (#1832)
  fix: duplicate packages, support pnpm lockfile v6 (#1778)
  chore(deps): update stereoscope to e14bc4437b2eac481c5b6f101890b22df4f33596 (#1834)
  chore(deps): bump github.com/stretchr/testify from 1.8.2 to 1.8.3 (#1829)
  chore(deps): bump github.com/docker/docker (#1833)
  Keep original FileInfo persisted on file.Metadata structs (#1794)
  ...

Signed-off-by: Christopher Phillips <[email protected]>
GijsCalis pushed a commit to GijsCalis/syft that referenced this pull request Feb 19, 2024
…nchore#1840)

* [wip] put in initial fix

Signed-off-by: Alex Goodman <[email protected]>

* capture expected behavior of dir resolver in tests

Signed-off-by: Alex Goodman <[email protected]>

* update tests + comments to reflect current dir resolver behavior

Signed-off-by: Alex Goodman <[email protected]>

* add additional test cases

Signed-off-by: Alex Goodman <[email protected]>

* fix linting

Signed-off-by: Alex Goodman <[email protected]>

* fix additional tests

Signed-off-by: Alex Goodman <[email protected]>

* fix bad merge conflict resolution

Signed-off-by: Alex Goodman <[email protected]>

---------

Signed-off-by: Alex Goodman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants