Make inputDigest hash calculation independent of workspace path #6522
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
|
@googlebot I fixed it. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
Codecov Report
@@ Coverage Diff @@
## main #6522 +/- ##
=======================================
Coverage 70.43% 70.43%
=======================================
Files 515 515
Lines 23127 23147 +20
=======================================
+ Hits 16289 16304 +15
- Misses 5780 5784 +4
- Partials 1058 1059 +1
Continue to review full report at Codecov.
|
pkg/skaffold/tag/input_digest.go
Outdated
| if workspacePath == "." { | ||
| h.Write([]byte(filepath.Clean(path))) | ||
| } else { | ||
| h.Write([]byte(filepath.Clean(strings.Replace(path, workspacePath+string(os.PathSeparator), "", 1)))) | ||
| } |
There was a problem hiding this comment.
I can change that but it would mean that we'd (try to) use a relative path even if the file is outside of the workspace. Not sure if that is what we want or if it is even a possible case? Up to you.
There was a problem hiding this comment.
Files should typically be within the workspace. filepath.Rel() returns an error if the file could not be made relative, so we could try to relativize and otherwise use the fullpath.
There was a problem hiding this comment.
Ok, makes sense. I'll change that shortly.
| cwdBackup, err := os.Getwd() | ||
| t.RequireNoError(err) | ||
| t.RequireNoError(os.Chdir(dir)) | ||
| defer func() { t.RequireNoError(os.Chdir(cwdBackup)) }() |
There was a problem hiding this comment.
This doesn't seem necessary. You could create two directories with the same file and validate that they return the same hash.
There was a problem hiding this comment.
Then we wouldn't be able to verify that a relative path to the workspace and it files yields the same result as absolute paths (see the if clause in the function under test). In order to find the file and hash its contents in the relative path case we need to change cwd here.
There was a problem hiding this comment.
If we create d1 and d2 with the same contents, then the fileHash of both should be equal. Currently this would fail since we're hashing the full paths of the files in d1 and d2.
Typically Skaffold doesn't change the working directory — it rather executes programs with a different working directory.
There was a problem hiding this comment.
The scenario that broke us before was that when calculating the tag for an image defined in the "root" skaffold config all paths were relative to the workspace directory. Even the workspace directory path was relative, i.e. just ".". When calculating the same image tag when imported via the requires section all paths, including the workspace path, were absolute. For the calculation to work at all in the first case cwd must be the directory of the skaffold config or we can't read the file contents.
So this is the scenario I'm trying to test here, that providing paths relative to the workspace directory to the digest calculation will yield the same result as when providing absolute paths. And I can't come up with any other way of getting the relative calculation to work without changing cwd.
I can add another test case that verifies that calculating for two different absolute paths works as expected to if you want.
There was a problem hiding this comment.
I added more tests, including one that tests two different absolute paths and would have failed without the fix.
I would rather leave the test we are discussing here as it is a different case that is closer to the situation where this hit us initially, but if you feel that it should be removed I can do so.
Description
The point of this change is to ensure that digest tag calculation is always independent of the workspace location. Put differently, with this change the same tag will be calculated for an image regardless of if it is defined directly in the skaffold configuration passed to skaffold or in a in another configuration referenced from the
requiressection. Previously the latter situation would result in absolute paths being used resulting in a different digest.