cluster-api-provider-proxmox icon indicating copy to clipboard operation
cluster-api-provider-proxmox copied to clipboard

Adding Finalizer on Secret

Open erwin-kok opened this issue 1 year ago • 6 comments
trafficstars

  • Added Finalizer on CredentialsRef Secret
  • Added test to test the secret Finalizer
  • The "ProxmoxClusterTemplate" was probably added manually, this lacked some files and an item in PROJECT. Added this properly using kubebuilder.
  • A bit of refactoring. i.s.o SetupWithManager, use AddProxmoxClusterReconciler/AddProxmoxMachineReconciler
  • Tested manually and using "make test"

erwin-kok avatar Sep 01 '24 21:09 erwin-kok

Thanks for reviewing Mohamed (@mcbenjemaa)!

Can you clarify why the secret should be re-used over clusters? So lets say I have an prod and a development cluster, they can share their secrets? A development cluster (accidentally using the wrong secret) can deploy something in prod? Or perhaps I misunderstood something.

erwin-kok avatar Sep 02 '24 16:09 erwin-kok

Thanks for reviewing Mohamed (@mcbenjemaa)!

Can you clarify why the secret should be re-used over clusters? So lets say I have an prod and a development cluster, they can share their secrets? A development cluster (accidentally using the wrong secret) can deploy something in prod? Or perhaps I misunderstood something.

That's not the case. The idea here is that the credentials secret should be global object. If you added a cluster that uses a secret for proxmox PROD, you no longer need to add the secret again to create another cluster in prod. Whereas, if you need to create a cluster in Dev proxmox, you will need to check if there's a secret already, or you create your own.

If the user chooses the wrong secret and creates a cluster in a different proxmox cluster, that's the user's fault, and we can't do anything about it. (cause we don't know which is correct and which is wrong)

mcbenjemaa avatar Sep 02 '24 16:09 mcbenjemaa

Thanks for clarifying. I assumed that the approach would be to deploy a different cluster to a different namespace (in the management cluster). So namespace"prod" manages the "prod" cluster, and namespace "dev" manages the "dev" cluster, etc. And every namespace has its own secrets, not shared over namespaces. In this way, a dev namespace (cluster) should not access prod secrets. If you have two prod clusters, prod1 and prod2, they both have their own (potentially the same) secrets. Of course, if you deploy prod secrets in a dev cluster then it uses that...

erwin-kok avatar Sep 02 '24 16:09 erwin-kok

You need to solve the conflict.

mcbenjemaa avatar Sep 30 '24 11:09 mcbenjemaa

I think we are close to merging this: @erwin-kok please fix the linter issues, once that's done @mcbenjemaa can do his final review.

wikkyk avatar Oct 08 '24 13:10 wikkyk

So, there were two linting issues, both not related to my code (but I solved them anyway):

  • Unblocking of else branch:
if .... {

  return
} else {
   something
}

because the if-statement ends with a return, the else branch can be "unblocked":

if ... {

  return
}
something
  • Removing unused lint statement: //nolint:dogsled

erwin-kok avatar Oct 09 '24 17:10 erwin-kok