Restriction on MaxRequestSize and maxWALEntrySize

[ahrtr]

etcd build-in gRPC services

Users can configure the MaxRequestSize using flag --max-request-bytes, and its default value is 1.5MB. When users configure a value greater than 10MB, then etcd generates a warning message. The existing logic on --max-request-bytes looks good to me, and we shouldn't set any hard limit on it. We just need to keep it as it's for now.

But there is a hard limit (10MB) on the maxWALEntrySize. If users configure a value > 10MB for the --max-request-bytes, then etcd can receive and process request > 10MB. But when etcd restarts later, it will fail to decode the WAL entry, see decoder.go#L88. This is definitely a bug, see also issues/14025. We can't just remove the max size limitation on the WAL entry, because etcd may run out of memory in case the WAL entry is somehow corrupted, and its size is a huge value. The proposed solution for this issue is to replace the hard code limitation 10MB with a dynamic value, which is the remaining size of the current WAL file.

Let's work with an example. Assuming the reading offset is 800 bytes, and the current WAL size is 64MB, then the max size of the current WAL entry should be (64MB - 800).

Currently the maxSendBytes is hard coded as math.MaxInt32. We may add a configuration item MaxSendMsgSize into embed.Config, and defaults to math.MaxInt32.

Users registered gRPC services

One related PR is 14066. @ptabor proposed to add an additional option, see below,

grpcAdditionalServerOptions grpc.ServerOption[]

Just as I mentioned in issuecomment-1155881593 and issuecomment-1155894742, the good side of the proposal is more flexibility; but the down side is more chance to make mistake. When users configure a different value for the max RecvMsgSize or MaxSendMsgSize via the grpcAdditionalServerOptions, then it may overwrite (or be overwritten by) the values configured by --max-request-bytes.

So I suggest not to add the grpcAdditionalServerOptions for now, until we receive valid use cases. If users want to change the max send/receive values, just use the configuration items for the etcd build-in gRPC services.

cc @ptabor @serathius @spzala @biosvs @algobardo @xiang90 @rleungx

[algobardo]

Thanks @ahrtr for putting this together, I think you show a deep understanding of the problem, and you indeed recognise that it is a bug that needs to be resolved.

About the proposed solution, I think it is fine, I just want to make sure that when you say replace the hard code limitation 10MB with a dynamic value, which is the remaining size of the current WAL file, you do not intend to just do SegmentSizeBytes - offset. The wal size is not fixed at 64Mb, but it might be larger than that, hence you should be actually using the actual wal file size. Are we aligned on this interpretation of your plan?

[ahrtr]

Yes, the dynamic value is the actual WAL file size - offset.

[biosvs]

Thank you for continuing with this task.

Proposal looks good to me. It keeps flexibility and keeps some security limitations and the same time.

As for ServerOption - maybe your concern could be solved through the hard config compatibility checks. In general it sounds safe when server cannot start because of conflicting config parameters. Moreover we're talking about new features, so no backward compatibility will be broken.

[tjungblu]

@ahrtr we have a customer upgrading from etcd 3.4 to 3.5, now dealing with this message:

{"level":"fatal","ts":"2023-01-06T12:12:58.709Z","caller":"etcdmain/etcd.go:204","msg":"discovery failed","error":"wal: max entry size limit exceeded, recBytes: 13279, fileSize(313430016) - offset(313418480) - padBytes(1) = entryLimit(11535)","stacktrace":"go.etcd.io/etcd/server/v3/etcdmain.startEtcdOrProxyV2\n\t/remote-source/cachito-gomod-with-deps/app/server/etcdmain/etcd.go:204\ngo.etcd.io/etcd/server/v3/etcdmain.Main\n\t/remote-source/cachito-gomod-with-deps/app/server/etcdmain/main.go:40\nmain.main\n\t/remote-source/cachito-gomod-with-deps/app/server/main.go:32\nruntime.main\n\t/usr/lib/golang/src/runtime/proc.go:225"}

I assume this is due to the backport in #14127, is there an easy mitigation the customer can take to get out of this issue?
I reckon we can take a snapshot with 3.4, upgrade locally with higher limits and then restore the live etcd instance from the snapshot?

[ahrtr]

It looks like the last record in the WAL file is corrupted.

Please workaround the issue using #15066 for now. I will deliver a formal fix later. Sorry for the inconvenience and thank you for raising the issue.

[ahrtr]

A workaround for now is to manually cut the corrupted record.

[ahrtr]

I reckon we can take a snapshot with 3.4, upgrade locally with higher limits and then restore the live etcd instance from the snapshot?

This should can also work.

[tjungblu]

Thanks. I can also share the WAL with you if need to dig any deeper.

[ahrtr]

Thanks. I can also share the WAL with you if need to dig any deeper.

Sure. Please, so that I can double confirm the PR indeed fixes the issue.

[tjungblu]

can I ping you on the Kubernetes slack under @ Benjamin Wang?

[ahrtr]

Confirmed the issue.

func TestLastRecordLengthExceedFileEnd(t *testing.T) {
	w, err := Open(zaptest.NewLogger(t), "/Users/wachao/Downloads/wal/var/lib/etcd/member/wal", walpb.Snapshot{
		Index: 134986736,
		Term:  0,
	})
	require.NoError(t, err)
	defer w.Close()

	_, _, _, err = w.ReadAll()
	t.Logf("#### w.ReadAll, error: %v\n", err)
	require.ErrorIs(t, err, io.ErrUnexpectedEOF)
}

$ go test -run TestLastRecordLengthExceedFileEnd -v
=== RUN   TestLastRecordLengthExceedFileEnd
    wal_test.go:1100: #### w.ReadAll, error: unexpected EOF: [wal] max entry size limit exceeded when reading "000000000000372b-00000000080bbbf0.wal", recBytes: 13279, fileSize(313430016) - offset(313418480) - padBytes(1) = entryLimit(11535)
--- PASS: TestLastRecordLengthExceedFileEnd (0.38s)
PASS
ok  	go.etcd.io/etcd/server/v3/storage/wal	1.846s