giantswarm / roadmap

Giant Swarm Product Roadmap
https://github.com/orgs/giantswarm/projects/273
Apache License 2.0
3 stars 0 forks source link

Refactor `aws-network-topology-operator` to put finalizer on the AWSCluster instead of Cluster #1827

Closed alex-dabija closed 10 months ago

alex-dabija commented 1 year ago

Task

Refactor aws-network-topology-operator to put finalizer on the AWSCluster instead of Cluster.

Towards epic.

Background

Currently, the aws-network-topology-operator adds a finalizer on the Cluster resources. This leads to a race condition because in certain situations the AWSCluster resource could be deleted before the operator is ready to remove the finalizer from the Cluster resource and it still expects the AWSCluster resource to be around.

This probably doesn't happen that often because in order for the AWSCluster resource to be deleted the transit gateway attachment needs to be removed.

Update 2023-08: When implementing this, ensure https://github.com/giantswarm/roadmap/issues/2714#issuecomment-1673135140 is mentioned as use case, and gets fixed.

### Tasks
- [ ] https://github.com/giantswarm/giantswarm/issues/28489
bdehri commented 1 year ago

It looks like finalizer is put on both Cluster and AWSCluster. So, what we want to do is put the finalizer only on AWSCluster ?

func (g *Cluster) AddFinalizer(ctx context.Context, capiCluster *capi.Cluster, finalizer string) error {
  originalCluster := capiCluster.DeepCopy()
  controllerutil.AddFinalizer(capiCluster, finalizer)
  if err := g.Client.Patch(ctx, capiCluster, client.MergeFrom(originalCluster)); err != nil {
    return err
  }
  capaCluster, err := g.GetAWSCluster(ctx, types.NamespacedName{Name: capiCluster.Name, Namespace: capiCluster.Namespace})
  if err != nil {
    return err
  }
  originalCAPACluster := capaCluster.DeepCopy()
  controllerutil.AddFinalizer(capaCluster, finalizer)
  return g.Client.Patch(ctx, capaCluster, client.MergeFrom(originalCAPACluster))
}
fiunchinho commented 1 year ago

I think the controller should only reconcile AWSClusters and put the finalizer only to AWSClusters.

mnitchev commented 10 months ago

Done in https://github.com/giantswarm/aws-resolver-rules-operator/releases/tag/v0.13.0