fix: wrong error message about missing symlink #138

mariux · 2022-01-05T21:06:25Z

fix: wrong error message about missing symlink

includes #140

run.go

mariux · 2022-01-05T23:47:30Z

katcipis · 2022-01-06T08:29:13Z

@katcipis @i4ki how to fix tests?

It seems that output is being writen on "stderr" (it is injected on the cli logic) on the tests, but on the tests the log level should be the default "none" no ? Overall it is failing because the tests are pedantic with output being on stderr/stdout, unless the test explicitely commands to ignore them.

Edited: Ah just saw that the default is now "info" not "none", so in this case the tests should ignore stderr when doing validation of command execution or we can change the log level on the tests. I think changing the log level on the tests for none will be the fastest/safest fix ASAP.

Edited 2: the way it is failing is quite bad, will improve the error message.

Edited 3 =P: now looking at the code and the test result, it should have been a different error message (more clear), so Im not quite sure what is happening, will work on a branch from this one and will let you know as soon as I got more concrete info.

manager.go

katcipis

Some comments regarding calling Fatal inside the terramate.Manager.

cmd/terramate/cli/cli.go

manager.go

codecov-commenter · 2022-01-06T17:29:37Z

Codecov Report

Merging #138 (b170b93) into main (32995b8) will decrease coverage by 1.89%.
The diff coverage is 5.40%.

❗ Current head b170b93 differs from pull request most recent head 844c17c. Consider uploading reports for the commit 844c17c to get more accurate results

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
- Coverage   55.32%   53.42%   -1.90%     
==========================================
  Files          29       29              
  Lines        3796     3751      -45     
==========================================
- Hits         2100     2004      -96     
- Misses       1549     1600      +51     
  Partials      147      147

Flag	Coverage Δ
tests	`53.42% <5.40%> (-1.90%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/terramate/cli/cli.go	`0.00% <0.00%> (ø)`
cmd/terramate/main.go	`0.00% <0.00%> (ø)`
manager.go	`46.79% <0.00%> (-1.66%)`	⬇️
run.go	`0.00% <0.00%> (ø)`
project/project.go	`34.14% <23.52%> (-2.22%)`	⬇️
test/exec.go	`0.00% <0.00%> (-62.50%)`	⬇️
hcl/printer.go	`62.50% <0.00%> (-37.50%)`	⬇️
test/sandbox/sandbox.go	`50.68% <0.00%> (-32.88%)`	⬇️
test/sandbox/git.go	`57.00% <0.00%> (-9.35%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32995b8...844c17c. Read the comment docs.

kassianh

lgtm :)

cmd/terramate/cli/cli.go

i4ki

The evalSymlinks is needed because our tests pass a --chdir <path> from t.TempDir() and in the OSX platform this function returns paths inside the /var link that points to /private/var whereas the paths returned from git command are all resolved, so they mismatch and tests fails. If you don't want to automatically resolve user paths by default, then you need to fix the tests to always resolve the temp dir passed as --chdir before calling terramate. The other changes are unrelated to this problem, but fine for me.

Having said that, the fix can be just:

