Skip to content
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

userapi: rename grpc package and change relative proto path #1037

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

elchead
Copy link
Contributor

@elchead elchead commented Nov 28, 2024

The origin of this PR started in this discussion.
As Contrast is migrating towards providing an SDK, the userapi should adhere to best practices and avoid compatibility issues.
As part of integrating the current SDK draft into Continuum, two issues were discovered.

  1. The gRPC package name is not globally unique (see https://protobuf.dev/reference/go/faq#namespace-conflict), which leads to conflicts when the consumer imports other gRPC packages with the same name.
  2. The protoc compiler currently takes in a source path of the form "userapi.proto". This leads to import issues when another package imports a proto file with the same path. This happened in the Continuum project:
panic: proto: file "userapi.proto" is already registered
                previously from: "github.com/edgelesssys/continuum/internal/proto/attestation-service/userapi"
                currently from:  "github.com/edgelesssys/contrast/internal/userapi"
        See https://protobuf.dev/reference/go/faq#namespace-conflict

A solution was suggested here which ensures that the path names are unlikely to collide. Hence, I moved the protoc generation to include the proto file path relative to the root.

Note: This change breaks compatibility with the current gRPC version, but it should be fine to release this with the next minor.

@elchead elchead requested a review from burgerdev November 28, 2024 10:49
@elchead elchead added the breaking change A user-affecting breaking change label Nov 28, 2024
@elchead elchead marked this pull request as ready for review November 28, 2024 10:53
@elchead elchead requested a review from katexochen as a code owner November 28, 2024 10:53
@katexochen katexochen added no changelog PRs not listed in the release notes and removed breaking change A user-affecting breaking change labels Nov 28, 2024
@katexochen katexochen removed their request for review November 29, 2024 06:17
Copy link
Contributor

@burgerdev burgerdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, breaking the internal gRPC layer is not user-visible right now.

@elchead elchead merged commit 65bae63 into main Dec 2, 2024
16 of 17 checks passed
@elchead elchead deleted the as/rename-grpc branch December 2, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants