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

proxy: make --cipher-suites take effect for the proxy #10674

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@rkday
Copy link
Contributor

commented Apr 23, 2019

This fixes #10659.

It'd be great if this could be back-applied to 3.3 as well as 3.4.

}
} else {
if lg != nil {
lg.Info("Using cipher suites:", zap.Strings("cipher-suites", cfg.ec.CipherSuites))

This comment has been minimized.

Copy link
@spzala

spzala Apr 23, 2019

Member

@rkday besides fixing a fmt error as in build, I would suggest to use lower case for all the log messages (using vs Using ..) per convention. Thanks!

@codecov-io

This comment has been minimized.

Copy link

commented Apr 24, 2019

Codecov Report

Merging #10674 into master will decrease coverage by 0.26%.
The diff coverage is 29.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10674      +/-   ##
==========================================
- Coverage   71.51%   71.25%   -0.27%     
==========================================
  Files         394      394              
  Lines       36658    36670      +12     
==========================================
- Hits        26217    26129      -88     
- Misses       8595     8687      +92     
- Partials     1846     1854       +8
Impacted Files Coverage Δ
embed/etcd.go 71% <0%> (ø) ⬆️
embed/config.go 56.37% <100%> (ø) ⬆️
etcdmain/etcd.go 29.83% <16.66%> (-0.39%) ⬇️
pkg/adt/interval_tree.go 75.07% <0%> (-10.52%) ⬇️
pkg/fileutil/read_dir.go 91.3% <0%> (-8.7%) ⬇️
pkg/fileutil/purge.go 65.9% <0%> (-6.82%) ⬇️
auth/store.go 68.14% <0%> (-6.53%) ⬇️
etcdctl/ctlv3/command/lease_command.go 65.34% <0%> (-5.95%) ⬇️
proxy/grpcproxy/watcher.go 89.79% <0%> (-4.09%) ⬇️
proxy/grpcproxy/watch.go 89.75% <0%> (-3.02%) ⬇️
... and 17 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 8146e1e...8296297. Read the comment docs.

@rkday rkday force-pushed the rkday:proxy_cipher_suites branch from 698ecad to 8296297 Apr 24, 2019

@rkday rkday closed this Apr 25, 2019

@rkday rkday reopened this Apr 25, 2019

@rkday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2019

OK, I've fixed the format error and the log text. There are a couple of test failures but I really can't see how they could be related - is there a chance they're spurious?

@jingyih

This comment has been minimized.

Copy link
Member

commented Apr 25, 2019

Looks like the failing test are unrelated, don't worry about it.

@spzala

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

@rkday I have mentioned it in #10682 which seems a dup PR but fyi to take a look. Thanks!

@spzala

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

OK, I've fixed the format error and the log text. There are a couple of test failures but I really can't see how they could be related - is there a chance they're spurious?

Thanks for addressing the comments @rkday

@rkday

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2019

Both this and #10682 look fairly similar - this has some extra logs, and we've made different (ultimately stylistic) choices about whether UpdateCipherSuites should be exported or not. I don't really mind which gets merged, but it would be good to have one of them in soon.

@spzala

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@rkday yup, thanks and sounds good. @hexfusion is looking at it as he commented in the other PR, we can go with his decision :-) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.