-
-
Notifications
You must be signed in to change notification settings - Fork 117
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
feat(provider): add support for ssh-agent
on Windows
#1270
Conversation
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
One concern I had: ssh-agent on Windows doesn't set the A couple of possible solutions: either add a fallback to |
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
@bpg For consistency, I think assuming the socket path (if the user doesn't provide one) is the right behavior. I made a note of this in the documentation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Sparta142, thanks a lot for contributing this!
I've made a few comments, could you please take a look?
Other than that, it is good to go! 🙂
proxmoxtf/provider/provider.go
Outdated
// On Windows, binaries using SSH typically assume a hardcoded name for the ssh-agent socket | ||
if runtime.GOOS == "windows" && sshAgentSocket == "" { | ||
sshAgentSocket = `\\.\pipe\openssh-ssh-agent` | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to move this block to windows-specific proxmox/ssh/client_windows.go, inside the dialSocket
func
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dialSocket
will never get an empty address because of this:
terraform-provider-proxmox/proxmox/ssh/client.go
Lines 534 to 537 in 6ae9b58
if c.agentSocket == "" { | |
return nil, errors.New("failed connecting to SSH agent socket: the socket file is not defined, " + | |
"authentication will fall back to password") | |
} |
What if I change this so that it fills in the default socket address on Windows, and returns the error otherwise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point, missed that. This check can be moved into platform-specific dialSocket
as well. So if address is missing we'll supply the default value on Windows or throw an error on other platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the change, let me know what you think
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
Signed-off-by: Brian Karshick <Sparta142@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
@all-contributors please add @Sparta142 for code |
I've put up a pull request to add @Sparta142! 🎉 |
Contributor's Note
/docs
for any user-facing features or additions./fwprovider/tests
for any new or updated resources / data sources.make example
to verify that the change works as expected.Proof of Work
Community Note
Closes #1257