Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding Finalizer on Secret #279

Merged
merged 23 commits into from
Oct 9, 2024
Merged

Adding Finalizer on Secret #279

merged 23 commits into from
Oct 9, 2024

Conversation

erwin-kok
Copy link
Contributor

  • 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"

Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Credentials Secret is meant to be a global secret that is used by multiple clusters.
Please change the logic.

internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
api/v1alpha1/proxmoxmachinetemplate_types.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
@erwin-kok
Copy link
Contributor Author

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.

@mcbenjemaa
Copy link
Member

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)

@erwin-kok
Copy link
Contributor Author

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...

internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/suite_test.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/suite_test.go Outdated Show resolved Hide resolved
internal/controller/proxmoxcluster_controller_test.go Outdated Show resolved Hide resolved
@mcbenjemaa
Copy link
Member

You need to solve the conflict.

@erwin-kok erwin-kok marked this pull request as ready for review September 30, 2024 18:39
@wikkyk
Copy link
Collaborator

wikkyk commented Oct 8, 2024

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.

@erwin-kok
Copy link
Contributor Author

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

Copy link

sonarqubecloud bot commented Oct 9, 2024

@mcbenjemaa mcbenjemaa enabled auto-merge (squash) October 9, 2024 17:49
@mcbenjemaa mcbenjemaa merged commit 7d9b12b into ionos-cloud:main Oct 9, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants