Add flags to provide certificate PEM string #628
Conversation
Add new admission-webhook-cert, admission-webhook-key, and kong-admin-ca-cert flags. These allow users to provide a PEM-encoded certificate or key string directly to the controller, for the same purpose as the existing *-path equivalents of these flags. This allows loading certificates into the controller using secret key references in its environment.
| glog.Fatalf("failed to load admission webhook cert: %s", err) | ||
| } | ||
| } | ||
| if cliConfig.AdmissionWebhookCertPath != "" && cliConfig.AdmissionWebhookCert == "" { |
There was a problem hiding this comment.
The second condition is redundant. Due to earlier checks in the code, at this point in execution, you can be sure that cliConfig.AdmissionWebhookCert == "".
There was a problem hiding this comment.
Superfluous but helps with reading a bit--the link back to the check to confirm that only one is set isn't immediately obvious, and without the check it looks like it will overwrite the certs, even though it doesn't. Minor quibble though, so I've removed it.
There was a problem hiding this comment.
Did you actually remove it? It shows up in the current diff.
Another edge case here:
If a user mounts the cert on file-system and also sets the env vars, then the certs on the file-system take precedence here. Is this intentional? I'd recommend to have the certs in env var take precedence over path based cert because that's how I've seen precedence take place.
There was a problem hiding this comment.
It would probably behoove me to push commits after making them.
Thinking over this again in the morning re the second comment, I remembered I added it originally. This should prefer the envvar certs always, but the default path cert complicates that.
- The envvar cert is set, and the path cert is the default. The initial sanity checks succeed.
- The envvar cert is successfully loaded.
- The path cert block is evaluated. With the check, this does nothing, because the envvar setting is nonempty and must have successfully loaded the cert, so we skip this block. Without the check, the path is nonempty because of the default, so we run the block and overwrite the cert.
We can place the path block first and allow the envvar block to override any cert set in the path block, but that's complicated by the potential for either block to fail fatally. I don't think it's likely for the default to fail (if it happens, you probably have bigger problems), but it can theoretically block loading of valid configuration.
Restored the check with an explanatory comment. We could alternately handle this by placing the path block first and emit a fatal error only if admission-webhook-cert is empty, but IMO both options are equally awkward.
There was a problem hiding this comment.
Good call. Resolving and proceeding with merge now.
We don't have any unit testing around this and based on the bugs and corner-cases we have discussed, we should have added tests to assert the right behavior.
Not a blocker, merging now.
This reverts commit f5906c5.
What this PR does / why we need it:
Adds flags to provide the admin CA certificate and admission webhook cert/key pair as strings, as opposed to their existing file path flags.
Special notes for your reviewer:
Tests are for the flags only. I didn't see tests for the details of what main.go loads, or tests elsewhere that appear to set these flags and confirm the content of internal structures.
Manual debugger verification looked fine (insofar as the variables populated and didn't fatally abort), and the webhook listen returned the correct certificate. Didn't have a proper Kong setup for the CA cert part to confirm successful verification.
Ditto negative tests: the other failure cases for args don't appear to have tests in main.go, so none added for these. Manual checks look fine.