Skip to content
This repository has been archived by the owner on Oct 31, 2023. It is now read-only.

chore: bump wascap #361

Merged
merged 3 commits into from
Jan 10, 2023
Merged

chore: bump wascap #361

merged 3 commits into from
Jan 10, 2023

Conversation

ricochet
Copy link
Contributor

@ricochet ricochet commented Jan 4, 2023

No description provided.

@autodidaddict
Copy link
Member

I'm okay with this if @brooksmtownsend and @mattwilkinsonn are.

@autodidaddict
Copy link
Member

@ricochet - the DCO requirement needs the commits to be signed

for the curious - this change is significant because anything signed by this version of wash/washlib will not be backwards compatible with old versions of the host (the module hash verification check will fail)

@autodidaddict
Copy link
Member

This seems to have broken an integration test. I'm not sure if it's because one of the fixtures is different but there's a claims mismatch

@ricochet
Copy link
Contributor Author

ricochet commented Jan 5, 2023

Ah, this is a good test.

wash claims inspect output is different. The hashes changed (expected), there is now a version on the claim (expected) but two other diffs look suspect:

{
...
        "capabilities": ["HTTP Server", "Logging"],
        "call_alias": "wasmcloud/logger_onedotzero"
 }
{
...
        "capabilities": ["HTTP Server", "wasmcloud:logging"],
         "call_alias": "(Not set)"
}

It seems to have lost the call_alias and the logging claim name is odd. I think I prefer the wasmcloud prefix, but why didn't the HTTP Server change? Digging in...

ricochet added a commit to ricochet/wascap that referenced this pull request Jan 5, 2023
While investigating diffs in wasmCloud/wash#361
I wanted to see assertions for a roundtrip of call_alias
and the human friendly capability name.

NOTE: I found it surprising that the capability names are not
normalized, e.g. if I request "Logging", I get "Logging" back.
If I claim "wasmcloud:builtin:logging", I expect
"wasmcloud:builtin:logging".
ricochet added a commit to ricochet/wascap that referenced this pull request Jan 5, 2023
While investigating diffs in wasmCloud/wash#361
I wanted to see assertions for a roundtrip of call_alias
and the human friendly capability name.

NOTE: I found it surprising that the capability names are not
normalized, e.g. if I request "Logging", I get "Logging" back.
If I claim "wasmcloud:builtin:logging", I expect
"wasmcloud:builtin:logging".

Signed-off-by: Bailey Hayes <behayes2@gmail.com>
ricochet added a commit to ricochet/wascap that referenced this pull request Jan 5, 2023
While investigating diffs in wasmCloud/wash#361
I wanted to see assertions for a roundtrip of call_alias
and the human friendly capability name.

NOTE: I found it surprising that the capability names are not
normalized, e.g. if I request "Logging", I get "Logging" back.
If I claim "wasmcloud:builtin:logging", I expect
"wasmcloud:builtin:logging".

Signed-off-by: Bailey Hayes <behayes2@gmail.com>
@ricochet
Copy link
Contributor Author

ricochet commented Jan 5, 2023

I think there must be history around naming as this claim is for "wasmcloud:logging" and not "wasmcloud:builtin:logging". "Logging" should now == "wasmcloud:builtin:logging" and not "wasmcloud:logging".

This could be avoided if wascap normalized friendly name capabilities or if friendly names were removed entirely.

@autodidaddict
Copy link
Member

Yep we're looking at some scary tissue here from early days when there were a handful of predetermined contract IDs. The upside is this is mostly showing the age in the test and not a big problem with the app itself.

100% agree that we should be consistent in how we treat this

@ricochet
Copy link
Contributor Author

ricochet commented Jan 5, 2023

I think that mostly leaves call_alias as the key change that I can't explain. I added a test over in wascap and locally also tried with the same string as the one here with success.

I manually ran the integration test with a wash before/after wascap update... and yeah call_alias is gone Jim.

@ricochet ricochet closed this Jan 5, 2023
@ricochet ricochet reopened this Jan 5, 2023
@autodidaddict
Copy link
Member

Doing some digging on this. I've verified that doing this manually with the previous/released version of wash works just fine. Looking into some more things

@autodidaddict
Copy link
Member

@ricochet if you bump wascap to 0.9.1 now the double-sign test should pass.

autodidaddict pushed a commit to wasmCloud/wascap that referenced this pull request Jan 6, 2023
While investigating diffs in wasmCloud/wash#361
I wanted to see assertions for a roundtrip of call_alias
and the human friendly capability name.

NOTE: I found it surprising that the capability names are not
normalized, e.g. if I request "Logging", I get "Logging" back.
If I claim "wasmcloud:builtin:logging", I expect
"wasmcloud:builtin:logging".

Signed-off-by: Bailey Hayes <behayes2@gmail.com>

Signed-off-by: Bailey Hayes <behayes2@gmail.com>
@ricochet ricochet force-pushed the bump-wascap branch 2 times, most recently from eb65d5a to 47f2e48 Compare January 6, 2023 16:21
Cargo.toml Outdated Show resolved Hide resolved
autodidaddict
autodidaddict previously approved these changes Jan 6, 2023
This updates to a newer version of wasmparser
which should fix attempting to sign newer wasi modules.

The integration test caught an issue introduced a long
time ago with wascap v0.5.0 and a very old module
signed with that version from way back when.
v0.9.2 of wascap fixes this issue in our integration
tests by correctly removing the old metadata.

Bump wascap - looks small but NOTE:

The hashes computed with v0.9.0 and later of wascap
are not compatible with the hashes signed by prior versions.
As a result, modules signed with older versions of wascap
will not have their module hashes validated
(they'll be ignored).

Once the module has been signed with 0.9.0 or greater,
it will go back to having its module hash verified.

Signed-off-by: Bailey Hayes <behayes2@gmail.com>
autodidaddict
autodidaddict previously approved these changes Jan 9, 2023
connorsmith256
connorsmith256 previously approved these changes Jan 9, 2023
Copy link
Contributor

@connorsmith256 connorsmith256 left a comment

Choose a reason for hiding this comment

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

Woohoo, tests pass now! 🎉

brooksmtownsend
brooksmtownsend previously approved these changes Jan 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants