Add flag to follow symlinks when running opa build #6800
Conversation
…undles Signed-off-by: Tyler Schade <[email protected]>
Signed-off-by: Tyler Schade <[email protected]>
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
32341ef
to
5de7d4b
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Tyler Schade <[email protected]>
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
b58f28c
to
6ee5e4c
Compare
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Tyler Schade <[email protected]>
tjons
force-pushed
the
tjons/6495-symlinks-building-bundles
branch
from
8329087
to
6bd3b39
Compare
| } | ||
|
|
||
| // GetBundleDirectoryLoaderFS returns a bundle directory loader which can be used to load | ||
| // files in the directory. | ||
| func GetBundleDirectoryLoaderFS(fsys fs.FS, path string, filter Filter) (bundle.DirectoryLoader, bool, error) { | ||
| func GetBundleDirectoryLoaderFS(fsys fs.FS, path string, filter Filter, followSymlinks bool) (bundle.DirectoryLoader, bool, error) { |
There was a problem hiding this comment.
This will be a breaking change for anyone using this public function directly, although probably unlikely. Given that we also add WithFollowSymlinks(), couldn't callers simply use that on the returned value instead?
| @@ -188,6 +190,12 @@ func (fl *fileLoader) WithRegoVersion(version ast.RegoVersion) FileLoader { | |||
| return fl | |||
| } | |||
|
|
|||
| // WithFollowSymLinks enables or disables following symlinks when loading files | |||
There was a problem hiding this comment.
| // WithFollowSymLinks enables or disables following symlinks when loading files | |
| // WithFollowSymlinks enables or disables following symlinks when loading files |
|
|
||
| params := newBuildParams() | ||
| params.outputFile = path.Join(rootDir, "test.tar.gz") | ||
| params.bundleMode = true |
There was a problem hiding this comment.
Can we drop This? Reading in bundle-mode doesn't seem like a requirement.
| @@ -238,6 +239,7 @@ against OPA v0.22.0: | |||
| buildCommand.Flags().VarP(&buildParams.revision, "revision", "r", "set output bundle revision") | |||
| buildCommand.Flags().StringVarP(&buildParams.outputFile, "output", "o", "bundle.tar.gz", "set the output filename") | |||
| buildCommand.Flags().StringVar(&buildParams.ns, "partial-namespace", "partial", "set the namespace to use for partially evaluated files in an optimized bundle") | |||
| buildCommand.Flags().BoolVar(&buildParams.followSymlinks, "follow-symlinks", false, "follow symlinks in the input directory when building the bundle") | |||
There was a problem hiding this comment.
Are we limited to only following symlinks in directories, and not also direct paths to symlinks? Doesn't look like that'd be the case 🤔; i.e. the FileLoader created and called in LoadPathsForRegoVersion looks like it'd follow links.
If this isn't a limitation, we should update this description to say that symlinks in the provided set of paths are followed (not just dirs). And we should update the test to assert this (or add an additional test).
Why the changes in this PR are needed?
Per #6495, it is important to users using build systems like babel to allow
opa buildto include symlinked files in the bundle directory. Bazel and other build tools create sandboxes of symlinked files before building. As it stands today,opa buildonly includes regular files and directories by default.What are the changes in this PR?
This PR adds a boolean flag,
--follow-symlinksto theopa buildcommand to allow users to build directories with symlinked files, and have the contents of those symlinked files included in the built bundle.Notes to assist PR review:
I've included tests for the build command but would appreciate a critical eye as to any other areas that could use testing. Also, I've kept the symlink changes as constrained to the
buildfeatures as possible, but there are many layers of method calls to get to the core filesystem loading logic, so any ideas for cleaning up this code is welcome.Further comments: