-
Notifications
You must be signed in to change notification settings - Fork 240
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
bundle validation: check for bundle preset mismatch during setup #4343
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Also, I think |
7c9961b
to
1921db4
Compare
1921db4
to
497e0d9
Compare
pkg/crc/machine/bundle/metadata.go
Outdated
filenameInfo.Driver = filenameParts[2] | ||
filenameInfo.Version = filenameParts[3] | ||
filenameInfo.Arch = filenameParts[4] | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey, apologies i should've mentioned this earlier, but forgot, there's a scenario where the filenameParts
can be 6, we have a custom bundle generation command crc bundle generate
that creates new bundle out of existing one and it names them as crc_microshift_vfkit_4.16.7_arm64_2345.crcbundle
for a microshift bundle and crc_vfkit_4.16.7_arm64_2345.crcbundle
for an openshift bundle which are valid names; see: https://github.com/crc-org/crc/blob/main/pkg/crc/machine/bundle/metadata.go#L227
so if i use a custom bundle currently this fails, for an openshift bundle as:
% crc config set bundle ./crc_vfkit_4.16.7_arm64_2345.crcbundle
WARN Using custom bundle crc_vfkit_4.16.7_arm64_2345.crcbundle
Successfully configured bundle to ./crc_vfkit_4.16.7_arm64_2345.crcbundle
% crc setup --log-level debug
DEBU CRC version: 2.41.0+1921db
DEBU OpenShift version: 4.16.7
DEBU MicroShift version: 4.16.7
DEBU Running 'crc setup'
DEBU Got bundle path: ./crc_vfkit_4.16.7_arm64_2345.crcbundle
DEBU Failed to parse url: parse "./crc_vfkit_4.16.7_arm64_2345.crcbundle": invalid URI for request
WARN Using custom bundle crc_vfkit_4.16.7_arm64_2345.crcbundle
Cannot parse preset 'vfkit'
for a microshift bundle, as:
% crc config set bundle ./crc_microshift_vfkit_4.16.7_arm64_232.crcbundle
WARN Using custom bundle crc_microshift_vfkit_4.16.7_arm64_232.crcbundle
Successfully configured bundle to ./crc_microshift_vfkit_4.16.7_arm64_232.crcbundle
% crc setup --log-level debug
DEBU CRC version: 2.41.0+1921db
DEBU OpenShift version: 4.16.7
DEBU MicroShift version: 4.16.7
DEBU Running 'crc setup'
DEBU Got bundle path: ./crc_microshift_vfkit_4.16.7_arm64_232.crcbundle
DEBU Failed to parse url: parse "./crc_microshift_vfkit_4.16.7_arm64_232.crcbundle": invalid URI for request
WARN Using custom bundle crc_microshift_vfkit_4.16.7_arm64_232.crcbundle
bundle filename is in unrecognized format
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be easier to parse with a regexp and submatches?
The regexp would be something like this, crc(_[[:alpha:]]+)?_([[:alpha:]]+)_([0-9.]+)(_custom)?.crcbundle
(it probably can be improved to be more specific, dunno if it can match directly vfkit or libvirt or hyperv for example).
Then I think https://pkg.go.dev/regexp#Regexp.FindSubmatch will parse the filename for you, and give you the various submatches between parentheses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised a custom bundle has a _232
suffix as in your example Anjan, I thought they were suffixed with only _custom
I've only handled the latter in the regexp above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anjannath Hmm, good to know. So this is going to be a little more difficult. What is the meaning of the last field (ex. 2345
)? Or how should I name it in the FilenameInfo
struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last field is only present in a user generated custom bundle, it is just to differentiate that it is not the usual bundle we ship, so maybe we can name it as customBundleSuffix
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I used that name.
pkg/crc/machine/bundle/metadata.go
Outdated
filenameInfo.Driver = filenameParts[2] | ||
filenameInfo.Version = filenameParts[3] | ||
filenameInfo.Arch = filenameParts[4] | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be easier to parse with a regexp and submatches?
The regexp would be something like this, crc(_[[:alpha:]]+)?_([[:alpha:]]+)_([0-9.]+)(_custom)?.crcbundle
(it probably can be improved to be more specific, dunno if it can match directly vfkit or libvirt or hyperv for example).
Then I think https://pkg.go.dev/regexp#Regexp.FindSubmatch will parse the filename for you, and give you the various submatches between parentheses.
pkg/crc/machine/bundle/metadata.go
Outdated
filenameInfo.Driver = filenameParts[2] | ||
filenameInfo.Version = filenameParts[3] | ||
filenameInfo.Arch = filenameParts[4] | ||
default: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised a custom bundle has a _232
suffix as in your example Anjan, I thought they were suffixed with only _custom
I've only handled the latter in the regexp above.
i think this got changed, now they don't contain |
Ah ok, I was tricked by |
497e0d9
to
3fa3f31
Compare
Thanks for the suggestion @cfergeau, I modified your regex a bit and it worked really well. |
|
||
return &filenameInfo, nil | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have unit test for this function which takes all the valid bundle name and provide info what it suppose to do and some dummy one to check the error.
@redbeam: The following tests failed, say
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. |
Fixes: Issue #3615
Added a check for bundle type mismatch with preset in function
ValidateBundle
.crc/pkg/crc/validation/validation.go
Lines 91 to 107 in 7b04ce5
The check is performed before checking for custom bundle (and potentially displaying a warning).
Function
BundleMismatchWithPreset
was moved frommachine/start.go
tovalidation/validation.go
and is used during start as well.Testing
crc config set preset openshift
crc setup --bundle <path_to_bundle>
Output:
FATA Preset openshift is used but bundle is provided for microshift preset