feat: enforce labels on InstanceStart/InstanceStop operations#893
feat: enforce labels on InstanceStart/InstanceStop operations#893louisdtt wants to merge 32 commits into
Conversation
|
Hello @louisdtt Since then, we've merged a lot of code, all of it seems out of date unfortunately. Would you be able to resolve the conflicts ? |
|
Hello @acouvreur, should be good now 🙂 |
acouvreur
left a comment
There was a problem hiding this comment.
I think we should try to do no code changes in the providers as we expect the same behavior out of them all.
Can we simply call inspect when we receive an instance expiration, and skip the call to InstanceStop if we ignore those ?
Let's park other use cases for now. We might do this in two pull request and two different configurations because they are not the same things:
- One is to fail when a request target resources with instances not tracked by sablier
- The other is to ignore the shutdown for an instance that was (probably) started by sablier but was later removed
| if errors.As(err, &ErrInstanceNotManaged{}) { | ||
| logger.WarnContext(ctx, "instance expired but is not managed by sablier, skipping stop", slog.String("instance", key)) | ||
| return | ||
| } |
There was a problem hiding this comment.
I feel like we could avoid all provider code changes and put it at the sablier package level. For example, when we receive an instance expired event, we could get the instance information using InstanceInspect, and check if it is enabled or not.
If it is not enabled, we can log this.
|
I think you are right, this is a bit much, i'll leave this PR as a draft for now and reopen one focusing on your first point |
|
Superseded by #908 |
Summary
This feature follows up issue #857 and @mzyy94 work, switching
strict-labelstoignore-unlabeled.It adds a
provider.ignore-unlabeledflag, disabled by default, to make Sablier ignore resources that are not labeled withsablier.enable=true.When enabled, providers reject start/stop operations for unlabeled instances and filter stopped/remove events so unmanaged resources do not trigger Sablier.
Changes
provider.ignore-unlabeledconfig and docs.Tests
Ran project tests and currently using this fork with
provider.ignore-unlabeled=trueon a Kind and an AKS cluster.