-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Support Check-And-Set deletion of config entries #11419
Conversation
🤔 This PR has changes in 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.
Awesome work with your first PR! Thanks for tackling this gap.
I left some comments/questions below.
reply.Deleted = deleted | ||
} else { | ||
// For non-CAS deletions any non-error result indicates a successful deletion. | ||
reply.Deleted = 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.
Is this backwards compatible with older clients?
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.
I think so, yeah. Older clients will unmarshal the response into a struct{}
which seems to be fine:
package compattest
import (
"testing"
"bytes"
"github.com/hashicorp/go-msgpack/codec"
"github.com/hashicorp/consul/agent/structs"
)
func TestBackwardCompatibility(t *testing.T) {
var b bytes.Buffer
encoder := codec.NewEncoder(&b, structs.MsgpackHandle)
decoder := codec.NewDecoder(&b, structs.MsgpackHandle)
if err := encoder.Encode(&structs.ConfigEntryDeleteResponse{Deleted: true}); err != nil {
t.Fatal(err)
}
var foo struct{}
if err := decoder.Decode(&foo); err != nil {
t.Fatal(err)
}
var bar string
if err := decoder.Decode(&bar); err == nil {
t.Fatal("should not have been able to decode into a string")
}
}
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.
Cool, I also tried it out locally with two agents on different versions and it worked
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.
🚀
These were failing because of the new return value of the `ConfigEntry.Delete` RPC, which is only a problem for the `inmemCodec` used by servers — the msgpack codec provides wire backwards-compatibility.
7a2f93d
to
79824b6
Compare
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/490134. |
Adds support for Check-And-Set (CAS) deletion of config entries, similar to that of KVs.
This is useful for cases where it is desirable to delete a config entry, but only if it has not been modified by another process since reading it (such as the scenario described in #11372).
Sidenote: this is my first PR to Consul 🎉 so any meta/style feedback is greatly appreciated!