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

Validate allowed key-value stores #1235

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

itowlson
Copy link
Contributor

@itowlson itowlson commented Mar 7, 2023

Fixes #1232.

$ spin up
Error: Invalid key-value store 'hello'. This version of Spin supports only the 'default' store.

$ spin deploy
Error: Invalid key-value store 'hello'. This version of Spin supports only the 'default' store.

Learn more at https://developer.fermyon.com/cloud/faq

Unlike allowed_http_hosts, this implementation puts the validation logic in the loader, rather than putting it with the key-value component and adding a crate reference. I did this solely to avoid making loader dependent on EVEN MOAR things. It does risk a maintenance burden; if we ever need more sophisticated parsing and validation logic for these IDs, then it's probably worth doing it in the host component and taking the dependency. Happy to do that now if people feel it's better.

@itowlson
Copy link
Contributor Author

itowlson commented Mar 7, 2023

We have integration tests that want to verify the difference between a store that doesn't exist and a store that isn't included in the manifest. CURSE OUR OWN FORESIGHT

@itowlson
Copy link
Contributor Author

itowlson commented Mar 7, 2023

For now, I have taken out the integration test check that opening a store that is permitted but does not exist produces a distinctive error within the guest (compared to opening a store that is nor permitted at all). This feels like it might leave a gap in the test regime but I can't see a way around it. Ideas welcome!

Comment on lines 6 to 8
match key_value_stores {
None => Ok(()),
Some(ids) => match ids.iter().find(|id| *id != ONLY_ALLOWED_KV_STORE) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another option here:

Suggested change
match key_value_stores {
None => Ok(()),
Some(ids) => match ids.iter().find(|id| *id != ONLY_ALLOWED_KV_STORE) {
match key_value_stores.iter().flatten().find(|id| *id != ONLY_ALLOWED_KV_STORE) {

@kate-goldenring
Copy link
Contributor

For now, I have taken out the integration test check that opening a store that is permitted but does not exist produces a distinctive error within the guest (compared to opening a store that is nor permitted at all).

@itowlson I think this is fine. It was verifying the behavior we previously had but didn't necessarily want. Once we add non-default store capability we can add it back. We could add a TODO as a reminder

Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson merged commit 7c90569 into fermyon:main Mar 7, 2023
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 this pull request may close these issues.

Catch invalid key/value store at build time
3 participants