Fix the etcd api dependency in pkg. And Update Cobra Version to1.4.0 #13802

yankay · 2022-03-15T06:27:33Z

To solve the CVE-2020-26160, the cobra has been upgrade to 1.2.1 before.
And the api has been add to the pkg dependency.

But it's not necessary. ahrtr advise :

“etcd already contains lots of packages and dependency relationship in-between, we shouldn't introduce unnecessary dependencies without good reason.
So I suggest to bump spf13/cobra to 1.3.0 or 1.4.0, and rollback the change on go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY. I would suggest to make the change on the main branch firstly, and cherry-picked to 3.5 afterwards.
”

So the go.mod in pkg is needed to rollback , upgrade the corbra to 1.3.0 or 1.4.0 can solve the CVE too.

Refer to:

codecov-commenter · 2022-03-15T06:52:43Z

Codecov Report

Merging #13802 (2b7f5c2) into main (4504daa) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main   #13802      +/-   ##
==========================================
+ Coverage   72.49%   72.56%   +0.06%     
==========================================
  Files         467      467              
  Lines       38280    38332      +52     
==========================================
+ Hits        27752    27814      +62     
+ Misses       8739     8725      -14     
- Partials     1789     1793       +4

Flag	Coverage Δ
all	`72.56% <ø> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
client/v3/txn.go	`73.33% <0.00%> (-26.67%)`	⬇️
client/v3/concurrency/session.go	`88.63% <0.00%> (-4.55%)`	⬇️
client/v3/leasing/txn.go	`88.09% <0.00%> (-3.18%)`	⬇️
server/etcdserver/util.go	`85.71% <0.00%> (-3.18%)`	⬇️
client/v3/experimental/recipes/key.go	`75.34% <0.00%> (-2.74%)`	⬇️
pkg/traceutil/trace.go	`96.15% <0.00%> (-1.93%)`	⬇️
server/etcdserver/api/rafthttp/msgappv2_codec.go	`71.30% <0.00%> (-1.74%)`	⬇️
server/embed/serve.go	`62.10% <0.00%> (-0.92%)`	⬇️
server/etcdserver/server.go	`84.68% <0.00%> (-0.17%)`	⬇️
server/etcdserver/v3_server.go	`79.01% <0.00%> (+0.17%)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4504daa...2b7f5c2. Read the comment docs.

ahrtr

LGTM

Thank you!

The only concern is that the spf13/cobra 1.4.0 was just released 5 days ago. Usually it isn't a best practice to upgrade to a new version too soon.

ahrtr · 2022-03-15T21:54:13Z

cc @serathius @spzala @ptabor

serathius · 2022-03-16T13:31:02Z

I share @ahrtr concern to use fresh cobra release, this definitely could not be cherry-picked to v3.5. Is there any reason that we cannot just fix the api dependency without upgrading cobra further thus allow a safe cherry-pick?

ahrtr · 2022-03-16T16:30:03Z

Is there any reason that we cannot just fix the api dependency without upgrading cobra further thus allow a safe cherry-pick?

We have discussed this in pull/13797#discussion_r826516645 and pull/13797#discussion_r826596581. In short, spf13/cobra@1.2.1 depends on etcd back via a long long dependency chain, so we have to introduce the unnecessary dependency go.etcd.io/etcd/api/v3 => ../api in pkg's go.mod. In order to remove the unnecessary dependency, we can bump spf13/cobra to 1.3.0 or 1.4.0.

I had a quick review on the changes between spf13/cobra 1.3.0 and 1.4.0, basically there isn't much change, so it should be low risk to bump to 1.4.0. I am open on this. If others still have concern, we can bump to 1.3.0 for now.

yankay · 2022-03-18T03:26:27Z

HI @serathius .

The main change of cobra 1.4.0, is thinned the dependency tree. https://github.com/spf13/cobra/releases/tag/v1.4.0.
It's useful for ETCD, so the 1.4.0 version is choosed

But it's only released for 7 days . It we have concern. We can use 1.3.0 (released for 3 month) either, so that it's good to cherry pick to the ETCD 3.5.x :-)

