Don't assign IPs from terminating / finalizing ippools

[liuxu623]

Description

We shouldn't use a IPPool if it's deletionTimestamp is not zero.

Related issues/PRs

Todos

• Tests
• Documentation
• Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

• docs-pr-required: This change requires a change to the documentation that has not been completed yet.
• docs-completed: This change has all necessary documentation completed.
• docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

• release-note-required: This PR has user-facing changes. Most PRs should have this label.
• release-note-not-required: This PR has no user-facing changes.

Other optional labels:

• cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
• needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

[caseydavenport]

@liuxu623 nice, this seems like a good enhancement although I believe the scenario this protects against is meant to be covered by deletion logic already via this logic:

calico/libcalico-go/lib/clientv3/ippool.go

Lines 169 to 187 in 522e2d5

	if !pool.Spec.Disabled {
	logCxt.Debug("Disabling pool to release affinities")
	pool.Spec.Disabled = true
	// If the Delete has been called with a ResourceVersion then use that to perform the
	// update - that way we'll catch update conflicts (we could actually check here, but
	// the most likely scenario is there isn't one - so just pass it through and let the
	// Update handle any conflicts).
	if opts.ResourceVersion != "" {
	pool.ResourceVersion = opts.ResourceVersion
	}
	if _, err := r.Update(ctx, pool, options.SetOptions{}); err != nil {
	return nil, err
	}
	// Reset the resource version before the actual delete since the version of that resource
	// will now have been updated.
	opts.ResourceVersion = ""
	}

Either way, I think what you've done here is better since it protects against anyone sneaking around our validation logic.

[caseydavenport]

/sem-approve

[caseydavenport]

I think a unit test to verify this would be a nice addition, if you have some time.

[liuxu623]

I think a unit test to verify this would be a nice addition, if you have some time.

ok I will add unit test later.

[liuxu623]

#10260