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: Check for RIR and Perspective mismatches at runtime when they're provided #7841

Merged
merged 16 commits into from
Dec 6, 2024

Conversation

beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Nov 22, 2024

  • Ensure the Perspective and RIR reported by each remoteVA in the *vapb.ValidationResult returned by VA.PerformValidation, matches the expected local configuration when that configuration is present.
  • Correct "AfriNIC" to "AFRINIC", everywhere.

Part of #7819

@beautifulentropy beautifulentropy marked this pull request as ready for review November 22, 2024 15:09
@beautifulentropy beautifulentropy requested a review from a team as a code owner November 22, 2024 15:09
@beautifulentropy beautifulentropy requested review from aarongable and removed request for a team November 22, 2024 15:09
va/va.go Outdated Show resolved Hide resolved
va/va.go Show resolved Hide resolved
cmd/remoteva/main.go Outdated Show resolved Hide resolved
Base automatically changed from trust-but-verify to main November 25, 2024 18:02
jprenken
jprenken previously approved these changes Nov 25, 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! Looking at the "do not merge" instructions makes me think: is there a way we can land most of this code without waiting, to avoid complicated merge stacks?

For instance, the change from

Perspective string `validate:"omitempty"`

to

Perspective string `validate:"required"`

Could land right now, since we're not yet using the remoteva in prod.

In general, I think all of this PR could land right now, with the small exception of va.go:243. If we leave the va1.Perspective != "" check in place, this all becomes mergeable right away. Effectively we're saying "if the primary VA has perspective and RIR configured, it will check them against the backend values, but if they're not configured, it won't." Then after the necessary deploys happen we could remove that check (and also set the relevant fields as validate:required in the configs).

va/va.go Outdated Show resolved Hide resolved
@beautifulentropy
Copy link
Member Author

In general, I think all of this PR could land right now, with the small exception of va.go:243. If we leave the va1.Perspective != "" check in place, this all becomes mergeable right away.

FWIW, I think va.go:487 has the same issue as va.go:243. Similarly, while requiring Perspective for remoteva might work for now, requiring it for boulder-va wouldn't be immediately deployable. We could make it optional as well, but I’m less concerned about resolving conflicts in a couple of weeks than about splitting this into two PRs that collectively have the effect of making Perspective and RIR required in the VA.

@beautifulentropy
Copy link
Member Author

beautifulentropy commented Nov 25, 2024

Ah, missed making Perspective and RIR required in boulder-va. Adding those lines now.

This was actually correct in the original PR but it was reverted by a merge: 64a9a3a

jprenken
jprenken previously approved these changes Nov 26, 2024
jsha
jsha previously approved these changes Dec 2, 2024
@beautifulentropy beautifulentropy dismissed stale reviews from jsha and jprenken via 9c149fd December 5, 2024 15:03
aarongable
aarongable previously approved these changes Dec 5, 2024
jprenken
jprenken previously approved these changes Dec 5, 2024
@beautifulentropy beautifulentropy dismissed stale reviews from jprenken and aarongable via 78aa24b December 6, 2024 18:50
@beautifulentropy beautifulentropy changed the title va: Require RIR and Perspective and check for mismatches at runtime va: Check for RIR and Perspective mismatches at runtime when they're not empty strings Dec 6, 2024
@beautifulentropy beautifulentropy changed the title va: Check for RIR and Perspective mismatches at runtime when they're not empty strings va: Check for RIR and Perspective mismatches at runtime when they're provided Dec 6, 2024
@beautifulentropy beautifulentropy merged commit 87104b0 into main Dec 6, 2024
12 checks passed
@beautifulentropy beautifulentropy deleted the trust-but-verify-2 branch December 6, 2024 19:27
@jsha
Copy link
Contributor

jsha commented Dec 6, 2024

Late to the party, but this looks good to me! 🎉

beautifulentropy added a commit that referenced this pull request Dec 6, 2024
…provided (#7841)

- Ensure the Perspective and RIR reported by each remoteVA in the
*vapb.ValidationResult returned by VA.PerformValidation, matches the
expected local configuration when that configuration is present.
- Correct "AfriNIC" to "AFRINIC", everywhere.

Part of #7819
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.

4 participants