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

Change to the same way of linting as ocm repo, and upgrade golint-ci version and configuration. #481

Merged
merged 1 commit into from
Feb 13, 2025

Conversation

@xuezhaojun xuezhaojun force-pushed the upgrade-golint-version branch from 73b5e5d to 2e14a18 Compare February 10, 2025 05:14
@xuezhaojun xuezhaojun changed the title chore(lint): bump golangci-lint version to 1.63.4 Upgrade golangci.yaml and change to the same way of linting as ocm repo did. Feb 10, 2025
golangci.yml Outdated Show resolved Hide resolved
@xuezhaojun
Copy link
Contributor Author

/retest

@xuezhaojun xuezhaojun force-pushed the upgrade-golint-version branch from f3737a3 to a483670 Compare February 10, 2025 08:38
@xuezhaojun xuezhaojun changed the title Upgrade golangci.yaml and change to the same way of linting as ocm repo did. Change to the same way of linting as ocm repo did and fix some lint issues. Feb 11, 2025
@xuezhaojun xuezhaojun force-pushed the upgrade-golint-version branch 2 times, most recently from 671c5ed to 799b024 Compare February 11, 2025 13:16
@xuezhaojun xuezhaojun changed the title Change to the same way of linting as ocm repo did and fix some lint issues. Change to the same way of linting as ocm repo, and upgrade golint-ci version and configuration. Feb 11, 2025
cmd/manager/main.go Outdated Show resolved Hide resolved
@@ -39,8 +39,7 @@ func extractBootstrapKubeConfigDataFromImportSecret(importSecret *corev1.Secret)

for _, yaml := range helpers.SplitYamls(importYaml) {
obj := helpers.MustCreateObject(yaml)
switch secret := obj.(type) {
case *corev1.Secret:
if secret, ok := obj.(*corev1.Secret); ok {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix:

pkg/controller/importconfig/cluster_info.go:42:3: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
		switch secret := obj.(type) {

@@ -980,10 +980,10 @@ func ValidateNodeSelector(nodeSelector map[string]string) error {
errs := []error{}
for key, val := range nodeSelector {
if errMsgs := validation.IsQualifiedName(key); len(errMsgs) != 0 {
errs = append(errs, fmt.Errorf(strings.Join(errMsgs, ";")))
errs = append(errs, fmt.Errorf("%s", strings.Join(errMsgs, ";")))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A big type of lint errors is non-constant format string:

pkg/source/source.go:120:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", obj))
				            ^
pkg/source/source.go:125:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", obj))
				            ^
pkg/source/source.go:142:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", oldObj))
				            ^
pkg/source/source.go:147:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", oldObj))
				            ^
pkg/source/source.go:153:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", newObj))
				            ^
pkg/source/source.go:158:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnAdd missing Object, type %T", newObj))
				            ^
pkg/source/source.go:177:18: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
					klog.Errorf(fmt.Sprintf("Error decoding objects. Expected cache.DeletedFinalStateUnknown, type %T", obj))
					            ^
pkg/source/source.go:187:17: printf: non-constant format string in call to k8s.io/klog/v2.Errorf (govet)
				klog.Errorf(fmt.Sprintf("OnDelete missing Object, type %T", obj))
				            ^
pkg/helpers/helpers.go:983:35: printf: non-constant format string in call to fmt.Errorf (govet)
			errs = append(errs, fmt.Errorf(strings.Join(errMsgs, ";")))
			                               ^
pkg/helpers/helpers.go:986:35: printf: non-constant format string in call to fmt.Errorf (govet)
			errs = append(errs, fmt.Errorf(strings.Join(errMsgs, ";")))
			                               ^
pkg/helpers/helpers.go:999:36: printf: non-constant format string in call to fmt.Errorf (govet)
				errs = append(errs, fmt.Errorf(strings.Join(errMsgs, ";")))
				                               ^
pkg/helpers/helpers.go:1020:36: printf: non-constant format string in call to fmt.Errorf (govet)
				errs = append(errs, fmt.Errorf(strings.Join(errMsgs, ";")))

@xuezhaojun xuezhaojun force-pushed the upgrade-golint-version branch from 6a2c0f0 to b65b6ad Compare February 11, 2025 15:24
@@ -124,7 +124,11 @@ func main() {
pflag.Parse()

logs.InitLogs()
defer logs.FlushLogs()
exitCode := 0
Copy link
Contributor Author

@xuezhaojun xuezhaojun Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes:

exitAfterDefer: os.Exit will exit, and `defer logs.FlushLogs()` will not run (gocritic)

@xuezhaojun
Copy link
Contributor Author

/assign @zhujian7

@@ -214,8 +127,15 @@ issues:
# Default value for this option is true.
exclude-use-default: true

# Maximum issues count per one linter. Set to 0 to disable. Default is 50.
max-per-linter: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this?

Copy link
Contributor Author

@xuezhaojun xuezhaojun Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The field is not supported by the latest golangci-lint, it will report error.

Copy link
Contributor Author

@xuezhaojun xuezhaojun Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also a dup of max-issues-per-linter: 0 set above.

Makefile Outdated
@@ -51,7 +51,8 @@ check-copyright:

.PHONY: lint
lint:
build/run-lint-check.sh
go install github.com/golangci/golangci-lint/cmd/[email protected]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to install the golint to the current project sub dir, so it will affect the global linter?

Copy link
Contributor Author

@xuezhaojun xuezhaojun Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated to put golint into _output dir with other tools.

Signed-off-by: xuezhaojun <[email protected]>
@xuezhaojun xuezhaojun force-pushed the upgrade-golint-version branch from b65b6ad to 95e2fc7 Compare February 12, 2025 09:06
@xuezhaojun
Copy link
Contributor Author

/retest

@xuezhaojun
Copy link
Contributor Author

xuezhaojun commented Feb 12, 2025

The EC check occasionally passes and sometimes fails. I'm unsure whether we need to address it or overwrite it.

@xuezhaojun xuezhaojun requested a review from zhujian7 February 12, 2025 12:43
@xuezhaojun
Copy link
Contributor Author

/retest

@zhujian7
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Feb 13, 2025
Copy link

openshift-ci bot commented Feb 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xuezhaojun, zhujian7

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 [xuezhaojun,zhujian7]

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

@zhujian7
Copy link
Contributor

zhujian7 commented Feb 13, 2025

The EC check occasionally passes and sometimes fails. I'm unsure whether we need to address it or overwrite it.

it seems there are 2 new cves:

✕ [Violation] cve.cve_blockers
  ImageRef: quay.io/redhat-user-workloads/crt-redhat-acm-tenant/managedcluster-import-controller-addon-mce-28@sha256:2041ac766ce359bddcdc1f71b94322f71d05a50b93f63c78857ac790103367b1
  Reason: Found 1 CVE vulnerabilities of high security level
  Title: Blocking CVE check
  Description: The SLSA Provenance attestation for the image is inspected to ensure CVEs that have a known fix and meet a certain
  security level have not been detected. If detected, this policy rule will fail. By default, only CVEs of critical and high
  security level cause a failure. This is configurable by the rule data key `restrict_cve_security_levels`. The available levels
  are critical, high, medium, low, and unknown. In addition to that leeway can be granted per severity using the `cve_leeway` rule
  data key containing days of allowed leeway, measured as time between found vulnerability's public disclosure date and current
  effective time, per severity level. To exclude this rule add "cve.cve_blockers:high" to the `exclude` section of the policy
  configuration.
  Solution: Make sure to address any CVE's related to the image. The CVEs are detected by the task that runs a Clair scan and
  emits a result named `SCAN_OUTPUT`.

✕ [Violation] cve.cve_blockers
  ImageRef: quay.io/redhat-user-workloads/crt-redhat-acm-tenant/managedcluster-import-controller-addon-mce-28@sha256:15633db1ce2d463f51a4af5fd6e7f066dc39da1515d41704408475b36e74018e
  Reason: Found 2 CVE vulnerabilities of high security level
  Title: Blocking CVE check
  Description: The SLSA Provenance attestation for the image is inspected to ensure CVEs that have a known fix and meet a certain
  security level have not been detected. If detected, this policy rule will fail. By default, only CVEs of critical and high
  security level cause a failure. This is configurable by the rule data key `restrict_cve_security_levels`. The available levels
  are critical, high, medium, low, and unknown. In addition to that leeway can be granted per severity using the `cve_leeway` rule
  data key containing days of allowed leeway, measured as time between found vulnerability's public disclosure date and current
  effective time, per severity level. To exclude this rule add "cve.cve_blockers:high" to the `exclude` section of the policy
  configuration.
  Solution: Make sure to address any CVE's related to the image. The CVEs are detected by the task that runs a Clair scan and
  emits a result named `SCAN_OUTPUT`.

@xuezhaojun can you use your script to figure out what they are?

@xuezhaojun
Copy link
Contributor Author

quay.io/redhat-user-workloads/crt-redhat-acm-tenant/managedcluster-import-controller-addon-mce-28@sha256:2041ac766ce359bddcdc1f71b94322f71d05a50b93f63c78857ac790103367b1

There are 3 High cve but not cases we can solve in our code:

- name: CVE-2024-12797
  description: A flaw was found in OpenSSL's RFC7250 Raw Public Key (RPK) authentication. This vulnerability allows man-in-the-middle (MITM) attacks via failure to abort TLS/DTLS handshakes when the server's RPK does not match the expected key despite the SSL_VERIFY_PEER verification mode being set.
  issued: "2025-02-11T15:00:00Z"
  normalized_severity: High
  package_name: openssl-libs
  fixed_in_version: 1:3.2.2-6.el9_5.1
- name: CVE-2022-49043
  description: xmlXIncludeAddNode in xinclude.c in libxml2 before 2.11.0 has a use-after-free.
  issued: "2025-01-26T00:00:00Z"
  normalized_severity: High
  package_name: libxml2
  fixed_in_version: ""
- name: CVE-2024-12797
  description: A flaw was found in OpenSSL's RFC7250 Raw Public Key (RPK) authentication. This vulnerability allows man-in-the-middle (MITM) attacks via failure to abort TLS/DTLS handshakes when the server's RPK does not match the expected key despite the SSL_VERIFY_PEER verification mode being set.
  issued: "2025-02-11T15:00:00Z"
  normalized_severity: High
  package_name: openssl-libs
  fixed_in_version: 1:3.2.2-6.el9_5.1

@xuezhaojun xuezhaojun merged commit 5b0c579 into stolostron:main Feb 13, 2025
11 of 13 checks passed
@xuezhaojun xuezhaojun deleted the upgrade-golint-version branch February 13, 2025 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants