-
Notifications
You must be signed in to change notification settings - Fork 24
Config follow-up: Implement component override functionality for Config resource #475
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
base: main
Are you sure you want to change the base?
Conversation
21f23b4 to
1fb3071
Compare
1fb3071 to
b183d8b
Compare
b183d8b to
ddcb996
Compare
|
@andreaskaris, this pull request is now in conflict and requires a rebase. |
This commit adds the ability to mark components as unmanaged in the bpfman-operator, preventing the operator from creating or updating specific objects. The implementation includes: - Added ComponentOverride struct to Config API with fields for kind, group, namespace, name, and unmanaged flag - Modified assureResource function to check for overrides and skip management of unmanaged components - Implemented isOverridden helper function to match objects against override specifications - Added tests to verify override functionality works correctly across all component types Signed-off-by: Andreas Karis <ak.karis@gmail.com>
Signed-off-by: Andreas Karis <ak.karis@gmail.com>
Signed-off-by: Andreas Karis <ak.karis@gmail.com>
ddcb996 to
8f6b916
Compare
frobware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConfigMap should not be overrideable given that it's now an internal implementation detail of the Config CRD (post #461). Should isOverrideable narrow the list down to the DaemonSet, CSIDriver, SCC -- everthing else is excluded.
| // deployed. | ||
| Namespace string `json:"namespace,omitempty"` | ||
|
|
||
| // overrides is list of overides for components that are managed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: s/overides/overrides
|
|
||
| // overrides is list of overides for components that are managed by | ||
| // the operator. Marking a component unmanaged will prevent | ||
| // the operator from creating or updating the object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is preventing creation desirable? Should the behaviour be to only prevent updates, not creation? What happens if a resource is accidentally deleted? Do we just drop the unmanaged state, and let the operator recreate it?
| // +required | ||
| Name string `json:"name"` | ||
|
|
||
| // unmanaged controls if cluster version operator should stop managing the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be: unmanaged controls if cluster version operator bpfman operator should stop managing the ...
| Group: "", | ||
| Namespace: "bpfman", | ||
| Name: "bpfman-config", | ||
| Unmanaged: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post #461, isn't the configmap now an implementation detail? Should we allow for the configmap to be unmanaged?
|
I played around with the functionality and have some suggestions for UX in #489. |
|
Replying to my own question here after experimenting with this. For a debug/escape hatch, I think blocking creation is the right behaviour. The semantics become simple and predictable: unmanaged = the operator pretends this resource doesn't exist. If you mark something as unmanaged and then delete it, you presumably meant to delete it. Recreating something marked "unmanaged" would be semantically incoherent - you're telling the operator not to manage it, but then expecting it to manage recreation. If deletion was accidental, the recovery is straightforward: remove the override, and the operator recreates the resource on the next reconcile. I tested this flow and it works as expected. The alternative (only blocking updates, allowing recreation) adds complexity and creates confusing edge cases. For a feature explicitly framed as a debug escape hatch for advanced users, I'd keep it simple and trust the user knows what they're doing. |
This commit adds the ability to mark components as unmanaged in the
bpfman-operator, preventing the operator from creating or updating
specific objects. The implementation includes:
kind, group, namespace, name, and unmanaged flag
management of unmanaged components
override specifications
across all component types
Documentation
This will need documentation that states how to generate the overrides, e.g. (generated with an LLM, lgtm but haven't had the time to retest the below combinations, yet):
Using JSON patch for more precise control:
The /- notation appends to the existing overrides array rather than replacing it.