(PUP-3526) Add configurable prefix for CA routes #3464
Conversation
|
This is blocked by #3444 and will need to be rebased once that goes in. |
|
CLA signed by all contributors. |
| "certificate" => :ca, | ||
| "certificate_request" => :ca, | ||
| "certificate_revocation_list" => :ca, | ||
| "certificate_status" => :ca |
There was a problem hiding this comment.
We need the plural of this too right? "certificate_statuses"
rlinehan
force-pushed
the
feature/master/PUP-3526-ca-api-prefix
branch
from
ec1e1f8 to
2a9ba69
Compare
|
Rebased on top of merged-to-master PUP-3645. This should be ready to go now. |
| any. | ||
| chain(ENVIRONMENTS, INDIRECTED) | ||
| end | ||
|
|
||
| def self.ca_routes | ||
| Puppet::Network::HTTP::Route.path(%r{v1}).any.chain(INDIRECTED) |
There was a problem hiding this comment.
I'm wondering if it's going to be problematic at all to have these coupled together in a file named after the version of the master's (but not the CA's) API.
Maybe I'm overthinking it but what happens if we need to bump CA to v2? Would we make the change in this file and leave it as http/api/v3?
There was a problem hiding this comment.
That's a good question. I think it's sort of weird right now because both the CA prefix at v1 and the master prefix at v3 end up in the same place - IndirectedRoutes. Other than here, the only place I can think that makes sense to define these would be directly in http/api.rb.
If we were bumping the CA routes to v2, then presumably it would be because we were separating them out to no longer use IndirectedRoutes, in which case it seems like it would make more sense to define them in their own file.
There was a problem hiding this comment.
This is going to sound ridiculous but I wonder if it wouldn't make sense to go ahead and have api/master/v3.rb and api/ca/v1.rb now. It's ridiculous because those namespaces would only have like 3 lines of code in them, but I kind of think it might still be worth it because otherwise the v3.rb file doesn't seem like an intuitive place to look for the v1 CA routes... :( I could survive with it like this but I think I might have a slight preference for going ahead and breaking it out now.
There was a problem hiding this comment.
This makes sense to me... my only question is where should the indirected_routes.rb file live?
There was a problem hiding this comment.
I think that with the minor refactors we discussed at various points in the PR comments, we could end up in a state where IndirectedRoutes doesn't contain any references to any version strings. If that's the case then we could put it in the api dir or even the parent dir of that, whichever seems like a better fit.
|
A fix for the spec failure has been merged in. Kicking travis to confirm. |
| { :acl => "#{ca_url_prefix}/v1/certificate/", :method => :find, :authenticated => :any }, | ||
| { :acl => "#{ca_url_prefix}/v1/certificate_request", :method => [:find, :save], :authenticated => :any }, | ||
| { :acl => "#{master_url_prefix}/v3/status", :method => [:find], :authenticated => true }, | ||
| { :acl => "#{master_url_prefix}/v3/environments", :method => :find, :allow => '*', :authenticated => true }, |
There was a problem hiding this comment.
Do you think it might be worth sorting these lines such that the CA ones are all grouped together, separate from the master ones?
@haus if we were to do that would it cause any more or fewer packaging/upgrade concerns?
There was a problem hiding this comment.
Sorting them such that CA ones are grouped together makes sense to me; I didn't know how much it mattered and figured I'd just leave it as is with the comment about auth any.
There was a problem hiding this comment.
@cprice404 since we're just scrapping and replacing this file during 3 => 4 upgrades, i'm not concerned here. Future upgrades within the 4 series will go fine, with any changes being handled by the package manager and user.
There was a problem hiding this comment.
@rlinehan ah, that makes sense about grouping them by the comment / auth level. If we do that we might as well change it so that for each one of the different comment sections, the ca routes are always at the top or the bottom of the individual section maybe? I'm good w/whatever you think is best, not a big deal.
|
I'm done with initial review; will pull the code down and play with it a bit. Looking good, just a few minor organizational questions / comments. |
|
👍 verified with agent and CLI tools using both/neither/combinations of the prefixes. |
|
|
||
| it "should include the correct url prefix if it is a ca request" do | ||
| request.stubs(:indirection_name).returns("certificate") | ||
| handler.class.request_to_uri(request).should == "#{ca_url_prefix}/certificate/with%20spaces?environment=myenv&foo=bar" |
There was a problem hiding this comment.
Probably not a big deal but "environment" is invalid for "certificate", right? But then again so is "foo" I guess and I suppose the validation isn't intended to work at the level of URI parameters anyway...
There was a problem hiding this comment.
No, environment is required for all indirector requests. Even the ones that don't use it for anything. (Spoiler alert: we should not carry that over to v2 of the CA API :) )
There was a problem hiding this comment.
Does this mean that external HTTP API callers to the CA v1 certificate* endpoints will be required to include "?environment=" or, if it is absent at that level, will some default be substituted to make the indirector happy?
There was a problem hiding this comment.
Requests originating from the agent will include the environment query parameter, because all agent URLs are constructed via the indirector. For the Rack/Webrick masters, the requests will fail if this parameter is not provided.
For the Puppet Server CA endpoints, I suspect that it would be acceptable to ignore this query parameter rather than returning a failure response, but that is a question for product, and is orthogonal to this PR.
We should not expect the agent to stop sending this parameter until there is a significant rework of the client-side code that involves removing the indirector from the process of constructing the URLs.
There was a problem hiding this comment.
That's fine. I don't see a problem there, mostly curious about what the expected behavior would be for CA v1. I agree it is orthogonal to this PR, but I agree with you that having Puppet Server's CA just "ignore" the environment for the CA v1 endpoints seems reasonable.
There was a problem hiding this comment.
👍 to ignoring the parameter
rlinehan
force-pushed
the
feature/master/PUP-3526-ca-api-prefix
branch
from
9a583f8 to
a7894fe
Compare
|
@cprice404 added some commits based on your comments. |
|
+1 from me. I'm going to pull the code down and play with it a bit, but unless anything jumps out at me from that, I'll plan on merging this on Friday or so. ping @joshcooper @hlindberg @kylog in case you guys have any interest in further review before we merge. |
|
Should I squash some of these commits, or is it fine as is? |
|
@rlinehan maybe let's wait and squash on Friday before we merge? |
|
@cprice404 sounds good. |
|
I pulled the latest code down and attacked it with curl for a while; lgtm |
|
@rlinehan @cprice404 Thinking out loud here... why do we need a prefix? As in, doesn't trapperkeeper know when a |
|
@rlinehan @cprice404 ok, I read the ticket, that helps, but I am still surprised that we're exposing this data to the agent as it seems like an implementation detail for how to retrieve something. For example, suppose the agent makes a request for a node information, and with this PR, it'd be under the |
|
@rlinehan @cprice404 @joshcooper Agree with Josh - the indirection is better done on the server side - no changed URLs on agents. If we want to also be able to redirect agents wouldn't using HTTP 30x responses be the way to do that? (But that would be for other reasons than the original "do not squat on the server's '/' namespace") |
|
My understanding has been that we're trying to move to a model where each service has a mount point that can be set via configuration -- not requiring code changes -- like what is afforded by https://github.com/puppetlabs/trapperkeeper-webserver-jetty9/blob/master/doc/webrouting-service.md. We could treat legacy Puppet as a special case that avoids this pattern for the sake of maintaining shorter-term compatibility (along with not addressing the aforementioned problems around the top-level environment in URL paths and squatting on the server's '/' namespace). This wouldn't seem like a good long-term solution, though, and essentially what I had thought had led to this whole set of work around redoing the URL paths for Puppet 4.0. One of the major benefits that I see to having the top-level service mount point be configurable both from the server and client side is the increased level of control given to users. For example, I could see where some users might want to take advantage of the configurable prefix as part of their load-balancing scheme. For example, users might want to include a top-level "region" path in the mount point which is set for agents as appropriate for the region in which they reside, e.g., some would make requests to "/americas/puppet/..." whereas others would make requests to "/europe/puppet/...", and the load-balancer upstream would use that context from the prefix to redirect to the appropriate masters. In this model, it would be possible for the Hardcoding the mount points into the implementation with no external configurability wouldn't seem to be progressing the implementation forward. One could ask the question about why we're not going further by making each of the endpoints in legacy Puppet independently configurable - e.g., by codifying a In short, I'm good with the overall direction of the implementation captured in this current set of PRs. |
|
@joshcooper @hlindberg fair questions. Here's what I'd say:
In short, I think that making the prefixes configurable on the agent gives us the maximum amount of flexibility going forward, even if these settings end up almost never being used. I'd be willing to entertain a conversation about hard-coding the prefixes on the agent, but my current opinion is that the tradeoff could theoretically paint us into a corner in the future that I feel that this solution avoids. |
|
@joshcooper also, since we will now have versioned URLs, we can make sure that in the future we still support things like |
|
I think we're all in violent agreement that the server should allow REST endpoints to be mapped to services in configuration as outlined in https://github.com/puppetlabs/trapperkeeper-webserver-jetty9/blob/master/doc/webrouting-service.md I'm not 100% sold on the argument that the load-balancer might segregate traffic to geographically different locations based on a URL prefix. I think more likely is that clients are configured to talk to their "local" master either by changing the If we were to allow REST endpoints to be configured on the client, then I'm not a fan of having a prefix setting, as it seems very specific to our implementation of how REST URLs are constructed. I'd prefer having an explicit URL, e.g. |
|
If there were a newer version of an endpoint available, it would probably be because there were API changes, in which case the client code would most likely need to be modified anyway. That also seems like a significantly more invasive change to me, and seems like it would jeopardize the Puppet 4.0 target dates. Given that what we're roughly shooting for here is a way to split out the monolithic HTTP API into more service-specific namespaces, the prefix approach seems like a fairly simple / standard way to handle it... perhaps we should schedule a meeting of interested stakeholders to hash this out, though... it seems like it's going to be challenging to come to a consensus over github comment threads... |
|
Also, just to clarify, I don't necessarily think that the goal is "to allow REST endpoints to be configured on the client". It's more like "allowing REST endpoints to be namespaced". I know that's not necessarily a black-and-white distinction, but I'm just saying that I'm not advocating for 100% configurability of the full URL... I think that the majority of the URL construction should be hard-coded for a given client version. |
|
One other random thought... in the world of today, we are using these settings to determine the URL namespaces for the Rack/Webrick server implementations, as well as for the client. So, even if we decided to change how we expose this stuff for the client, we will still need these for the server (unless we decide to just hard-code them). |
|
we just had a meeting to discuss this comment thread. I'm going to update the jira ticket with my interpretation of the outcome of that meeting if anyone is interested, but the tl;dr is that we're basically OK with this going in as-is. I'll leave it open for another day or so in case anyone wants to raise any last-minute objections, but, failing that, we'll merge it. |
This commit restructures the CA endpoints to allow for a url prefix '/ca', separate from the master url prefix. Split the api/ directory into master/ and ca/, with master/ having v2/ and v3/, and ca/ having v1/, to match the split of CA routes from master routes. Move indirected_routes.rb and indirection_type.rb under api/ rather than api/v3, since they are used by both the ca/v1 and master/v3 routes. Also, hardcode master and CA url prefixes, rather than allowing them to be configurable. There was no clear user need for having these be configurable settings, and it was determined that it was not worth the extra effort of supporting an a new setting.
rlinehan
force-pushed
the
feature/master/PUP-3526-ca-api-prefix
branch
from
a625926 to
0f044d7
Compare
There are now two ways to have a request get to v3/indirected_routes: `/puppet/v3` and `/ca/v1`. Thus, a request to `/puppet/v3/certificates` would be routed to indirected_routes and could be handled successfully, although issues would probably come up around authorizing the request. Such a request is incorrect - it has the wrong url prefix - and we should say so.
Previously, the HTTP::Handler spec tests defined a HandlerTesting class. This commit separates it into its own helper so it can be reused in other tests.
Previously, all of Puppet's routes were defined separately in the Webrick and Rack REST interfaces. This was okay when all that was being registered was the v1 and v2 routes. However, with the CA routes now separated from the master routes, there is quite a bit more logic there, and it is unwieldy (and hard to test) with it defined in both places. This commit defines the CA and master routes in one place, so that only ca_routes and master_routes have to be registered for each webserver.
Make the default CA url prefix '/puppet-ca', rather than '/ca'. Since the master url prefix is '/puppet', and since the routing will match '/puppet-ca' to '/puppet', this also means tweaking how we register routes, so that the prefix is registered with a slash on the end and the version strings do not begin with slashes.
Move all CA rights together. Also reorder the example auth.conf to match the order of default_acl. Add an entry for /puppet/v3/status, which was in default_acl but missing from the example auth.conf, since the example auth.conf is supposed to match the default ACLs.
rlinehan
force-pushed
the
feature/master/PUP-3526-ca-api-prefix
branch
from
0f044d7 to
80a108f
Compare
|
Per continued discussion on PUP-3526, we decided that it wasn't worth the effort to support settings for the master and ca url prefixes, and it would be better to remove them. I've updated this so that he url prefixes are now constants. I've also squashed some of the commits together, so that this is a bit cleaner. It should be ready to go. |
|
@joshcooper @kylog @hlindberg I'm going to go ahead and get this merged in because it's blocking some things. Happy to revisit any of the details if any questions come up. |
…-prefix (PUP-3526) Add configurable prefix for CA routes
Restructure the CA endpoints to allow for a configurable url prefix, separate from the master url prefix.