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.

Sign up for GitHub

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

Jump to bottom

Protocol version negotiation currently doesn't result in agreement #9584

Closed
Ericson2314 opened this issue Dec 10, 2023 · 3 comments · Fixed by #10782
Closed

Protocol version negotiation currently doesn't result in agreement #9584

Ericson2314 opened this issue Dec 10, 2023 · 3 comments · Fixed by #10782
Assignees
@Ericson2314
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.

Comments

@Ericson2314
Copy link
Member

As far as I can tell, here is how the "serve protocol" and "worker protocol" both do it:

  1. Each side send the higher version they support to the other side.
  2. The other side tries to use that version if it supports it.

But that means if the two versions are not the same, the two ends of the connection will be using different versions! Madness!

I think we forgot one simple step:

  1. Take the minimum of the version you sent and version you received.

Then both sides will end up with the same version. Problem solved!
Ericson2314 added a commit to obsidiansystems/hydra that referenced this issue Dec 10, 2023
@Ericson2314
Do not attempt to speak a newer version of the protocol
b56d238 
Both sides need to agree on a version (with `std::min`) for anything to
work. Somehow... we've never done this.

With this comment, the next commit succeeds. Without this commit, the
next commit fails. This is because the next commit exposes serializers
which do different things for proto version 2.7, and we're currently
requesting 2.6.

Opened NixOS/nix#9584 to track this issue
@Ericson2314 Ericson2314 mentioned this issue Dec 10, 2023
Merged
@Ericson2314 Ericson2314 changed the title Protocol version negotion currently makes no sense, allows mismatch Protocol version negotion currently doesn't result in agreement Dec 10, 2023
@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Dec 11, 2023
@Ericson2314 Ericson2314 self-assigned this Dec 11, 2023
@roberth roberth changed the title Protocol version negotion currently doesn't result in agreement Protocol version negotiation currently doesn't result in agreement Dec 11, 2023
@Ericson2314 Ericson2314 mentioned this issue Jan 22, 2024
Merged
@Ericson2314
Copy link
Member Author

Ericson2314 commented Jan 22, 2024 •
edited
Loading

#6134, and doing the equivalent for the handshake for ssh-ng://, can allow us to write some good unit test for this.

@edolstra
Copy link
Member

@Ericson2314 Is this still an issue now that #6134 is merged?

@Ericson2314
Copy link
Member Author

Ericson2314 commented Feb 23, 2024 •
edited
Loading

@edolstra yes it is. I did something for this in hydra but it needs to be moved to Nix and made part of the unit tests. The factoring out of serializers did not cover the handshakes. I just factored out the legacy ssh handshake so far.

This can stay assigned to me.

Ericson2314 added a commit that referenced this issue May 20, 2024
@Ericson2314
Query path infos (plural) and handshake version minimum for hydra
8b369f9 
1. Hydra currently queries for multiple path infos at once, so let us
   make a connection item for that.

2. The minimum of the two versions should always be used, see #9584.
   (The issue remains open because the daemon protocol needs to be
   likewise updated.)
@Ericson2314 Ericson2314 mentioned this issue May 20, 2024
Merged
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue May 27, 2024
@Ericson2314
Factor our connection code for worker proto like serve proto
3be36fc 
This increases test coverage, and gets the worker protocol ready to be
used by Hydra.

Why don't we just try to use the store interface in Hydra? Well, the
problem is that the store interface works on connection pools, with each
opreation getting potentially a different connection, but the way temp
roots work requires that we keep one logical "transaction" (temp root
session) using the same connection.

The longer-term solution probably is making connections themselves
implement the store interface, but that is something that builds on
this, so I feel OK that this is not churn in the wrong direction.

Fixes NixOS#9584
Ericson2314 added a commit to obsidiansystems/nix that referenced this issue May 27, 2024
@Ericson2314
Factor our connection code for worker proto like serve proto
f71b4da 
This increases test coverage, and gets the worker protocol ready to be
used by Hydra.

Why don't we just try to use the store interface in Hydra? Well, the
problem is that the store interface works on connection pools, with each
opreation getting potentially a different connection, but the way temp
roots work requires that we keep one logical "transaction" (temp root
session) using the same connection.

The longer-term solution probably is making connections themselves
implement the store interface, but that is something that builds on
this, so I feel OK that this is not churn in the wrong direction.

Fixes NixOS#9584
@Ericson2314 Ericson2314 mentioned this issue May 27, 2024
Merged
@lf- lf- mentioned this issue Aug 20, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Ericson2314 Ericson2314

Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Factor our connection code for worker proto like serve proto obsidiansystems/nix
3 participants
@tomberek @Ericson2314 @edolstra