fix: bad connection handling for in cluster dialer #1800

matejvasek · 2023-06-08T17:08:06Z

Changes

🐛 Fix bad connection handling for in cluster dialer which caused confusing error messages (although functionality was not affected). Connections were closed from wrong end of io.Pipe
which resulted in confusing error logs.
🎁 Better error handling:
- ContextDial() returns some errors immediately instead of deferring them on Read/Write operation on returned connection.
- ContextDial() tries to parse socat's stderr and translate it to Go's net.OpError instead of just creating error with whole stderr embedded in it.

/kind bug

resolves: #1792

fix: connection handling for `in cluster dialer` causing confusing error messages #1792

codecov · 2023-06-08T17:18:04Z

Codecov Report

Patch coverage: 89.38% and project coverage change: +1.31 🎉

Comparison is base (18fe643) 62.50% compared to head (7a18b2f) 63.81%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   62.50%   63.81%   +1.31%     
==========================================
  Files         106      106              
  Lines       13417    13508      +91     
==========================================
+ Hits         8386     8620     +234     
+ Misses       4244     4077     -167     
- Partials      787      811      +24

Flag	Coverage Δ
e2e-test	`37.44% <0.00%> (-0.27%)`	⬇️
e2e-test-oncluster	`33.07% <0.00%> (-0.31%)`	⬇️
e2e-test-oncluster-runtime	`28.39% <0.00%> (?)`
e2e-test-runtime-go	`27.11% <0.00%> (?)`
e2e-test-runtime-node	`28.20% <0.00%> (?)`
e2e-test-runtime-python	`28.20% <0.00%> (?)`
e2e-test-runtime-quarkus	`28.32% <0.00%> (?)`
e2e-test-runtime-springboot	`27.29% <0.00%> (?)`
e2e-test-runtime-typescript	`28.32% <0.00%> (?)`
integration-tests	`52.34% <89.38%> (+3.91%)`	⬆️
unit-tests-macos-latest	`49.56% <0.00%> (?)`
unit-tests-ubuntu-latest	`50.39% <0.00%> (-0.29%)`	⬇️
unit-tests-windows-latest	`49.61% <0.00%> (-0.34%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/k8s/dialer.go	`78.36% <89.38%> (+4.53%)`	⬆️

... and 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Connections were closed from wrong end of io.Pipe which resulted in confusing error logs. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Now DialContext() returns some errors immediately instead of deferring it Read/Write operation on the returned connection. Signed-off-by: Matej Vasek <mvasek@redhat.com>

Now ContextDial() tries to parse socat's stderr and translate it to Go's net.OpError instead of just creating error with whole stderr embedded in it. Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek · 2023-06-08T23:04:25Z

I recommend review each commit separately.

matejvasek · 2023-06-09T16:57:07Z

@jrangelramos @lance PTAL

pkg/k8s/dialer.go

pkg/k8s/dialer_test.go

Co-authored-by: Lance Ball <lball@redhat.com>

matejvasek · 2023-06-10T03:48:38Z

PTAL @zroubalik

matejvasek · 2023-06-12T13:36:32Z

@zroubalik @lance we need to override the linter error here.

matejvasek · 2023-06-12T18:22:57Z

@lkingland PTAL

lance · 2023-06-13T17:56:00Z

/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

knative-prow · 2023-06-13T17:56:04Z

@lance: Overrode contexts on behalf of lance: style / suggester / github_actions, style / suggester / shell, style / suggester / yaml

In response to this:

/override "style / suggester / shell"
/override "style / suggester / yaml"
/override "style / suggester / github_actions"

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/test-infra repository.

matejvasek · 2023-06-14T07:22:38Z

/ok-to-test

matejvasek · 2023-06-14T07:23:49Z

/test unit-tests_func_main
/test integration-tests_func_main
/test integration-tests-latest-release_func_main

knative-prow · 2023-06-14T07:23:51Z

@matejvasek: No presubmit jobs available for knative/func@main

In response to this:

/test unit-tests_func_main
/test integration-tests_func_main
/test integration-tests-latest-release_func_main

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/test-infra repository.

knative-prow · 2023-06-19T06:23:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lkingland, matejvasek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [lkingland,matejvasek]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* fix: bad connection handling for in cluster dialer Connections were closed from wrong end of io.Pipe which resulted in confusing error logs. Signed-off-by: Matej Vasek <mvasek@redhat.com> * feat: DialContext() better error handling Now DialContext() returns some errors immediately instead of deferring it Read/Write operation on the returned connection. Signed-off-by: Matej Vasek <mvasek@redhat.com> * feat: DialContext() more better error handling Now ContextDial() tries to parse socat's stderr and translate it to Go's net.OpError instead of just creating error with whole stderr embedded in it. Signed-off-by: Matej Vasek <mvasek@redhat.com> * Apply suggestions from code review Co-authored-by: Lance Ball <lball@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Lance Ball <lball@redhat.com>

…647) * fix: bad connection handling for in cluster dialer (knative#1800) * fix: bad connection handling for in cluster dialer Connections were closed from wrong end of io.Pipe which resulted in confusing error logs. Signed-off-by: Matej Vasek <mvasek@redhat.com> * feat: DialContext() better error handling Now DialContext() returns some errors immediately instead of deferring it Read/Write operation on the returned connection. Signed-off-by: Matej Vasek <mvasek@redhat.com> * feat: DialContext() more better error handling Now ContextDial() tries to parse socat's stderr and translate it to Go's net.OpError instead of just creating error with whole stderr embedded in it. Signed-off-by: Matej Vasek <mvasek@redhat.com> * Apply suggestions from code review Co-authored-by: Lance Ball <lball@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Lance Ball <lball@redhat.com> * fixup: update golangci-lit version Signed-off-by: Matej Vasek <mvasek@redhat.com> * fixup: update CI Go version Signed-off-by: Matej Vasek <mvasek@redhat.com> --------- Signed-off-by: Matej Vasek <mvasek@redhat.com> Co-authored-by: Lance Ball <lball@redhat.com>

knative-prow bot requested review from dsimansk and navidshaikh June 8, 2023 17:08

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 8, 2023

matejvasek force-pushed the fix-dialer branch from 2d45cb6 to d74fd44 Compare June 8, 2023 22:48

fix: bad connection handling for in cluster dialer

a02623f

Connections were closed from wrong end of io.Pipe which resulted in confusing error logs. Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the fix-dialer branch from d74fd44 to 6b30d80 Compare June 8, 2023 22:59

matejvasek added 2 commits June 9, 2023 01:00

feat: DialContext() better error handling

823f03a

Now DialContext() returns some errors immediately instead of deferring it Read/Write operation on the returned connection. Signed-off-by: Matej Vasek <mvasek@redhat.com>

feat: DialContext() more better error handling

9158617

Now ContextDial() tries to parse socat's stderr and translate it to Go's net.OpError instead of just creating error with whole stderr embedded in it. Signed-off-by: Matej Vasek <mvasek@redhat.com>

matejvasek force-pushed the fix-dialer branch from 6b30d80 to 9158617 Compare June 8, 2023 23:01

matejvasek changed the title ~~[WIP] fix: bad connection handling for in cluster dialer~~ fix: bad connection handling for in cluster dialer Jun 8, 2023

matejvasek marked this pull request as ready for review June 8, 2023 23:01

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2023

knative-prow bot requested a review from nainaz June 8, 2023 23:01

matejvasek requested review from zroubalik, lance, jrangelramos and lkingland and removed request for nainaz, dsimansk and navidshaikh June 8, 2023 23:02

lance reviewed Jun 9, 2023

View reviewed changes

pkg/k8s/dialer.go Show resolved Hide resolved

pkg/k8s/dialer.go Show resolved Hide resolved

pkg/k8s/dialer_test.go Outdated Show resolved Hide resolved

pkg/k8s/dialer_test.go Outdated Show resolved Hide resolved

Apply suggestions from code review

7a18b2f

Co-authored-by: Lance Ball <lball@redhat.com>

knative-prow bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 14, 2023

lkingland approved these changes Jun 19, 2023

View reviewed changes

knative-prow bot assigned lkingland Jun 19, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023

knative-prow bot merged commit 3d11766 into knative:main Jun 19, 2023

matejvasek mentioned this pull request Aug 23, 2023

[serverless-1.30] fix: bad connection handling for in cluster dialer openshift-knative/kn-plugin-func#647

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: bad connection handling for in cluster dialer #1800

fix: bad connection handling for in cluster dialer #1800

matejvasek commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading

matejvasek commented Jun 8, 2023

matejvasek commented Jun 9, 2023

matejvasek commented Jun 10, 2023

matejvasek commented Jun 12, 2023

matejvasek commented Jun 12, 2023

lance commented Jun 13, 2023

knative-prow bot commented Jun 13, 2023

matejvasek commented Jun 14, 2023

matejvasek commented Jun 14, 2023

knative-prow bot commented Jun 14, 2023

knative-prow bot commented Jun 19, 2023

fix: bad connection handling for in cluster dialer #1800

fix: bad connection handling for in cluster dialer #1800

Conversation

matejvasek commented Jun 8, 2023 • edited Loading

Changes

codecov bot commented Jun 8, 2023 • edited Loading

Codecov Report

matejvasek commented Jun 8, 2023

matejvasek commented Jun 9, 2023

matejvasek commented Jun 10, 2023

matejvasek commented Jun 12, 2023

matejvasek commented Jun 12, 2023

lance commented Jun 13, 2023

knative-prow bot commented Jun 13, 2023

matejvasek commented Jun 14, 2023

matejvasek commented Jun 14, 2023

knative-prow bot commented Jun 14, 2023

knative-prow bot commented Jun 19, 2023

matejvasek commented Jun 8, 2023 •

edited

Loading

codecov bot commented Jun 8, 2023 •

edited

Loading