Split root_ca in CSR API #3480
Split root_ca in CSR API #3480
Conversation
|
🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test? Courtesy of your friendly test nag. |
istio-testing
added
the
size/XS
|
@howardjohn: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
that's a good call, I only really consider older control plane than data plane, not newer control plane... but that's an important issue |
| // root_cert contains [root1, root2]. | ||
| // * if root_cert is not specified, cert_chain contains [leaf, intermediate1, intermediate2, root1+root2] concatenated into one entry. | ||
| // Note that the individual cert_chain is only signed by a single root. The roots provided here is the full bundle of trusted roots. | ||
| Roots root_cert = 2; |
There was a problem hiding this comment.
I wonder if having a more significant addition which introduces a new structured type containing a full replacement for cert_chanin would be better/cleaner than making a small change like this. Don't make them mutually exclusive and then we could have an interop mode where cert_chain and name_tbd are both populated with the ~same information.
There was a problem hiding this comment.
+1 to that, let's introduce a real replacement struct, potentially with arrays for intermediates and roots and a separate leaf variable
There was a problem hiding this comment.
I could get behind that
Opening this for discussion.
Good background reading: istio/istio#55793. Note we do NOT need this API to fix the bug. This PR is about making the API more clear and less error-prone.
This API still is not ideal. A better approach would be to have a separate stream of root CAs and perhaps some way to tell the client they should send a new CSR. This is a much bigger change than is present here, though.