From b9c4635d6fc1b20b51ad856e3a3a24d81ee1a1b9 Mon Sep 17 00:00:00 2001 From: wafuwafu13 Date: Tue, 20 Dec 2022 22:25:46 +0900 Subject: [PATCH] refactor(client): remove duplicate processing Signed-off-by: wafuwafu13 refactor(clientv3): add comment Signed-off-by: wafuwafu13 refactor: use assert Signed-off-by: wafuwafu13 test: add TestIsUnavailableErr Signed-off-by: wafuwafu13 fix(client): change error message Signed-off-by: wafuwafu13 fix: change error message Signed-off-by: wafuwafu13 clientv3: add protection code to prevent SnapshotWithVersion from panicking Signed-off-by: Benjamin Wang changelog: update the release date of etcd v3.4.23 Signed-off-by: Benjamin Wang remove the dependency on busybox Signed-off-by: Benjamin Wang update changelog to update base image to static-debian11 and removd dependency on busybox Signed-off-by: Benjamin Wang update nsswitch.conf file Signed-off-by: Benjamin Wang tests: Reproduce issue 14685 Signed-off-by: Marek Siarkowicz clientv3/naming/endpoints: fix endpoints prefix bug fixes bug with multiple endpoints with same prefix Signed-off-by: Ramil Mirhasanov build(deps): bump ossf/scorecard-action from 2.1.0 to 2.1.2 Bumps [ossf/scorecard-action](https://github.com/ossf/scorecard-action) from 2.1.0 to 2.1.2. - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](https://github.com/ossf/scorecard-action/compare/937ffa90d79c7d720498178154ad4c7ba1e4ad8c...e38b1902ae4f44df626f11ba0734b14fb91f8f86) --- updated-dependencies: - dependency-name: ossf/scorecard-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] Added 'secrets.GITHUB_TOKEN' for the static-analysis workflow Refer to: https://github.com/arduino/setup-protoc/issues/63 Signed-off-by: Benjamin Wang --- .github/workflows/scorecards.yml | 2 +- .github/workflows/static-analysis.yaml | 1 + CHANGELOG/CHANGELOG-3.4.md | 8 +- CHANGELOG/CHANGELOG-3.5.md | 1 + Dockerfile-release.amd64 | 14 ++-- Dockerfile-release.arm64 | 12 +-- Dockerfile-release.ppc64le | 11 +-- Dockerfile-release.s390x | 12 +-- client/v3/client_test.go | 73 +++++++++++++++---- client/v3/maintenance.go | 6 +- client/v3/naming/endpoints/endpoints_impl.go | 9 ++- client/v3/retry_interceptor.go | 15 ++-- client/v3/snapshot/v3_snapshot.go | 15 +--- nsswitch.conf | 22 ++++++ scripts/build-docker.sh | 2 + .../clientv3/naming/resolver_test.go | 39 ++++++++++ tests/linearizability/linearizability_test.go | 8 ++ 17 files changed, 173 insertions(+), 77 deletions(-) create mode 100644 nsswitch.conf diff --git a/.github/workflows/scorecards.yml b/.github/workflows/scorecards.yml index b554d5f39775..0394941ebe58 100644 --- a/.github/workflows/scorecards.yml +++ b/.github/workflows/scorecards.yml @@ -27,7 +27,7 @@ jobs: persist-credentials: false - name: "Run analysis" - uses: ossf/scorecard-action@937ffa90d79c7d720498178154ad4c7ba1e4ad8c # tag=v2.1.0 + uses: ossf/scorecard-action@e38b1902ae4f44df626f11ba0734b14fb91f8f86 # tag=v2.1.2 with: results_file: results.sarif results_format: sarif diff --git a/.github/workflows/static-analysis.yaml b/.github/workflows/static-analysis.yaml index 0d9c564ef321..1d35a96d0fb3 100644 --- a/.github/workflows/static-analysis.yaml +++ b/.github/workflows/static-analysis.yaml @@ -17,5 +17,6 @@ jobs: uses: arduino/setup-protoc@64c0c85d18e984422218383b81c52f8b077404d3 # v1.1.2 with: version: '3.14.0' + repo-token: ${{ secrets.GITHUB_TOKEN }} - run: make verify - run: make fix diff --git a/CHANGELOG/CHANGELOG-3.4.md b/CHANGELOG/CHANGELOG-3.4.md index 95dfe40023cd..bafcea1ddbc4 100644 --- a/CHANGELOG/CHANGELOG-3.4.md +++ b/CHANGELOG/CHANGELOG-3.4.md @@ -3,8 +3,14 @@ Previous change logs can be found at [CHANGELOG-3.3](https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.3.md).
+## v3.4.24 (TBD) -## v3.4.23 (TBD) +### Other +- Updated [base image from base-debian11 to static-debian11 and removed dependency on busybox](https://github.com/etcd-io/etcd/pull/15038). + +
+ +## v3.4.23 (2022-12-21) ### Package `clientv3` - Fix [Refreshing token on CommonName based authentication causes segmentation violation in client](https://github.com/etcd-io/etcd/pull/14792). diff --git a/CHANGELOG/CHANGELOG-3.5.md b/CHANGELOG/CHANGELOG-3.5.md index a47ae6eb0e66..eadbcec50f71 100644 --- a/CHANGELOG/CHANGELOG-3.5.md +++ b/CHANGELOG/CHANGELOG-3.5.md @@ -16,6 +16,7 @@ Previous change logs can be found at [CHANGELOG-3.4](https://github.com/etcd-io/ ### Security - Use [distroless base image](https://github.com/etcd-io/etcd/pull/15016) to address critical Vulnerabilities. +- Updated [base image from base-debian11 to static-debian11 and removed dependency on busybox](https://github.com/etcd-io/etcd/pull/15037). - Bumped [some dependencies](https://github.com/etcd-io/etcd/pull/15018) to address some HIGH Vulnerabilities. ### Go diff --git a/Dockerfile-release.amd64 b/Dockerfile-release.amd64 index 67400b69686e..09ed48076bb9 100644 --- a/Dockerfile-release.amd64 +++ b/Dockerfile-release.amd64 @@ -1,20 +1,16 @@ -FROM --platform=linux/amd64 busybox:1.34.1 as source -FROM --platform=linux/amd64 gcr.io/distroless/base-debian11 - -COPY --from=source /bin/sh /bin/sh -COPY --from=source /bin/mkdir /bin/mkdir +FROM --platform=linux/amd64 gcr.io/distroless/static-debian11 ADD etcd /usr/local/bin/ ADD etcdctl /usr/local/bin/ ADD etcdutl /usr/local/bin/ -RUN mkdir -p /var/etcd/ -RUN mkdir -p /var/lib/etcd/ + +WORKDIR /var/etcd/ +WORKDIR /var/lib/etcd/ # Alpine Linux doesn't use pam, which means that there is no /etc/nsswitch.conf, # but Golang relies on /etc/nsswitch.conf to check the order of DNS resolving # (see https://github.com/golang/go/commit/9dee7771f561cf6aee081c0af6658cc81fac3918) -# To fix this we just create /etc/nsswitch.conf and add the following line: -RUN echo 'hosts: files mdns4_minimal [NOTFOUND=return] dns mdns4' >> /etc/nsswitch.conf +ADD nsswitch.conf /etc/nsswitch.conf EXPOSE 2379 2380 diff --git a/Dockerfile-release.arm64 b/Dockerfile-release.arm64 index b8ce477afd49..c93763f661bd 100644 --- a/Dockerfile-release.arm64 +++ b/Dockerfile-release.arm64 @@ -1,15 +1,11 @@ -FROM --platform=linux/arm64 busybox:1.34.1 as source -FROM --platform=linux/arm64 gcr.io/distroless/base-debian11 - -COPY --from=source /bin/sh /bin/sh -COPY --from=source /bin/mkdir /bin/mkdir +FROM --platform=linux/arm64 gcr.io/distroless/static-debian11 ADD etcd /usr/local/bin/ ADD etcdctl /usr/local/bin/ ADD etcdutl /usr/local/bin/ -ADD var/etcd /var/etcd -ADD var/lib/etcd /var/lib/etcd -ENV ETCD_UNSUPPORTED_ARCH=arm64 + +WORKDIR /var/etcd/ +WORKDIR /var/lib/etcd/ EXPOSE 2379 2380 diff --git a/Dockerfile-release.ppc64le b/Dockerfile-release.ppc64le index 9cfe5d433314..268e397410c3 100644 --- a/Dockerfile-release.ppc64le +++ b/Dockerfile-release.ppc64le @@ -1,14 +1,11 @@ -FROM --platform=linux/ppc64le busybox:1.34.1 as source -FROM --platform=linux/ppc64le gcr.io/distroless/base-debian11 - -COPY --from=source /bin/sh /bin/sh -COPY --from=source /bin/mkdir /bin/mkdir +FROM --platform=linux/ppc64le gcr.io/distroless/static-debian11 ADD etcd /usr/local/bin/ ADD etcdctl /usr/local/bin/ ADD etcdutl /usr/local/bin/ -ADD var/etcd /var/etcd -ADD var/lib/etcd /var/lib/etcd + +WORKDIR /var/etcd/ +WORKDIR /var/lib/etcd/ EXPOSE 2379 2380 diff --git a/Dockerfile-release.s390x b/Dockerfile-release.s390x index d901b410c98e..4a280551deb3 100644 --- a/Dockerfile-release.s390x +++ b/Dockerfile-release.s390x @@ -1,15 +1,11 @@ -FROM --platform=linux/s390x busybox:1.34.1 as source -FROM --platform=linux/s390x gcr.io/distroless/base-debian11 - -COPY --from=source /bin/sh /bin/sh -COPY --from=source /bin/mkdir /bin/mkdir - +FROM --platform=linux/s390x gcr.io/distroless/static-debian11 ADD etcd /usr/local/bin/ ADD etcdctl /usr/local/bin/ ADD etcdutl /usr/local/bin/ -ADD var/etcd /var/etcd -ADD var/lib/etcd /var/lib/etcd + +WORKDIR /var/etcd/ +WORKDIR /var/lib/etcd/ EXPOSE 2379 2380 diff --git a/client/v3/client_test.go b/client/v3/client_test.go index 8534536e3d4f..d8a4b07fe3df 100644 --- a/client/v3/client_test.go +++ b/client/v3/client_test.go @@ -17,12 +17,15 @@ package clientv3 import ( "context" "errors" + "fmt" "io" "net" "sync" "testing" "time" + "github.com/stretchr/testify/assert" + "go.etcd.io/etcd/api/v3/etcdserverpb" "go.etcd.io/etcd/api/v3/v3rpc/rpctypes" "go.etcd.io/etcd/client/pkg/v3/testutil" @@ -152,23 +155,63 @@ func TestDialNoTimeout(t *testing.T) { } func TestIsHaltErr(t *testing.T) { - if !isHaltErr(context.TODO(), errors.New("etcdserver: some etcdserver error")) { - t.Errorf(`error prefixed with "etcdserver: " should be Halted by default`) - } - if isHaltErr(context.TODO(), rpctypes.ErrGRPCStopped) { - t.Errorf("error %v should not halt", rpctypes.ErrGRPCStopped) - } - if isHaltErr(context.TODO(), rpctypes.ErrGRPCNoLeader) { - t.Errorf("error %v should not halt", rpctypes.ErrGRPCNoLeader) - } + assert.Equal(t, + isHaltErr(context.TODO(), errors.New("etcdserver: some etcdserver error")), + true, + "error created by errors.New should be unavailable error", + ) + assert.Equal(t, + isHaltErr(context.TODO(), rpctypes.ErrGRPCStopped), + false, + fmt.Sprintf(`error "%v" should not be halt error`, rpctypes.ErrGRPCStopped), + ) + assert.Equal(t, + isHaltErr(context.TODO(), rpctypes.ErrGRPCNoLeader), + false, + fmt.Sprintf(`error "%v" should not be halt error`, rpctypes.ErrGRPCNoLeader), + ) ctx, cancel := context.WithCancel(context.TODO()) - if isHaltErr(ctx, nil) { - t.Errorf("no error and active context should not be Halted") - } + assert.Equal(t, + isHaltErr(ctx, nil), + false, + "no error and active context should be halt error", + ) cancel() - if !isHaltErr(ctx, nil) { - t.Errorf("cancel on context should be Halted") - } + assert.Equal(t, + isHaltErr(ctx, nil), + true, + "cancel on context should be halte error", + ) +} + +func TestIsUnavailableErr(t *testing.T) { + assert.Equal(t, + isUnavailableErr(context.TODO(), errors.New("etcdserver: some etcdserver error")), + false, + "error created by errors.New should not be unavailable error", + ) + assert.Equal(t, + isUnavailableErr(context.TODO(), rpctypes.ErrGRPCStopped), + true, + fmt.Sprintf(`error "%v" should be unavailable error`, rpctypes.ErrGRPCStopped), + ) + assert.Equal(t, + isUnavailableErr(context.TODO(), rpctypes.ErrGRPCNotCapable), + false, + fmt.Sprintf("error %v should not be unavailable error", rpctypes.ErrGRPCNotCapable), + ) + ctx, cancel := context.WithCancel(context.TODO()) + assert.Equal(t, + isUnavailableErr(ctx, nil), + false, + "no error and active context should not be unavailable error", + ) + cancel() + assert.Equal(t, + isUnavailableErr(ctx, nil), + false, + "cancel on context should not be unavailable error", + ) } func TestCloseCtxClient(t *testing.T) { diff --git a/client/v3/maintenance.go b/client/v3/maintenance.go index 38810630500e..f47808d5f925 100644 --- a/client/v3/maintenance.go +++ b/client/v3/maintenance.go @@ -239,6 +239,7 @@ func (m *maintenance) SnapshotWithVersion(ctx context.Context) (*SnapshotRespons resp, err := ss.Recv() if err != nil { m.logAndCloseWithError(err, pw) + return nil, err } go func() { // Saving response is blocking @@ -260,10 +261,11 @@ func (m *maintenance) SnapshotWithVersion(ctx context.Context) (*SnapshotRespons } } }() + return &SnapshotResponse{ - Header: resp.Header, + Header: resp.GetHeader(), Snapshot: &snapshotReadCloser{ctx: ctx, ReadCloser: pr}, - Version: resp.Version, + Version: resp.GetVersion(), }, err } diff --git a/client/v3/naming/endpoints/endpoints_impl.go b/client/v3/naming/endpoints/endpoints_impl.go index 7796f7c9cbb7..f88a3eed13f4 100644 --- a/client/v3/naming/endpoints/endpoints_impl.go +++ b/client/v3/naming/endpoints/endpoints_impl.go @@ -92,7 +92,8 @@ func (m *endpointManager) DeleteEndpoint(ctx context.Context, key string, opts . } func (m *endpointManager) NewWatchChannel(ctx context.Context) (WatchChannel, error) { - resp, err := m.client.Get(ctx, m.target, clientv3.WithPrefix(), clientv3.WithSerializable()) + key := m.target + "/" + resp, err := m.client.Get(ctx, key, clientv3.WithPrefix(), clientv3.WithSerializable()) if err != nil { return nil, err } @@ -126,7 +127,8 @@ func (m *endpointManager) watch(ctx context.Context, rev int64, upch chan []*Upd lg := m.client.GetLogger() opts := []clientv3.OpOption{clientv3.WithRev(rev), clientv3.WithPrefix()} - wch := m.client.Watch(ctx, m.target, opts...) + key := m.target + "/" + wch := m.client.Watch(ctx, key, opts...) for { select { case <-ctx.Done(): @@ -171,7 +173,8 @@ func (m *endpointManager) watch(ctx context.Context, rev int64, upch chan []*Upd } func (m *endpointManager) List(ctx context.Context) (Key2EndpointMap, error) { - resp, err := m.client.Get(ctx, m.target, clientv3.WithPrefix(), clientv3.WithSerializable()) + key := m.target + "/" + resp, err := m.client.Get(ctx, key, clientv3.WithPrefix(), clientv3.WithSerializable()) if err != nil { return nil, err } diff --git a/client/v3/retry_interceptor.go b/client/v3/retry_interceptor.go index e692b1c30de1..e4a46ddd881c 100644 --- a/client/v3/retry_interceptor.go +++ b/client/v3/retry_interceptor.go @@ -108,15 +108,12 @@ func (c *Client) streamClientInterceptor(optFuncs ...retryOption) grpc.StreamCli intOpts := reuseOrNewWithCallOptions(defaultOptions, optFuncs) return func(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) { ctx = withVersion(ctx) - // getToken automatically - // TODO(cfc4n): keep this code block, remove codes about getToken in client.go after pr #12165 merged. - if c.authTokenBundle != nil { - // equal to c.Username != "" && c.Password != "" - err := c.getToken(ctx) - if err != nil && rpctypes.Error(err) != rpctypes.ErrAuthNotEnabled { - c.GetLogger().Error("clientv3/retry_interceptor: getToken failed", zap.Error(err)) - return nil, err - } + // getToken automatically. Otherwise, auth token may be invalid after watch reconnection because the token has expired + // (see https://github.com/etcd-io/etcd/issues/11954 for more). + err := c.getToken(ctx) + if err != nil { + c.GetLogger().Error("clientv3/retry_interceptor: getToken failed", zap.Error(err)) + return nil, err } grpcOpts, retryOpts := filterCallOptions(opts) callOpts := reuseOrNewWithCallOptions(intOpts, retryOpts) diff --git a/client/v3/snapshot/v3_snapshot.go b/client/v3/snapshot/v3_snapshot.go index cc27f643af36..10b51885e4c7 100644 --- a/client/v3/snapshot/v3_snapshot.go +++ b/client/v3/snapshot/v3_snapshot.go @@ -68,7 +68,7 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d start := time.Now() resp, err := cli.SnapshotWithVersion(ctx) if err != nil { - return resp.Version, err + return "", err } defer resp.Snapshot.Close() lg.Info("fetching snapshot", zap.String("endpoint", cfg.Endpoints[0])) @@ -99,16 +99,3 @@ func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, d lg.Info("saved", zap.String("path", dbPath)) return resp.Version, nil } - -// Save fetches snapshot from remote etcd server and saves data -// to target path. If the context "ctx" is canceled or timed out, -// snapshot save stream will error out (e.g. context.Canceled, -// context.DeadlineExceeded). Make sure to specify only one endpoint -// in client configuration. Snapshot API must be requested to a -// selected node, and saved snapshot is the point-in-time state of -// the selected node. -// Deprecated: Use SaveWithVersion instead. -func Save(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, dbPath string) error { - _, err := SaveWithVersion(ctx, lg, cfg, dbPath) - return err -} diff --git a/nsswitch.conf b/nsswitch.conf new file mode 100644 index 000000000000..3c827f100c6f --- /dev/null +++ b/nsswitch.conf @@ -0,0 +1,22 @@ +# /etc/nsswitch.conf +# +# Example configuration of GNU Name Service Switch functionality. +# If you have the `glibc-doc-reference' and `info' packages installed, try: +# `info libc "Name Service Switch"' for information about this file. + +passwd: compat +group: compat +shadow: compat +gshadow: files + +hosts: files dns +networks: files + +protocols: db files +services: db files +ethers: db files +rpc: db files + +netgroup: nis +hosts: files mdns4_minimal [NOTFOUND=return] dns mdns4 + diff --git a/scripts/build-docker.sh b/scripts/build-docker.sh index a255dc2cb159..698a70d62385 100755 --- a/scripts/build-docker.sh +++ b/scripts/build-docker.sh @@ -32,6 +32,8 @@ mkdir -p "${IMAGEDIR}"/var/etcd mkdir -p "${IMAGEDIR}"/var/lib/etcd cp "${BINARYDIR}"/etcd "${BINARYDIR}"/etcdctl "${BINARYDIR}"/etcdutl "${IMAGEDIR}" +cp ./nsswitch.conf "${IMAGEDIR}" + cat ./"${DOCKERFILE}" > "${IMAGEDIR}"/Dockerfile if [ -z "$TAG" ]; then diff --git a/tests/integration/clientv3/naming/resolver_test.go b/tests/integration/clientv3/naming/resolver_test.go index 1f9d2a5fcb67..14c3ad72374d 100644 --- a/tests/integration/clientv3/naming/resolver_test.go +++ b/tests/integration/clientv3/naming/resolver_test.go @@ -20,6 +20,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "go.etcd.io/etcd/client/v3/naming/endpoints" "go.etcd.io/etcd/client/v3/naming/resolver" "go.etcd.io/etcd/pkg/v3/grpc_testing" @@ -111,3 +112,41 @@ func TestEtcdGrpcResolver(t *testing.T) { break } } + +func TestEtcdEndpointManager(t *testing.T) { + integration2.BeforeTest(t) + + s1PayloadBody := []byte{'1'} + s1 := grpc_testing.NewDummyStubServer(s1PayloadBody) + err := s1.Start(nil) + assert.NoError(t, err) + defer s1.Stop() + + s2PayloadBody := []byte{'2'} + s2 := grpc_testing.NewDummyStubServer(s2PayloadBody) + err = s2.Start(nil) + assert.NoError(t, err) + defer s2.Stop() + + clus := integration2.NewCluster(t, &integration2.ClusterConfig{Size: 3}) + defer clus.Terminate(t) + + // Check if any endpoint with the same prefix "foo" will not break the logic with multiple endpoints + em, err := endpoints.NewManager(clus.Client(0), "foo") + assert.NoError(t, err) + emOther, err := endpoints.NewManager(clus.Client(1), "foo_other") + assert.NoError(t, err) + + e1 := endpoints.Endpoint{Addr: s1.Addr()} + e2 := endpoints.Endpoint{Addr: s2.Addr()} + + em.AddEndpoint(context.Background(), "foo/e1", e1) + emOther.AddEndpoint(context.Background(), "foo_other/e2", e2) + + epts, err := em.List(context.Background()) + assert.NoError(t, err) + eptsOther, err := emOther.List(context.Background()) + assert.NoError(t, err) + assert.Equal(t, len(epts), 1) + assert.Equal(t, len(eptsOther), 1) +} diff --git a/tests/linearizability/linearizability_test.go b/tests/linearizability/linearizability_test.go index 78780e700c8f..3295c9650c3e 100644 --- a/tests/linearizability/linearizability_test.go +++ b/tests/linearizability/linearizability_test.go @@ -70,6 +70,14 @@ func TestLinearizability(t *testing.T) { e2e.WithGoFailEnabled(true), ), }, + { + name: "Issue14685", + failpoint: DefragBeforeCopyPanic, + config: *e2e.NewConfig( + e2e.WithClusterSize(1), + e2e.WithGoFailEnabled(true), + ), + }, } for _, tc := range tcs { t.Run(tc.name, func(t *testing.T) {