azure-rest-api-specs icon indicating copy to clipboard operation
azure-rest-api-specs copied to clipboard

[Azure Confidential Computing Ledger] API Review

Open azure-sdk opened this issue 2 years ago • 2 comments

New API Review meeting has been requested.

Service Name: Azure Confidential Computing Ledger Review Created By: Andrea Piccione Review Date: 1/4/2023 9:00 AM PT PR: https://github.com/Azure/azure-rest-api-specs/pull/21659 Hero Scenarios Link: Not Provided Core Concepts Doc Link: Not Provided

Description: See PR description: https://github.com/Azure/azure-rest-api-specs/pull/21659#issue-1464593680

Detailed meeting information and documents provided can be accessed here For more information that will help prepare you for this review, the requirements, and office hours, visit the documentation here

azure-sdk avatar Dec 07 '22 14:12 azure-sdk

Meeting updated by Andrea Piccione

Service Name: Azure Confidential Computing Ledger Review Created By: Andrea Piccione Review Date: 1/4/2023 5:00 PM PT PR: https://github.com/Azure/azure-rest-api-specs/pull/21659 Hero Scenarios Link: here Core Concepts Doc Link: here

Description: See PR description: https://github.com/Azure/azure-rest-api-specs/pull/21659#issue-1464593680

Detailed meeting information and documents provided can be accessed here

azure-sdk avatar Jan 03 '23 19:01 azure-sdk

API Stewardship Board Review: 1-Jan-23

Happy New Year

  • We are recommending that this be a preview version. This would enable customer studies on the API.
  • IF there is any chance that the list will become large, consider declaring the returned list to be pageable to avoid a breaking change.
    • Alternately, consider adding a limit to ensure that you can avoid a breaking change.
  • applicationClaims collection could be modeled as polymorphic with distinct revealed and unrevealed claim model. It's a single model now with only version being required.
  • [1:02 PM] Johan Stenberg
  • We should be very much aware that adding one more item to the array is - even though it is not breaking the schema - a kind of change that many customers won't expect (and thus will cause problems for their previously working applications). Same thing when you add a new claim kind - especially if you only have one claim kind to begin with. But we are trying to avoid common ticking bombs like this as much as we can (I'd almost argue that the stewardship group's primary value is exactly that ) While changes can always cause problems, some changes are significantly more commonly known to cause problems than others....

Next Steps

  • Andrea & Jeff to get together to design the model.
  • Andrea / Jeff post to this thread the result
  • Andrea to update the PR & request re-view

markweitzel avatar Jan 04 '23 18:01 markweitzel

@JeffreyRichter and I had a meeting to discuss the model for the new applicationClaims property we would like to add inside an Azure Confidential Ledger receipt. The initial proposed model for this property is an array of objects, each containing various fields that describe a claim:

{
    "applicationClaims": [
        {
            "version": "v1",
            "secretKey": "secret",
            "claimData": {
                "collectionId": "name",
                "contents": "value",
            }
        },
        {
            "version": "v1",
            "claimDigest": "some digest",
        }
    ]
}

It was noted that this model is not clear for customers, as they must check each field to understand how to process a single claim element, which could come in different, mutually exclusive formats. To address this issue, as also discussed during the API review meeting, we decided to add a kind property to each claim element, which would indicate how the element should be processed. Additionally, we will encapsulate the specific fields for each kind inside separate structures, one for each kind. An example of this updated model is shown below:

{
    "applicationClaims": [
        {
            "version": "v1",
            "kind": "data",
            "data": {
                "collectionId": "sample connection",
                "contents": "sample content",
                "secretKey": "sample secret"
            }
        },
        {
            "version": "v1",
            "kind": "digest",
            "digest": {
                "value": "sample digest"
            }
        }
    ]
}

We also discussed the version property in the model. This field is intended to provide customers with information about how to process a claim element and would actively be used in the receipt verification process. For example, if the kind is data, the version field tells customers how to derive a claim digest from plain data. If the kind is digest, the version field tells customers how the digest was computed. It is important that a claim element has the same version regardless of whether it is presented in a revealed or unrevealed (i.e., with kind = digest) format.

However, it was pointed out by Jeffrey that if we add a new kind or version in the future, older clients using an older API version may not be able to process claims elements with the new, unsupported kind or version, and may therefore receive unexpected results when processing the elements in the array. The team will need to further discuss how to handle this situation and whether we should segregate claim versions under different API versions to support forward compatible changes. I will provide you with an update once we take a decision and make the required changes to the model.

cc @lynshi

andpiccione avatar Jan 06 '23 19:01 andpiccione

Yesterday, @JeffreyRichter, @lynshi and I had a follow-up meeting to further discuss the model and resolve the last few doubts after a recent email exchange on the topic. To summarize the outcome respect to the previous comment:

  • We will have a separate claim model for each value of kind: this will ensure that one single model for claim would be present at a time and the client processing would be less ambiguous.
  • Move the version property inside each claim model, as it intended to be used with the rest of the properties of a claim and makes more sense for data locality
  • version should be renamed to something more precise and concrete. I decided to opt for protocol as it seems it can better express the fact that its value specifies how the claim should be processed to derive a digest, but any better suggestion for the name are welcomed
  • When a client tries to fetch a receipt with an unsupported claim kind or protocol, the applicationClaims property will be omitted from the receipt. The team will specify this behaviour in the documentation and will provide users with enough context around this decision.

andpiccione avatar Jan 18 '23 20:01 andpiccione

Regarding the other pending points:

We are recommending that this be a preview version. This would enable customer studies on the API.

We agree and we moved the API version from stable to preview

IF there is any chance that the list will become large, consider declaring the returned list to be pageable to avoid a breaking change.

As discussed during the review meeting, we don't envision the list to become very large at the moment since the number of claims depends on the maximum number of entries that can be written per transaction (currently only 1). We will make sure to add some limit on the claims list if the number of ledger entries per transaction will increase in the future.

These should cover all the pending points. I updated the PR based on the latest decisions and discussion, could I please have a new review? Thanks a lot in advance! https://github.com/Azure/azure-rest-api-specs/pull/21659

cc @lynshi @JeffreyRichter @johanste @markweitzel

andpiccione avatar Jan 18 '23 21:01 andpiccione

PR is signed off -- we can close.

mikekistler avatar Mar 06 '23 15:03 mikekistler