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

✨ Use network resources managed outside cluster api #456

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Joseph94m
Copy link
Contributor

What type of PR is this?

/kind feature
/kind bug

What this PR does / why we need it:

By default, cluster API outscale provider create all network object: NatGW/PublicIP/IS/Net/Subnet. In our use case, we import all these object in the cluster YAML definition using the “resourceId” field. Nevertheless, when deleting a cluster with resource imported using “resourceId” field, some resources are deleted and it should not be the case. We should fix the outscale provider to prevent that.

Code of the outscale API provider:GitHub - outscale/cluster-api-provider-outscale

Issue is already opened : [Feature]: Allow to reuse parts of existing infrastructure · Issue #381 · outscale/cluster-api-provider-outscale

Controller to fix to not clean resources when a resourceId is given as an input in YAML:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #381

  • squashed commits
  • adds unit tests
  • includes documentation

cf @alistarle

@Joseph94m
Copy link
Contributor Author

/kind feature

@Joseph94m
Copy link
Contributor Author

/kind bug

@Joseph94m Joseph94m force-pushed the prevent-resource-deletion branch from 60f1429 to bfe021a Compare February 14, 2025 16:21
@Joseph94m
Copy link
Contributor Author

This could be extended to include other resources, we included what we needed.

@jfbus
Copy link
Contributor

jfbus commented Feb 17, 2025

This could be extended to include other resources, we included what we needed.

Bastion being one of them. But still it it a step in the right direction and we thank you for your contribution.

We still need to decide wether we only merge bugfixes (#451 being more a security fix than a feature) for now. We might wait until the next release for new features to be merged.

@jfbus jfbus self-assigned this Feb 17, 2025
@jfbus jfbus changed the title Prevent resource deletion ✨ Use network resources managed outside cluster api Feb 17, 2025
Copy link
Contributor

@jfbus jfbus left a comment

Choose a reason for hiding this comment

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

Your code only works for new clusters.

A cluster created by a previous version will not have the managed_by key, and the first update reconciliation with the new version will mark all resources as external, preventing their deletion.

@Joseph94m
Copy link
Contributor Author

Joseph94m commented Feb 17, 2025

Indeed, we did that because we did not want to change the resource spec.

We have thought of another implementation that can fix this issue, but it requires a modification to the resource spec.

Add a SkipReconcile Flag to every resource (default false so doesn't change the current behavior)

This flag, combined with the resourceId field in the yaml spec generates the following use cases:

We will propose another PR implementing the behavior soon. This should allow existing clusters to choose future reconciliation behavior simply by updating the yaml files defining the Cluster and applying it to the cluster.

Legend:

  • Flags:
    • Red=set to false for SkipReconcile / resourceID not set
    • Green = set to true for SkipReconcile / resourceID set
  • Reconciliation Loops:
    • Green = Will reconcile
    • Red = Won't reconcile

image

@jfbus
Copy link
Contributor

jfbus commented Feb 17, 2025

I'm not a fan of adding schema changes if we can avoid them.

I'm sure there is a way of doing it without impacting the schema using annotations. For each resource, adding an annotation would both set the resource id to use and declare it "not managed by cluster-api".

@Joseph94m Joseph94m force-pushed the prevent-resource-deletion branch 5 times, most recently from e52d7de to b75c950 Compare February 27, 2025 14:59
@Joseph94m Joseph94m force-pushed the prevent-resource-deletion branch from b75c950 to 93c04ec Compare February 27, 2025 15:00
@Joseph94m
Copy link
Contributor Author

We are using this implementation in-house for the time being.
The flag SkipReconcile is set to false by default and should not cause unintended changes in existing clusters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Allow to reuse parts of existing infrastructure
2 participants