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

feat: SSH-Agent Support #306

Merged
merged 12 commits into from
May 22, 2023
Merged

feat: SSH-Agent Support #306

merged 12 commits into from
May 22, 2023

Conversation

zoop-btc
Copy link
Contributor

@zoop-btc zoop-btc commented Apr 18, 2023

Hi,

Since I cannot allow ssh password authentication I have added a quick and dirty ssh-agent support for linux and a ssh user override.
I have not implemented anything in windows since it would require multiple source files and I'm not familiar with go. This repo seems to do that so may be a good guideline for future implementation.

This draft is meant for discussion only.

Contributor's Note

Please mark the following items with an [x] if they apply to your PR.
Leave the [ ] if they are not applicable, or if you have not completed the item.

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated templates in /examples for any new or updated resources / data sources.
  • I have ran make examples to verify that the change works as expected.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates to #307

@zoop-btc zoop-btc mentioned this pull request Apr 18, 2023
@zoop-btc zoop-btc changed the title SSH-Agent Support feat: SSH-Agent Support Apr 18, 2023
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Hi @zoop-btc, thanks for contribution!

To be honest, I would skip Windows support for ssh agent for now and add it later if/when it will be requested.

Overall I think this change makes sense.
I've added some other comments, could you please take a look?

proxmoxtf/provider/schema.go Outdated Show resolved Hide resolved
proxmoxtf/provider/provider.go Outdated Show resolved Hide resolved
proxmoxtf/provider/schema.go Outdated Show resolved Hide resolved
proxmox/virtual_environment_client.go Outdated Show resolved Hide resolved
proxmox/virtual_environment_nodes.go Outdated Show resolved Hide resolved
proxmoxtf/provider/schema.go Outdated Show resolved Hide resolved
proxmoxtf/provider/schema.go Outdated Show resolved Hide resolved
proxmoxtf/provider/schema.go Outdated Show resolved Hide resolved
@bpg bpg mentioned this pull request Apr 22, 2023
@zoop-btc
Copy link
Contributor Author

zoop-btc commented May 1, 2023

I think the pull request is in a good place. However I am unsure how to approach the ssh-host override as it's a per-node setting. Do you think it makes sense to define a map of proxmox-node names => proxmox ssh-hosts inside the provider configuration?
@bpg

@bpg
Copy link
Owner

bpg commented May 2, 2023

Do you think it makes sense to define a map of proxmox-node names => proxmox ssh-hosts inside the provider configuration?

I was thinking about it, but that doesn't looks great in the template though.
Maybe try another option: use existing approach with IP address lookup, but don't stop on the first interface, try them all from the list until connected?

@bpg
Copy link
Owner

bpg commented May 10, 2023

Hey @zoop-btc, do you need a hand with this PR? I can help with testing / implementation of the last piece. Or we can do that in a separate PR.

@zoop-btc
Copy link
Contributor Author

@bpg I can do it, but since I'm busy I'll only come around to it till late next week. If you want to do it though feel free :)
Also can you stop merging the main branch? I will rebase this branch when I get back to it anyways.

@bpg
Copy link
Owner

bpg commented May 17, 2023

@zoop-btc sorry about all that annoyance, I'm working on a new PVE cluster setup and the agent feature is really helpful for my case, so I used your branch for testing 😅
Yeah, I can continue with this, if you don't mind. Will refrain from pushing until it's done.

@bpg bpg marked this pull request as ready for review May 22, 2023 17:27
Copy link
Owner

@bpg bpg left a comment

Choose a reason for hiding this comment

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

Nice work @zoop-btc to put this together!

I've added agent_socket argument and made a few minor tweaks to allow agent functionality on MacOS as well. Also, rebased on main

I'll add you as a co-author when merge the PR

@bpg bpg merged commit 9fa9242 into bpg:main May 22, 2023
@ghost ghost mentioned this pull request May 22, 2023
@zoop-btc zoop-btc deleted the agent branch June 4, 2023 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants