fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs #18186

gyuho · 2024-06-17T10:22:36Z

Otherwise, it will fail with

cannot listen on TLS for 127.0.0.1:8080: KeyFile and CertFile are not presented

We should instead explicitly fail fast, with a clear error message.

Also, adding some documentation how to configure TLS for metrics URLs.

c.f., #8060

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

k8s-ci-robot · 2024-06-17T10:22:46Z

Hi @gyuho. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov-commenter · 2024-06-17T10:40:48Z

Codecov Report

Attention: Patch coverage is 46.15385% with 7 lines in your changes missing coverage. Please review.

Project coverage is 68.86%. Comparing base (debc8fb) to head (0734121).

❗ Current head 0734121 differs from pull request most recent head 22f20a8

Please upload reports for the commit 22f20a8 to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Files	Coverage Δ
server/embed/etcd.go	`75.77% <46.15%> (+0.25%)`	⬆️

... and 21 files with indirect coverage changes

@@            Coverage Diff             @@
##             main   #18186      +/-   ##
==========================================
- Coverage   68.89%   68.86%   -0.04%     
==========================================
  Files         416      416              
  Lines       35151    35157       +6     
==========================================
- Hits        24218    24211       -7     
- Misses       9521     9541      +20     
+ Partials     1412     1405       -7

Continue to review full report in Codecov by Sentry.

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

server/embed/etcd.go

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho · 2024-06-17T14:01:25Z

/retest

k8s-ci-robot · 2024-06-17T14:01:40Z

@gyuho: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ivanvc · 2024-06-17T18:54:22Z

/ok-to-test

server/embed/etcd_test.go

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

gyuho · 2024-06-18T01:14:26Z

/retest

serathius · 2024-06-18T06:43:19Z

tests/e2e/etcd_config_test.go

+		_ = proc.Close()
+	}()
+
+	if err := e2e.WaitReadyExpectProc(context.TODO(), proc, []string{embed.ErrMissingClientTLSInfoForMetricsURL.Error()}); err != nil {


No need to fix it now as there are more config e2e tests with this problem, but it would be a nice followup.

In case that etcd doesn't produce the log, would be nice to test to exit after couple of seconds instead of waiting infinitely.

serathius · 2024-06-18T06:44:07Z

tests/e2e/etcd_config_test.go

+		if err := proc.Stop(); err != nil {
+			t.Error(err)
+		}
+		proc.Wait() // ensure the port has been released


Looked at other test, not all of them do this. Would be nice to have a consistent approach to shutting down proc.

Haha, yes. Some archaic code base here :)

Heh, we have been trying to clean it slowly up but we run out of steam.

#13637

serathius · 2024-06-18T06:44:26Z

cc @ahrtr

ahrtr · 2024-06-18T08:52:35Z

tests/e2e/etcd_config_test.go

+		proc.Wait() // ensure the port has been released
+		_ = proc.Close()


Minor comment: proc.Wait() seems to be unnecessary, proc.Close() already calls ep.wg.Wait().

etcd/pkg/expect/expect.go

Lines 356 to 362 in c70e0e4

func (ep *ExpectProcess) Wait() {

ep.wg.Wait()

}

// Close waits for the expect process to exit and return its error.

func (ep *ExpectProcess) Close() error {

ep.wg.Wait()

ahrtr

LGTM with a minor comment.

You might want to backport the change to 3.5.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

serathius · 2024-06-18T11:33:46Z

/retest

gyuho · 2024-06-18T11:57:43Z

/retest

gyuho · 2024-06-18T12:26:55Z

/retest

server/embed/etcd.go

jmhbnz · 2024-07-06T10:58:08Z

/cherrypick release-3.5

k8s-infra-cherrypick-robot · 2024-07-06T10:58:44Z

@jmhbnz: #18186 failed to apply on top of branch "release-3.5":

Applying: fix(server/embed): enforce non-empty client TLS if scheme is https/unixs
Applying: test(e2e): add a case where client tls is missing for https metrics url
Using index info to reconstruct a base tree...
M	server/embed/etcd.go
M	tests/e2e/etcd_config_test.go
Falling back to patching base and 3-way merge...
Auto-merging tests/e2e/etcd_config_test.go
CONFLICT (content): Merge conflict in tests/e2e/etcd_config_test.go
Auto-merging server/embed/etcd.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 test(e2e): add a case where client tls is missing for https metrics url
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-3.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

…s https/unixs Backports a657f06, 22f20a8, 497f1a4 and 3e86af6 from etcd-io#18186. Also backports required test util elements of 4c77726 from etcd-io#17661. Signed-off-by: James Blair <mail@jamesblair.net>

fix(server/embed): enforce non-empty client TLS if scheme is https/unixs

a657f06

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

k8s-ci-robot added the needs-ok-to-test label Jun 17, 2024

k8s-ci-robot added the size/M label Jun 17, 2024

serathius reviewed Jun 17, 2024

View reviewed changes

server/embed/etcd.go Show resolved Hide resolved

test(e2e): add a case where client tls is missing for https metrics url

22f20a8

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

k8s-ci-robot added area/testing size/L and removed size/M labels Jun 17, 2024

gyuho changed the title ~~fix(server/embed): enforce non-empty client TLS if scheme is https/unixs~~ fix(server/embed): enforce listen-metrics-urls non-empty client TLS for https/unixs Jun 17, 2024

gyuho changed the title ~~fix(server/embed): enforce listen-metrics-urls non-empty client TLS for https/unixs~~ fix(server): enforce listen-metrics-urls non-empty client TLS for https/unixs Jun 17, 2024

gyuho changed the title ~~fix(server): enforce listen-metrics-urls non-empty client TLS for https/unixs~~ fix(server): enforce listen-metrics-urls non-empty client TLS info when its scheme is https/unixs Jun 17, 2024

gyuho changed the title ~~fix(server): enforce listen-metrics-urls non-empty client TLS info when its scheme is https/unixs~~ fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs Jun 17, 2024

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jun 17, 2024

henrybear327 reviewed Jun 17, 2024

View reviewed changes

server/embed/etcd_test.go Show resolved Hide resolved

license

497f1a4

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

serathius approved these changes Jun 18, 2024

View reviewed changes

serathius reviewed Jun 18, 2024

View reviewed changes

ahrtr reviewed Jun 18, 2024

View reviewed changes

ahrtr approved these changes Jun 18, 2024

View reviewed changes

remove unnecessary wait call

3e86af6

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>

ahrtr approved these changes Jun 18, 2024

View reviewed changes

server/embed/etcd.go Show resolved Hide resolved

ahrtr merged commit 9c59b28 into etcd-io:main Jun 18, 2024
48 checks passed

ahrtr mentioned this pull request Jun 18, 2024

Update the error message when client certificate isn't provided for secure metrics url #18203

Merged

gyuho deleted the enforce-non-empty-client-tls-if-metrics-url-scheme-is-https branch June 20, 2024 05:33

ivanvc mentioned this pull request Jun 28, 2024

Plan to release etcd v3.5.15 #18247

Closed

4 tasks

jmhbnz added the backport/v3.5 label Jul 6, 2024

jmhbnz mentioned this pull request Jul 6, 2024

[3.5] Backport fix(server/embed) enforce non-empty client TLS if scheme is https/unixs #18289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs #18186

fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs #18186

gyuho commented Jun 17, 2024 •

edited

Loading

k8s-ci-robot commented Jun 17, 2024

codecov-commenter commented Jun 17, 2024 •

edited

Loading

gyuho commented Jun 17, 2024

k8s-ci-robot commented Jun 17, 2024

ivanvc commented Jun 17, 2024

gyuho commented Jun 18, 2024

serathius Jun 18, 2024

serathius Jun 18, 2024

gyuho Jun 18, 2024

serathius Jun 18, 2024

serathius commented Jun 18, 2024

ahrtr Jun 18, 2024

ahrtr left a comment

serathius commented Jun 18, 2024

gyuho commented Jun 18, 2024

gyuho commented Jun 18, 2024

jmhbnz commented Jul 6, 2024

k8s-infra-cherrypick-robot commented Jul 6, 2024

		proc.Wait() // ensure the port has been released
		_ = proc.Close()

	func (ep *ExpectProcess) Wait() {
	ep.wg.Wait()
	}

	// Close waits for the expect process to exit and return its error.
	func (ep *ExpectProcess) Close() error {
	ep.wg.Wait()

fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs #18186

fix(server): enforce listen-metrics-urls client TLS info when its scheme is https/unixs #18186

Conversation

gyuho commented Jun 17, 2024 • edited Loading

k8s-ci-robot commented Jun 17, 2024

codecov-commenter commented Jun 17, 2024 • edited Loading

Codecov Report

gyuho commented Jun 17, 2024

k8s-ci-robot commented Jun 17, 2024

ivanvc commented Jun 17, 2024

gyuho commented Jun 18, 2024

serathius Jun 18, 2024

Choose a reason for hiding this comment

serathius Jun 18, 2024

Choose a reason for hiding this comment

gyuho Jun 18, 2024

Choose a reason for hiding this comment

serathius Jun 18, 2024

Choose a reason for hiding this comment

serathius commented Jun 18, 2024

ahrtr Jun 18, 2024

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

serathius commented Jun 18, 2024

gyuho commented Jun 18, 2024

gyuho commented Jun 18, 2024

jmhbnz commented Jul 6, 2024

k8s-infra-cherrypick-robot commented Jul 6, 2024

gyuho commented Jun 17, 2024 •

edited

Loading

codecov-commenter commented Jun 17, 2024 •

edited

Loading