How do you think the the change ?

serathius

I think it doesn't make sense to force using same cobra version for both main and release-3.5 branches. We definitely don't want to have dependencies between etcd packages, so upgrading cobra to 1.4.0 on main is ok.

However for release-3.5 branch we want to make minimal changes required to fix the CVE, while not introducing the dependency. Here 1.3.0 is definitely better than 1.4.0.

I think we can merge this PR as it is.

serathius · 2022-03-18T09:35:55Z

cc @ptabor

yankay · 2022-03-18T10:24:48Z

I think it doesn't make sense to force using same cobra version for both main and release-3.5 branches. We definitely don't want to have dependencies between etcd packages, so upgrading cobra to 1.4.0 on main is ok.

However for release-3.5 branch we want to make minimal changes required to fix the CVE, while not introducing the dependency. Here 1.3.0 is definitely better than 1.4.0.

I think we can merge this PR as it is.

Thanks for your advise very much.

yankay · 2022-03-18T10:58:57Z

When I try to fix the release 3.5.0 branch with update cobra to 1.3.0. There are still some denpendency cycle error.

# Change the pkg/go.mod file "	github.com/spf13/cobra v1.1.3 to github.com/spf13/cobra v1.3.0 "
root@kay200:~/oss/etcd/pkg# go mod tidy
go: github.com/spf13/cobra@v1.3.0 requires
        github.com/spf13/viper@v1.10.0 requires
        go.etcd.io/etcd/api/v3@v3.5.1 (replaced by ./FORBIDDEN_DEPENDENCY): reading FORBIDDEN_DEPENDENCY/go.mod: open /root/oss/etcd/pkg/FORBIDDEN_DEPENDENCY/go.mod: no such file or directory
go: github.com/spf13/cobra@v1.3.0 requires
        github.com/spf13/viper@v1.10.0 requires
        go.etcd.io/etcd/api/v3@v3.5.1 (replaced by ./FORBIDDEN_DEPENDENCY): reading FORBIDDEN_DEPENDENCY/go.mod: open /root/oss/etcd/pkg/FORBIDDEN_DEPENDENCY/go.mod: no such file or directory

So the release 3.5.0 branch cannot upgrade to the cobra 1.3.0.

yankay · 2022-03-23T03:26:23Z

HI, @ptabor.
Would you please review the PR ?

serathius · 2022-03-23T10:27:02Z

Please resolve merge conflicts.
cc @spzala @ahrtr for review

yankay · 2022-03-24T05:42:07Z

Please resolve merge conflicts. cc @spzala @ahrtr for review

HI @serathius , The confict has been resolved

ahrtr · 2022-03-24T06:36:01Z

pkg/go.mod

 replace (
 	go.etcd.io/etcd => ./FORBIDDEN_DEPENDENCY
+	go.etcd.io/etcd/api/v3 => ./FORBIDDEN_DEPENDENCY
+	go.etcd.io/etcd/client/pkg/v3 => ../client/pkg


Can you move the go.etcd.io/etcd/client/pkg/v3 => ../client/pkg into a separate replace block above, and keep the comment "Bad imports are sometimes cause attempts to ..." for this replace block?

ahrtr · 2022-03-24T07:56:46Z

Looks good to me.

serathius · 2022-03-24T10:02:13Z

Please fix conflicts.

yankay · 2022-03-25T06:16:06Z

HI @serathius , the confict has been fixed. :-)

ahrtr · 2022-03-25T06:24:33Z

Please squash the commits. It doesn't make sense to merge multiple intermediate dev commits into the main branch.

ahrtr

Please squash the commits

Signed-off-by: Kay Yan <kay.yan@daocloud.io>

yankay · 2022-03-28T01:53:24Z

HI @spzala , would you please review the PR?

serathius · 2022-03-28T08:40:20Z

Ok, let's merge it to fix dependencies.

yankay · 2022-03-28T11:11:58Z

Thank you @serathius @ahrtr very much.

yankay mentioned this pull request Mar 15, 2022

backport 3.5: #13571 Update Cobra version to 1.2.1 to fix CVE-2020-26160 #13797

