Skip to content
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

Add nil check for billing mode in AWS DynamoDB events driver #12445

Merged
merged 13 commits into from
May 5, 2022

Conversation

xacrimon
Copy link
Contributor

@xacrimon xacrimon commented May 5, 2022

Turns out, AWS is inconsistent in what fields they set and they don't always set the BillingModeSummary field if the table is in provisioned mode. Wasn't able to find that noted anywhere so I assumed it would be, this turned out to be incorrect.

This PR adds some pessimistic null-checks and assumed the billing mode is provisioned if the fields are not set. Still not sure when they are set, just that they sometimes aren't. This was further not helped by our AWS tests somehow not failing on a panic, they just seem to pause and continue? That's for another PR though.

This also fixes a panic case in dynamic.go if the provided map is lacking an event type key which caused AWS tests to fail.

Fix #12443

@github-actions github-actions bot requested review from nklaassen and Tener May 5, 2022 05:12
@github-actions github-actions bot added the audit-log Issues related to Teleports Audit Log label May 5, 2022
@xacrimon xacrimon force-pushed the joel/fix-nil-check branch from b7f4b57 to 49610cf Compare May 5, 2022 05:17
@xacrimon xacrimon force-pushed the joel/fix-nil-check branch from 49610cf to 886971b Compare May 5, 2022 05:44
@Tener
Copy link
Contributor

Tener commented May 5, 2022

It would be good to have some tests along with these changes.

@Tener
Copy link
Contributor

Tener commented May 5, 2022

From failed unit tests:

panic: reflect: call of reflect.Value.IsNil on string Value

goroutine 11921 [running]:
reflect.Value.IsNil(...)
	/opt/go/src/reflect/value.go:1440
github.com/gravitational/teleport/lib/events.isInterfaceNil({0x4449fa0, 0xc00422f4b0})
	/workspace/lib/events/dynamic.go:33 +0x25f
github.com/gravitational/teleport/lib/events.FromEventFields(0x5a1fff8)
	/workspace/lib/events/dynamic.go:50 +0xc5
github.com/gravitational/teleport/lib/events.(*FileLog).searchEventsWithFilter(0xc008fc4658, {0xcb17b4, 0x0, 0x0}, {0xc008fc4df8, 0x0, 0x0}, {0x0, 0x75b18c0}, 0x1f4, ...)
	/workspace/lib/events/filelog.go:229 +0x57a
github.com/gravitational/teleport/lib/events.(*FileLog).SearchEvents(0xc0036ea120, {0xc008fc4a88, 0x0, 0x0}, {0x59e25a0, 0xc0076ce480, 0x0}, {0xc00962f049, 0x7}, {0xc005745340, ...}, ...)
	/workspace/lib/events/filelog.go:171 +0x347
github.com/gravitational/teleport/lib/events.(*AuditLog).SearchEvents(0xc001760000, {0x5a584b0, 0xc008ce0360, 0x0}, {0x5a9ee88, 0x4668da0, 0x0}, {0xc00962f049, 0x7}, {0xc005745340, ...}, ...)
	/workspace/lib/events/auditlog.go:874 +0x5eb
github.com/gravitational/teleport/lib/auth.(*ServerWithRoles).SearchEvents(0xc006fc4310, {0x48e6244, 0x2, 0x0}, {0xc001287050, 0x29, 0x0}, {0xc00962f049, 0x7}, {0xc005745340, ...}, ...)
	/workspace/lib/auth/auth_with_roles.go:3760 +0x1b8
github.com/gravitational/teleport/lib/auth.(*GRPCServer).GetEvents(0x0, {0x5a584b0, 0xc008ce0390}, 0xc004bdab40)
	/workspace/lib/auth/grpcserver.go:2867 +0x26c
github.com/gravitational/teleport/api/client/proto._AuthService_GetEvents_Handler.func1({0x5a584b0, 0xc008ce0390}, {0x482b000, 0xc004bdab40})
	/workspace/api/client/proto/authservice.pb.go:16443 +0x8c
github.com/gravitational/teleport/lib/auth.(*Middleware).withAuthenticatedUserUnaryInterceptor(0x5a30e5, {0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40}, 0x9, 0xc009016ca8)
	/workspace/lib/auth/middleware.go:394 +0xc6
github.com/gravitational/teleport/lib/utils.ChainUnaryServerInterceptors.func1.1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40})
	/workspace/lib/utils/grpc.go:48 +0x8f
github.com/gravitational/teleport/lib/limiter.(*Limiter).UnaryServerInterceptorWithCustomRate.func1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40}, 0xc006a67aa0, 0xc006a67b40)
	/workspace/lib/limiter/limiter.go:149 +0x2da
github.com/gravitational/teleport/lib/utils.ChainUnaryServerInterceptors.func1.1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40})
	/workspace/lib/utils/grpc.go:48 +0x8f
github.com/gravitational/teleport/lib/utils.ErrorConvertUnaryInterceptor({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40}, 0x3, 0xc006a67ba0)
	/workspace/lib/utils/grpc.go:27 +0x5c
github.com/gravitational/teleport/lib/utils.ChainUnaryServerInterceptors.func1.1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40})
	/workspace/lib/utils/grpc.go:48 +0x8f
github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors.UnaryServerInterceptor.func1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40}, 0xc006a67aa0, 0xc006a67bc0)
	/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware/[email protected]/interceptors/server.go:22 +0x2e4
github.com/gravitational/teleport/lib/utils.ChainUnaryServerInterceptors.func1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40}, 0xc006a67aa0, 0xc009016ca8)
	/workspace/lib/utils/grpc.go:52 +0x21f
google.golang.org/grpc.chainUnaryInterceptors.func1.1({0x5a584b0, 0xc008ce00c0}, {0x482b000, 0xc004bdab40})
	/go/pkg/mod/google.golang.org/[email protected]/server.go:1116 +0x126
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc.UnaryServerInterceptor.func1({0x5a584b0, 0xc002508360}, {0x482b000, 0xc004bdab40}, 0xc006a67aa0, 0xc0041a43c0)
	/go/pkg/mod/go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/[email protected]/interceptor.go:325 +0x797
google.golang.org/grpc.chainUnaryInterceptors.func1.1({0x5a584b0, 0xc002508360}, {0x482b000, 0xc004bdab40})
	/go/pkg/mod/google.golang.org/[email protected]/server.go:1119 +0x227
google.golang.org/grpc.chainUnaryInterceptors.func1({0x5a584b0, 0xc002508360}, {0x482b000, 0xc004bdab40}, 0xc006a67aa0, 0xc009016ca8)
	/go/pkg/mod/google.golang.org/[email protected]/server.go:1121 +0x286
github.com/gravitational/teleport/api/client/proto._AuthService_GetEvents_Handler({0x48d81c0, 0xc00411fcb0}, {0x5a584b0, 0xc002508360}, 0xc008cca8a0, 0xc006e47820)
	/workspace/api/client/proto/authservice.pb.go:16445 +0x1db
google.golang.org/grpc.(*Server).processUnaryRPC(0xc001bc7880, {0x5a9fca0, 0xc0054d96c0}, 0xc002b51e60, 0xc003479cb0, 0x75c9588, 0x0)
	/go/pkg/mod/google.golang.org/[email protected]/server.go:1282 +0x1532
google.golang.org/grpc.(*Server).handleStream(0xc001bc7880, {0x5a9fca0, 0xc0054d96c0}, 0xc002b51e60, 0x0)
	/go/pkg/mod/google.golang.org/[email protected]/server.go:1619 +0xfd4
google.golang.org/grpc.(*Server).serveStreams.func1.2()
	/go/pkg/mod/google.golang.org/[email protected]/server.go:921 +0xfe
created by google.golang.org/grpc.(*Server).serveStreams.func1
	/go/pkg/mod/google.golang.org/[email protected]/server.go:919 +0x4d1
FAIL	github.com/gravitational/teleport/lib/web	130.579s

@xacrimon
Copy link
Contributor Author

xacrimon commented May 5, 2022

@Tener I'd love to have reliable tests where I can query against AWS or use an emulator to test this. The issue is that AWS is at best inconsistent here and I've observed multiple variations of populated fields in cases that aren't really distinct which worries me. Same goes for the AWS emulator, which populates them all regardless as one would expect.

@xacrimon
Copy link
Contributor Author

xacrimon commented May 5, 2022

I'll add some test checks for the dynamic.go changes though. I could potentially construct some nil-check tests for AWS but they seem of dubious value to me since I hardly know what behaviour to expect from AWS.

lib/events/dynamic_test.go Outdated Show resolved Hide resolved
xacrimon and others added 2 commits May 5, 2022 13:59
@zmb3
Copy link
Collaborator

zmb3 commented May 5, 2022

Please try running this against a real dynamo instance in both billing modes before merging.

@xacrimon
Copy link
Contributor Author

xacrimon commented May 5, 2022

Please try running this against a real dynamo instance in both billing modes before merging.

@zmb3 Yep, done. Did that previously as well for the first PR. Issue was that AWS API is inconsistent in what fields it populates and contains zero docs on when it decides to populate certain fields.

xacrimon and others added 2 commits May 5, 2022 15:35
lib/events/dynamic.go Outdated Show resolved Hide resolved
lib/events/dynamic_test.go Show resolved Hide resolved
@xacrimon xacrimon requested a review from zmb3 May 5, 2022 14:09
@zmb3 zmb3 enabled auto-merge (squash) May 5, 2022 14:14
@zmb3 zmb3 merged commit bb6c4b4 into master May 5, 2022
@github-actions
Copy link

github-actions bot commented May 5, 2022

@xacrimon See the table below for backport results.

Branch Result
branch/v9 Failed

xacrimon added a commit that referenced this pull request May 5, 2022
* Check for nil in the billing mode getter + fix panic if eventtype is not set

* use a better strategy to check if the interface is nil

* add godoc

* simplify switch

* update godoc

* simplify unknown field extraction code

* ensure underlying type is correct

* Update lib/events/dynamic_test.go

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

* fix lint

* Update AWS dynamo tests as a result of the dynamic.go changes, I don't think they've ever worked in some time as they appeared quite broke.

* simplify

* simplify 2

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>
xacrimon added a commit that referenced this pull request May 5, 2022
* Check for nil in the billing mode getter + fix panic if eventtype is not set

* use a better strategy to check if the interface is nil

* add godoc

* simplify switch

* update godoc

* simplify unknown field extraction code

* ensure underlying type is correct

* Update lib/events/dynamic_test.go

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>

* fix lint

* Update AWS dynamo tests as a result of the dynamic.go changes, I don't think they've ever worked in some time as they appeared quite broke.

* simplify

* simplify 2

Co-authored-by: Krzysztof Skrzętnicki <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-log Issues related to Teleports Audit Log backport-required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teleport 9.2.0 panics with DynamoDB
3 participants