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

Pure SSH-based protocol #4446

Merged
merged 33 commits into from
Jul 20, 2021
Merged

Pure SSH-based protocol #4446

merged 33 commits into from
Jul 20, 2021

Conversation

bk2204
Copy link
Member

@bk2204 bk2204 commented Mar 17, 2021

This is an implementation of a pure SSH-based protocol. It uses the proposal outlined in the docs/proposals directory, with a few changes, which are added to the documentation.

The implementation here is complete. It should be fully functional.

This implementation is being tested against Scutiger's git-lfs-transfer binary, which is our reference implementation. That binary will be used as part of our CI jobs in the future to validate that we continue to work correctly with it. It is built from the latest crate version now that it is published on crates.io.

The goal is to have the code fall back to git-lfs-authenticate and use the HTTP-based protocol with SSH authentication if the git-lfs-transfer binary fails for whatever reason.

Fixes #1044

@bk2204 bk2204 mentioned this pull request Mar 17, 2021
@bk2204 bk2204 force-pushed the ssh-protocol branch 4 times, most recently from 3dc6a37 to 24be525 Compare March 26, 2021 16:41
@bk2204 bk2204 force-pushed the ssh-protocol branch 2 times, most recently from 775121e to 9114a57 Compare March 31, 2021 17:25
@bk2204 bk2204 force-pushed the ssh-protocol branch 3 times, most recently from 2d5aa0b to 17befd5 Compare April 7, 2021 16:55
@bk2204 bk2204 force-pushed the ssh-protocol branch 9 times, most recently from 0039c02 to c99b843 Compare May 19, 2021 13:57
@bk2204 bk2204 marked this pull request as ready for review May 26, 2021 15:52
@bk2204 bk2204 requested a review from a team May 26, 2021 15:52
Copy link
Member

@chrisd8088 chrisd8088 left a comment

Choose a reason for hiding this comment

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

This is an amazing contribution, thank you so much! It will be greatly appreciated by a significant portion of the Git LFS community, and it will be a major new feature that distinguishes the 3.0 release. 🚀

I hope my comments and handful of suggestions are clear; please let me know if any are not!

Thank you again for working on this so thoroughly and diligently over many months. ❤️

git/filter_process_scanner.go Outdated Show resolved Hide resolved
ssh/pktline.go Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
t/t-locks.sh Show resolved Hide resolved
t/testhelpers.sh Outdated Show resolved Hide resolved
t/t-locks.sh Outdated Show resolved Hide resolved
tq/ssh.go Outdated Show resolved Hide resolved
tq/ssh.go Outdated Show resolved Hide resolved
@bk2204
Copy link
Member Author

bk2204 commented Jun 21, 2021

Okay, I've gone through all of the comments, including the ones I didn't reply to directly, and I believe I've addressed all of the concerns unless I've specifically said I didn't think it needed to be addressed. For the moment, I've put in some squash and fixup patches to help you in a second round. The only changes to existing patches are those to resolve conflicts from the insertion of patches earlier into the series (which was required in order to maintain bisectable commits).

Once you're happy with these changes as they stand, I'll just squash them down and then merge.

Copy link
Member

@chrisd8088 chrisd8088 left a comment

Choose a reason for hiding this comment

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

Whew! Thank you for the many, many revisions and responses! ❤️

I think I have marked everything that looked resolved as such, leaving only a couple of comments from my original review with additional notes or questions, and I had only a few extras turn up and they should be in the review now too.

I confess I did not go through the whole body of changes from top to bottom; I thought I'd wait on that and do it as part of a final pass.

Thanks again and please do let me know if anything of my comments are unclear, or just plain nonsensical or wrong. 😄

