Skip to content

[Azure Confidential Computing Ledger] API Review #21804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
azure-sdk opened this issue Dec 7, 2022 · 6 comments · Fixed by #21659
Closed

[Azure Confidential Computing Ledger] API Review #21804

azure-sdk opened this issue Dec 7, 2022 · 6 comments · Fixed by #21659
Assignees
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes. APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team.

Comments

@azure-sdk
Copy link
Collaborator

azure-sdk commented Dec 7, 2022

New API Review meeting has been requested.

Service Name: Azure Confidential Computing Ledger
Review Created By: Andrea Piccione @andpiccione
Review Date: 1/4/2023 9:00 AM PT
PR: #21659
Hero Scenarios Link: Not Provided
Core Concepts Doc Link: Not Provided

Description: See PR description: #21659 (comment)

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 azure-sdk added the API Review Scoping This is an issue that will track work on a specific set of API changes. label Dec 7, 2022
@azure-sdk
Copy link
Collaborator Author

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: #21659
Hero Scenarios Link: here
Core Concepts Doc Link: here

Description: See PR description: #21659 (comment)

Detailed meeting information and documents provided can be accessed here

@markweitzel
Copy link
Member

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 markweitzel added the APIStewardship-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review label Jan 4, 2023
@markweitzel markweitzel moved this from In Progress to Backlog in API Stewardship Jan 4, 2023
@andpiccione
Copy link
Member

andpiccione commented Jan 6, 2023

@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
Copy link
Member

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
Copy link
Member

andpiccione commented Jan 18, 2023

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! #21659

cc @lynshi @JeffreyRichter @johanste @markweitzel

@mikekistler mikekistler added APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team. and removed APIStewardship-ChangesRequested <valid label in PR review process>add this label when assignee request changes after review labels Jan 29, 2023
@mikekistler mikekistler moved this from Backlog to Ready for Re-review in API Stewardship Jan 29, 2023
@mikekistler mikekistler moved this from Ready for Re-review to In Progress in API Stewardship Jan 29, 2023
@mikekistler mikekistler moved this from In Progress to Done in API Stewardship Jan 29, 2023
@mikekistler
Copy link
Member

PR is signed off -- we can close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Review Scoping This is an issue that will track work on a specific set of API changes. APIStewardshipBoard-ReviewRequested This should be reviewed by the Azure API Stewardship team in partnership with the service team.
Projects
Status: Done
6 participants