Fix --in-memory-build restricting disk access - #5974
Conversation
…to CWD Signed-off-by: rycli <cyril@ryc.li>
Signed-off-by: rycli <cyril@ryc.li>
Signed-off-by: rycli <cyril@ryc.li>
rycli
changed the title
--in-memory-build restricting disk access
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| return nil, "", action, fmt.Errorf("failed to get working directory: %w", err) | ||
| } | ||
|
|
||
| diskFS, err := buildfs.MakeFsOnDiskSecure(cwd) | ||
| if err != nil { | ||
| return nil, "", action, fmt.Errorf("failed to create secure filesystem: %w", err) | ||
| } | ||
| fs := buildfs.MakeFsInMemory(diskFS) |
There was a problem hiding this comment.
Was this never in scope in the original issue? Why are we switching from secure to non-secure? Was this shipped entirely unintentionally?
There was a problem hiding this comment.
The previous implementation of the file access logic did not in fact make use of the "secure layer". It came up when implementing the in-memory filesystem, and it was pointed by @stefanprodan that the underlying pkg implementation should respect/use the secure filesystem; see discussion on the corresponding PR fluxcd/pkg#1153 (comment)
The underlying topic of restricting the root/loading restrictions was explicitly discussed in #5794 (comment) . Looking back at it though, I think the error happened due to misunderstanding of the actual behaviour at the time; kustomization building did not in fact respect the secure root previously, but we thought it did (#5794 (comment)).
| buildKsCmd.Flags().BoolVarP(&buildKsArgs.recursive, "recursive", "r", false, "Recursively build Kustomizations") | ||
| buildKsCmd.Flags().StringToStringVar(&buildKsArgs.localSources, "local-sources", nil, "Comma-separated list of repositories in format: Kind/namespace/name=path") | ||
| buildKsCmd.Flags().BoolVar(&buildKsArgs.inMemoryBuild, "in-memory-build", false, | ||
| buildKsCmd.Flags().BoolVar(&buildKsArgs.inMemoryBuild, "in-memory-build", true, |
There was a problem hiding this comment.
You know, for as much as a I want Flux to have better defaults, we don't normally implement a new feature and make it replace a default behavior in Flux in the same release. See for example the FluxStorage feature gate in image-reflector-controller in Flux 2.9. It allows opting into a feature that we introduced and are planning to make the default in Flux 2.10 because we think it's pretty good and should become the default. I don't think this in-memory build should rush and skip that graduation phase.
There was a problem hiding this comment.
For what it's worth, it may be good to point out that this feature was originally reported & implemented as a bug fix; it addresses some real issues we ran into (see original related issue: #5781).
Making it a default was suggested by @stefanprodan in the follow-up discussion on the PR: #5794 (comment)
Ultimately I believe this was a genuine bug, borne from an incorrect shared assumption, as opposed to any kind of intention to change user-facing behavior. I can switch it false, if that's the way forward though.
| diffKsCmd.Flags().BoolVarP(&diffKsArgs.recursive, "recursive", "r", false, "Recursively diff Kustomizations") | ||
| diffKsCmd.Flags().StringToStringVar(&diffKsArgs.localSources, "local-sources", nil, "Comma-separated list of repositories in format: Kind/namespace/name=path") | ||
| diffKsCmd.Flags().BoolVar(&diffKsArgs.inMemoryBuild, "in-memory-build", false, | ||
| diffKsCmd.Flags().BoolVar(&diffKsArgs.inMemoryBuild, "in-memory-build", true, |
2 participants
Addresses #5968
The PR that introduced the bug #5794 implemented a new
--in-memory-buildoption; it wraps a 'filesystem' instance, specifically thebuildfs.MakeFsOnDiskSecure. This restricts access to files in PWD.The fix is pretty straightforward; use the unrestricted
MakeFsOnDiskoption. I've also updated one of the tests accordingly, as it wasn't testing the desired behavior.Side-note: as far as I can see, the
MakeFsOnDiskSecurelogic is only implemented for theflux bootstrapcommand, which is likely why this issue didn't occur previously 🤔