ssh/protocol.go Outdated Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
ssh/protocol.go Outdated Show resolved Hide resolved
tq/ssh.go Show resolved Hide resolved
locking/ssh.go Show resolved Hide resolved
t/testhelpers.sh Show resolved Hide resolved
t/t-lock.sh Outdated Show resolved Hide resolved
t/cmd/lfs-ssh-echo.go Outdated Show resolved Hide resolved
@bk2204 bk2204 force-pushed the ssh-protocol branch 2 times, most recently from 9c5f49c to b7ad928 Compare July 2, 2021 16:31
@bk2204
Copy link
Member Author

bk2204 commented Jul 2, 2021

I think I've addressed all of your suggestions; I took all but one, where I think one of us may have a misunderstanding, which could very well be me (and if so, please say so). I've rebased onto the main branch, but left my changes as fixup commits at the end for easy reviewing. As mentioned before, once we're happy with where things are, I'll squash them down before merging.

@chrisd8088
Copy link
Member

How do I set this up? Just add git-lfs-transfer into $PATH?

@shamefulCake1 -- I would recommend opening a new discussion if you're interested in this topic, as this PR has been merged and closed for a few years now. Or you might find the answers you're looking for in an existing discussion like #5877.

In brief, the Git LFS client as of v3.0.0 doesn't need additional tools to perform the client-side portions of the pure SSH-based Git LFS transfer protocol. However, not all Git LFS hosting services offer support for the server-side portions of the protocol yet, so the answer to your question depends a lot on what service you are using. GitLab supports the protocol now, and I believe Gitea has added support as well, but GitHub has not.

If you are setting up your own Git LFS hosting service, you could use the git-lfs-transfer binary from the bk2204/scutiger project along with the rest of your Git and Git LFS services, or you could write your own implementation, as appropriate, so long as it conforms to the proposal for the protocol.

@shamefulCake1
Copy link

Thank you, it seems that #5877 is indeed what I need.

I recognise that this PR is several years old, but it's still the first one to come up on Google and DDG.

If you are setting up your own Git LFS hosting service,

I'm not doing anything even remotely so complicated. I'm just trying to clone my a git repo from my workstation to my my laptop over ssh. Git is a distributed VCS, and it doesn't need a server.

chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 22, 2024
With PR git-lfs#4446 we defined a new SSHTransfer structure and set of methods
on that structure in our ssh/connection.go source file, and used the
name "tr" for the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file, in order to format and output an error message from the
startConnection() function.  As it happens this function is not one of
the methods of the SSHTransfer structure, and so there was no conflict
with the methods' "tr" receiver variables.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr variable,
which we can not do with the current name of the "tr" receiver variable,
as it masks the "tr" package name within the method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 22, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 22, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to create multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
created with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the separate bug described in git-lfs#5880, where we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
tried to reference that nil pointer in the batchInternal() method of
the SSHBatchClient structure in the "tq" package, causing a Go panic
condition.

In prior commits in this PR we resolved the first problem with the
lfs-ssh-echo test utility, and adjusted the Connection() method of
the SSHTransfer structure to return a non-nil error when the requested
connection number exceeds the maximum number of permitted SSH connections,
which covers the case where all the connections have already been closed.

Therefore, we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests, we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

