Make the CoreClient really dumb

[hug-dev]

This is just an issue to think about it. I am sorry to not have mentionned it earlier in a PR.

I think the CoreClient should not contain any abstraction, that is any structure that is not created in the interface or any logic more than executing the operation.

I propose then the following changes:

1. put a provider_id field inside the CoreClient (ProviderID type), which is the exact number used to send the operation on the wire protocol. Add setters and getters for that field. Use that field when sending operation and not the Provider parameter of the methods.
2. do not restrict the core operations to be executed only on the core providers: I think that the "intelligence" needed to know which operation is executable on which provider should not be part of the core client but at a higher level. It should be possible to send operations with the CoreClient that are not supported. It should be possible to do anything with the CoreClient
3. move the Provider structure out of the CoreClient. I think this structure is our first abstraction over the core client and should be in its own module, like providers.rs. I think that the logic needed to retrieve the provider ID of a known provider should not be part of the CoreClient

CC @ionut-arm what do you think?

[ionut-arm]

I think the CoreClient should not contain any abstraction, that is any structure that is not created in the interface or any logic more than executing the operation.

Why?

[hug-dev]

I think, on a conceptual level, it should be the base layer on which any abstraction is built: it should permit anything that the operation/wire protocol permit.
Adding abstraction, logic or intelligence in the CoreClient imply that we are making choices, restricting the possible set of things it is possible to do with it. If we do that, we might restrict what layers based on the core client might be able to do, or we might duplicate work that higher levels should do.

[ionut-arm]

It should be possible to do anything with the CoreClient

I think, on a conceptual level, it should be the base layer on which any abstraction is built: it should permit anything that the operation/wire protocol permit.

No, it shouldn't... Just because it's the "core" client doesn't mean we have to expose "unsafe" operations or expect the users to know and implement everything, otherwise why not just leave them with the interface - or better yet, with the wire specification and protobuf files. Then they're free to implement everything themselves and can send whatever they like. The whole point of having a client is to provide some sort of added value - not just a glorified wrapper - to help you use the API.

It should be possible to send operations with the CoreClient that are not supported.

Indeed, and hence why all the algorithms and parameters are free to be used, even if only one is actually supported, but there's obviously a plan to support those in the future. In the Ping case, for example, there is literally no reason to support sending it to any provider since that will never be a thing...

Adding abstraction, logic or intelligence in the CoreClient imply that we are making choices, restricting the possible set of things it is possible to do with it. If we do that, we might restrict what layers based on the core client might be able to do, or we might duplicate work that higher levels should do.

Any code is some added logic/intelligence, that's why we write it. We are restricting some things, indeed, but only because it makes no sense to expose them above this layer. If you look at all the operations, they're simply wrapping up their inputs into the interface-structs for the specific operation. The logic added is for:

• preventing operations that only make sense for the core provider to only be sent to it
• identifying providers in a generic way, not just through an ID that, as you've said, is probably going to be generated at runtime in the future
  If those aren't core pieces of functionality, I don't really know what is...

If you want more flexibility, I can make the operation and request handlers also public and users can use that, instead, since that's what you're advocating for - being able to send any NativeOperation or even any Request through the IPC. But those are not Parsec clients because they don't help you in any way with using the API.

[ionut-arm]

From the doc overview:

A key aim of Parsec is to evolve an ecosystem of developer-friendly client libraries in multiple programming languages.
...
But Parsec's focus on developer ergonomics goes further than this. Parsec's client interface is filled with conveniences to eliminate complexity unless complexity is required.

I don't see why the complexity would be required here, apart from (maybe) a purity concern.

[hug-dev]

We are having this discussion because the design of the CoreClient was not clearly defined at start, we should have really discussed this before 😅

I guess we can discuss our ideas of what a CoreClient is, what are his goals and what he should do and hopefully reach an agreement.

Some of my thoughts of the goals of the CoreClient:

• the CoreClient is not made to be a user-facing Rust client. It is an abstraction over the operation client which is itself an abstraction over the request client. It leaves as much freedom as those two other clients but using abstraction that are available in the interface, and are safer to use.
• as the highest-level client of the ones that give you full freedom (request, operation and client ones), it is the client of choice to implement any higher level client and avoid duplication of code. The higher-level clients I am talking about are (the ones we know of): high-level user-facing Rust clients, test client and Mbed Crypto SE driver.
• the choice of any abstraction that is not in the Parsec interface should be left to higher-level clients and not the Rust client

---

Now to respond to your specific points.

The whole point of having a client is to provide some sort of added value - not just a glorified wrapper - to help you use the API.

A CoreClient I described would definitely provide added value and safe operations. It would allow to call any operation on any provider with all parameters. It should allow any combination of those three things as they are defined on the interface, it will still be safe.

