Modified provider checks to be version independant #193

aish-where-ya · 2023-10-12T21:42:45Z

Feature or Problem

Bug: Providers deployed in a manifest on a link name needed to have a unique image reference associated with it. This allowed for providers with the same reference but different versions to be created on the same link name.

Fix: This PR improves upon this issue by associating an image ref, regardless of the version, with a link name. By ignoring the version, we permit only unique image references into the manifest/lattice.

Related Issues

Release Information

Capabilities with duplicate image refs will be removed.

Testing

manually tested

Built on platform(s)

Tested on platform(s)

Unit Test(s)

Modified test_manifest_validation

Acceptance or Integration

Updated e2e test named e2e_upgrades to check for provider versioning

Manual Verification

manually verified.

Signed-off-by: aish-where-ya <[email protected]>

brooksmtownsend

This does explicitly block people from pushing different providers to the same image repository, but I think that's an okay assertion 🤔

Thoughts @thomastaylor312?

aish-where-ya · 2023-10-13T15:45:27Z

We can always pick the other route and explicitly perform checks against the version. It would involve retrieving items from the linkedef hash map, splitting it into reference and version and performing checks for this specific case where the user is trying to put in an existing provider with a different version.

I'm thinking about cases where the approach in the PR will fail/succeed to catch what the other approach can/cannot. This one is a lesser number of steps that's all.

connorsmith256 · 2023-10-13T22:44:51Z

Does wadm have access to the lattice's metadata KV bucket? If so, wadm should be able to translate from the providers' image ref to its public key, which might be a better test for uniqueness

autodidaddict · 2023-10-13T23:03:11Z

Does wadm have access to the lattice's metadata KV bucket? If so, wadm should be able to translate from the providers' image ref to its public key, which might be a better test for uniqueness

The image ref to public key lookup can be gleaned from making a control interface API request for the claims cache. Giving wadm direct access to buckets like those of the lattice cache can be potentially problematic in some production scenarios, especially multi-tenant and high security.

Signed-off-by: aish-where-ya <[email protected]>

brooksmtownsend

A couple requests for small efficiency improvements 🙂

brooksmtownsend · 2023-10-17T19:57:27Z

src/server/handlers.rs

+                        return;
+                    }
+                };
+                if stored_manifest.name() != name {


I think a comment here would be helpful to indicate that we don't want to compare to other versions of this application since upgrading versions is a valid scenario

brooksmtownsend · 2023-10-17T20:02:16Z

src/server/handlers.rs

+                    }
+                };
+                if stored_manifest.name() != name {
+                    if let Some(deployed_manifest) = stored_manifest.get_version(deployed_version) {


Consider using stored_manifest.get_deployed to simplify this loop

brooksmtownsend · 2023-10-17T20:02:36Z

src/server/handlers.rs

+            if let Some(deployed_version) = &model_summary.deployed_version {
+                let (stored_manifest, _) = match self
+                    .store
+                    .get(account_id, lattice_id, &model_summary.name)
+                    .await
+                {
+                    Ok(Some(m)) => m,
+                    Ok(None) => (StoredManifest::default(), 0),
+                    Err(e) => {
+                        error!(error = %e, "Unable to fetch data");
+                        self.send_error(msg.reply, "Internal storage error".to_string())
+                            .await;
+                        return;
+                    }
+                };


I think if you use the suggestion in the above comment with get_deployed you can get rid of this check altogether 😄

Modifying the check instead of removing since we don't want to perform a fetch operation if there is no currently deployed version.

brooksmtownsend · 2023-10-17T20:05:37Z

src/server/handlers.rs

+                } else {
+                    manifests.get_current()
+                }
+            }
+            None => manifests.get_current(),


If we can't get the version that we requested, we actually want to send an error back to the sender (see below.) It may be worth moving this check up above where you start looking through the manifests so that we can early-failure ack

if !manifests.deploy(req.version) { trace!("Requested version does not exist"); self.send_reply( msg.reply, // NOTE: We are constructing all data here, so this shouldn't fail, but just in // case we unwrap to nothing serde_json::to_vec(&DeployModelResponse { result: DeployResult::Error, message: format!( "Model with the name {name} does not have the specified version to deploy" ), }) .unwrap_or_default(), ) .await; return;

brooksmtownsend · 2023-10-17T20:06:47Z

src/server/handlers.rs

+                let image_ref_split: Vec<&str> = image_name.split(':').collect();
+                if let (Some(&ref_link), Some(&ref_version)) =
+                    (image_ref_split.first(), image_ref_split.last())


This shows up in a place or two, if it makes sense consider a function like

fn parse_image_ref_and_tag(image_name: &str) -> Option<(String, String)>

I'm bad at naming but you get the idea 😄

brooksmtownsend · 2023-10-17T20:07:43Z

src/server/handlers.rs

+                                "Provider {image_name} is currently deployed. Forbidden operation.",
+                            );
+                            self.send_error(
+                                msg.reply,
+                                format!(
+                                "Provider {image_name} is currently deployed. Forbidden operation."


Is there a way here to indicate which model is deployed with the offending provider?

Suggested change

"Provider {image_name} is currently deployed. Forbidden operation.",

);

self.send_error(

msg.reply,

format!(

"Provider {image_name} is currently deployed. Forbidden operation."

"Provider {image_name} is currently deployed with a different version. Forbidden operation.",

);

self.send_error(

msg.reply,

format!(

"Provider {image_name} is currently deployed with a different version. Forbidden operation."

Signed-off-by: aish-where-ya <[email protected]>

Modified provider checks to be version independant

309e523

Signed-off-by: aish-where-ya <[email protected]>

aish-where-ya requested review from autodidaddict, brooksmtownsend, thomastaylor312 and connorsmith256 as code owners October 12, 2023 21:42

Fix test top reflect change

b03840c

Signed-off-by: aish-where-ya <[email protected]>

brooksmtownsend reviewed Oct 13, 2023

View reviewed changes

Added deploy time check for duplicate provider refs

d79b37f

Signed-off-by: aish-where-ya <[email protected]>

aish-where-ya requested a review from brooksmtownsend October 16, 2023 21:12

aish-where-ya added 2 commits October 17, 2023 11:21

Fixed edge cases for version checks

63a129f

Signed-off-by: aish-where-ya <[email protected]>

Added e2e test to check for duplicate provider versions

4b465da

Signed-off-by: aish-where-ya <[email protected]>

brooksmtownsend requested changes Oct 17, 2023

View reviewed changes

aish-where-ya added 2 commits October 17, 2023 17:34

Addressed review comments

58a3be1

Signed-off-by: aish-where-ya <[email protected]>

Minor fix to retrieving staged model

33b4f75

Signed-off-by: aish-where-ya <[email protected]>

brooksmtownsend approved these changes Oct 18, 2023

View reviewed changes

brooksmtownsend merged commit e0f9bc1 into wasmCloud:main Oct 18, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified provider checks to be version independant #193

Modified provider checks to be version independant #193

aish-where-ya commented Oct 12, 2023 •

edited

Loading

brooksmtownsend left a comment

aish-where-ya commented Oct 13, 2023

connorsmith256 commented Oct 13, 2023

autodidaddict commented Oct 13, 2023

brooksmtownsend left a comment

brooksmtownsend Oct 17, 2023

brooksmtownsend Oct 17, 2023

brooksmtownsend Oct 17, 2023

aish-where-ya Oct 17, 2023

brooksmtownsend Oct 17, 2023

brooksmtownsend Oct 17, 2023

brooksmtownsend Oct 17, 2023

Modified provider checks to be version independant #193

Modified provider checks to be version independant #193

Conversation

aish-where-ya commented Oct 12, 2023 • edited Loading

Feature or Problem

Related Issues

Release Information

Consumer Impact

Testing

Unit Test(s)

Acceptance or Integration

Manual Verification

brooksmtownsend left a comment

Choose a reason for hiding this comment

aish-where-ya commented Oct 13, 2023

connorsmith256 commented Oct 13, 2023

autodidaddict commented Oct 13, 2023

brooksmtownsend left a comment

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

aish-where-ya Oct 17, 2023

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

brooksmtownsend Oct 17, 2023

Choose a reason for hiding this comment

aish-where-ya commented Oct 12, 2023 •

edited

Loading