-
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
WatchList(..) endpoint for the resource service #16726
Merged
analogue
merged 34 commits into
main
from
spatel/NET-2692-resource-service-watchlist-endpoint
Mar 27, 2023
Merged
Changes from 31 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
e8b49f8
storage: add backend interface and related types
boxofrad ae28bc8
storage: add conformance test suite
boxofrad 531e995
storage: implement in-memory backend
boxofrad 74eef9a
storage: better error messages
boxofrad 2731ba8
storage: owner references should be anchored to a specific uid
boxofrad c25a7ec
Remove unused test function
boxofrad d5733f9
storage: clarify List docs
boxofrad 56c4c3a
storage: fix potential out-of-order events
boxofrad 033b612
storage: clarify WatchList consistency model
boxofrad f54bcb8
storage: s/Check-And-Set/Compare-And-Swap/
boxofrad 0167a5b
storage: move event construction into publishEvent
boxofrad 048712d
storage: document Read-Modify-Write patterns
boxofrad fbbbec9
storage: add clarifying comment about resource creation
boxofrad e17b50b
storage: clarify OwnerReferences consistency
boxofrad f441187
storage: separate ErrConflict into two errors
boxofrad 84ff540
storage: make backends responsible for managing the version
boxofrad d7ac1c6
storage: make consistency an argument to Read
boxofrad ba94207
storage: add consistency parameter to List
boxofrad 6e39fd5
storage: more correct consistency documentation
boxofrad 4580e51
storage: fix integer alignment in inmem backend
boxofrad 0748073
storage: support eventual consistency in conformance tests
boxofrad 0a49eff
storage: correct eventLock comment
boxofrad 8c75333
storage: rearrange inmem store files
boxofrad 8bc4a80
storage: fix bug where watches could emit duplicate events
boxofrad 730a73a
WatchList(..) endpoint for the resource service
analogue dc0f42b
Remove duplicate watch event
analogue 7aaffec
Factor out resource.resolveType(..) so it can be re-used by all endpo…
analogue c11fbca
DRY server creation in tests to testServer(...)
analogue ed45648
Parallelize tests and shorten wait for no event
analogue 2d85d82
Fix import
analogue 28c49ff
make proto-format happy
analogue 7e34f99
Merge branch 'main' into spatel/NET-2692-resource-service-watchlist-e…
analogue 28caf48
Fix dupe WatchEvent creation
analogue e816a84
make proto-lint happy
analogue File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,43 @@ | ||||||||||||
package resource | ||||||||||||
|
||||||||||||
import ( | ||||||||||||
"github.com/hashicorp/consul/internal/storage" | ||||||||||||
"github.com/hashicorp/consul/proto-public/pbresource" | ||||||||||||
) | ||||||||||||
|
||||||||||||
func (s *Server) WatchList(req *pbresource.WatchListRequest, stream pbresource.ResourceService_WatchListServer) error { | ||||||||||||
// check type exists | ||||||||||||
if _, err := s.resolveType(req.Type); err != nil { | ||||||||||||
return err | ||||||||||||
} | ||||||||||||
|
||||||||||||
unversionedType := storage.UnversionedTypeFrom(req.Type) | ||||||||||||
watch, err := s.backend.WatchList( | ||||||||||||
stream.Context(), | ||||||||||||
unversionedType, | ||||||||||||
req.Tenancy, | ||||||||||||
req.NamePrefix, | ||||||||||||
) | ||||||||||||
if err != nil { | ||||||||||||
return err | ||||||||||||
} | ||||||||||||
|
||||||||||||
for { | ||||||||||||
event, err := watch.Next(stream.Context()) | ||||||||||||
if err != nil { | ||||||||||||
return err | ||||||||||||
} | ||||||||||||
|
||||||||||||
// drop versions that don't match | ||||||||||||
if event.Resource.Id.Type.GroupVersion != req.Type.GroupVersion { | ||||||||||||
continue | ||||||||||||
} | ||||||||||||
|
||||||||||||
if err = stream.Send(&pbresource.WatchEvent{ | ||||||||||||
Operation: event.Operation, | ||||||||||||
Resource: event.Resource, | ||||||||||||
}); err != nil { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super nit 🐜 I think you could simplify this to just:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Fixed in 28caf48 |
||||||||||||
return err | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,222 @@ | ||
package resource | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"io" | ||
"testing" | ||
"time" | ||
|
||
"github.com/hashicorp/consul/internal/resource" | ||
"github.com/hashicorp/consul/internal/storage/inmem" | ||
"github.com/hashicorp/consul/proto-public/pbresource" | ||
"github.com/hashicorp/consul/proto/private/prototest" | ||
|
||
"github.com/stretchr/testify/require" | ||
"google.golang.org/grpc/codes" | ||
"google.golang.org/grpc/status" | ||
"google.golang.org/protobuf/proto" | ||
) | ||
|
||
func TestWatchList_TypeNotFound(t *testing.T) { | ||
t.Parallel() | ||
server := testServer(t) | ||
client := testClient(t, server) | ||
|
||
stream, err := client.WatchList(context.Background(), &pbresource.WatchListRequest{ | ||
Type: typev1, | ||
Tenancy: tenancy, | ||
NamePrefix: "", | ||
}) | ||
require.NoError(t, err) | ||
rspCh := handleResourceStream(t, stream) | ||
|
||
err = mustGetError(t, rspCh) | ||
require.Equal(t, codes.InvalidArgument.String(), status.Code(err).String()) | ||
require.Contains(t, err.Error(), "resource type mesh/v1/service not registered") | ||
} | ||
|
||
func TestWatchList_GroupVersionMatches(t *testing.T) { | ||
t.Parallel() | ||
server := testServer(t) | ||
client := testClient(t, server) | ||
server.registry.Register(resource.Registration{Type: typev1}) | ||
ctx := context.Background() | ||
|
||
// create a watch | ||
stream, err := client.WatchList(ctx, &pbresource.WatchListRequest{ | ||
Type: typev1, | ||
Tenancy: tenancy, | ||
NamePrefix: "", | ||
}) | ||
require.NoError(t, err) | ||
rspCh := handleResourceStream(t, stream) | ||
|
||
// insert and verify upsert event received | ||
r1, err := server.backend.WriteCAS(ctx, resourcev1) | ||
require.NoError(t, err) | ||
rsp := mustGetResource(t, rspCh) | ||
require.Equal(t, pbresource.WatchEvent_OPERATION_UPSERT, rsp.Operation) | ||
prototest.AssertDeepEqual(t, r1, rsp.Resource) | ||
|
||
// update and verify upsert event received | ||
r2 := clone(r1) | ||
r2, err = server.backend.WriteCAS(ctx, r2) | ||
require.NoError(t, err) | ||
rsp = mustGetResource(t, rspCh) | ||
require.Equal(t, pbresource.WatchEvent_OPERATION_UPSERT, rsp.Operation) | ||
prototest.AssertDeepEqual(t, r2, rsp.Resource) | ||
|
||
// delete and verify delete event received | ||
err = server.backend.DeleteCAS(ctx, r2.Id, r2.Version) | ||
require.NoError(t, err) | ||
rsp = mustGetResource(t, rspCh) | ||
require.Equal(t, pbresource.WatchEvent_OPERATION_DELETE, rsp.Operation) | ||
} | ||
|
||
func TestWatchList_GroupVersionMismatch(t *testing.T) { | ||
// Given a watch on typev2 that only differs from typev1 by GroupVersion | ||
// When a resource of typev1 is created/updated/deleted | ||
// Then no watch events should be emitted | ||
t.Parallel() | ||
server := testServer(t) | ||
client := testClient(t, server) | ||
ctx := context.Background() | ||
server.registry.Register(resource.Registration{Type: typev1}) | ||
server.registry.Register(resource.Registration{Type: typev2}) | ||
|
||
// create a watch for typev2 | ||
stream, err := client.WatchList(ctx, &pbresource.WatchListRequest{ | ||
Type: typev2, | ||
Tenancy: tenancy, | ||
NamePrefix: "", | ||
}) | ||
require.NoError(t, err) | ||
rspCh := handleResourceStream(t, stream) | ||
|
||
// insert | ||
r1, err := server.backend.WriteCAS(ctx, resourcev1) | ||
require.NoError(t, err) | ||
|
||
// update | ||
r2 := clone(r1) | ||
r2, err = server.backend.WriteCAS(ctx, r2) | ||
require.NoError(t, err) | ||
|
||
// delete | ||
err = server.backend.DeleteCAS(ctx, r2.Id, r2.Version) | ||
require.NoError(t, err) | ||
|
||
// verify no events received | ||
mustGetNoResource(t, rspCh) | ||
} | ||
|
||
func testServer(t *testing.T) *Server { | ||
t.Helper() | ||
|
||
backend, err := inmem.NewBackend() | ||
require.NoError(t, err) | ||
go backend.Run(testContext(t)) | ||
|
||
registry := resource.NewRegistry() | ||
return NewServer(Config{registry: registry, backend: backend}) | ||
} | ||
|
||
func mustGetNoResource(t *testing.T, ch <-chan resourceOrError) { | ||
t.Helper() | ||
|
||
select { | ||
case rsp := <-ch: | ||
require.NoError(t, rsp.err) | ||
require.Nil(t, rsp.rsp, "expected nil response with no error") | ||
case <-time.After(250 * time.Millisecond): | ||
return | ||
} | ||
} | ||
|
||
func mustGetResource(t *testing.T, ch <-chan resourceOrError) *pbresource.WatchEvent { | ||
t.Helper() | ||
|
||
select { | ||
case rsp := <-ch: | ||
require.NoError(t, rsp.err) | ||
return rsp.rsp | ||
case <-time.After(1 * time.Second): | ||
t.Fatal("timeout waiting for WatchListResponse") | ||
return nil | ||
} | ||
} | ||
|
||
func mustGetError(t *testing.T, ch <-chan resourceOrError) error { | ||
t.Helper() | ||
|
||
select { | ||
case rsp := <-ch: | ||
require.Error(t, rsp.err) | ||
return rsp.err | ||
case <-time.After(2 * time.Second): | ||
t.Fatal("timeout waiting for WatchListResponse") | ||
return nil | ||
} | ||
} | ||
|
||
func handleResourceStream(t *testing.T, stream pbresource.ResourceService_WatchListClient) <-chan resourceOrError { | ||
t.Helper() | ||
|
||
rspCh := make(chan resourceOrError) | ||
go func() { | ||
for { | ||
rsp, err := stream.Recv() | ||
if errors.Is(err, io.EOF) || | ||
errors.Is(err, context.Canceled) || | ||
errors.Is(err, context.DeadlineExceeded) { | ||
return | ||
} | ||
rspCh <- resourceOrError{ | ||
rsp: rsp, | ||
err: err, | ||
} | ||
} | ||
}() | ||
return rspCh | ||
} | ||
|
||
var ( | ||
tenancy = &pbresource.Tenancy{ | ||
Partition: "default", | ||
Namespace: "default", | ||
PeerName: "local", | ||
} | ||
typev1 = &pbresource.Type{ | ||
Group: "mesh", | ||
GroupVersion: "v1", | ||
Kind: "service", | ||
} | ||
typev2 = &pbresource.Type{ | ||
Group: "mesh", | ||
GroupVersion: "v2", | ||
Kind: "service", | ||
} | ||
resourcev1 = &pbresource.Resource{ | ||
Id: &pbresource.ID{ | ||
Uid: "someUid", | ||
Name: "someName", | ||
Type: typev1, | ||
Tenancy: tenancy, | ||
}, | ||
Version: "", | ||
} | ||
) | ||
|
||
type resourceOrError struct { | ||
rsp *pbresource.WatchEvent | ||
err error | ||
} | ||
|
||
func clone[T proto.Message](v T) T { return proto.Clone(v).(T) } | ||
|
||
func testContext(t *testing.T) context.Context { | ||
ctx, cancel := context.WithCancel(context.Background()) | ||
t.Cleanup(cancel) | ||
return ctx | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Reminder to myself 😄
When #16619 is merged, we'll need to update this to handle
ErrWatchClosed
(which I've just noticed is in the wrong package too).