Repository: kubesphere/ks-devops
Pull request #441 (2022-01-12 09:18:46)

fix: Using defer in loops and end of loop call fileData.Close() can lead to resource leaks
### What type of PR is this?
/kind bug

### What this PR does / why we need it:
may lead to resource leaks
1.  Using defer in loops
2. call fileData.Close() is too late


### Which issue(s) this PR fixes:

Fixes #

### Special notes for reviewers:

```
```

### Does this PR introduce a user-facing change??

```release-note
None
```
/cc @LinuxSuRen 

ks-ci-bot: @gaffeyQiu: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our [release note process](https://git.k8s.io/community/contributors/guide/release-notes.md) to remove it.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
ks-ci-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubesphere/ks-devops/pull/441#" title="Author self-approved">gaffeyQiu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **linuxsuren** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @linuxsuren` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubesphere%2Fks-devops).

<details open>
Needs approval from an approver in each of these files:

- **[pkg/client/devops/OWNERS](https://github.com/kubesphere/ks-devops/blob/master/pkg/client/devops/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["linuxsuren"]} -->
LinuxSuRen: hi @gaffeyQiu I guess you might didn't set your GitHub-related email address to your git config. See the following screenshot. Your avatar's color is gray.

But this is not a block comment. You can try to fix it or not.

![image](https://user-images.githubusercontent.com/1450685/149105797-b60ce835-ff75-42c5-980b-cfab332614c1.png)

codecov[bot]: # [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) Report
> Merging [#441](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (4673682) into [master](https://codecov.io/gh/kubesphere/ks-devops/commit/098e5871155fd88b4e337f38b23f9db6131c4d4b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) (098e587) will **decrease** coverage by `0.05%`.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/kubesphere/ks-devops/pull/441/graphs/tree.svg?width=650&height=150&src=pr&token=qSK4RBbyNg&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)

```diff
@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
- Coverage   76.78%   76.72%   -0.06%     
==========================================
  Files         108      108              
  Lines       22022    22024       +2     
==========================================
- Hits        16909    16899      -10     
- Misses       4943     4953      +10     
- Partials      170      172       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `76.72% <0.00%> (-0.06%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere) | Coverage Δ | |
|---|---|---|
| [pkg/client/devops/jenkins/request.go](https://codecov.io/gh/kubesphere/ks-devops/pull/441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-cGtnL2NsaWVudC9kZXZvcHMvamVua2lucy9yZXF1ZXN0Lmdv) | `0.00% <0.00%> (ø)` | |
| [controllers/pipeline/pipeline\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvcGlwZWxpbmUvcGlwZWxpbmVfY29udHJvbGxlci5nbw==) | `38.23% <0.00%> (-3.68%)` | :arrow_down: |
| [controllers/s2irun/s2irun\_controller.go](https://codecov.io/gh/kubesphere/ks-devops/pull/441/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere#diff-Y29udHJvbGxlcnMvczJpcnVuL3MyaXJ1bl9jb250cm9sbGVyLmdv) | `33.57% <0.00%> (-3.65%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Last update [098e587...4673682](https://codecov.io/gh/kubesphere/ks-devops/pull/441?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kubesphere).

LinuxSuRen: hi @gaffeyQiu we are using the CI to do some necessary checks. The Lint checking was failed. Please take a look at it.

By the way, we have [a bi-weekly meeting](https://kubesphere.com.cn/contribution/) at 8 PM tonight. Please add me as a WeChat contact (my ID: `linuxsuren`) if you want to join us.
LinuxSuRen: /label tide/merge-method-squash
ks-ci-bot: New changes are detected. LGTM label has been removed.
ks-ci-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubesphere/ks-devops/pull/441#" title="Author self-approved">gaffeyQiu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please ask for approval from **linuxsuren** after the PR has been reviewed.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubesphere%2Fks-devops).

<details open>
Needs approval from an approver in each of these files:

- **[pkg/client/devops/OWNERS](https://github.com/kubesphere/ks-devops/blob/master/pkg/client/devops/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["linuxsuren"]} -->
gaffeyQiu: @LinuxSuRen Maybe we should improve coverage for gojenkins
gaffeyQiu: > 

sorry I don't see this message, you can ignore a last commit, just for test coverage
gaffeyQiu: > /lgtm
> 
> We can ignore the code coverage part.

sorry I don't see this message, you can ignore a last commit, just for test coverage


Patch:
diff --git a/pkg/client/devops/jenkins/request.go b/pkg/client/devops/jenkins/request.go
index 10beb27..2f1918f 100644
--- a/pkg/client/devops/jenkins/request.go
+++ b/pkg/client/devops/jenkins/request.go
@@ -25,8 +25,6 @@ import (
 	"mime/multipart"
 	"net/http"
 	"net/url"
-	"os"
-	"path/filepath"
 	"strings"
 
 	//"github.com/dgrijalva/jwt-go"
@@ -207,21 +205,10 @@ func (r *Requester) DoGet(ar *APIRequest, responseStruct interface{}, options ..
 		body := &bytes.Buffer{}
 		writer := multipart.NewWriter(body)
 		for _, file := range files {
-			fileData, err := os.Open(file)
-			if err != nil {
+			if err := UploadFunc(file, writer); err != nil {
 				Error.Println(err.Error())
 				return nil, err
 			}
-
-			part, err := writer.CreateFormFile("file", filepath.Base(file))
-			if err != nil {
-				Error.Println(err.Error())
-				return nil, err
-			}
-			if _, err = io.Copy(part, fileData); err != nil {
-				return nil, err
-			}
-			defer fileData.Close()
 		}
 		var params map[string]string
 		json.NewDecoder(ar.Payload).Decode(&params)
@@ -310,22 +297,12 @@ func (r *Requester) Do(ar *APIRequest, responseStruct interface{}, options ...in
 	if fileUpload {
 		body := &bytes.Buffer{}
 		writer := multipart.NewWriter(body)
-		for _, file := range files {
-			fileData, err := os.Open(file)
-			if err != nil {
-				Error.Println(err.Error())
-				return nil, err
-			}
 
-			part, err := writer.CreateFormFile("file", filepath.Base(file))
-			if err != nil {
+		for _, file := range files {
+			if err := UploadFunc(file, writer); err != nil {
 				Error.Println(err.Error())
 				return nil, err
 			}
-			if _, err = io.Copy(part, fileData); err != nil {
-				return nil, err
-			}
-			defer fileData.Close()
 		}
 		var params map[string]string
 		json.NewDecoder(ar.Payload).Decode(&params)
diff --git a/pkg/client/devops/jenkins/utils.go b/pkg/client/devops/jenkins/utils.go
index 1b040f5..158e3ba 100644
--- a/pkg/client/devops/jenkins/utils.go
+++ b/pkg/client/devops/jenkins/utils.go
@@ -19,8 +19,11 @@ import (
 	"encoding/json"
 	"io"
 	"io/ioutil"
+	"mime/multipart"
 	"net/http"
 	"net/url"
+	"os"
+	"path/filepath"
 	"strings"
 	"time"
 	"unicode/utf8"
@@ -100,6 +103,23 @@ func ParseJenkinsQuery(query string) (result url.Values, err error) {
 	return
 }
 
+func UploadFunc(fileName string, writer *multipart.Writer) error {
+	fileData, err := os.Open(fileName)
+	if err != nil {
+		return err
+	}
+	defer fileData.Close()
+
+	part, err := writer.CreateFormFile("file", filepath.Base(fileName))
+	if err != nil {
+		return err
+	}
+	if _, err = io.Copy(part, fileData); err != nil {
+		return err
+	}
+	return nil
+}
+
 type JenkinsBlueTime time.Time
 
 func (t *JenkinsBlueTime) UnmarshalJSON(b []byte) error {


Test patch:
diff --git a/pkg/client/devops/jenkins/request_test.go b/pkg/client/devops/jenkins/request_test.go
new file mode 100644
index 0000000..43d67c4
--- /dev/null
+++ b/pkg/client/devops/jenkins/request_test.go
@@ -0,0 +1,38 @@
+package jenkins
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func newFakeRequester() *Requester {
+	// init log
+	new(Jenkins).initLoggers()
+
+	return &Requester{
+		Base: "localhost",
+		Client: http.DefaultClient,
+	}
+}
+
+func newFakeAPIRequest() *APIRequest {
+	return NewAPIRequest("POST", "/test", nil)
+}
+
+func TestRequesterDo(t *testing.T) {
+	// test upload fail logic
+	requester := newFakeRequester()
+	fileNames := []string{"a.tmp", "b.tmp"}
+	_, err := requester.Do(newFakeAPIRequest(), nil, fileNames)
+	assert.NotNil(t, err)
+}
+
+func TestRequesterDoGet(t *testing.T) {
+	// test upload fail logic
+	requester := newFakeRequester()
+	fileNames := []string{"a.tmp", "b.tmp"}
+	_, err := requester.DoGet(newFakeAPIRequest(), nil, fileNames)
+	assert.NotNil(t, err)	
+}
\ No newline at end of file
diff --git a/pkg/client/devops/jenkins/utils_test.go b/pkg/client/devops/jenkins/utils_test.go
index 0acc088..23ac445 100644
--- a/pkg/client/devops/jenkins/utils_test.go
+++ b/pkg/client/devops/jenkins/utils_test.go
@@ -1,7 +1,10 @@
 package jenkins
 
 import (
+	"bytes"
+	"mime/multipart"
 	"net/url"
+	"os"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -19,18 +22,18 @@ func TestParseJenkinsQuery(t *testing.T) {
 		},
 		{
 			param: "branch=master", expected: url.Values{
-				"branch": []string{"master"},
-			}, err: false,
+			"branch": []string{"master"},
+		}, err: false,
 		},
 		{
 			param: "&branch=master", expected: url.Values{
-				"branch": []string{"master"},
-			}, err: false,
+			"branch": []string{"master"},
+		}, err: false,
 		},
 		{
 			param: "branch=master&", expected: url.Values{
-				"branch": []string{"master"},
-			}, err: false,
+			"branch": []string{"master"},
+		}, err: false,
 		},
 		{
 			param: "branch=%gg", expected: url.Values{}, err: true,
@@ -56,3 +59,36 @@ type testData struct {
 	expected interface{}
 	err      bool
 }
+
+func TestUploadFunc(t *testing.T) {
+	testFileName := "/tmp/upload.tmp"
+	testWriter := multipart.NewWriter(&bytes.Buffer{})
+	// The first call should fail because the file doesn't exist 
+	err := UploadFunc(testFileName, testWriter)
+	assert.NotNil(t, err)
+	
+	// Create tmp file
+	_, err = os.Create(testFileName)
+	assert.Nil(t, err, "create tmp file has error: %v", err)
+	defer func ()  {
+		err := os.Remove(testFileName)
+		assert.Nil(t, err, "delete tmp file has error: %v", err)
+	}()
+	
+	// The second call Bad should fail because writer is bad
+	badWriter := multipart.NewWriter(&badWriter{}) 
+	err = UploadFunc(testFileName, badWriter)
+	assert.NotNil(t, err)
+
+	// Final should succeed
+	err = UploadFunc(testFileName, testWriter)
+	assert.Nil(t, err, "UploadFunc has error: %v", err)
+}
+
+type badWriter struct {
+	err error
+}
+
+func (w badWriter) Write([]byte) (int, error) {
+	return 0, w.err
+}
\ No newline at end of file