In the Ping case, for example, there is literally no reason to support sending it to any provider since that will never be a thing...

I agree that, for us developpers, it does not make sense to call Core operations on any providers. But I don't think the core client should make the decision on what operation can be called on which provider. I see a few reasons to this:

• it is not consistent to the fact that Crypto operations can be called on the core provider
• which operation can be called on which provider should be the result of operation discovery of a high-level client, in an automated step. For me that includes the core operations as well, or at least, should include them for consistency
• not permitting this would prevent testing that operations not supported on some providers return PsaErrorNotSupported

The logic added is for:

I totally agree that this logic makes sense and should be implemented. But I do not think it should be implemented in the Core client but at level higher. Carefully identifying and separating abstractions in layers will allow us to design them better and separately from a Core client that should not really change...
For your two points, I would say that a higher level should do that, for example:

• automatically calling ListProviders to get all providers and map their known uuid to the provider ID to use. Then for each of them call ListOpcodes to check which operation their support. I think that should be the canonical way of determining which provider supports which operation
• we do not really know at that stage how the provider ID is going to be generated and what structure in the CoreClient would make most sense to represent it and use it. Having it separated from the core client is a bit safer for a development point of view and would prevent us to change too much things there...

[ionut-arm]

the CoreClient is not made to be a user-facing Rust client.

I think most of the misunderstanding here stems from the fact that I see the CoreClient as something meant to be public and used by the more specialised Parsec users in the future and you see it as a helper towards something that should be public (and used by said developers).

@paulhowardarm - could you please clarify this for us? Is the plan for this initial version of the client to simply expose a thin wrapper that we can use for building the SE driver, or is it supposed to be the proper low-level Parsec client?

[paulhowardarm]

It feels to me like we have some distinct questions tangling together here:-

• Should the CoreClient be a closure over a single server-side provider, such that all operations are routed to that same provider rather than being specified on a per-method basis? (Therefore, if you want to talk to two providers, you need two CoreClient instances).
• What level of abstraction should CoreClient operate at?
• Is the CoreClient intended for "public" consumption (ie. would developers ever use it directly, or is it only a hidden layer)?

These questions are somewhat orthogonal, although related.

We also have a slightly unfortunate terminology clash on "Core", which hadn't occurred to me until I read this thread. The "core" of CoreClient has a different meaning from the "core" in core provider. I wonder if we should address that before it becomes too entrenched.

The dispute here may stem from the fact that we're being a bit too literal about our stated design goals and how they should translate to the code that we write.

Our design goals are that client libraries should have a core layer and a convenience layer. The core layer is definitely intended for developer consumption. But the only design goal is that it allows developers to access all Parsec operations with total control over every parameter. So developers would only use this layer when they need to break out of what we consider to be typical patterns of use. The convenience layer is really aimed at supporting common use cases with minimal code. Those are the design principles. These principles should guide us, but we don't have to directly translate them to code. There is no rule that says we can only have one low-level and one high-level notion of a client in the actual source code. Indeed, look at any typical middleware system, and you'll find that they tend to be more "onion skin" in structure, with layers of gradually-increasing abstraction.

Taking the onion skin view, I think that there is value in having a layer that functions as per Hugues's suggestion, where the client class is very dumb and allows any legal wire message to be formed. But I think there's also value in Ionut's view of a layer that offers some protection against operations that will inevitably fail.

We should also remember that the current focus is on supporting an SE driver, which means there is no immediate requirement for any of this Rust client to be intended for direct developer consumption.

So I'm inclined to take the view that CoreClient is actually neither of the stated design layers. I think it's thinner and lower-level than even the low-level vision of what the Rust client should ultimately look like. I'm also slightly worried about the term "core" as mentioned above. At the risk of having things thrown at me, can we think of a better name? A change of name might also help to illuminate and resolve the disagreement here. Suppose it was WireClient for instance. Would that help to characterise its role?

There's then the fairly separate question of whether the client object (whatever we call it) should be a closure over the provider ID. I think that's largely a matter of aesthetics, but it would feel cleaner to me if it was a closure. I think I agree with Hugues that this layer should operate purely in terms of provider IDs that are fed in by the caller. I'm not keen on having the symbolic enumeration of providers here, because it incurs a maintenance cost when new providers are added.

Not sure how much this helps, but I don't want to wade in and force decisions on how to structure the code. I suppose what I'm really saying is that we can build as many client layers as we like without invalidating the written design (which is what I mainly care about). We have lots of freedom here to build up the system in whatever layers feel right to us.

[ionut-arm]

Paul, I think there's some misunderstanding over the "closure over provider" of the implementation. My suggestion was never that "all operations are routed to that same provider rather than being specified on a per-method basis" - I was only restricting the operations to be calleable for the providers that can supported them (as per the specification - e.g. ping only works for core provider).

