-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make builder read keys from database #122
Comments
👍 Ref #81 |
👍 |
Proposed API contract: Call from BuilderPOST /v2/hooks/key
{"app": "$APP_NAME", "public_key": "$PUBLIC_KEY"} Response From Controller When Key is Valid for App
Response from Controller When Key is Valid for App
All Other Responses From Controller
cc/ @helgi |
Where are you going to get the https://github.com/deis/builder/blob/master/rootfs/etc/confd/templates/authorized_keys gets it from I'm thinking you may want
That'd get you keys for the given app, unless you have an alternative way to get the keys from? |
wouldn't it be nice if controller updates builder whenever there is a new user key ? |
This avoids builder dependency on database and k8s API but coupling builder with workflow |
That seems too complicated - the builder should never talk to the DB, ever. I can see your point about creating the tight coupling between the two but running a HTTP server on the builder creates unnecessary state (IMO). If we go down the route of tightly coupling things up between the two then we need to discuss the implications of that, same way we do have to consider / discuss why we wouldn't have each component depend on the k8s API (more and more rather than less and less). |
@helgi I misspoke in the payload that I posted above (the result of doing too many things at once). Yes, the builder w/o etcd wouldn't have a public key. I agree that the endpoint should look very similar to the one you proposed. However, I advocate for putting the user in the path, since a I'd like to address some of the general comments in sections below, and I'd like to hear thoughts. TL;DR my suggestion is to make the builder query the controller for both app existence and key validity. Regarding the Controller Updating Builder vs. the Builder Querying the ControllerIf the controller updates the builder, the builder has to hold more state. However, if the builder queries the controller (as described in the previous section), then state is isolated to the controller, where it already resides (more precisely, in the DB that the controller queries). Managing state in a distributed system is challenging, so let's try to keep it as isolated as possible, and minimize the amount of state in the builder, in this refactor. Regarding Tight Coupling Between the Builder and ControllerThe builder already makes 3 RPCs to the controller on every build, and we've accepted the fact that the two components are tightly coupled and will be through the beta. Adding a 4th RPC is not ideal from a performance perspective, but adding the RPC now and reducing them later is a manageable task. Regarding Builder Accessing the DBI 100% agree with @helgi that the builder should never access the DB. I'll expand - nothing but the controller should ever access the DB (see my comments above on isolating state). Additionally, we've built an abstraction layer - the controller API - on top of the DB. If anything circumvents that layer, the abstraction is leaky and we will have introduced a large maintenance problem (imagine if many components are directly accessing the DB and we try to refactor the schema). Regarding Listening to the Kubernetes Event StreamI do agree that the event stream could be a good source from which to listen for app deletion events. However, if we do so, then we rely more heavily on builder state. Instead, why not rely on the aforementioned I also disagree that the event stream is a good data source for adding and removing SSH keys (again, for builder state reasons). I'm not clear if anyone is proposing that key management strategy above, but I'd like to make my position clear. |
In my example the If we use the API for app existence and user existence then you'd get a I'm not seeing how much more state the builder would have to maintain if it gets app adds / removals from k8s API. Not via an event stream but rather label selectors. The state is owned by k8s and the builder is simply using it to see when it should for example clean up older apps. If you rely on the API on |
Thanks for the clarification, and I agree - it'd be nice to determine whether the app or user doesn't exist without having to check error messages. I see what you're saying RE the Kubernetes API now, but regardless, the builder would still hold state - the state of polling - and we can get race conditions if the builder runs with more than one replica. For example, say we're running 3 builders pods ( On the other hand, if builder makes the controller API call on each I don't see a need for the builder to need to know when a given app exists at any other time. Am I missing something crucial there? |
Are old apps ever left behind if you are not cleaning things up? Are checkouts always blown away so there is no old code / apps around on the builder pods? |
In that scheme, old apps would be left behind if someone deletes an app and never tries a However, old code would be deleted when the RPC happens on a |
What I'm confused about is how the reaper determines what to delete and what to keep, without checking with something somewhere. It isn't end of the world having an active app reaped I suppose. |
So all app checkouts live in the same directory (see here), so the reaper's job would be to walk that directory and query the controller for each app (at a slow interval). Note that the app checkout directories and the reaper's state are expendable. |
So instead of querying the k8s API you want to do the same against the controller API. I'm not really seeing the difference or benefit beyond perhaps not having to deal with the k8s API. |
I'm fine with querying the k8s API, I'm just advocating for querying something before running the receive hook on each Maybe we're saying the same thing... |
I think we are ... So here is how I see it:
|
👍 from me 💯 |
Ref deis/workflow#336 for the keys endpoint |
here are bunch of use cases I'm not clear about.
|
@smothiki I've posted answers to some of your questions below. I'm unclear on some others so can you please point out if I've left something unanswered?
Not necessarily. Since we know the app and the user, we could make the call to just get the key for the (app, user) pair.
No, because multiple users may
On what metric? |
An App is not associated with the key and user can have many keys . |
Right, so the builder would make a call to the controller's |
let's say user has 100 apps running and uses 200 keys . The call returns 200 keys for an APP. |
My concern is about optimizing the git push operation if we are making a call to controller for each git push. |
Also practically the number of time git pushes are made are more than the number of times doing deis keys:add |
Correct. There is no way around this, regardless of the method of acquiring keys from the controller.
We already make 3 API calls to the controller for each
I agree. Again, though, there's no way around this regardless of key delivery mechanism. The RPC method as proposed returns the full list every time, however, and that's non-optimal. Perhaps a future optimization would be to have the controller only deliver deltas.
I agree. See my above comment on a possible future optimization. |
Builder reads keys from etcd. We should avoid doing that and read keys from database
The text was updated successfully, but these errors were encountered: