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

Address gosec G601 issues #210

Merged

Conversation

frobware
Copy link
Contributor

@frobware frobware commented Nov 5, 2020

$ gosec -version
Version: 2.5.0
Git tag: v2.5.0
Build date: 2020-10-26T11:52:22Z

On simple inspection the G601 issues from gosec looked innocuous
enough but I decided to clean these up anyway.

For the changes to router.go and template_helper.go I took the
simplest approach of creating a new variable (and copy). I could have
changed everything to be index based but I chose to go with the
smallest change.

PR inspired by #208

It still leaves open the question asked in #208 on whether we will run
this tool as during build and/or prior to a release.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2020
$ gosec -version
Version: 2.5.0
Git tag: v2.5.0
Build date: 2020-10-26T11:52:22Z

On simple inspection the G601 issues from gosec looked innocuous
enough but I decided to clean these up anyway.

For the changes to router.go and template_helper.go I took the
simplest approach of creating a new variable (and copy). I could have
changed everything to be index based but I chose to go with the
smallest change.

PR inspired by openshift#208

It still leaves open the question asked in openshift#208 as to whether we will
run this tool during builds and/or prior to a release.
@frobware
Copy link
Contributor Author

frobware commented Nov 5, 2020


Run multi-stage test e2e-agnostic - e2e-agnostic-gather-audit-logs container test expand_less | 1m1s
-- | --
[must-gather      ] OUT Get "https://api.ci-op-6tjfrnvr-4c798.origin-ci-int-gce.dev.openshift.com:6443/apis/image.openshift.io/v1/namespaces/openshift/imagestreams/must-gather": dial tcp 34.75.225.219:6443: i/o timeout [must-gather      ] OUT  [must-gather      ] OUT Using must-gather plug-in image: registry.redhat.io/openshift4/ose-must-gather:latest Unable to connect to the server: dial tcp 34.75.225.219:6443: i/o timeout error: failed to execute wrapped command: exit status 1

/retest

@sgreene570
Copy link

On simple inspection the G601 issues from gosec looked innocuous
enough but I decided to clean these up anyway.

Are there any other gosec codes we should be worried about?

@frobware
Copy link
Contributor Author

frobware commented Nov 5, 2020

Are there any other gosec codes we should be worried about?

Those were the immediate ones but there's a big list in #208 - some are more interesting than others (e.g., not handling errors from write(). I chose to do the memory aliasing as a discrete change.

@sgreene570
Copy link

Maybe we should consider setting something up like this

https://github.com/openshift/cluster-logging-operator/blob/master/golangci.yaml

@frobware
Copy link
Contributor Author

frobware commented Nov 5, 2020

Maybe we should consider setting something up like this

https://github.com/openshift/cluster-logging-operator/blob/master/golangci.yaml

Depends on how many false positives all of those throw up. Often there's too much noise. I used to run openshift/origin#25663 https://github.com/alecthomas/gometalinter in development to make sure that for at least the lines I touch I don't introduce anything new.

@frobware
Copy link
Contributor Author

frobware commented Nov 6, 2020

/retest

@frobware
Copy link
Contributor Author

frobware commented Nov 9, 2020

/retest

1 similar comment
@sgreene570
Copy link

/retest

@sgreene570
Copy link

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 10, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, sgreene570

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 [frobware,sgreene570]

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

@openshift-merge-robot openshift-merge-robot merged commit 50ac19c into openshift:master Nov 10, 2020
@frobware frobware deleted the address-gosec-G601-issues branch November 16, 2020 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants