Add pki --api option #4867
Add pki --api option #4867
Conversation
edewata
force-pushed
the
api
branch
2 times, most recently
from
a967b53
to
26680f2
Compare
There was a problem hiding this comment.
I have several concerns on this change. We are not going to have a CLI working with v1 and a CLI working with v2. It is possible for the moment to share the code because there are very few differences but these could diverge.
Additionally, I do not see a use case where you have to call a mix of v1 and v2 endpoints to complete a task since v1 and v2 implement the same functionalities (almost 1:1).
We have a switch at server level which change the default APIs provided by the server, and the client should work with them, v1 and v2 are needed to external tools unless we want to develop two CLI interface. The switch is not to support multiple API versions but to have a soft switch before removing the old version.
The only reason I see for the switch in the client is if we have the v2 without resteasy client and we need to test and compare with current implementation. Is this the goal?
|
Generally to ensure a smooth transition, protocol changes (including API version, data format, encryption algorithm, etc.) should be done incrementally:
If the protocol is changed immediately (only one protocol is supported) on the server without a transition period, that means all clients need to be switched simultaneously too, otherwise they will stop working. If there are many clients or the clients are managed by different people, this could be difficult to accomplish. This challenge applies to both live production environment as well as code still in development. In our case we'll just have a single PKI CLI, but to allow a smooth transition during development the CLI will need to be able to select whether to use API v1 or v2, and it will need a separate code for parts of the API that have diverged, but still one CLI. This way we can individually migrate each PKI CLI command (there are a lot of them), add a new test for it using v2 API, and we can still change the v2 API for that command if needed (e.g. fixing the path, params). Once all PKI CLI commands support v2, we can change the default in PKI CLI to v2, but we still have other clients: Console, Web UI, IPA, third-party clients, so we cannot disable v1 yet. We can migrate IPA the same way too. IPA is making many different calls to PKI and we need to migrate each call to v2 as well. Without a transition period, it will be difficult to implement. |
PKI CLI already support v2. The PR #4837 runs all 145 tests we have upstream using v2 and all pass. The only failing tests are from IPA which uses its client. Not sure if it is reasonable double the tests to try with v1 and v2. For the other clients it is reasonable to test incrementally. I was not saying that we have to drop v1 too early but that the CLI was used and tested during the development to make v2 identical to v1 and we can go wild to find some corner cases. Different the situation for the other clients. IMHO these should move to v1 endpoints to avoid problems and when the path has changed for all known clients then we can move to v2 default. v1 will be dropped when it is not used by clients. |
The pki CLI has been updated to provide an option to specify the REST API version to use when communicating with the server. By default the CLI will use API v1, but it might change in the future. The PKIClient class has been modified to store the API version which will automatically be used by other client classes (e.g. InfoClient). The basic CA test has been updated to run pki info with the default API and API v2 and verify the access logs generated by these commands.
|
@fmarco76 Please see the updated patch and the PR description above. I've changed the My concern with PR #4837 is that we don't actually have a comprehensive test for API v1, and we haven't done much improvements in API v2 that we discussed previously (e.g. fixing the paths, params), so if we just change the default API to v2 on the server side now we won't see any problems except in IPA tests. Also, in order to merge that PR, all failures in IPA test have to be fixed, and the changes in PKI and IPA have to be merged at the same time, and the RPMs also have to be released at the same time, which could make things more complicated. I added another patch in this PR to update |
| @@ -107,7 +107,7 @@ public void getCert(HttpServletRequest request, HttpServletResponse response) th | |||
| } | |||
| } | |||
|
|
|||
| @WebAction(method = HttpMethod.POST, paths = {"search"}) | |||
| @WebAction(method = HttpMethod.POST, paths = {""}) | |||
There was a problem hiding this comment.
The general REST semantic for POST is to create something or modify something in the server and it is generally used across the APIs so if we post to /ca/certs I am expecting to generate a cert entry so I would not modify this path
There was a problem hiding this comment.
I've reverted the path changes for now, we can revisit this issue another time.
My concern with this path is that the search keyword in POST /ca/v2/certs/search occupies the same position as the cert ID in GET /ca/v2/certs/{id}. Although they use different HTTP methods, it doesn't look RESTful since /ca/v2/certs/search does not point to an actual object.
For creating/adding an object I think usually it's advised to use PUT. POST is kind of a generic method that can be used for anything that doesn't fit other HTTP methods. Maybe it would be better to use something like POST /ca/v2/certs?action=search.
There was a problem hiding this comment.
For creating/adding an object I think usually it's advised to use
PUT.POSTis kind of a generic method that can be used for anything that doesn't fit other HTTP methods. Maybe it would be better to use something likePOST /ca/v2/certs?action=search.
Actually, POST is the method to create a new resource. PUT is used to create or modify a resource where you already know the ID (these some example from a very quick search: restfulapi web, mozilla or geeksforgeeks).
There was a problem hiding this comment.
In our case the certs are always generated by the CA, so the client will never call this API to create a cert (enrollment/renewal is done using a different interface and it takes a CSR instead of a cert). Suppose we change the API to allow an authorized user to upload an existing cert to the CA, we might use PUT /ca/v2/certs/<cert ID>. If we don't want to require the client to include the cert ID in the request we probably can use POST, but I'd suggest we use POST /ca/v2/certs?action=upload. That way we can still use POST /ca/v2/certs?action=search for search.
Generally I'd say that all REST resources should support a GET operation. If GET doesn't make sense for that resource, like GET /ca/v2/certs/search, it's not really a RESTful design.
The CertServlet.listCerts() has modified to no longer return the total certs found to allow future performance optimization. Calculating the total certs found with Simple Paged Results requires retrieving the full search results from the database so it should be avoided. The basic CA test has been updated to test pki ca-cert-find with the default API and API v2 then verify the access logs generated by these commands. The test-ca-certs.sh script is no longer used so it has been removed.
|
@fmarco76 Thanks for the comments! I'll merge once the CI is done. |
|
The
pkiCLI has been updated to provide an option to specify the REST API version to use when communicating with the server. By default the CLI will use API v1, but it might change in the future.The
PKIClientclass has been modified to store the API version which will automatically be used by other client classes (e.g.InfoClient).The basic CA test has been updated to run
pki infowith the default API and API v2 and verify the access logs generated by these commands.The
CertServlethas been modified to use the same path (i.e./ca/v2/certs) for list and search operations, but list will will use aGETmethod and search will use aPOSTmethod.The
CACertClienthas been updated to use the proper path based on the API version.The search operation has also been modified to no longer return the total certs found to allow future performance optimization. Calculating the total certs found with Simple Paged Results requires retrieving the full search results from the database so it should be avoided.
The basic CA test has been updated to test
pki ca-cert-findwith the default API and API v2 then verify the access logs generated by these commands. Thetest-ca-certs.shscript is no longer used so it has been removed.