Closed

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from c3e4b96 to 662d7c4 Compare March 15, 2022 07:00

ahrtr approved these changes Mar 15, 2022

View reviewed changes

serathius approved these changes Mar 18, 2022

View reviewed changes

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch 4 times, most recently from e8949ae to eff7fd4 Compare March 24, 2022 05:02

ahrtr reviewed Mar 24, 2022

View reviewed changes

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from eff7fd4 to 2b7f5c2 Compare March 24, 2022 07:25

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from 2b7f5c2 to d2c0b95 Compare March 24, 2022 08:08

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch 6 times, most recently from 87df81c to 06e4565 Compare March 25, 2022 04:37

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from 06e4565 to f7fe6a3 Compare March 25, 2022 05:29

ahrtr requested changes Mar 25, 2022

View reviewed changes

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch 3 times, most recently from e8dbb3a to d148bae Compare March 25, 2022 08:12

ahrtr approved these changes Mar 25, 2022

View reviewed changes

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from d148bae to f5b2a9b Compare March 25, 2022 08:37

fix the api dependency in pkg, and update cobra to 1.4.0

afecd31

Signed-off-by: Kay Yan <kay.yan@daocloud.io>

yankay force-pushed the fix-the-api-dependency-in-pkg-and-update-cobra-to-1.4.0 branch from f5b2a9b to afecd31 Compare March 25, 2022 09:19

serathius merged commit 27e222e into etcd-io:main Mar 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the etcd api dependency in pkg. And Update Cobra Version to1.4.0 #13802

Fix the etcd api dependency in pkg. And Update Cobra Version to1.4.0 #13802

yankay commented Mar 15, 2022 •

edited

codecov-commenter commented Mar 15, 2022 •

edited

ahrtr left a comment

ahrtr commented Mar 15, 2022

serathius commented Mar 16, 2022

ahrtr commented Mar 16, 2022

yankay commented Mar 18, 2022

serathius left a comment

serathius commented Mar 18, 2022

yankay commented Mar 18, 2022 •

edited

yankay commented Mar 18, 2022 •

edited

yankay commented Mar 23, 2022

serathius commented Mar 23, 2022

yankay commented Mar 24, 2022

ahrtr Mar 24, 2022

yankay Mar 24, 2022

ahrtr commented Mar 24, 2022

serathius commented Mar 24, 2022

yankay commented Mar 25, 2022

ahrtr commented Mar 25, 2022

ahrtr left a comment

yankay commented Mar 28, 2022

serathius commented Mar 28, 2022

yankay commented Mar 28, 2022

Fix the etcd api dependency in pkg. And Update Cobra Version to1.4.0 #13802

Fix the etcd api dependency in pkg. And Update Cobra Version to1.4.0 #13802

Conversation

yankay commented Mar 15, 2022 • edited

codecov-commenter commented Mar 15, 2022 • edited

Codecov Report

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr commented Mar 15, 2022

serathius commented Mar 16, 2022

ahrtr commented Mar 16, 2022

yankay commented Mar 18, 2022

serathius left a comment

Choose a reason for hiding this comment

serathius commented Mar 18, 2022

yankay commented Mar 18, 2022 • edited

yankay commented Mar 18, 2022 • edited

yankay commented Mar 23, 2022

serathius commented Mar 23, 2022

yankay commented Mar 24, 2022

ahrtr Mar 24, 2022

Choose a reason for hiding this comment

yankay Mar 24, 2022

Choose a reason for hiding this comment

ahrtr commented Mar 24, 2022

serathius commented Mar 24, 2022

yankay commented Mar 25, 2022

ahrtr commented Mar 25, 2022

ahrtr left a comment

Choose a reason for hiding this comment

yankay commented Mar 28, 2022

serathius commented Mar 28, 2022

yankay commented Mar 28, 2022

yankay commented Mar 15, 2022 •

edited

codecov-commenter commented Mar 15, 2022 •

edited

yankay commented Mar 18, 2022 •

edited

yankay commented Mar 18, 2022 •

edited