diff --git a/cmd/terramate/cli/cli.go b/cmd/terramate/cli/cli.go
index 970c5b7..d8b2d60 100644
--- a/cmd/terramate/cli/cli.go
+++ b/cmd/terramate/cli/cli.go
@@ -202,8 +202,11 @@ func newCLI(

        configureLogging(logLevel, logFmt, stderr)

-       log.Trace().
+       logger := log.With().
                Str("action", "newCli()").
+               Logger()
+
+       logger.Trace().
                Msg("Get working directory.")
        wd := parsedArgs.Chdir
        if wd == "" {
@@ -213,23 +216,25 @@ func newCLI(
                }
        }

-       logger := log.With().
-               Str("action", "newCli()").
-               Str("stack", wd).
-               Logger()
+       logger.Trace().
+               Msgf("Get absolute file path of %q.", wd)
+       wd, err = filepath.Abs(wd)
+       if err != nil {
+               return nil, fmt.Errorf("getting absolute path of %q: %w", wd, err)
+       }

        logger.Trace().
-               Msgf("Evaluate symbolic links for %q.", wd)
-       wd, err = filepath.EvalSymlinks(wd)
+               Msgf("stating directory %q", wd)
+       _, err = os.Stat(wd)
        if err != nil {
-               return nil, fmt.Errorf("failed evaluating symlinks for %q: %w", wd, err)
+               return nil, err
        }

        logger.Trace().
-               Msgf("Get absolute file path of %q.", wd)
-       wd, err = filepath.Abs(wd)
+               Msgf("Evaluate symbolic links for %q.", wd)
+       wd, err = filepath.EvalSymlinks(wd)
        if err != nil {
-               return nil, fmt.Errorf("getting absolute path of %q: %w", wd, err)
+               return nil, fmt.Errorf("failed evaluating symlinks for %q: %w", wd, err)
        }

        logger.Trace().

mariux · 2022-01-07T11:25:19Z

The evalSymlinks is needed because our tests pass a --chdir <path> from t.TempDir() and in the OSX platform this function returns paths inside the /var link that points to /private/var whereas the paths returned from git command are all resolved, so they mismatch and tests fails. If you don't want to automatically resolve user paths by default, then you need to fix the tests to always resolve the temp dir passed as --chdir before calling terramate. The other changes are unrelated to this problem, but fine for me.

i would really not fix tests with extra code to be honest... a chdir should not fail as long as the directory exists and is accessible by the caller..

evaluating symlink and changing to the evaluated dir should be avoided by all means as there are things like automounter that dismount unused mounts... so when not in the actual mounted path the automounter might not notice it still being used and just dismount. automounter works with symlinks in that case... so the implementation will fail on systems depending on short-lived mounts.. (terraform can run for multiple hours)

so i would argue to fix the tests in this case. - so should i fix tests? would that be fine?

i4ki · 2022-01-07T12:42:08Z

i would really not fix tests with extra code to be honest... a chdir should not fail as long as the directory exists and is accessible by the caller..

yeah, chdir will not fail, it's just that user provides --chdir /var/tmp/something but git returns paths with /private/var/tmp/something and we have functions like cli.filterStacksByWorkingDir() that filters by cwd but then they don't match.

evaluating symlink and changing to the evaluated dir should be avoided by all means as there are things like automounter that dismount unused mounts... so when not in the actual mounted path the automounter might not notice it still being used and just dismount. automounter works with symlinks in that case... so the implementation will fail on systems depending on short-lived mounts.. (terraform can run for multiple hours)

This is only valid if someone has a mounted path containing a symlink to somewhere outside the mount, so in this case you use the symlink just to keep the mount up but the process do not use it.

Something like:

/mnt/data/var -> /var

If that's the case, if you provide /mnt/data/var as --chdir the terramate (and terraform) will never use the /mnt/data mount anyway.

Now this phrase:

evaluating symlink and changing to the evaluated dir should be avoided by all means...

this is very dangerous (as a general statement) and very easy to exploit if the symlink is in a globally writable directory like /tmp, even more if the tool is going to be run in CIs as in this case it probably run as root and so it can go anywhere in the filesystem. There are lots of attacks possible in this case (like TOCTOU) and the paths could even be protected and vulnerable. This would be a big issue if someone uses terramate automated in a backend app (maybe a cloud orchestration product).

But maybe makes sense in this case (really don't know)... if that's the case then we need to change the "project detection" to not use git rev-parse --git-dir.

Terraform seems to evaluate the symlinks at some entrypoints, not sure if all of them:
https://github.com/hashicorp/terraform/search?q=EvalSymlinks

So I would say we have two options:

Keep evaluating user's symlink but then someone could not pin a mount by providing a symlink in an unused mount point.
Dont evaluate (never) and don't use git to discover the git repository path.

I'm ok with both, but 1 could enable some attack vectors. If you chose 2, just remove this part: https://github.com/mineiros-io/terramate/blob/main/cmd/terramate/cli/cli.go#L952:L988

You don't need to fix the tests, we can do it if they fail.

i4ki · 2022-01-07T12:56:07Z

The project's testing is now ready for Fatal() and Chdir() calls anywhere, so you can enable the tests again.

we aligned on the changes

katcipis

🚀

mariux changed the title ~~mariux/logging~~ fix: wrong error message about missing symlink Jan 5, 2022

mariux force-pushed the mariux/logging branch 3 times, most recently from 12170c6 to 849be50 Compare January 5, 2022 23:34

mariux commented Jan 5, 2022

View reviewed changes

run.go Show resolved Hide resolved

mariux requested review from i4ki, katcipis and kassianh January 5, 2022 23:47

mariux marked this pull request as ready for review January 5, 2022 23:47

mariux force-pushed the mariux/logging branch 2 times, most recently from c1db580 to 426b8b9 Compare January 6, 2022 03:42

katcipis mentioned this pull request Jan 6, 2022

fix: avoid calling log.Fatal on core logic #141

Closed

katcipis reviewed Jan 6, 2022

View reviewed changes

manager.go Outdated Show resolved Hide resolved

mariux force-pushed the mariux/logging branch from 710f314 to 7739fcd Compare January 6, 2022 16:09

katcipis self-requested a review January 6, 2022 16:22

katcipis reviewed Jan 6, 2022

View reviewed changes

cmd/terramate/cli/cli.go Outdated Show resolved Hide resolved

manager.go Outdated Show resolved Hide resolved

mariux force-pushed the mariux/logging branch from 7a17dab to b170b93 Compare January 6, 2022 17:38

kassianh previously approved these changes Jan 6, 2022

View reviewed changes

kassianh reviewed Jan 6, 2022

View reviewed changes

cmd/terramate/cli/cli.go Show resolved Hide resolved

kassianh reviewed Jan 6, 2022

View reviewed changes

cmd/terramate/cli/cli.go Show resolved Hide resolved

kassianh reviewed Jan 6, 2022

View reviewed changes

cmd/terramate/cli/cli.go Show resolved Hide resolved

i4ki previously requested changes Jan 6, 2022

View reviewed changes

fix: wrong error message about missing symlink

3b4b7f5

mariux and others added 3 commits January 7, 2022 17:21

chore: cleanup variables in init

13267ef

tests: skip cli tests for now (#142)

8a77636

chore: use error instead of fatal in list()

41693e2

mariux dismissed kassianh’s stale review via 41693e2 January 7, 2022 16:25

mariux force-pushed the mariux/logging branch from b170b93 to 41693e2 Compare January 7, 2022 16:25

fix: tests

2306be5

mariux force-pushed the mariux/logging branch from 844c17c to 2306be5 Compare January 7, 2022 16:47

mariux requested review from i4ki, kassianh and katcipis and removed request for katcipis January 7, 2022 16:50

katcipis approved these changes Jan 7, 2022

View reviewed changes

mariux merged commit d2c4d71 into main Jan 7, 2022

mariux deleted the mariux/logging branch January 7, 2022 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: wrong error message about missing symlink #138

fix: wrong error message about missing symlink #138

mariux commented Jan 5, 2022 •

edited

Loading

mariux commented Jan 5, 2022

katcipis commented Jan 6, 2022 •

edited

Loading

katcipis left a comment

codecov-commenter commented Jan 6, 2022 •

edited

Loading

kassianh left a comment

i4ki left a comment

mariux commented Jan 7, 2022 •

edited

Loading

i4ki commented Jan 7, 2022

i4ki commented Jan 7, 2022

katcipis left a comment

fix: wrong error message about missing symlink #138

fix: wrong error message about missing symlink #138

Conversation

mariux commented Jan 5, 2022 • edited Loading

mariux commented Jan 5, 2022

katcipis commented Jan 6, 2022 • edited Loading

katcipis left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 6, 2022 • edited Loading

Codecov Report

kassianh left a comment

Choose a reason for hiding this comment

i4ki left a comment

Choose a reason for hiding this comment

mariux commented Jan 7, 2022 • edited Loading

i4ki commented Jan 7, 2022

i4ki commented Jan 7, 2022

katcipis left a comment

Choose a reason for hiding this comment

mariux commented Jan 5, 2022 •

edited

Loading

katcipis commented Jan 6, 2022 •

edited

Loading

codecov-commenter commented Jan 6, 2022 •

edited

Loading

mariux commented Jan 7, 2022 •

edited

Loading