If our intent is to expose gradually increasing layers of abstraction, then we can do that right now by simply making more of the structures available public. We'd have:

• One layer that takes in a Request and sends it over the IPC
• One layer that takes in a NativeOperation and a provider, converts the operation to a request and sends it
• One layer that splits up into a method for each supported operation and allows all parameters to be passed

I'm still very much confused about one thing, though - for the third layer I mention above, where we offer an explicit interface for each operation, why would we behave differently around restrictions placed in the specification compared with the other layers. For the first and second layer we strictly limit what can be sent across the wire. For example, this is how a RequestHeader is defined:

pub struct RequestHeader {
    pub version_maj: u8,
    pub version_min: u8,
    pub provider: ProviderID,
    pub session: u64,
    pub content_type: BodyType,
    pub accept_type: BodyType,
    pub auth_type: AuthType,
    pub opcode: Opcode,
}

Notice that, despite all fields being defined as integers in the spec, we impose limits on what can be sent because it's desirable to prevent misuse that has no tangible benefits (e.g. if there are only a limited set of BodyTypes supported, there's no reason to allow any other number in that field).

The second layer does the same thing for the RequestBody - you pass a Rust native object, and the correct protobuf-converted request body is sent over the wire.

So my question is - do we want each layer to follow the restrictions stated in the spec for its level or not? If yes, then the third layer in the list above should include restrictions like "ping can only be sent to the core provider" or "crypto operations cannot be sent to the core provider" (thanks Hugues for reminding me of that!). If no, then we have to completely change what our interface exposes, because it places some limits that shouldn't be there.

[ionut-arm]

Oh, nevermind about the first paragraph, I think I misunderstood what you were saying!

[hug-dev]

Therefore, if you want to talk to two providers, you need two CoreClient instances

My first thought was that you would have the Provider ID as a field of the CoreClient struct, all requests would use the value of that field in their header but that field could be modified during execution with a setter by the client.

[paulhowardarm]

I completely agree with the validations and restrictions that the existing layers are using. The question about ping is marginally less clear-cut, because I don't think we actually have written specification anywhere to say that only the core provider can support this opcode. If you take the spec literally, I think any provider can support any subset of the published opcodes. Of course there are heaps of pragmatics over what makes sense, but that's not quite the same as enforcing specification.

To be honest, I'm ambivalent on that point. I can see arguments either way. For instance, I can imagine writing a test where I query the supported opcodes of a provider, and then follow that up with a deliberate attempt to send an unsupported opcode to that provider, and check that I actually get back the correct NotSupported status as opposed to a crash or other kind of error.

Again, I'm anxious not to wade in with heavy boots and say it has to be one way or another.

As a thought experiment, I'll ask a different question. When we come to write the SE driver, what kind of internal Rust layer will we want to be coding against, such that the code will be reasonably elegant and not too verbose or repetitive. Is CoreClient giving us a useful level of pre-rolled boilerplate as an enabler for the SE driver? If the answer is "yes", then I think it's doing its job. I wouldn't want to get too hamstrung by concerns over how those layers might then be exposed later.

[paulhowardarm]

On the subject of mutable fields in CoreClient - what's the concurrency story here? Are we expecting that one instance of the client is used only by one thread?

[ionut-arm]

I completely agree with the validations and restrictions that the existing layers are using. The question about ping is marginally less clear-cut, because I don't think we actually have written specification anywhere to say that only the core provider can support this opcode. If you take the spec literally, I think any provider can support any subset of the published opcodes. Of course there are heaps of pragmatics over what makes sense, but that's not quite the same as enforcing specification.

https://parallaxsecond.github.io/parsec-book/parsec_client/operations/index.html#core-operations

From the API overview:

This is done by first referencing the core provider. The core provider is the only provider that is guaranteed to be available in any deployment of the service. It has a provider identifier of zero (the only reserved value for provider identifiers). The core provider is special in that it doesn't implement any security or cryptographic operations. The core provider is used to represent the service as a whole. The operations of the core provider can be used to gather information about the health and configuration of the service. It can be used to ping the service to check whether it is responsive, and to check the highest version of the wire protocol that it supports. The core provider can also be used to get information about the cryptographic providers, their characteristics and their 8-bit identifier values. Based on this information, the client can determine which provider is best suited to its requirements. It can then use the integer identifier of that provider to make API requests.

[hug-dev]

On the subject of mutable fields in CoreClient - what's the concurrency story here? Are we expecting that one instance of the client is used only by one thread?

If an instance of the client is shared amoung multiple threads, then I believe Rust ownership rules will force you to wrap it in Arc<Mutex<Coreclient>> before sharing it, and you would have to lock/unlock before modifying the field.