Multi-source apps should get an option to combine sources and render them together #12471
Comments
|
Would you just put the files in a subdirectory called “myjsonfiles”? |
|
For the example outlined above, that would be enough as |
|
Fair. Maybe: sources:
- repoURL: https://github.com/example/my-json-files.git
targetRevision: HEAD
path: '.'
ref: myjsonfiles
- chart: myhelmchart
repoURL: example.com/myhelmrepo
targetRevision: 0.1.0
from:
- ref: $myjsonfiles
destination: ./myjsonfilesThat would leave room for things like |
|
sounds great! @gczuczy do you want to give it a try? |
|
@lippertmarkus Yes. hopefully @crenshaw-dev can provide guidance regarding the project's bigger picture on the tiny details, and we can get to a point where everyone is sufficiently satisfied. Btw, is it the usual fork-PR-review-merge flow? |
|
@crenshaw-dev Adding a Regarding the details, my thoughts so far:
|
Yep!
It should be in a minor release. If I review it, it'll have to wait for 2.8. I've maxed out what I can reasonably hope to review before 2.7.0-rc1 (Mar. 20, 2023). You might be able to get another approver to review. I'd recommend looking for reviewers in the contributor office hours meeting tomorrow (and every Thursday) or in the #argo-contributors Slack channel.
Can you provide an example of the spec you imagine?
I dig that. We already have a function to check for out-of-bounds symlinks, so we could reuse that rather than yeeting all symlinks.
My OS fundamentals are rusty. Do we need to consider them? I don't think we'll have them in git repos. Maybe also not in Helm charts and OCI artifacts?
I think we should use an object in the |
Thank you. I've added this PR under my name as a suggestion to the agenda. Can be there easily.
Not exactly. I rather have requirements in my mind, and I think the actual wording of the spec as a minor detail. Everything works, as far as the ref, the srcpath and dst can be specified.
Could you please give me a hint to where to look for that function? That would simplify this part, reusing code is much welcome.
Actually I don't think it is an issue here. I was thinking about it purely from a filesystem perspective, rather then taking into account the nature of the source of the data. I don't think we'll encounter any, helm and git don't support it to the best of my knowledge, and I don't really know much about OCI artifacts. But I think it's safe to ignore this aspect.
Personally I would prefer to leave this kind of detail for later, and just keep this in mind when fiddling with the code, so expanding it later will be easier. I think the first goal should be getting the basic feature itself done, then we can discuss extending it, even maybe in another PR. |
Line 45 in b6cfe67
Makes sense, we'll just want to make sure that the first PR leaves room in the spec for any expansion we anticipate. |
Using that function will make the complete copy operation fail in case of a single symlink is out of bounds. I would prefer an implementation where symlinks are checked individually when processed, and out-of-bound entries are skipped, instead of the whole operation erroring out in case of a single out-of-bound one. Getting here seems rather straightforward, at path.go:50 the lambda can be moved to a separate function, called as a callback here, and in this PR's logic called directly, and not by the CheckOutOfBoundsSymlinks function. |
|
I've created a draft PR for now, so the work is linked here and can be tracked. |
|
May I ask what's the way of updating the protobuf specs after adding structures to the go code? From the source it seems like I've installed the tools with |
I'd prefer that as well. Unfortunately we've found through experience (and a handful of CVEs) that we can't reliably account for every build tool's (Helm, Kustomize, Jsonnet, plugins) ways of using symlinks. So instead we block them all by default if they leave the repo directory.
Make sure Argo CD is cloned into |
I think skipping out-of-bound symlinks individually satisfies that security requirement. If needed, a hardfail directive can be added which makes the whole operation error out if any found, instead of skipping them.
GOPATH wasn't set, but everything is under |
When you say "skipping," do you mean skipping copying them to the new shared build directory?
Ah I think you need to run |
Also, there might be usecases when the source of the copy has a symlink outside of the copy's scope for other purposes. Then that purpose and this feature would conflict. So, ignoring out-of-bound links selectively also makes it work for those cases.
Let's see. and similar messages for multiple Found a However, it's not missing vendor anymore. |
I'm not sure what you're proposing... how would out-of-bound links be selectively ignored? In general, allowing links outside the source repo isn't acceptable, because the environment (the repo-server) contains sensitive information.
Can you push the current state of your repo? I can try to reproduce it locally. |
When walking the source tree for copying, check every entry to be a symlink, if it's a symlink then check the target, if the target is outside of the sourcepath of the copy then simply continue to the next entry, ignore it. Hardfail would be running a check for out-of-bound symlinks before starting the copy, and not even starting the copy if any are found.
|
|
Ah I understand now. I agree, skip and move on makes sense. I think we could actually allow symlinks up to the boundary of the new top-level shared directory, rather than the bounds of the original repo.
Taking a look. |
Yup. The only thing I think not worth diving into is doing relative checks. There might be a few edge cases where symlinks temporarily are pointing out of the source/destinationpath but getting back inside again, and/or ending up with a different destination after the copy. For an example an src like: |
|
@crenshaw-dev If it's something broken in the forked branch, I've added you to the fork's contributors, so you can directly push to it. |
|
@gczuczy pushed the fixes. I think there was a race condition in the codegen-local make target. notifications-docs depended on codegen having already passed. |
|
Thank you guys for the great effort 🙏 |
i'm also interested. I would love to see this feature. |
Summary
For multi-source apps there are cases where it would make sense to put the files of one source into the directory of another source and render them together.
Motivation
Source 1: Helm Chart which uses
.Files.Glob "**.json"to put configuration files into configmaps.Source 2: Git Repo with additional json files that should be put into the configmaps by the Helm Chart (Source 1) as well.
Proposal
Could be implemented similarly like with the
helm.valueFiles. Example:The text was updated successfully, but these errors were encountered: