[WIP] Reject deleting default felixconfiguration#12110
[WIP] Reject deleting default felixconfiguration#12110mazdakn wants to merge 1 commit intoprojectcalico:masterfrom
Conversation
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull request overview
Prevents deletion of the global default FelixConfiguration (name: "default") via the libcalico-go v3 client, to protect the baseline Felix configuration object from being removed.
Changes:
- Add
DefaultFelixConfigurationName = "default"constant. - Reject
FelixConfigurations().Delete(..., "default", ...)by returningErrorOperationNotSupported.
| if name == DefaultFelixConfigurationName { | ||
| return nil, cerrors.ErrorOperationNotSupported{ | ||
| Identifier: name, | ||
| Operation: "Delete", | ||
| Reason: fmt.Sprintf("Cannot delete %v felixconfiguration", name), | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This change makes deleting the global FelixConfiguration named "default" impossible, but Felix FV infrastructure currently relies on being able to delete that object during topology setup when Create returns ErrorResourceAlreadyExists (see felix/fv/infrastructure/topology.go around the InitialFelixConfiguration install loop). With this change, that Eventually loop will keep failing and FV runs will likely hang/fail whenever an unexpected default FelixConfiguration exists. Update the callers to Get+Update the existing "default" FelixConfiguration (or otherwise reset it) instead of attempting Delete, or provide an alternative supported path for tests/tools that need to replace the default config.
| if name == DefaultFelixConfigurationName { | |
| return nil, cerrors.ErrorOperationNotSupported{ | |
| Identifier: name, | |
| Operation: "Delete", | |
| Reason: fmt.Sprintf("Cannot delete %v felixconfiguration", name), | |
| } | |
| } |
| return nil, cerrors.ErrorOperationNotSupported{ | ||
| Identifier: name, | ||
| Operation: "Delete", | ||
| Reason: fmt.Sprintf("Cannot delete %v felixconfiguration", name), |
There was a problem hiding this comment.
The ErrorOperationNotSupported reason string is hard to read and inconsistent with similar messages elsewhere (e.g., Profile uses quoted names and a full sentence). Consider using proper resource casing ("FelixConfiguration") and quoting the name (e.g., %q) so the final error is clearer for users and for any tooling that surfaces these errors.
| Reason: fmt.Sprintf("Cannot delete %v felixconfiguration", name), | |
| Reason: fmt.Sprintf("Cannot delete FelixConfiguration %q", name), |
| if name == DefaultFelixConfigurationName { | ||
| return nil, cerrors.ErrorOperationNotSupported{ | ||
| Identifier: name, | ||
| Operation: "Delete", | ||
| Reason: fmt.Sprintf("Cannot delete %v felixconfiguration", name), | ||
| } | ||
| } |
There was a problem hiding this comment.
New behavior (rejecting Delete for the "default" FelixConfiguration) should be covered by tests. There are existing clientv3 FelixConfiguration e2e CRUD tests; add an assertion that Delete("default") returns ErrorOperationNotSupported (and does not delete the resource) to prevent regressions and to make the contract explicit.
Release note: