Move node configuration to runtime #917

tiziano88 · 2020-04-29T22:11:35Z

The ApplicationConfiguration object is now more of a directory of
executable wasm modules, and an initial Node configuration.

Subsequent Nodes are created at runtime by passing the serialized
NodeConfiguration protobuf message to node_create directly.

This allows removing an extra indirection from application to the config
of individual nodes, in favour of inlining configs in the Wasm code of
the main node itself.

Also remove parts of the logic around the old ApplicationConfiguration
format from the C++ Oak Runtime, under the assumption that it is going
to be deleted soon.

Ref #688

Checklist

Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
- I have written tests that cover the code changes.
- I have checked that these tests are run by Cloudbuild
- I have updated documentation accordingly.

tiziano88 · 2020-04-29T22:14:31Z

Note that I have not fixed the glue code, and therefore some tests are currently failing.

My intention is to submit this only after we switch to using the Rust oak_loader, and in particular when the gRPC server pseudo-node stops being created in a special way by the Runtime before the actual entrypoint.

tiziano88 · 2020-04-30T11:48:27Z

(but please do review in its current state, and let me know if you have any suggestions on whether I should go ahead and fix oak_glue right away -- maybe it is simpler than I think?)

daviddrysdale

Just on a quick look at the config proto change, my immediate concern is still that this approach removes an important layer of protection from the system.

It looks like this change will mean that an Oak application can create a (say) GrpcClientConfiguration at runtime and then just connect to any gRPC server anywhere, whereas previously there were obvious and easily-checkable constraints imposed by the configuration: if the config didn't include a gRPC/Storage/Roughtime node, then you could be very sure that the running Node didn't do gRPC/Storage/Roughtime.

Of course, this sort of access to the outside world should eventually be policed by the IFC/label system, but (particularly) until that's fully in place I like the belt-and-braces approach of having defense-in-depth: an outer perimeter that places gross restrictions on what an app can do (gRPC:✗ Storage: ✓ Roughtime: ✗), combined with an inner IFC system (do the label flows allow this particular node/channel operation?).

(If I squint hard and talk fast, I can almost spin this in capability terms: in the current system, an app is born with the capabilities to create particular types of pseudo-Nodes, whereas a fully dynamic system is effectively making a global namespace available to the app.)

(Also, apologies if I'm getting the wrong end of the stick, I only took a quick look.)

daviddrysdale · 2020-04-30T11:57:37Z

docs/abi.md

+handle to the read half of a channel identified by arg 4.
+
+The Node configuration is a serialized
+[`NodeConfiguration`](/oak/proto/application.proto) probobuf message.


s/probobuf/protobuf/

oak/proto/application.proto

daviddrysdale · 2020-04-30T12:03:42Z

oak/proto/application.proto

+  //
+  // See https://webassembly.org/docs/binary-encoding/ .
+  //
+  // Entries in this map may be references by instances of WebAssemblyConfiguration


s/references/referenced/

daviddrysdale · 2020-04-30T12:07:11Z

oak/proto/application.proto

+  //
+  // Entries in this map may be references by instances of WebAssemblyConfiguration
+  // objects.
+  map<string, bytes> wasm_modules = 1;


Just checking: are all of our platforms happy with map types in protos?

Yes maps are just syntactic sugar for a repeated message generated by the proto compiler, they have to dedicated wire representation.

https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility

daviddrysdale · 2020-04-30T12:21:18Z

oak/proto/application.proto

-  // The name of an exported Web Assembly function in the initial Node to
-  // be used as the Node's main entrypoint.
-  string initial_entrypoint_name = 3;
+  // Map from Wasm module names to their bytecode representation.


Might be nice to make it clear that the only third-party code that gets run as part of an Oak application has to be present as an entry in this map.

tiziano88 · 2020-04-30T14:29:33Z

Thanks @daviddrysdale , yes I agree that this is indeed removing one layer of protection, but I think this is a layer that is not Oak's goal, and even if it is there, cannot really be leveraged in a meaningful way in its current form.

At least, I don't see a path forward in which this extra forced declaration (having the application declare upfront the kind of nodes it uses locally) could be made useful to clients. There is no mechanism for clients to inspect the configuration of a running application; even if there were, the application configuration is in a way an imprecise upper bound on the kind of capabilities that the application may rely on, and on the other hand it is not even good at that, because it does not enforce transitivity (e.g. if the application has a single egress gRPC client node, it may be tempting to conclude that it does not have storage, but the other side of the gRPC connection may actually be an application that has storage, etc.) this is the reason why we chose DIFC, which allows to reason about constraints in a distributed system.

Even if we wanted to limit the functionality of an application to have some reassurance against local untrusted code, I think the current mechanism couples configuration and constraints too much; if we wanted to disable gRPC connectivity entirely, I think an explicit top-level bool in the application configuration may be a more intentional way of expressing that. The problem again is that it would still not really propagate across applications, or to storage, or anything else.

This is similar to the discussion on read / write rights on handles vs relying on information flow: the point of Oak is not to be a pure capability-based OS, it is to enforce policies in distributed systems, and sometimes being a pure capability system is at odd with that, or at the very least it does not add value (except perhaps some very localized value that does not extend to the rest of the system). But it's also certainly not impossible that some theory can unify both approaches at some point.

In the end, the way I see these things resolved is that effectively, client labels are capabilities of the kind of nodes that may manipulate the data; for instance, in order to allow storage, data would need to be labelled with a hypothetical storage pseudo-node secrecy label, which would allow such data to flow there (and not to other nodes); it is as if the data comes along with the capability to unlock that particular node type. It is a bit more nuanced than that, but perhaps this captures your intuition of some of this being in the spirit of capability-oriented systems.

WDYT?

daviddrysdale

Defence-in-depth concerns aside, the code changes do fall out quite nicely…

daviddrysdale · 2020-04-30T14:05:29Z

examples/aggregator/module/rust/src/lib.rs

+            config_type: Some(ConfigType::GrpcClientConfig(GrpcClientConfiguration {
+                address: "127.0.0.1:8888".to_string(),
+            })),
+        })


Having helpers in the SDK for configuring standard pseudo-Node types would be helpful across all of the examples, e.g. so this could look something like:

match oak::grpc::client::Client::new(&oak::node_config::grpc_client("127.0.0.1:888")) .map(AggregatorClient) {

Nice idea, done.

daviddrysdale · 2020-04-30T14:06:29Z

examples/hello_world/config/BUILD

@@ -45,5 +45,5 @@ serialized_config(
        "app": "//:target/wasm32-unknown-unknown/release/hello_world.wasm",
        "translator": "//:target/wasm32-unknown-unknown/release/translator.wasm",
    },
-    textproto = ":config_translator.textproto",
+    textproto = ":config.textproto",


Why the change here?

I deleted the other config, since now they are identical. I can keep both if you prefer though, let me know.

daviddrysdale · 2020-04-30T14:07:33Z

examples/hello_world/config/config_translator.textproto

@@ -1,19 +0,0 @@
-node_configs {


Why is this removed? It seems to remove the ability to run hello_world with a translator module configured.

daviddrysdale · 2020-04-30T14:21:37Z

oak/server/rust/oak_runtime/src/config.rs

-                Some(ConfigType::WasmConfig(WebAssemblyConfiguration { module_bytes, .. })) => {
-                    load_wasm(&module_bytes).map_err(|e| {
-                        error!("Error loading Wasm module: {}", e);
-                        OakStatus::ErrInvalidArgs


I like not having the data structures that mirror the protobuf config structure (and I think this would fix #755 along the way); are the validity checks (check_port, load_wasm) now done at a later point?

[Edit: yes they are]

Affirmative.

daviddrysdale · 2020-04-30T14:24:27Z

oak/server/rust/oak_runtime/src/node/mod.rs

+            initial_reader,
+        ))),
+        Some(ConfigType::RoughtimeClientConfig(_config)) => Ok(Box::new(
+            external::PseudoNode::new(name, runtime, initial_reader),


Yeah, this will need to pass a copy of the node config over the Rust->C++ FFI boundary. As you say, probably easiest to block this until main() flips into Rust and oak_glue gets reworked.

daviddrysdale · 2020-04-30T14:26:29Z

oak/server/rust/oak_runtime/src/node/wasm/mod.rs

@@ -140,65 +147,41 @@ impl WasmInterface {
        (interface, handle)
    }

-    /// Corresponds to the host ABI function [`node_create: (usize, usize, usize, usize,
-    /// u64) -> u32`](oak_abi::node_create).
+    /// Corresponds to the host ABI function [`node_create: (usize, usize, usize, usize, u64) ->


Comment now out of date.

daviddrysdale · 2020-04-30T14:27:49Z

oak/server/rust/oak_abi/src/lib.rs

    pub fn node_create(
        config_buf: *const u8,
        config_len: usize,
-        entrypoint_buf: *const u8,
-        entrypoint_len: usize,


Also need to update sdk/rust/oak/src/stubs.rs

tiziano88

Thanks @daviddrysdale

tiziano88 · 2020-04-30T14:45:45Z

docs/abi.md

+handle to the read half of a channel identified by arg 4.
+
+The Node configuration is a serialized
+[`NodeConfiguration`](/oak/proto/application.proto) probobuf message.


tiziano88 · 2020-04-30T16:39:33Z

examples/aggregator/module/rust/src/lib.rs

+            config_type: Some(ConfigType::GrpcClientConfig(GrpcClientConfiguration {
+                address: "127.0.0.1:8888".to_string(),
+            })),
+        })


Nice idea, done.

tiziano88 · 2020-04-30T16:45:06Z

examples/hello_world/config/BUILD

@@ -45,5 +45,5 @@ serialized_config(
        "app": "//:target/wasm32-unknown-unknown/release/hello_world.wasm",
        "translator": "//:target/wasm32-unknown-unknown/release/translator.wasm",
    },
-    textproto = ":config_translator.textproto",
+    textproto = ":config.textproto",


I deleted the other config, since now they are identical. I can keep both if you prefer though, let me know.

tiziano88 · 2020-04-30T16:47:59Z

oak/proto/application.proto

-  // The name of an exported Web Assembly function in the initial Node to
-  // be used as the Node's main entrypoint.
-  string initial_entrypoint_name = 3;
+  // Map from Wasm module names to their bytecode representation.


tiziano88 · 2020-04-30T16:48:04Z

oak/proto/application.proto

+  //
+  // See https://webassembly.org/docs/binary-encoding/ .
+  //
+  // Entries in this map may be references by instances of WebAssemblyConfiguration


tiziano88 · 2020-04-30T16:48:55Z

oak/proto/application.proto

+  //
+  // Entries in this map may be references by instances of WebAssemblyConfiguration
+  // objects.
+  map<string, bytes> wasm_modules = 1;


Yes maps are just syntactic sugar for a repeated message generated by the proto compiler, they have to dedicated wire representation.

https://developers.google.com/protocol-buffers/docs/proto3#backwards-compatibility

oak/proto/application.proto

tiziano88 · 2020-04-30T16:50:22Z

oak/server/rust/oak_abi/src/lib.rs

    pub fn node_create(
        config_buf: *const u8,
        config_len: usize,
-        entrypoint_buf: *const u8,
-        entrypoint_len: usize,


tiziano88 · 2020-04-30T16:50:48Z

oak/server/rust/oak_runtime/src/config.rs

-                Some(ConfigType::WasmConfig(WebAssemblyConfiguration { module_bytes, .. })) => {
-                    load_wasm(&module_bytes).map_err(|e| {
-                        error!("Error loading Wasm module: {}", e);
-                        OakStatus::ErrInvalidArgs


Affirmative.

tiziano88 · 2020-04-30T16:51:59Z

oak/server/rust/oak_runtime/src/node/wasm/mod.rs

@@ -140,65 +147,41 @@ impl WasmInterface {
        (interface, handle)
    }

-    /// Corresponds to the host ABI function [`node_create: (usize, usize, usize, usize,
-    /// u64) -> u32`](oak_abi::node_create).
+    /// Corresponds to the host ABI function [`node_create: (usize, usize, usize, usize, u64) ->


ipetr0v · 2020-05-01T11:56:52Z

examples/abitest/config/config.textproto

-  }
-}
-node_configs {
-  name: "grpc-client"


Are these configs deleted because they are not yet supported by Rust Runtime or they are no longer needed for abitest?

They are now in the code of abitest module itself!

ipetr0v · 2020-05-01T12:48:12Z

examples/aggregator/module/rust/src/lib.rs

@@ -92,7 +92,9 @@ impl AggregatorNode {
            bucket, svec
        );

-        match oak::grpc::client::Client::new("grpc-client", "").map(AggregatorClient) {
+        match oak::grpc::client::Client::new(&oak::node_config::grpc_client("127.0.0.1:8888"))


Since we are passing an address as a function argument, does it mean that we also need to have TLS keys baked into the code?

I guess we will have root CAs pinned in the oak_loader binary, yes. I am not sure how this works currently though.

ipetr0v · 2020-05-01T12:54:44Z

oak/common/app_config.cc


 }  // namespace

 std::unique_ptr<ApplicationConfiguration> DefaultConfig(const std::string& module_bytes) {
  auto config = absl::make_unique<ApplicationConfiguration>();
  config->set_grpc_port(kDefaultGrpcPort);
+  (*config->mutable_wasm_modules())[kAppWasmModuleName] = module_bytes;


Does kAppWasmModuleName always exist in this map?

No, my understanding is that this will create the entry if it's not there (it seems to work so I guess that's fine?)

Let me know if I'm doing it wrong though

ipetr0v · 2020-05-01T13:12:33Z

oak/common/app_config_serializer.cc

    }
+    (*config->mutable_wasm_modules())[module_name] = std::move(module_bytes);


Can [module_name] return nullptr?

Hopefully it should be obvious if it does, anyways the size of module_info was checked above already. Or are you suggesting something else?

ipetr0v · 2020-05-01T13:21:45Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

-    ) -> Self {
-        Self {
-            config_name: config_name.to_string(),
+    ) -> Result<Self, ConfigurationError> {


Is new supposed to return Result instead of just Self.
Should this function be called create instead? WDYT?

I am also not quite sure about naming, but I think given the return type is obvious, it should be fine either way.

ipetr0v · 2020-05-01T13:21:52Z

oak/server/rust/oak_runtime/src/node/grpc_server.rs

 /// Struct that represents a gRPC server pseudo-Node.
 ///
 /// For each gRPC request from a client, gRPC server pseudo-Node creates a pair of temporary
 /// channels (to write a request to and to read a response from) and passes corresponding handles to
 /// the [`GrpcServerNode::channel_writer`].
+#[derive(Clone)]


Why do we need Clone ?

That's a question for you I guess :) I just removed the manual implementation and replaced it with derive.

ipetr0v · 2020-05-01T13:23:09Z

oak/server/rust/oak_runtime/src/node/logger.rs


 pub struct LogNode {
-    config_name: String,
+    name: String,


Should this field be called display_name (just like in GrpcServerNode and WasmNode) ?

tiziano88 · 2020-05-22T13:17:00Z

@ipetr0v @daviddrysdale this is now ready for review.

Note that I removed parts of the logic around the old ApplicationConfiguration
format from the C++ Oak Runtime, under the assumption that it is going
to be deleted soon.

ipetr0v · 2020-05-26T11:23:24Z

examples/abitest/tests/Cargo.toml

@@ -15,6 +15,9 @@ log = "*"
 oak = "=0.1.0"
 oak_abi = "=0.1.0"
 prost = "*"
+# Using an old version that is supported by `cargo-raze`:
+# https://github.com/google/cargo-raze/issues/41#issuecomment-592274128
+tonic = { version = "=0.1.1", features = ["tls"] }


I think after #1016 we can use the latest version of tonic

daviddrysdale

The code changes LGTM and the resulting API is definitely neater and more flexible.

However, I still have the fundamental concern that this extra flexibility removes an outer security bound on the system as a whole, leaving the IFC system (which isn't implemented yet) as the only intrinsic security boundary. Maybe re-awaken the earlier discussion on Slack (which seemed to wind down without really reaching a conclusion/consensus)?

daviddrysdale · 2020-05-26T11:56:03Z

docs/programming-oak.md

 ```
 <!-- prettier-ignore-end -->

-The `module_bytes: "<bytes>"` means that this value will be filled with
-WebAssembly module bytes after serialization using the
+The `wasm_modules` field (not shown above because empty in the template


"because it is empty"

Done. (I thought my version was grammatically correct though, was it not?)

daviddrysdale · 2020-05-26T12:00:12Z

docs/programming-oak.md

+            grpc_client_root_tls_certificate: Some(Certificate::from_pem("invalid-cert")),
+        },
+    )
+    .expect("unable to configure runtime with test wasm");
 ```


I think there are other docs that need updating for this API change:

concepts.md section "Oak Application": still talks about "allowed contents of the Nodes"

"Running an Oak Application" section earlier in this doc: still talks about the config including the gRPC port.

daviddrysdale · 2020-05-26T12:05:51Z

oak/proto/application.proto

@@ -22,27 +22,33 @@ package oak.application;
 //
 // An Oak Application is built from a collection of interconnected Nodes,
 // each of which is running the code described by an entry in this
-// configuration.  These Nodes are created dynamically at runtime, with
+// configuration. These Nodes are created dynamically at runtime, with


The first sentence of this comment is no longer accurate.

daviddrysdale · 2020-05-26T12:34:26Z

sdk/rust/oak_tests/Cargo.toml

 lazy_static = "*"
 log = { version = "*", features = ["std"] }
 oak = "=0.1.0"
 oak_abi = "=0.1.0"
 oak_runtime = { version = "=0.1.0", features = ["test_build"] }
 prost = "*"
 rand = { version = "*" }
-cargo_metadata = "*"
+# Using an old version that is supported by `cargo-raze`:
+# https://github.com/google/cargo-raze/issues/41#issuecomment-592274128


Up-rev here too?

daviddrysdale · 2020-05-26T12:37:05Z

sdk/rust/oak_tests/src/lib.rs

+            // Some of the tests require a gRPC client, so we populate the required certificate with
+            // an invalid value here, even though it will still fail when instantiating the actual
+            // gRPC client.
+            grpc_client_root_tls_certificate: Some(Certificate::from_pem("invalid-cert")),


Why doesn't None work here, btw?

Because it would cause it to fail too early, for some of the tests (they would not be able to create the gRPC client at all otherwise).

daviddrysdale · 2020-05-26T12:55:04Z

oak/server/rust/oak_runtime/src/node/mod.rs

@@ -32,8 +29,8 @@ mod storage;
 mod wasm;

 /// Trait encapsulating execution of a Node or pseudo-Node.
-pub trait Node {
-    /// Execute the Node, using the provided `Runtime` reference and initial handle. The method
+pub trait Node: Send {


Ouf of interest, why does this now need Send?

It was already needed, but it was specified as an additional type bound on the actual node_create function (which was quite confusing), I just moved it here instead.

daviddrysdale · 2020-05-26T12:55:33Z

oak/server/rust/oak_runtime/src/node/mod.rs

@@ -108,112 +69,65 @@ impl std::fmt::Display for ConfigurationError {
                write!(f, "Failed to parse an address: {}", e)
            }
            ConfigurationError::IncorrectPort => write!(f, "Incorrect port (must be > 1023)"),
+            ConfigurationError::IncorrectURI => write!(f, "Incorrect URI"),


nit: incorrect how?

It is only logged at the point where the parsing happens. I start thinking that we should probably just either log or propagate all errors, there is not much value in this halfway split. Created #1027 to track.

The ApplicationConfiguration object is now more of a directory of executable wasm modules, and an initial Node configuration. Subsequent Nodes are created at runtime by passing the serialized NodeConfiguration protobuf message to `node_create` directly. This allows removing an extra indirection from application to the config of individual nodes, in favour of inlining configs in the Wasm code of the main node itself. Also remove parts of the logic around the old ApplicationConfiguration format from the C++ Oak Runtime, under the assumption that it is going to be deleted soon. Ref project-oak#688

github-actions · 2020-05-26T19:46:08Z

Reproducibility index:

fad788689ce720de602a0ac0a283841e18ed732dc8bd3a49b248371ef679091d  ./target/wasm32-unknown-unknown/release/abitest_0_frontend.wasm
309920b1a713aeccef1ef2d86b6dd7fcec0f546b64481b9d0249c0a50025b3da  ./target/wasm32-unknown-unknown/release/abitest_1_backend.wasm
bc5069ef4a2ee4290dc9c27bf76121b189a4688eceb3d21851bc3301af6cfc93  ./target/wasm32-unknown-unknown/release/aggregator.wasm
8d26c5c254734431b3d9989ed6cd35098d29bc4030ab06acf9cb778f7ea19466  ./target/wasm32-unknown-unknown/release/chat.wasm
66bfa58c5d81ed3e5789fbd8e81a9dd1010fb3493898c0cf74e300892f54d4cb  ./target/wasm32-unknown-unknown/release/hello_world.wasm
b0fd14f5f3bf92e1e3f87f81673c75231891fd94844423ca7bd1406ac0b67a77  ./target/wasm32-unknown-unknown/release/machine_learning.wasm
56dc8b22013c1407d54796970d68c679e9527c796e983facce58716811c94567  ./target/wasm32-unknown-unknown/release/private_set_intersection.wasm
63b2e0cc081afc32b832270b7b51dde15283e3359f25b6405b03beca65814ae5  ./target/wasm32-unknown-unknown/release/running_average.wasm
27d2e5e653229e89fea8610c61434957e999e409a995528fd67a7679618c3e8d  ./target/wasm32-unknown-unknown/release/translator.wasm
d16ebb2b095c789cd0a9e249627637c82a6d60b857532ff0eb0c6455a9a6a857  ./target/x86_64-unknown-linux-musl/release/oak_loader

tiziano88 added the blocked This pull request is blocked by another pull request or issue label Apr 29, 2020

tiziano88 requested review from daviddrysdale and ipetr0v April 29, 2020 22:11

googlebot added the cla: yes label Apr 29, 2020

daviddrysdale reviewed Apr 30, 2020

View reviewed changes

tiziano88 force-pushed the tzn_node_config_proto branch from b826bf9 to ff8a731 Compare April 30, 2020 16:52

tiziano88 commented Apr 30, 2020

View reviewed changes

tiziano88 mentioned this pull request Apr 30, 2020

Refactor handle processing in Rust runtime #894

Merged

6 tasks

ipetr0v reviewed May 1, 2020

View reviewed changes

tiziano88 force-pushed the tzn_node_config_proto branch from ff8a731 to 9981b0f Compare May 1, 2020 15:55

tiziano88 mentioned this pull request May 6, 2020

Pass TLS credentials to gRPC server pseudo-node without modifying the ApplicationConfiguration #961

Closed

tiziano88 force-pushed the tzn_node_config_proto branch 3 times, most recently from cf344dc to a5fc54e Compare May 12, 2020 22:51

tiziano88 force-pushed the tzn_node_config_proto branch 2 times, most recently from ce3cbad to 974c3ba Compare May 18, 2020 21:52

tiziano88 mentioned this pull request May 19, 2020

Introduce config map format and parsing logic #1004

Merged

3 tasks

tiziano88 force-pushed the tzn_node_config_proto branch 8 times, most recently from 827157c to 9fd3153 Compare May 22, 2020 13:08

tiziano88 removed the blocked This pull request is blocked by another pull request or issue label May 22, 2020

tiziano88 requested review from daviddrysdale and ipetr0v May 22, 2020 13:08

ipetr0v approved these changes May 26, 2020

View reviewed changes

tiziano88 force-pushed the tzn_node_config_proto branch 4 times, most recently from 1926457 to faf5dac Compare May 26, 2020 14:30

daviddrysdale reviewed May 26, 2020

View reviewed changes

tiziano88 force-pushed the tzn_node_config_proto branch from faf5dac to b7cba0d Compare May 26, 2020 16:53

tiziano88 mentioned this pull request May 26, 2020

defense-in-depth configuration of oak_loader #1029

Closed

tiziano88 force-pushed the tzn_node_config_proto branch 2 times, most recently from 7d4ab4a to ee7f1f3 Compare May 26, 2020 18:19

tiziano88 force-pushed the tzn_node_config_proto branch from ee7f1f3 to 045dc31 Compare May 26, 2020 18:45

tiziano88 merged commit 3463024 into project-oak:master May 26, 2020

tiziano88 deleted the tzn_node_config_proto branch May 26, 2020 19:15

		}
		(*config->mutable_wasm_modules())[module_name] = std::move(module_bytes);

Move node configuration to runtime #917

Move node configuration to runtime #917

Conversation

tiziano88 commented Apr 29, 2020 • edited Loading

Checklist

tiziano88 commented Apr 29, 2020

tiziano88 commented Apr 30, 2020

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 commented Apr 30, 2020

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiziano88 commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddrysdale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 26, 2020

tiziano88 commented Apr 29, 2020 •

edited

Loading