One of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, just the first of which will have the ControlMaster
option set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use our new assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make some small adjustments to other tests in the same
test script, so as to remove unnecessary redirections of stderr, and
to add one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test, just to confirm that an object
was successfully pushed over HTTP to our lfstest-gitserver test
helper program following the initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 23, 2024
With PR git-lfs#4446 we defined a new SSHTransfer structure and set of methods
on that structure in our ssh/connection.go source file, and used the
name "tr" for the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file, in order to format and output an error message from the
startConnection() function.  As it happens this function is not one of
the methods of the SSHTransfer structure, and so there was no conflict
with the methods' "tr" receiver variables.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr variable,
which we can not do with the current name of the "tr" receiver variable,
as it masks the "tr" package name within the method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 23, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 23, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 23, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 29, 2024
With PR git-lfs#4446 we defined a new SSHTransfer structure and set of methods
on that structure in our ssh/connection.go source file, and used the
name "tr" for the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file, in order to format and output an error message from the
startConnection() function.  As it happens this function is not one of
the methods of the SSHTransfer structure, and so there was no conflict
with the methods' "tr" receiver variables.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr variable,
which we can not do with the current name of the "tr" receiver variable,
as it masks the "tr" package name within the method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 29, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Oct 29, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 1, 2024
With PR git-lfs#4446 we defined a new SSHTransfer structure and set of methods
on that structure in our ssh/connection.go source file, and used the
name "tr" for the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file, in order to format and output an error message from the
startConnection() function.  As it happens this function is not one of
the methods of the SSHTransfer structure, and so there was no conflict
with the methods' "tr" receiver variables.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr variable,
which we can not do with the current name of the "tr" receiver variable,
as it masks the "tr" package name within the method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 1, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 1, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 2, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 2, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
Since PR git-lfs#4446 we have output several trace log messages when attempting
to instantiate a new SSHTransfer structure in the SSHTransfer() method
of the Client structure in our "lfsapi" package.  This method takes
"operation" and "remote" string arguments, and the returned SSHTransfer
structure is specific to those values.

As we may potentially call this method several times in a single
process, with different arguments, we add the "operation" and "remote"
variables to our trace log messages, which will help us when we analyze
logs and want to distinguish the different contexts in which the
SSHTransfer() method was called.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
With PR git-lfs#4446 we defined a new SSHTransfer structure and set of methods
on that structure in our ssh/connection.go source file, and used the
name "tr" for the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file, in order to format and output an error message from the
startConnection() function.  As it happens this function is not one of
the methods of the SSHTransfer structure, and so there was no conflict
with the methods' "tr" receiver variables.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr variable,
which we can not do with the current name of the "tr" receiver variable,
as it masks the "tr" package name within the method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

The first of our new tests leaves the maximum number of concurrent object
transfers set to the default value of 8, so that all three of the objects
we create in the test are pushed or fetched in the same batch.  This
requires that the Git LFS client establish three separate SSH connections,
two of which should have the ControlMaster option set to "no", and the
first of which should have the option set to "yes".

The second of our new tests sets the maximum number of concurrent
transfers to two, so we expect to see only two SSH connections
established, the first of which should have the ControlMaster option
set to "yes".  Since there are only two concurrent connections,
we can assume the third object was transferred in a second batch.

(Note that the Git LFS client, at present, always creates an extra
SSH connection with the ControlMaster option set to "yes" when
pushing objects over the SSH object transfer protocol.  It uses this
extra connection exclusively to perform locking commands.  Our tests
therefore expect this extra connection to be reported in the push
trace logs, and we include comments to explain why the expected
connection totals are one higher than might otherwise be the case.)

