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

VA: Use performValidation for IsCAAValid remote checks #7850

Merged
merged 8 commits into from
Nov 28, 2024

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Nov 26, 2024

  • Remove undeployed feature flag MultiCAAFullResults
  • Perform local CAA checks prior to initiating remote checks, instead of starting remote checks and proceeding to perform local checks.
  • Remove VA.IsCAAValid specific remote validation logic, use VA.performRemoteOperation instead
  • Refactor va.logRemoteResults to be easier to test and omit the RVA problem
  • Drive-by fix: Calculate logEvent.Latency with va.clk.Since() instead of time.Since() like everything else in VA.performRemoteOperation

This change must be merged after #7847.

@beautifulentropy beautifulentropy marked this pull request as ready for review November 26, 2024 22:10
@beautifulentropy beautifulentropy requested a review from a team as a code owner November 26, 2024 22:10
@beautifulentropy beautifulentropy requested review from jsha and removed request for a team November 26, 2024 22:10
Copy link
Contributor

@beautifulentropy, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values.

jsha
jsha previously approved these changes Nov 27, 2024
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

This looks great. I love seeing performRemoteCAACheck and processRemoteCAAResults deleted in favor of using shared code.

It's a bummer to see performRemoteValidation, which was made so beautifully generic in #7847, wind up needing special-case code for CAA. I think we talked about the reasons on our call yesterday. Let me check my understanding:

  • The current tests in TestMultiCAARechecking check that:
    • There is a log line detailing the remote differentials (logRemoteDifferentials JSON=, which is logged by logRemoteResults)
    • When a ProblemDetails is returned by any remote, it gets logged in performRemoteValidation (by comparison, the PerformValidation doesn't log ProblemDetails in performRemoteValidation, only RPC-level errors)
  • Because these current tests actually test properties that are very similar to the properties we will be checking for MPIC, we want to preserve them so that we can see clearly in the MPIC-related diffs that the same tests pass the same sort of checks.
  • Once the MPIC code lands we can remove the CAA-related special cases from performRemoteValidation?

Is that all sound correct?

Base automatically changed from generic-perform-remote-validation to main November 27, 2024 20:29
@beautifulentropy beautifulentropy dismissed jsha’s stale review November 27, 2024 20:29

The base branch was changed.

@beautifulentropy
Copy link
Member Author

This looks great. I love seeing performRemoteCAACheck and processRemoteCAAResults deleted in favor of using shared code.

It's a bummer to see performRemoteValidation, which was made so beautifully generic in #7847, wind up needing special-case code for CAA. I think we talked about the reasons on our call yesterday. Let me check my understanding:

  • The current tests in TestMultiCAARechecking check that:

    • There is a log line detailing the remote differentials (logRemoteDifferentials JSON=, which is logged by logRemoteResults)
    • When a ProblemDetails is returned by any remote, it gets logged in performRemoteValidation (by comparison, the PerformValidation doesn't log ProblemDetails in performRemoteValidation, only RPC-level errors)
  • Because these current tests actually test properties that are very similar to the properties we will be checking for MPIC, we want to preserve them so that we can see clearly in the MPIC-related diffs that the same tests pass the same sort of checks.

  • Once the MPIC code lands we can remove the CAA-related special cases from performRemoteValidation?

Is that all sound correct?

Yes, all of this is correct. I'd also like to add that we have not and never plan to turn EnforceMultiCAA on in staging or production. Thus these carve outs in performRemoteOperation will only be exercised in test.

jsha
jsha previously approved these changes Nov 27, 2024
va/caa.go Outdated Show resolved Hide resolved
Co-authored-by: Jacob Hoffman-Andrews <[email protected]>
@beautifulentropy beautifulentropy merged commit d64132e into main Nov 28, 2024
14 checks passed
@beautifulentropy beautifulentropy deleted the generic-perform-remote-validation-2 branch November 28, 2024 20:24
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