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.

Sign up for GitHub

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

Jump to bottom

Fix CPUVulnerabilities() reporting from sysfs #532

Merged
merged 10 commits into from Jun 15, 2023
Merged

Fix CPUVulnerabilities() reporting from sysfs #532

merged 10 commits into from Jun 15, 2023

Conversation

mwasilew2
Copy link
Contributor

@mwasilew2 mwasilew2 commented Jun 2, 2023 •
edited

closes #531

This PR contains a few changes (each in a separate commit):

  • adds fixtures and tests for parsing cpu vulnerabilities from sysfs, fixes the typo in the vulnerability string
  • switches to using a map of vulnerabilities instead of a slice (makes working with the result easier, e.g. when testing)
  • switches to using encoding of the state
  • adds license in the test file
  • adds linux build flags
  • adds test cases for vulnerable state, brings state string to lower cases (to try and make it case insensitive)

@mwasilew2 mwasilew2 marked this pull request as draft June 2, 2023 14:22
SuperQ
SuperQ reviewed Jun 2, 2023
View reviewed changes
sysfs/vulnerability.go Outdated Show resolved Hide resolved
@mwasilew2 mwasilew2 marked this pull request as ready for review June 2, 2023 15:22
SuperQ
SuperQ requested changes Jun 3, 2023
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise great.

sysfs/vulnerability_test.go Outdated Show resolved Hide resolved
sysfs/vulnerability.go Show resolved Hide resolved
SuperQ
SuperQ requested changes Jun 3, 2023
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise great.

@mwasilew2
Copy link
Contributor Author

@discordianfish @SuperQ is there anything else I can help with here?

SuperQ
SuperQ approved these changes Jun 15, 2023
View reviewed changes
Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

mwasilew2 and others added 10 commits June 15, 2023 11:05
@mwasilew2
add tests for sysfs vulnerabilities
03b4c5e 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
use a vulnerability indexed map instead of a slice
fd0c9fd 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
switch to encoding for vuln state instead of plain strings
a297895 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
add missing license in the test file
254efce 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
add missing build tags
9c26b1d 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
add test cases for vulnerable, unify char cases when parsing state
1faaea2 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2 @SuperQ
Update sysfs/vulnerability_test.go
953794a 
Co-authored-by: Ben Kochie <superq@gmail.com>
Signed-off-by: Michal <mwasilewski@gmx.com>
@mwasilew2
minor update to the readme
3096943 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
use switch instead of multiple ifs (no semantic changes)
fbbf002 
Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2
Explain purpose of VulnerabilityHumanEncoding
3197304 
adds a go doc explaining the purpose of the map containing mapping from
int encoded state to human friendly strings

Signed-off-by: Michal Wasilewski <mwasilewski@gmx.com>
@mwasilew2 mwasilew2 force-pushed the mwasilew2/vulnerability-state-parsing branch from 0b132c1 to 3197304 Compare June 15, 2023 09:06
@mwasilew2
Copy link
Contributor Author

mwasilew2 commented Jun 15, 2023 •
edited

rebased onto upstream

edit: due to unrelated failures in the CI

@mwasilew2
Copy link
Contributor Author

the codespell job is failing, but it's unrelated to this change

Here's the job definition:

procfs/.circleci/config.yml

Lines 38 to 45 in abbfda0

codespell:
docker:
- image: circleci/python
steps:
- checkout
- run: sudo pip install codespell
- run: codespell --skip=".git,./vendor,ttar,fixtures.ttar,./fixtures,go.mod,go.sum" -I scripts/codespell_ignore.txt

the docker image hasn't changed recently: https://hub.docker.com/r/circleci/python/tags

codespell pip package was updated yesterday: https://pypi.org/project/codespell/#history

I think this is unrelated and should be handled in a separate issue/PR. I'll open a separate one

@mwasilew2 mwasilew2 mentioned this pull request Jun 15, 2023
Closed
@SuperQ
Copy link
Member

SuperQ commented Jun 15, 2023

Going to ignore the codespell, it's fixed in HEAD.

@SuperQ SuperQ merged commit a76f400 into prometheus:master Jun 15, 2023
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@discordianfish discordianfish discordianfish approved these changes

@SuperQ SuperQ SuperQ approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CPU vulnerability reporting from sysfs is broken
3 participants
@mwasilew2 @SuperQ @discordianfish