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

Possible Bug In VLAN parsing #180

Closed
dvintel opened this issue Aug 25, 2023 · 2 comments · Fixed by #181
Closed

Possible Bug In VLAN parsing #180

dvintel opened this issue Aug 25, 2023 · 2 comments · Fixed by #181

Comments

@dvintel
Copy link

dvintel commented Aug 25, 2023

On line: https://github.com/tinkerbell/hook/blob/1c438a9b6dbd72a5611c54a0b60bd0dfc8a87888/files/dhcp.sh#L14C19-L14C50

The command:

sed -n 's/.*vlan_id=\([0-9]*\).*/\1/p' /proc/cmdline

will also match the string: testvlan_id=2.

I am expecting that it would only match on vlan_id=2
Note: only match on vlan_id= rather than also on testvlan_id=

@dvintel
Copy link
Author

dvintel commented Aug 25, 2023

One possible solution would be to change the sed command to something like:

's/.* vlan_id=\([0-9]*\).*/\1/p'

add a space before the vlan_id=

@jacobweinstock
Copy link
Member

Hey @dvintel, thanks for finding and reporting this! I've opened #181 to resolve this. Thanks!

@mergify mergify bot closed this as completed in #181 Aug 25, 2023
mergify bot added a commit that referenced this issue Aug 25, 2023
## Description


This fixes an issue where "vlan_id" is a substring. For example: "thisisavlan_id=123". This example will be treated as if the vlan_id was not set at all.

## Why is this needed



Fixes: #180 

## How Has This Been Tested?





## How are existing users impacted? What migration steps/scripts do we need?





## Checklist:

I have:

- [ ] updated the documentation and/or roadmap (if required)
- [ ] added unit or e2e tests
- [ ] provided instructions on how to upgrade
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants