Update golangci t0 1.61.0 #4360

praveenkumar · 2024-09-20T05:50:17Z

No description provided.

cfergeau

This introduces new linting failures.

update-go-version.sh

cfergeau · 2024-09-20T14:38:53Z

pkg/os/shell/shell_windows.go

@@ -30,6 +30,7 @@ func getProcessEntry(pid int) (pe *syscall.ProcessEntry32, err error) {
 	}

 	for {
+		//#nosec G115 -- pid is coming from os.Getppid() as int


the os.Getppid() call is 2 level above getProcessEntry in the call chain. Nothing guarantees that in the future we are not going to reuse this function with a pid value coming from something different.
I'd prefer to change the getProcessEntry(pid int) to getProcessEntry(pid uint32), and to change detect to convert the value returned by os.Getppid() to uint32

cfergeau · 2024-09-20T14:43:32Z

pkg/crc/machine/start.go

@@ -265,8 +265,8 @@ func configureSharedDirs(vm *virtualMachine, sshRunner *crcssh.Runner) error {

 func (client *client) Start(ctx context.Context, startConfig types.StartConfig) (*types.StartResult, error) {
 	telemetry.SetCPUs(ctx, startConfig.CPUs)
-	telemetry.SetMemory(ctx, uint64(startConfig.Memory)*1024*1024)
-	telemetry.SetDiskSize(ctx, uint64(startConfig.DiskSize)*1024*1024*1024)
+	telemetry.SetMemory(ctx, uint64(startConfig.Memory)*1024*1024)          //#nosec G115 -- validation make sure memory value is not negative


actually I haven't been able to find such >=0 checks in the validation code?

for memory as part of validation we use the base memory as default preset one (in case of OCP it is 10752 and for microshift 4096

// ValidateMemory checks if provided Memory count is valid func ValidateMemory(value int, preset crcpreset.Preset) error { if value < constants.GetDefaultMemory(preset) { return fmt.Errorf("requires memory in MiB >= %d", constants.GetDefaultMemory(preset)) } return ValidateEnoughMemory(value)

cfergeau · 2024-09-20T14:45:53Z

pkg/drivers/hyperv/hyperv_windows.go

@@ -74,6 +74,7 @@ func (d *Driver) UpdateConfigRaw(rawConfig []byte) error {
 		}
 	}
 	if newDriver.DiskCapacity != d.DiskCapacity {
+		//#nosec G115 -- validation make sure disk value is in range


I'd prefer if code in pkg/drivers did not make assumptions regarding how it's used, and in particular that we don't rely on code in pkg/crc having to be used before passing data to the drivers.
Moreover, I'm not sure we have the needed range check in validation

praveenkumar · 2024-09-27T09:03:16Z

@cfergeau Can you take another look to this PR, it have machine update and also golint ci update.

cfergeau · 2024-09-27T10:15:17Z

pkg/crc/config/config.go

+			return SettingValue{
+				Invalid: true,
+			}
+		}


Unrelated to this PR, but wondering if we could use go generics to have a generic 'cast' function instead of switching over the types.

Right, that would be a good exercise, we can put a tracker issue for it.

cmd/crc/cmd/start.go

pkg/crc/machine/config/config.go

tools/go.mod

pkg/os/shell/shell_windows.go

cfergeau · 2024-09-27T10:44:54Z

pkg/os/shell/shell_windows_test.go

 	assert.NoError(t, err)
 }

 func TestGetNameAndItsPpidOfParent(t *testing.T) {
-	shell, _, err := getNameAndItsPpid(os.Getppid())
+	//#nosec G115 -- pid is coming from os.Getppid() as int


Same comment as before for these 3 ignores

cfergeau · 2024-09-27T10:47:25Z

pkg/drivers/vfkit/driver_darwin.go

@@ -97,20 +97,21 @@ func (d *Driver) getDiskPath() string {
 	return d.ResolveStorePath(fmt.Sprintf("%s.img", d.MachineName))
 }

-func (d *Driver) resize(newSize int64) error {
+func (d *Driver) resize(newSize uint64) error {
+	newSizeInt64 := int64(newSize) //#nosec G115 -- os.stat provide the size as int64 and os.Truncate use int64 as arg


This explains why there's a warning, but this should be explaining why it's ok to ignore it. This needs range checks before doing the cast.

pkg/crc/machine/kubeconfig.go

pkg/crc/api/api_client_test.go

cfergeau · 2024-10-01T09:15:11Z

pkg/os/shell/shell_windows.go

+		if pid < 0 || pid > math.MaxUint32 {
+			return "", fmt.Errorf("integer overflow for pid: %v", pid)
+		}
+		//#nosec G115 -- Above pid is checked for integer overflow and an error is returned


With the range check above, the #nosec directive is not needed.

pkg/os/shell/shell_windows_test.go

cfergeau · 2024-10-01T12:21:35Z

pkg/os/shell/shell_windows_test.go

+	if pid < 0 || pid > math.MaxUint32 {
+		assert.Fail(t, "integer overflow detected")
+	}
+	//#nosec G115 -- Above pid is checked for integer overflow and fail the test


And same here

cfergeau · 2024-10-01T12:23:04Z

pkg/drivers/vfkit/driver_darwin.go

+	if newSize > math.MaxInt64 {
+		return fmt.Errorf("integer overflow detected for resize: %v", newSize)
+	}
+	//#nosec G115 -- os.stat provide the size as int64 and os.Truncate use int64 as arg. Also range is checked (above)


#nosec G115 is not needed since you added the range check. You can keep the rest of the comment if you think it's useful.

openshift-ci · 2024-10-03T07:12:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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~~ [cfergeau]

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

praveenkumar · 2024-10-03T08:45:34Z

/hold

make check on windows failing, need to check what's happening.

With this update type of memory and cpu is changed to uint from int which will allow us to fix some of interter overflow issue with latest golint ci.

It help us to fix integer overflow issue with latest version of golint-ci.

Looks like it is just the to pass value if command is shown or not and `int32` can handle it. Also it will fix the following golanglint error. ``` pkg/os/windows/win32/shell32_windows.go:40:77: G115: integer overflow conversion int -> int32 (gosec) return windows.ShellExecute(hwnd, op, toUint16ptr(file), params, dir, int32(showCmd)) ```

With that we don't need to do type conversion and avoid following golint error. ``` pkg/drivers/vfkit/driver_darwin.go:407:40: G115: integer overflow conversion int -> int32 (gosec) exists, err := process.PidExists(int32(pid)) ^ pkg/drivers/vfkit/driver_darwin.go:414:36: G115: integer overflow conversion int -> int32 (gosec) p, err := process.NewProcess(int32(pid)) ^ ```

openshift-ci · 2024-10-08T09:32:12Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2024-10-08T09:45:43Z

@praveenkumar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`d7f74fd`	link	true	`/test e2e-crc`
ci/prow/integration-crc	`d7f74fd`	link	true	`/test integration-crc`
ci/prow/e2e-microshift-crc	`d7f74fd`	link	true	`/test e2e-microshift-crc`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

os.Getppid() return int value but syscall.ProcessEntry32 struct have `ProcessID` as uint32 so we are doing the type conversion as part of `os.Getppid()` instead later in stage. It will fix following golint error. ``` pkg/os/shell/shell_windows.go:33:38: G115: integer overflow conversion int -> uint32 (gosec) if processEntry.ProcessID == uint32(pid) { ```

It will reduce following error for golint ci. ``` pkg/drivers/libhvee/libhvee_windows.go:92:31: G115: integer overflow conversion uint64 -> int64 (gosec) if err := d.resizeDisk(int64(update.DiskCapacity)); err != nil { ^ pkg/drivers/libhvee/libhvee_windows.go:196:27: G115: integer overflow conversion uint64 -> int64 (gosec) return d.resizeDisk(int64(d.DiskCapacity)) ^ pkg/drivers/libhvee/libhvee_windows.go:288:26: G115: integer overflow conversion int64 -> uint64 (gosec) newSize := strongunits.B(newSizeBytes) ```

It will fix following golint ci error ``` pkg/drivers/vfkit/driver_darwin.go:135:23: G115: integer overflow conversion uint64 -> int64 (gosec) return d.resize(int64(d.DiskCapacity)) ^ pkg/drivers/vfkit/driver_darwin.go:176:7: G115: integer overflow conversion int -> uint (gosec) uint(d.CPU), ^ pkg/drivers/vfkit/driver_darwin.go:177:9: G115: integer overflow conversion int -> uint64 (gosec) uint64(d.Memory), ^ ```

This will fix follow golint ci warning ``` pkg/crc/machine/kubeconfig.go:189:29: G115: integer overflow conversion uint -> int (gosec) port = strconv.Itoa(int(ingressHTTPSPort)) ```

Latest release of golangci-lint uses `go 1.22.1` as part of go mod file so when our tooling try to consume this version then following is added for `tools/go.mod` because in our mod file we mention minimum required go version is `1.22.0` and as per go toolchain guideline it should run in in newer toolchain ``` go 1.22.0 toolchain go1.22.5 ``` snip from: https://go.dev/doc/toolchain ``` When the go or toolchain line is newer than the bundled toolchain, the go command runs the newer toolchain instead. For example, when using the go command bundled with Go 1.21.3 in a main module that says go 1.21.9, the go command finds and runs Go 1.21.9 instead. It first looks in the PATH for a program named go1.21.9 and otherwise downloads and caches a copy of the Go 1.21.9 toolchain. ``` - https://github.com/golangci/golangci-lint/blob/v1.61.0/go.mod#L3

Bumps [github.com/golangci/golangci-lint](https://github.com/golangci/golangci-lint) from 1.59.1 to 1.61.0. - [Release notes](https://github.com/golangci/golangci-lint/releases) - [Changelog](https://github.com/golangci/golangci-lint/blob/master/CHANGELOG.md) - [Commits](golangci/golangci-lint@v1.59.1...v1.61.0) --- updated-dependencies: - dependency-name: github.com/golangci/golangci-lint dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

openshift-ci bot requested review from evidolob and gbraad September 20, 2024 05:50

cfergeau reviewed Sep 20, 2024

View reviewed changes

update-go-version.sh Show resolved Hide resolved

cfergeau reviewed Sep 20, 2024

View reviewed changes

praveenkumar self-assigned this Sep 23, 2024

openshift-merge-robot added the needs-rebase label Sep 23, 2024

praveenkumar force-pushed the update_golangci branch from 4f2e533 to dd235a1 Compare September 27, 2024 07:33

openshift-merge-robot removed the needs-rebase label Sep 27, 2024

praveenkumar force-pushed the update_golangci branch from dd235a1 to 1371826 Compare September 27, 2024 09:00

praveenkumar force-pushed the update_golangci branch from 1371826 to bdbe6e5 Compare September 27, 2024 09:47

cfergeau reviewed Sep 27, 2024

View reviewed changes

praveenkumar force-pushed the update_golangci branch 2 times, most recently from 85ef8d0 to bdff643 Compare October 1, 2024 09:26

cfergeau reviewed Oct 1, 2024

View reviewed changes

praveenkumar force-pushed the update_golangci branch from bdff643 to 829ce5f Compare October 3, 2024 05:51

cfergeau approved these changes Oct 3, 2024

View reviewed changes

openshift-ci bot assigned cfergeau Oct 3, 2024

openshift-ci bot added the lgtm label Oct 3, 2024

openshift-ci bot added the approved label Oct 3, 2024

openshift-ci bot added the do-not-merge/hold label Oct 3, 2024

praveenkumar added 4 commits October 8, 2024 14:20

Update crc-org/machine module

b1b1e12

With this update type of memory and cpu is changed to uint from int which will allow us to fix some of interter overflow issue with latest golint ci.

Change type of DiskSize to int to uint

d22f34f

It help us to fix integer overflow issue with latest version of golint-ci.

praveenkumar force-pushed the update_golangci branch from 829ce5f to d7f74fd Compare October 8, 2024 09:32

openshift-ci bot removed the lgtm label Oct 8, 2024

praveenkumar and others added 6 commits October 8, 2024 15:17

kubeconfig: For string conversion use FormatUint instead Itoa

305900f

This will fix follow golint ci warning ``` pkg/crc/machine/kubeconfig.go:189:29: G115: integer overflow conversion uint -> int (gosec) port = strconv.Itoa(int(ingressHTTPSPort)) ```

praveenkumar force-pushed the update_golangci branch from d7f74fd to 0a39b01 Compare October 8, 2024 09:55

praveenkumar merged commit f5cec01 into crc-org:main Oct 8, 2024
21 of 32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update golangci t0 1.61.0 #4360

Update golangci t0 1.61.0 #4360

praveenkumar commented Sep 20, 2024

cfergeau left a comment

cfergeau Sep 20, 2024

cfergeau Sep 20, 2024

praveenkumar Sep 23, 2024

cfergeau Sep 20, 2024

praveenkumar commented Sep 27, 2024

cfergeau Sep 27, 2024

praveenkumar Oct 1, 2024

cfergeau Sep 27, 2024

cfergeau Sep 27, 2024

cfergeau Oct 1, 2024

cfergeau Oct 1, 2024

cfergeau Oct 1, 2024

openshift-ci bot commented Oct 3, 2024

praveenkumar commented Oct 3, 2024

openshift-ci bot commented Oct 8, 2024

openshift-ci bot commented Oct 8, 2024 •

edited

Loading

Update golangci t0 1.61.0 #4360

Update golangci t0 1.61.0 #4360

Conversation

praveenkumar commented Sep 20, 2024

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar commented Sep 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2024

praveenkumar commented Oct 3, 2024

openshift-ci bot commented Oct 8, 2024

openshift-ci bot commented Oct 8, 2024 • edited Loading

openshift-ci bot commented Oct 8, 2024 •

edited

Loading