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

Update panel match client to use internal proto definitions. #1670

Merged
merged 1 commit into from
Jul 2, 2024

Conversation

robinsons
Copy link
Contributor

@robinsons robinsons commented Jun 21, 2024

Updates most of the panel match code to remove dependencies on classes in the org.wfanet.measurement.api.v2alpha package.

Most of the interesting changes are in the src/main/kotlin/org/wfanet/panelmatch/client/launcher directory, where some of the core interfaces for claiming and launching exchange tasks have been changed slightly.

A small number of dependencies on the v2alpha API still remain:

  • ProtoConversions.kt - This class is specifically meant for converting the v2alpha messages into the panel match internal ones.
  • ExchangeWorkflowDaemonFromFlags.kt and related integration test classes - These will continue to depend on v2alpha -- or the whatever the current API version is -- as long as there is an option for running Kingdom-based exchanges.
  • GrpcApiClient.kt and V2AlphaCertificateManager.kt - Same as the previous bullet. These will continue to be used by any daemon that runs a Kingdom-based exchange.
  • A few standalone tools continue to support the v2alpha message definitions alongside the new internal ones.

@wfa-reviewable
Copy link

This change is Reviewable

@robinsons robinsons force-pushed the robinsons-panelmatch-client-api branch 2 times, most recently from 2848d2b to 1474324 Compare June 21, 2024 20:40
@robinsons robinsons marked this pull request as ready for review June 24, 2024 16:28
Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 71 of 71 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @robinsons)


src/main/kotlin/org/wfanet/panelmatch/client/launcher/ExchangeStepValidatorImpl.kt line 44 at r1 (raw file):

          "No ExchangeWorkflow known for RecurringExchange $recurringExchangeId",
        )
    val existingWorkflowFingerprint = farmHashFingerprint64(serializedExistingExchangeWorkflow)

so, if you're going to fingerprint these to ensure another workflow was not used nefariously, they need to use a cryptographically secure hash like sha256.


src/main/kotlin/org/wfanet/panelmatch/client/launcher/GrpcApiClient.kt line 83 at r1 (raw file):

      stepIndex = exchangeStep.stepIndex,
      workflow = packedV2AlphaWorkflow.unpack<ExchangeWorkflow>().toInternal(),
      workflowFingerprint = farmHashFingerprint64(packedV2AlphaWorkflow.value),

as stated elsewhere, this should be a cryptographically secure hash rather than a fingerprint


src/main/kotlin/org/wfanet/panelmatch/client/tools/BeamJobsMain.kt line 111 at r1 (raw file):

  @Option(
    names = ["--exchange-workflow-format"],

the format actually implies if the workflow is kingdomless or not. I'd prefer a --kingdomless flag instead or something like that that implies what format is used.


src/main/kotlin/org/wfanet/panelmatch/common/certificates/CertificateManager.kt line 33 at r1 (raw file):

   *
   * @param exchange the Exchange that the certificate is scoped for
   * @param certName the name for the certificate we want to retrieve

i think a better comment is justified how the certName is derived. It could either be the public resource name or however it is decided for kingdomless, correct?


src/main/kotlin/org/wfanet/panelmatch/integration/ExchangeWorkflowDaemonForTest.kt line 102 at r1 (raw file):

      is DataProviderKey -> Identity(provider.dataProviderId, Party.DATA_PROVIDER)
      is ModelProviderKey -> Identity(provider.modelProviderId, Party.MODEL_PROVIDER)
      else -> error("Invalid provider: $provider")

i believe the best practice is not include an else but make this exhaustive

@robinsons robinsons force-pushed the robinsons-panelmatch-client-api branch from 1474324 to 2813738 Compare July 1, 2024 20:41
Copy link
Contributor Author

@robinsons robinsons left a comment

Choose a reason for hiding this comment

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

Reviewable status: 61 of 71 files reviewed, 5 unresolved discussions (waiting on @stevenwarejones)


src/main/kotlin/org/wfanet/panelmatch/client/launcher/ExchangeStepValidatorImpl.kt line 44 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

so, if you're going to fingerprint these to ensure another workflow was not used nefariously, they need to use a cryptographically secure hash like sha256.

Done.


src/main/kotlin/org/wfanet/panelmatch/client/launcher/GrpcApiClient.kt line 83 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

as stated elsewhere, this should be a cryptographically secure hash rather than a fingerprint

Done.


src/main/kotlin/org/wfanet/panelmatch/client/tools/BeamJobsMain.kt line 111 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

the format actually implies if the workflow is kingdomless or not. I'd prefer a --kingdomless flag instead or something like that that implies what format is used.

Done.


src/main/kotlin/org/wfanet/panelmatch/common/certificates/CertificateManager.kt line 33 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i think a better comment is justified how the certName is derived. It could either be the public resource name or however it is decided for kingdomless, correct?

Added more description. LMK if you want it tweaked further.


src/main/kotlin/org/wfanet/panelmatch/integration/ExchangeWorkflowDaemonForTest.kt line 102 at r1 (raw file):

Previously, stevenwarejones (Steven Ware Jones) wrote…

i believe the best practice is not include an else but make this exhaustive

provider is a ResourceKey. Since it's not a sealed interface I don't think I can make it exhaustive.

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @robinsons)

@robinsons robinsons merged commit 284cdc5 into main Jul 2, 2024
4 checks passed
@robinsons robinsons deleted the robinsons-panelmatch-client-api branch July 2, 2024 13:49
ple13 pushed a commit that referenced this pull request Aug 16, 2024
Updates most of the panel match code to remove dependencies on classes
in the `org.wfanet.measurement.api.v2alpha` package.

Most of the interesting changes are in the
`src/main/kotlin/org/wfanet/panelmatch/client/launcher` directory, where
some of the core interfaces for claiming and launching exchange tasks
have been changed slightly.

A small number of dependencies on the v2alpha API still remain:

* `ProtoConversions.kt` - This class is specifically meant for
converting the v2alpha messages into the panel match internal ones.
* `ExchangeWorkflowDaemonFromFlags.kt` and related integration test
classes - These will continue to depend on v2alpha -- or the whatever
the current API version is -- as long as there is an option for running
Kingdom-based exchanges.
* `GrpcApiClient.kt` and `V2AlphaCertificateManager.kt` - Same as the
previous bullet. These will continue to be used by any daemon that runs
a Kingdom-based exchange.
* A few standalone tools continue to support the v2alpha message
definitions alongside the new internal ones.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants