stage0: added support for fetching plugins, refactored docker/http(s) #2830
Conversation
| defer aciFile.Close() | ||
|
|
||
| if !f.InsecureFlags.SkipImageCheck() { | ||
| ascFile, err := os.Open(ascFilePath) |
There was a problem hiding this comment.
aren't you double opening this file (if the if above is true)?
There was a problem hiding this comment.
my bad, it was early in the morning here..
As an example, there's the need to both send and receive http caching information to/from an http plugin (like ETag and Cache controls saved in the store remote data). Since in this way every plugin may have different inputs/outputs I'm not sure which is the best way to make generic input config and outputs or every plugin (or schema) should have different inputs and outputs. |
|
@sgotti The hope is that this change would enable 3rd parties to write plugins without our intervention, and tweaking the config for each plugin means you need rkt support added in for each additional plugin. Maybe the answer here is just in the input include all the possible information the plugin could want, and each one will probably only need a subset of it? Also as far as an output config, I could only pass the plugin's stderr through to the user, and have plugins write a report in json to stdout that rkt can parse, so as to pass rkt Etag and Cache controls and such. |
2fe3cf9 to
7a930d3
Compare
|
Just pushed a change that moves the https fetching logic into a plugin. Also changed the plugins, such that they're expected to write a result json to stdout when they're finished. This is used to pass things like cache data back to rkt. |
| InsecureOpts *InsecureOpts `json:"insecure"` | ||
| Debug bool `json:"debug"` | ||
| Headers map[string]map[string][]string `json:"headers"` | ||
| OutputACIPath string `json:"output_aci_path"` |
There was a problem hiding this comment.
This implies ACI being the image format interface format between rkt and fetcher plugins, don't we want to make the leap and go straight to OCI?
|
Status update: haven't forgotten about this. Currently working through the broken tests and figuring out what functionality I'm still missing. |
|
bump |
|
I'm currently in the middle of refactoring hell, getting the https plugin to have all the functionality of the previous code. As I'm not working today/tomorrow, I hope to have this finished sometime next week.
|
|
before commenting on the code I have a broader doubt. In relation to #2925 looks like these are plugins for fetching given an input scheme. But the input scheme used with rkt (http://, docker://) isn't a transport scheme but covers the distribution concept. To be clear http:// is not just the transport but covers returning an aci and optionally a provided signature url, the docker:// covers calling docker2aci the return an aci using as a distribution a docker registry Did I get it right? I originally thought (reading the linked issues) that the idea was to provide plugins for a transport, also for being able to lower some privileges. Additionally there're other parts that fetch from a remote for example the appc pubkeys are fetched in Perhaps I missed some discussions around this. @lucab? |
| } | ||
| defer ascFile.Close() | ||
| _, err = fetchURL(config, "Signature", config.Scheme+"://"+config.Name+".asc", ascFile) | ||
| if err == nil { |
There was a problem hiding this comment.
The original httpfetcher, fetches the asc if provided externally by the user. Instead this assumes that an asc exists and is the aci path + .asc.
There was a problem hiding this comment.
Yeah you're right. Haven't pushed it yet, but I tossed this and am refactoring the fetching logic rkt already had to be part of the https plugin, and I'll fix the asc logic.
|
@sgotti I had been implementing this with the assumption that distribution and image format would remain bundled like they are in rkt today, but after reading over #2925 I think this is just splitting out the distribution mechanism, and we could evaluate making the image format logic more pluggable after this. This implementation hands a plugin a string, and the plugin is expected to perform the distribution logic to convert that string into a local ACI (and possible ASC), after which the stage0 binary imports this into the store. In the case of the The objective of all this is to allow users to write their own plugins without needing to modify rkt. If a user wants to fetch images using some specialized s3 protocol, they could create a fetching plugin and it would just work, instead of needing to make a feature request from us. I could see it possibly making more sense to only allow fetching ACIs via plugins, and keeping the |
Ok, Just to clarify my concepts (but I may be wrong 😄) I think of distribution as the whole process, starting from an input string, to fetch the required image. In this case since the discovery will be done by the stage0 the plugin is just doing the transport logic (http, s3, bittorrent, ipfs). So I'm going to call them transport plugin.
Seems Good. As an example, when rkt will support native OCI images the 'docker://' distribution should check the docker registry/manifest version and choose to use docker2aci for v1 and directly handle oci for v2, oci images fetched from a docker registry (the docker:// will mean docker registry distribution). Additionally when natively fetching the OCI blobs they aren't just one file but multiple blobs with different mediaType so I'm not sure how a full distribution plugin could handle this, instead this logic could use an http transport plugin. |
8e329e4 to
e5a18a8
Compare
|
Much closer now. I believe all the relevant tests should pass, but I have yet to tweak autotools to build the plugins. Also after talking to @sgotti I think I'd like to hold off on merging this until the new store work gets further along, as it's not clear to me how that will impact this. |
|
Still, if anyone has further feedback or would be willing to give this another review pass, that would be awesome. |
Adds support for having images fetched via a plugin. Plugins take the form of a binary on the user's path named "rkt-fetcher-%s", with the %s taking the place of the schema, so for example fetching `docker://alpine` is redirected to the `rkt-fetcher-docker` binary. When the plugin is started it has a JSON configuration written to it on stdin, its stderr is forwarded on to the user, and its expected to write result json to stdout. The configuration contains paths to write the ACI and ASC files to, which rkt will then verify (unless the insecure image option is specified). This commit also changes the behavior of the `--insecure-options=http` flag. It used to only impact the AppC discovery phase, and post-discovery images could be fetched over http regardless of the flag. Now rkt will refuse to fetch an image over http unless the flag is provided.
e5a18a8 to
e0f52ce
Compare
|
From an out-of-band update: this should be receiving more attention and an update around the end of this week. |
|
I've been too busy to give this much attention, and am busy this week with LinuxCon. I was going to focus on this next week. Unless you mean someone else got impatient and is going to pick this up for me.
|
|
Nope, just capturing that you'll be back to it soon ;-) On 22 August 2016 at 16:47, Derek Gonyeo notifications@github.com wrote:
|
|
This is stale and has a ton of conflicts. Closing. |
Adds support for having images fetched via a plugin. Plugins take the
form of a binary on the user's path named "rkt-fetcher-%s", with the %s
taking the place of the schema, so for example fetching
docker://alpineis redirected to therkt-fetcher-dockerbinary.When the plugin is started it has a JSON configuration written to it on
stdin, and its stdout and stderr are forwarded on to the user. The
configuration contains paths to write the ACI and ASC files to, which
rkt will then verify (unless the insecure image option is specified).
This isn't done yet, just putting this up for review to see what people think before I work on it further.
Things done:
Things left to do:
Questions:
To try it out:
go build -o rkt-fetcher-docker rkt/image/fetchers/remote-aci/main.gorkt-fetcher-dockeron your pathRelated issue: #1831