feat(plugin): add gRPC v1 plugin backend support #1500
No reviewers
Labels
No labels
FreeBSD
Kind/Breaking
Kind/Bug
Kind/Chore
Kind/DependencyUpdate
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Windows
linux-powerpc64le
linux-riscv64
linux-s390x
run-end-to-end-tests
run-forgejo-tests
run-multi-platform-tests
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
forgejo/runner!1500
Loading…
Reference in a new issue
No description provided.
Delete branch "eleboucher/runner:plugin"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
implement forgejo/forgejo-actions-feature-requests#107 (comment)
7764310894ff3bfc7532ff3bfc7532870ea62b11870ea62b117df1656f9b6441c1da3bfed7286109fed7286109266be7e9bb266be7e9bbeef6a8f04ceef6a8f04c7df964c1197df964c1190e8a4fe6820e8a4fe682277b97a3a7277b97a3a73103fe44763103fe4476e46f11d039Hello @aahlenst
so this will be the next and last PR for the plugin right ? should we tackle it now ?
e46f11d03900ecf1906c00ecf1906c2ad15396152ad1539615c74cb26490@eleboucher wrote in #1500 (comment):
It's been a while, and I'm focused on something else, so my mental model might be outdated or incomplete. As far as I remember, there's no back-end discovery or loading, yet. Would it be possible for the plug-in to land without that?
It looks like this PR contains some commits that have already landed. Would a rebase help making it smaller?
c74cb26490eb54e71bc8@aahlenst wrote in #1500 (comment):
Hey, i squashed some commit so the history is better, but this is everything and rebased.
Yes no discovery yet it's config driven for now
@eleboucher wrote in #1500 (comment):
That doesn't really answer my question. 🙂 It was: Would it be possible for the plug-in to land without that [back-end discovery]?
I have even more questions: Would it be sensible to land it without back-end discovery? What does that mean for existing users of Forgejo Runner? Or other people that want to use the plug-in interface?
@aahlenst wrote in #1500 (comment):
oh yeah no problem in releasing that already without the backend discovery, here is how i use my config
we can probably rework the config to fit more the backend interface we worked on but no behavior changes for anyone already the forgejo runner already
Before I try to digest over 5000 lines in vain: What do you think, @mfenniak?
I'm good with the broad direction. With a few minutes of reviewing, it's clear to me that this will not get a good review if it isn't broken down severely into smaller chunks of reviewable work.
This is certainly subject to @eleboucher's expertise, but I'd say:
delegates_to_docker-- seems like an entirely separate capability that doesn't need to be in one PRI'm not prescribing how to split it, but, I wouldn't be able to review this. This is really important functionality, with really subtle possible behaviours, and doing it right requires the ability for a reviewer to be able to fully grasp each change and think about the code that isn't present. "is this going to work when a job is cancelled", "what happens when this times out", "will error handling or resource leakage occur here" -- and that requires small chunks of work that build up the capabilities. Even if something doesn't have a meaningful outcome/output/deliverable in the first PRs.
eb54e71bc8fdf5ce3f3c@mfenniak wrote in #1500 (comment):
I refactored the commit history to make more sense i hope now it's ready for review and i'm still reachable here or on matrix for more
I've reviewed the first one and a half commits. My comments on the protocol are extensive and I'm starting to repeat myself, so I want to send this feedback for you first -- the rest of the protocol likely also needs more documentation, more consistency, and more independence from docker-related concepts.
@ -0,0 +24,18 @@return x.ep}// NewJobContainer creates the container the job runs in.func (x *ExecutionEnvironment) NewJobContainer(input *actcontainer.NewContainerInput) actcontainer.ExecutionsEnvironment {return NewContainer(x.ep, input)}// NewServiceContainer creates a service container for the job.func (x *ExecutionEnvironment) NewServiceContainer(input *actcontainer.NewContainerInput) actcontainer.ExecutionsEnvironment {return NewContainer(x.ep, input)}// NewStepContainer creates the container a single step runs in (container// actions and `uses: docker://`).func (x *ExecutionEnvironment) NewStepContainer(input *actcontainer.NewContainerInput) actcontainer.ExecutionsEnvironment {return NewContainer(x.ep, input)}Why are three different methods with the same signature, and same implementation, required? In what case would an execution environment have different behaviour based upon whether you're working in a job, service, or step?
@ -0,0 +17,4 @@rpc Create(CreateRequest) returns (CreateResponse);// Start boots a previously created environment.rpc Start(StartRequest) returns (StartResponse);What do errors look like from this request, or any of these requests? How are errors encoded, and do they contain any information that is actionable / machine-readable? Are there any need for structured errors?
@ -0,0 +44,4 @@message CapabilitiesRequest {}message CapabilitiesResponse {All the fields in the protocol requests and response deserve documentation to explain them. It would be reasonable to assume this is the primary source of information for a developer working on implementing the protocol.
@ -0,0 +53,4 @@string default_path_variable = 6;string path_separator = 7;bool supports_docker_actions = 8;bool manages_own_networking = 9;There are no APIs on
BackendPluginto manage networking. As an author of a plugin, what am I expecting this parameter to do?@ -0,0 +58,9 @@bool environment_case_insensitive = 11;map<string, string> runner_context = 12;bool supports_local_copy = 13;// delegates_to_docker indicates the plugin only manages an execution// environment lifecycle and exposes a Docker daemon endpoint for the// runner to drive containers against. When true, the runner ignores// Exec/CopyIn/CopyLocal/CopyOut/UpdateEnv/IsHealthy on this plugin and// talks directly to the daemon returned in CreateResponse.delegate.bool delegates_to_docker = 14;This is such a different protocol implementation that I'm inclined to think that it shouldn't be part of the same service. Would it be plausible to split the service along this dimension?
e.g. a plugin can implement either
BackendPluginorDockerTunnelPlugin, and the later doesn't require defining 6 methods that do nothing.@ -0,0 +79,4 @@}message CreateRequest {string image = 1;What is
imageintended to contain in execution environments that are not docker? Can we document that? Is it just plugin-defined arbitrary string, from the protocol's perspective?@ -0,0 +81,4 @@message CreateRequest {string image = 1;string name = 2;repeated string env = 3;Would this make more sense as a key-value pair so that a plugin doesn't have to parse it?
@ -0,0 +82,4 @@string image = 1;string name = 2;repeated string env = 3;string working_dir = 4;Kinda surprising that this comes from the runner and not from the plugin to the runner. How does the runner know the right field value here, assuming that, for example, you're writing a plugin that creates a VM in a different OS like Windows?
@ -0,0 +83,5 @@string name = 2;repeated string env = 3;string working_dir = 4;repeated string cap_add = 5;repeated string cap_drop = 6;These are pretty Linux-specific, and, I wouldn't expect every execution environment can mange this even within Linux. What is a plugin author expected to do if this is not possible on their environment? It wasn't listed in the
CapabilitiesResponse...@ -0,0 +85,4 @@string working_dir = 4;repeated string cap_add = 5;repeated string cap_drop = 6;repeated ServiceContainer services = 7;What are the expectations if I authored my plugin and indicated
supports_service_containers = false?@ -0,0 +86,4 @@repeated string cap_add = 5;repeated string cap_drop = 6;repeated ServiceContainer services = 7;map<string, string> backend_options = 8;What are
backend_options?@ -0,0 +89,4 @@map<string, string> backend_options = 8;// force_pull asks the plugin to re-fetch the image. Plugins that cannot// honor it should still succeed using the cached image.bool force_pull = 9;I'm not sure that this belongs in the protocol; it doesn't seem like a generically meaningful parameter. It implies a lot about the nature of the executor and the image.
@ -0,0 +92,4 @@bool force_pull = 9;// platform is the OS/arch (e.g. "linux/amd64") the runner label selected for// image pulls and container creation. Empty means auto-detect.string platform = 10;This requires the backend to parse a docker-specific platform string (which, aside, contains an optional third parameter in the architecture variant). I'd suggest (a) separating this out into separate fields so it doesn't need to be parsed by a non-docker backend, and (b) maybe turning it into structured data like an enum? I'm not so sure about (b); I don't want to be updating the protocol for new OSes, but, I'm not loving the idea of a plugin author having no idea what is coming here.
Or the field can be described less in terms of
linux/amd64, and more in terms of "this is an execution-environment specific value; the runner and the protocol treat it opaquely; whatever is passed in over here arrives over here at the plugin."Empty means auto-detect-- I'd prefer marking the fieldoptionaland omitting it, what do you think?@ -0,0 +97,4 @@message CreateResponse {string environment_id = 1;// delegate is set iff CapabilitiesResponse.delegates_to_docker is true.I assume
iffis intended aif and only if; please elaborate it@ -0,0 +100,4 @@// delegate is set iff CapabilitiesResponse.delegates_to_docker is true.// Carries the Docker daemon endpoint and TLS material the runner uses// to drive containers inside the plugin-managed environment.DockerDelegate delegate = 2;This field should be marked
optional?@ -0,0 +111,4 @@string endpoint = 1;bytes tls_ca = 2;bytes tls_cert = 3;bytes tls_key = 4;Having a TLS private key transferred is surprising....
What kind of trust model are we trying to establish here? Like... this feels a bit like we connected all the dots required to connect to a TLS-encrypted docker container, but without any thought put into whether any real security is involved in doing so. CA private keys floating around in-memory between are effectively compromised.
This ends up being, why are we using TLS, what security do we want to gain from it, and are we meeting that objective?
@ -0,0 +123,4 @@}message StartResponse {map<string, string> image_env = 1;What is this?
@ -0,0 +130,4 @@message ExecRequest {string environment_id = 1;repeated string command = 2;map<string, string> env = 3;Here you've gone for
map<string, string>forenv, where earlier it wasrepeated string.I'm overall inclined to think that, before we can land this plugin capability, it might be reasonable to have two independent implementations of plugins built on it. The protocol is going to be important and long-standing, so that might be the only way to gain a lot of confidence that it hits the right notes... 🤔
ce1c26f5936e5e2dc356View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.