Next, we expand the existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test to perform the same set of checks as our new
tests, bringing the tests into close alignment with each other.  We
again use the assert_remote_object() function to confirm that the
single object pushed in this test is written to the "remote" repository.
We also check that only a single SSH connection is reported in the
trace logs (plus the extra one used for locking commands during the
push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding one extra check to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test which just confirms that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program, following an initial authorization handshake over SSH.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH connections during the
batch object transfer phase.  For this reason, no SSH connections are
started with the ControlMaster option set to "no".

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from accepting SSH
connections with the ControlMaster option set to "no", so the
"batch transfers with ssh endpoint (git-lfs-transfer)" test would
have failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had closed all the SSH connections, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, both the bug
in the lfs-ssh-echo test utility and the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client
code.  In the case of the latter issue, we adjusted the Connection()
method to return a non-nil error when the requested connection number
exceeds the maximum number of permitted SSH connections, which also
covers the case where all the connections have already been closed.

Therefore we can now add additional tests of the SSH object transfer
protocol which push multiple objects and then fetch them.  In these
tests we use the new assert_remote_object() assertion function that we
added in a prior commit in this PR to confirm that the objects we push
are all written to the "remote" repository.  As well, we check that
the expected number of SSH connections are reported in the trace logs
from both the push and clone operations, and that some connections are
made with the ControlMaster option set to "no" as well as "yes".

To perform these SSH connection count checks we add two dedicated
assertion functions, assert_ssh_transfer_connections() and
assert_ssh_transfer_connection_counts(), with the former calling the
latter several times.  These functions are unlikely to be useful
outside of the context of our tests of the SSH object transfer
protocol, so we define them in the t/t-batch-transfer.sh test
script rather than in our generic t/testhelpers.sh library.

The first of our new tests leaves the maximum number of concurrent
object transfers set to the default value of 8, so that all three of
the objects we create in the test are pushed or fetched in the same
batch.  This requires that the Git LFS client establish as many as
three separate SSH connections, of which only the first should create
a control socket.  The second of our new tests sets the maximum number
of concurrent transfers to two, so we expect to see a maximum of only
two SSH connections established, of which only the first should create
the control socket.

We have to adjust these basic expectations to account for two different
issues, however.  One issue pertains to push operations, and the other
to fetches.  When pushing using the SSH object transfer protocol, the
Git LFS client currently always creates an extra socket control SSH
connection, which it uses exclusively to perform locking commands, and
does not terminate cleanly.  And as a separate issue, if Git is older
than version 2.11.0, it does not support the "process" filter and so
Git LFS is instead invoked via the "smudge" filter once for each object.
This means a unique socket control SSH connection will be created and
used to fetch each object.  Our new assert_ssh_transfer_connections()
function adjusts its expected connection counts as necessary based
on whether either of these two conditions applies.

In addition to adding our two tests, we also expand our existing
"batch transfers with ssh endpoint (git-lfs-transfer)" test to perform
the same set of checks as the new tests, which brings all the SSH
object transfer tests into alignment with each other.  We again use
the our assert_remote_object() function to confirm that the single
object pushed in the existing test is written to the "remote" repository.
We also use our assert_ssh_transfer_connections() function to check
that only a single SSH connection is reported in the trace logs (plus
the extra one used for locking commands during the push operation).

Finally, we make a few small adjustments to other tests in the same
test script, removing some unnecessary redirections of stderr, and
adding two extra checks to our "batch transfers with ssh endpoint
(git-lfs-authenticate)" test.  We confirm that an authorization
handshake was performed over SSH, and validate that an object was
successfully pushed over HTTP to our lfstest-gitserver test helper
program following the handshake.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
In subsequent commits in this PR we expect to resolve a pair of issues
which prevent us from testing the SSH object transfer protocol with
more than a single object.  This transfer protocol was introduced in
PR git-lfs#4446, and in commit 691de51 of
the PR, the "batch transfers with ssh endpoint (git-lfs-transfer)" test
in our t/t-batch-transfer.sh test script was added to validate that the
new protocol worked as expected.

This test pushes and then fetches a single object, so the issues which
arise when handling multiple objects in a batch do not cause the test
to fail.  Nevertheless, before addressing those issues, we first expand
the existing test so it checks that the git-lfs-transfer command is
seen in the trace log messages during a push operation.  The test
already checks that this command is used during a clone operation.

As well, we enhance the "batch transfers with ssh endpoint
(git-lfs-authenticate)" test, which checks that when the SSH transfer
protocol is not available but Git is configured to use SSH, the
Git LFS client performs an authorization handshake over SSH prior
to using HTTP for its object transfers.  We now confirm that the
git-lfs-authenticate command is seen in the trace log messages
generated during a push operation, and that the object was
successfully pushed and has been stored by the remote server.

We also make a small revision to the "batch transfers succeed with an
empty hash algorithm" test to remove an unnecessary file redirection.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 3, 2024
In subsequent commits in this PR we expect to resolve a pair of issues
which prevent us from testing the SSH object transfer protocol with
more than a single object.  This transfer protocol was introduced in
PR git-lfs#4446, and in commit 691de51 of
the PR, the "batch transfers with ssh endpoint (git-lfs-transfer)" test
in our t/t-batch-transfer.sh test script was added to validate that the
new protocol worked as expected.

This test pushes and then fetches a single object, so the issues which
arise when handling multiple objects in a batch do not cause the test to
fail.  Hence we intend to add two other tests to accompany the existing
one so as to validate the SSH transfer protocol with multiple objects.

As these tests will all perform object transfers only over SSH, our
HTTP-based lfstest-gitserver test helper program will not be used in
any of them.  That program retains a copy of each object it receives
in memory to simulate a remote storage server.  As a consequence, many
of our tests use the assert_server_object() function defined in our
t/testhelpers.sh library to confirm that an object has been received
by the remote server; this function makes an HTTP batch request to
the lfstest-gitserver program and checks the JSON response.

In our tests of the SSH transfer protocol, by contrast, object
data will be proxied by the lfs-ssh-echo helper program to the
git-lfs-transfer command, which will write the data into a separate
bare Git repository in the location provided as command argument.
In fact this is how the existing test already operates; it uses
the ssh_remote() function from our t/testhelpers.sh library to
establish the SSH URL for its remote repository.  The URLs returned
by this function always include the directory path from our REMOTEDIR
variable.  We expect to also use the ssh_remote() function in our
new tests to establish their remote repositories.

In both our existing test and the ones we will add in a subsequent
commit in this PR, we would like to confirm that the Git LFS objects
we push have been successfully written into the "lfs/objects" cache
in the remote repositories.  To simplify such checks, we define a new
assert_remote_object() assertion function in our shell test library.

Unlike the assert_server_object() function, which makes an HTTP
request, our new assertion acts in a similar manner to the existing
assert_local_object() function by simply validating the size and
existence of an object file in the appropriate subdirectory of the
"lfs/objects" cache hierarchy of a repository.  However, our new
assert_remote_object() constructs the path to the repository using the
REMOTEDIR variable, rather than checking the object's presence in the
current local repository as the assert_local_object() function does.

With the assert_remote_object() function defined, we then update our
existing "batch transfers with ssh endpoint (git-lfs-transfer)" test
to make use of it after pushing an object over the SSH transfer
protocol, and we will use it for the same purpose in the additional
tests we will introduce in a later commit in this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
In subsequent commits in this PR we expect to resolve a pair of issues
which prevent us from testing the SSH object transfer protocol with
more than a single object.  The SSH transfer protocol was introduced in
PR git-lfs#4446, and in commit 691de51 of
that PR, the "batch transfers with ssh endpoint (git-lfs-transfer)" test
in our t/t-batch-transfer.sh test script was added to validate that the
new protocol worked as expected.

This test pushes and then fetches a single object, so the issues which
arise when handling multiple objects in a batch do not cause the test
to fail.  Nevertheless, before addressing those issues, we first expand
the existing test so it checks that the git-lfs-transfer command is
seen in the trace log messages during a push operation.  The test
already checks that this command is used during a clone operation.

As well, we enhance the "batch transfers with ssh endpoint
(git-lfs-authenticate)" test, which checks that when the SSH transfer
protocol is not available but Git is configured to use SSH for a remote,
the Git LFS client performs an authorization handshake over SSH prior
to using HTTP for its object transfers.  In the test we now confirm
that the git-lfs-authenticate command is seen in the trace log messages
generated during a push operation, and that the object was
successfully pushed and has been stored by the remote server.

We also make a small revision to the "batch transfers succeed with an
empty hash algorithm" test to remove an unnecessary file redirection.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
In subsequent commits in this PR we expect to resolve a pair of issues
which prevent us from testing the SSH object transfer protocol with
more than a single object.  The SSH transfer protocol was introduced in
PR git-lfs#4446, and in commit 691de51 of
that PR, the "batch transfers with ssh endpoint (git-lfs-transfer)" test
in our t/t-batch-transfer.sh test script was added to validate that the
new protocol worked as expected.

This test pushes and then fetches a single object, so the issues which
arise when handling multiple objects in a batch do not cause the test to
fail.  Hence we intend to add two other tests to accompany the existing
one so as to validate the SSH transfer protocol with multiple objects.

As these tests will all perform object transfers only over SSH, our
HTTP-based lfstest-gitserver test helper program will not be used in
any of them.  That program retains a copy of each object it receives
in memory to simulate a remote Git LFS server.  As a consequence, many
of our tests use the assert_server_object() function defined in our
t/testhelpers.sh library to confirm that an object has been received
by the remote server; this function makes an HTTP batch request to
the lfstest-gitserver program and checks the JSON response.

In our tests of the SSH transfer protocol, by contrast, object
data will be proxied by the lfs-ssh-echo helper program to the
git-lfs-transfer command, which will write the data into a separate
bare Git repository in the location provided as command argument.
In fact this is how the existing test already operates; it uses
the ssh_remote() function from our t/testhelpers.sh library to
establish the SSH URL for its remote repository.  The URLs returned
by this function always include the directory path from our REMOTEDIR
variable.  We expect to also use the ssh_remote() function in our
new tests to establish their remote repositories.

In both our existing test and the ones we will add in a subsequent
commit in this PR, we would like to confirm that the Git LFS objects
we push have been successfully written into the "lfs/objects" cache
in the remote repositories.  To simplify such checks, we define a new
assert_remote_object() assertion function in our shell test library.

Unlike the assert_server_object() function, which makes an HTTP
request, our new assertion acts in a similar manner to the existing
assert_local_object() function by simply validating the size and
existence of an object file in the appropriate subdirectory of the
"lfs/objects" cache hierarchy of a bare repository.  However, our new
assert_remote_object() function constructs the path to the repository
using the REMOTEDIR variable, rather than checking the object's
presence in the current local repository as the assert_local_object()
function does.

With the assert_remote_object() function defined, we then update our
existing "batch transfers with ssh endpoint (git-lfs-transfer)" test
to make use of it after pushing an object over the SSH transfer
protocol, and we will use the function for the same purpose in the
additional tests we expect to introduce in a later commit in this PR.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
In subsequent commits in this PR we expect to resolve a pair of issues
which prevent us from testing the SSH object transfer protocol with
more than a single object.  The SSH transfer protocol was introduced in
PR git-lfs#4446, and in commit 691de51 of
that PR, the "batch transfers with ssh endpoint (git-lfs-transfer)" test
in our t/t-batch-transfer.sh test script was added to validate that the
new protocol worked as expected.

This test pushes and then fetches a single object, so the issues which
arise when handling multiple objects in a batch do not cause the test to
fail.  Hence we intend to add two other tests to accompany the existing
one so as to validate the SSH transfer protocol with multiple objects.

All these tests will perform object transfers over SSH, but with varying
numbers and types of SSH sessions.  While our existing test only pushes
and fetches a single object, the tests we will add in a later commit
in this PR will push and fetch multiple objects in each batch.

By default, the Begin() method of the adapterBase structure in our "tq"
package starts eight goroutines to process the objects in a batch, each
running the structure's worker() method.  The first worker routine must
start an SSH session, and depending on how quickly other workers pull
objects from the batch transfer queue, additional SSH sessions may be
created as well, one per active worker.

On platforms other than Windows, we set the default value of the
"lfs.ssh.autoMultiplex" configuration option to "true".  When this
option is "true" and the available SSH client is considered to be
compatible with OpenSSH, we attempt use OpenSSH's ControlMaster option
to create a control socket and multiplex all SSH sessions over a
single common connection.  We expect the first SSH session to be
established with a ControlMaster argument value of "yes", and other
sessions with a value of "no".

At present, our existing "batch transfers with ssh endpoint
(git-lfs-transfer)" test does not check the value of the ControlMaster
argument.  As we expect to add tests which may create more than one SSH
session, however, we would like to validate the number of sessions that
create new control sockets, and the number of SSH sessions overall.

To perform these SSH session count checks we add two dedicated
assertion functions, assert_ssh_transfer_sessions() and
assert_ssh_transfer_session_counts(), with the former calling the
latter several times.  These functions are unlikely to be useful
outside of the context of our tests of the SSH object transfer
protocol, so we define them directly in the t/t-batch-transfer.sh
test script rather than in our generic t/testhelpers.sh library.

The primary function, assert_ssh_transfer_sessions(), checks the
number of times the git-lfs-transfer command is executed over SSH,
and confirms that each session has the ControlMaster option set to
"yes", which is always the case for our existing test, so long as we
update the test to force the use of SSH connection multiplexing on
Windows by explicitly setting the "lfs.ssh.autoMultiplex"
configuration option to "true".

The assert_ssh_transfer_sessions() function then uses the secondary
assert_ssh_transfer_session_counts() function to validate that the
expected number of startup, success, and termination trace log messages
are seen for each SSH session.

One specific issue arises in regard to the number of SSH sessions
and git-lfs-transfer commands we expect to see started during a Git
push operation.  At present, the Git LFS client always starts a unique
SSH session to run the git-lfs-transfer command when checking for
Git LFS locks on the objects being pushed, and this session will
create a control socket if the SSH client in use is considered
compatible with OpenSSH and the "lfs.ssh.autoMultiplex" configuration
option is set to "true".  The control socket opened for this session
is distinct from the one created later by the first SSH session
started by the batch transfer worker routines.

The unique SSH session used for the lock verification request during
push operations is managed with a separate instance of the SSHTransfer
structure from our "ssh" package.  At present, the Git LFS client never
calls the setConnectionCount() method with a zero argument on this
instance of the SSHTransfer structure, so the dedicated SSH session
used for lock verification is never explicitly terminated.

Therefore our new assert_ssh_transfer_sessions() function adjusts
the number of git-lfs-transfer command execution messages and the
number of start and success trace log messages it expects when a push
(i.e., "upload") operation is performed to be one higher than the
number of termination trace log messages it expects.  This allows
the function to account for the additional SSH control socket
session started for the lock verification step, and then never
explicitly terminated.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
Since PR git-lfs#4446 the SSHTransfer() method of the Client structure in
our "lfsapi" package has output several trace log messages when
attempting to instantiate a new SSHTransfer structure.  This method
takes "operation" and "remote" string arguments, and the returned
SSHTransfer structure is specific to those values.

As this method may be called several times with distinct arguments
during the execution of a Git LFS process, we add the "operation"
and "remote" variables to the trace log messages, which will help
clarify the calling context of the method in our diagnostic logs.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
In PR git-lfs#4446 we defined a new SSHTransfer structure and a set of methods
for it in our ssh/connection.go source file, and used the name "tr" for
the receiver variables of those methods.

Later, in commit 5dbbf13 of PR git-lfs#5674,
we imported our "tr" message translation package into the same source
file in order to format and localize an error message generated by the
startConnection() function.  Because this function is not one of the
methods of the SSHTransfer structure there was no conflict with the
"tr" receiver variables of those methods.

However, in subsequent commits in this PR we expect to revise one of
the SSHTransfer structure's methods to output an error message, and
will want to use the Get() method of the "tr" package's global Tr
variable.  We are not able to do this with the current name of the
"tr" receiver variable as it masks the "tr" package name within the
method's scope.

Therefore we now rename all our "tr" receiver variables to "st" in the
ssh/connection.go source file, so as to avoid any namespace conflicts
with our "tr" package.
chrisd8088 added a commit to chrisd8088/git-lfs that referenced this pull request Nov 4, 2024
In commit 691de51 of PR git-lfs#4446 we added
the "batch transfers with ssh endpoint (git-lfs-transfer)" test to our
t/t-batch-transfer.sh test script in order to validate that the new
SSH object transfer protocol for Git LFS was operating as expected.

This test only creates a single test object, and so does not cause
the Git LFS client to establish multiple SSH sessions during the batch
object transfer phase.  While the sole SSH session started during this
phase may create a control socket to multiplex its connection, it will
not be shared by any other SSH sessions, since only one is started.

In practice, this means that even if the "lfs.ssh.autoMultiplex"
configuration option is set to "true" and the available SSH client
is considered to be compatible with OpenSSH, the "batch transfers with
ssh endpoint (git-lfs-transfer)" test will never try to start an SSH
session with a value for the ControlMaster argument other than "yes".

Our test suite sets the GIT_SSH environment variable to refer to our
lfs-ssh-echo test utility program rather than an actual SSH client.
As noted in git-lfs#5903, at present the Git LFS client treats this program,
which it does not recognize as matching the filename of any known
SSH client, as compatible with OpenSSH, and so our tests invoke our
lfs-ssh-echo utility with the ControlMaster and ControlPath arguments.

As described in a prior commit in this PR, our lfs-ssh-echo test
utility program had a bug which prevented it from starting SSH
sessions with the ControlMaster argument set to "no", so the "batch
transfers with ssh endpoint (git-lfs-transfer)" test would have
failed if it tried to push more than a single object.

Such a test would also fail if it tried to fetch more than a single
object, due to the bug described in git-lfs#5880.  Specifically, we returned
a nil from the Connection() method of the SSHTransfer structure in our
"ssh" package when we had terminated all the SSH sessions, but then
sometimes tried to reference that nil pointer in the batchInternal()
method of the SSHBatchClient structure in the "tq" package, causing
a Go panic condition.

In prior commits in this PR we resolved these problems, first the bug
in the lfs-ssh-echo test utility and then the bad return value from the
Connection() method of the SSHTransfer structure in the Git LFS client.
In the case of the latter issue, we adjusted the Connection() method
to return a non-nil error when the requested session ID exceeds the
maximum number of sessions permitted, which also covers the case where
all the sessions have already been terminated.

Therefore we can now introduce additional tests of the SSH object
transfer protocol which push and fetch multiple objects.  In these
tests we use the new assert_remote_object() function that we defined
in a prior commit in this PR to confirm that the pushed objects are
all written to the remote repository.  As well, we use the new
assert_ssh_transfer_sessions() function we defined in another prior
commit in this PR to check that the number of SSH sessions that
create a control socket matches our expectations, as do the number
of startup, success, and termination trace log messages.

The first of our new tests leaves the maximum number of concurrent
object transfers set to the default value of eight, so that all three
of the objects we create in the test are pushed in a single batch,
and may also be fetched in a single batch if the available version
of Git is 2.11.0 or higher.  To push or fetch these objects in a
single batch requires that the Git LFS client establish as many as
three separate SSH sessions per invocation, of which only the first
should create a control socket.

The second of our new tests sets the maximum number of concurrent
object transfers to two, so we expect to see a maximum of only two SSH
sessions per invocation of the Git LFS client, and only the first of
these sessions should start an SSH connection with a control socket.

Prior to version 2.11.0, Git did not support the "process" filter
attribute, and so during a clone operation Git LFS would be invoked
via the "smudge" filter instead, once for each object.  When testing
with such a version of Git, our assert_ssh_transfer_sessions()
function adjusts its expectations to account for the fact that each
invocation of Git LFS during a clone operation will establish its
own SSH session with a control socket, and so the total number of
trace log messages from these sessions should match the total number
of objects being fetched.
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.

Pure SSH-based protocol
9 participants