feat: create a Certificate object for each VM #1213
Conversation
conradludgate
force-pushed
the
conrad/compute-certificate-provisioning
branch
from
7f8372b to
e9121eb
Compare
No changes to the coverage.
HTML Report |
conradludgate
force-pushed
the
conrad/compute-certificate-provisioning
branch
from
c7a422c to
4592eb0
Compare
conradludgate
force-pushed
the
conrad/compute-certificate-provisioning
branch
from
4592eb0 to
13ed571
Compare
|
|
Yeah - when paired with the other PR, yes it does reload as expected. Still needs work to make postgres to recognise the change, but yes it should work |
| @@ -3,7 +3,7 @@ cloud.google.com/go/compute v1.23.0 h1:tP41Zoavr8ptEqaW6j+LQOnyBBhO7OkOMAGrgLopT | |||
| cloud.google.com/go/compute v1.23.0/go.mod h1:4tCnrn48xsqlwSAiLf1HXMQk8CONslYbdiEZc9FEIbM= | |||
| cloud.google.com/go/compute/metadata v0.2.3 h1:mg4jlk7mCAj6xXp9UJ4fjI9VUI5rubuGBW5aJ7UnBMY= | |||
| cloud.google.com/go/compute/metadata v0.2.3/go.mod h1:VAV5nSsACxMJvgaAuX6Pk2AawlZn8kiOGuCv6gTkwuA= | |||
| github.com/Azure/azure-sdk-for-go v66.0.0+incompatible h1:bmmC38SlE8/E81nNADlgmVGurPWMHDX2YNXVQMrBpEE= | |||
| github.com/Azure/azure-sdk-for-go v67.3.0+incompatible h1:QEvenaO+Y9ShPeCWsSAtolzVUcb0T0tPeek5TDsovuM= | |||
There was a problem hiding this comment.
question(non-blocking): Any idea why this was bumped? from adding a dependency on cert manager (?)
| @@ -442,6 +442,8 @@ kind-setup: kind kubectl ## Create local cluster by kind tool and prepared confi | |||
| $(KIND) create cluster --name $(CLUSTER_NAME) --config kind/config.yaml | |||
| $(KUBECTL) --context kind-$(CLUSTER_NAME) apply -f https://github.com/cert-manager/cert-manager/releases/latest/download/cert-manager.yaml | |||
| $(KUBECTL) --context kind-$(CLUSTER_NAME) -n cert-manager rollout status deployment cert-manager | |||
| $(KUBECTL) --context kind-$(CLUSTER_NAME) -n cert-manager rollout status deployment cert-manager-webhook | |||
| $(KUBECTL) --context kind-$(CLUSTER_NAME) apply -f k3d/certs.yaml | |||
There was a problem hiding this comment.
suggestion: If possible, let's avoid "kind cluster with 'k3d' certs"? Might be worth duplicating the files. Not sure... WDYT?
| // If a TlsCertificateIssuer is provided, this is required to set the duration that the certificate should | ||
| // be valid for before expiring |
There was a problem hiding this comment.
If a TlsCertificateIssuer is provided, this is required
suggestion: Maybe make a unified TlsConfig struct, so that that including TLS information is optional, but if present, each field in the struct is required?
| @@ -142,6 +142,20 @@ type VirtualMachineSpec struct { | |||
| // +optional | |||
| EnableSSH *bool `json:"enableSSH,omitempty"` | |||
|
|
|||
| // The CertificateIssuer for the certificates issued to this VM | |||
| // +optional | |||
| TlsCertificateIssuer *string `json:"tlsCertificateIssuer,omitempty"` | |||
There was a problem hiding this comment.
nit: Mixed Tls and TLS ? Let's pick one — IIRC the go style guide recommends all-caps for acronyms in this case, but probably better to judge by whatever the ecosystem uses more frequently
There was a problem hiding this comment.
TLS is fine by me. I got stuck on Rust styling :D
There was a problem hiding this comment.
(tbf, the rust styling is much better...)
| // If a TlsCertificateIssuer is provided, this is required to set the duration before certificate expiration | ||
| // that the certificate is renewed | ||
| // +optional | ||
| TLSRenewal *metav1.Duration `json:"tlsRenewal,omitempty"` |
There was a problem hiding this comment.
suggestion: IIUC we have have both TLSRenewal as a duration in the spec (indicating the renewal period?) and TLSRenewal as a timestamp in the status (indicating the time of next renewal?). Let's differentiate these somehow? Not sure about the duration, but for timestamps, I think I often see FooAt and FooTimestamp
|
|
||
| certSecret := &corev1.Secret{} | ||
|
|
||
| if enableTLS { |
There was a problem hiding this comment.
commentary (feel free to ignore): I think medium-/long-term this logic shouldn't be outside the switch statement on VM status, but that's blocked on larger refactorings, so I don't think there's anything to do on that front for now.
cc @Omrigan re: neonvm-controller refactoring, if you're still looking to do that
| } else if err != nil { | ||
| log.Error(err, "Failed to get vm-runner Secret") | ||
| return nil, err |
There was a problem hiding this comment.
This if err != nil is far down from the original call. Let's flip the ordering? e.g.
if err != nil && !apierrors.IsNotFound(err) {
// return err
} else if err != nil /* not found */ {
// generate new secret...
}
key, err := pki.DecodePrivateKeyBytes(tmpKeySecret)
// ...| @@ -786,6 +835,134 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) | |||
| return nil | |||
| } | |||
|
|
|||
| func (r *VMReconciler) doReconcileCertificateSecret(ctx context.Context, vm *vmv1.VirtualMachine, certSecret *corev1.Secret) (*corev1.Secret, error) { | |||
There was a problem hiding this comment.
suggestion: It seems like:
- This function has some mixing of high-level logic ("pass the secret key we stored into creating a signing request") with lower level operations ("generate a key, generate a secret, create the secret in k8s").
- We're doing a few similar operations ("check if X exists, if not create it")
- Some of the operations we're doing are mirrored in the place this function is called ("check if TLSSecret exists, if not call
doReconcileCertificateSecret")
WDYT about:
- Unifying all the certificate secret operations into a single function
- Splitting out the individual "create X if not exists" operations into separate functions, so the high level logic is easier to follow? (still valuable to have separation of object generation and k8s operations -- in this case, I'm asking to have glue functions that do "check if object exists; if not, generate it with existing function, and create in k8s")
?
(of course, doReconcile already commits these sins in abundance. We'd like to refactor that, but haven't gotten around to it — just trying to make sure that at least new code is up to higher standards 😄)
There was a problem hiding this comment.
just trying to make sure that at least new code is up to higher standards
aww man, and there was me thinking I could get away with copy-pasting everything :D
| func certSpec( | ||
| vm *vmv1.VirtualMachine, | ||
| ) *certv1.Certificate { |
| key crypto.PrivateKey, | ||
| ) (*corev1.Secret, error) { | ||
| runnerVersion := api.RunnerProtoV1 | ||
| labels := labelsForVirtualMachine(vm, &runnerVersion) |
There was a problem hiding this comment.
suggestion: Do we have labels on the SSH secret that we create? if no, I propose we leave them out here, as it's added complexity and we're unlikely to uphold the guarantees we typically make (that labels are continuously propagated from the VM to the object — which is what we do for pods)
+ Same thing re: certificate request, certificate, and the TLS secret
https://github.com/neondatabase/cloud/issues/22997
Initially, this PR set out to create a
cert-manager.io/Certificateobject for each VM. However, a design decision for cert-manager has the associated secrets produced as orphans. This caused secrets to accumulate without cleanup.Instead, I have augmented the flow to emulate
Certificate.This only takes place if the VM spec lists a TlsCertificateIssuer value