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

allow proxy during SSH host key scan in FluxCD CLI #827

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adone
Copy link

@adone adone commented Nov 27, 2024

ssh.Dial uses net.DialTimeout under the hood src
and there is no possibility to use a proxy when running command like flux create source git --url ssh://[email protected]/repository.git (unlike for the source controller with ALL_PROXY)

so I replaced ssh.Dial with its almost full internal implementation
except net.DialTimeout replaced with proxy.Dial
like it is done in go-git

@adone adone requested a review from a team as a code owner November 27, 2024 13:52
ssh.Dial uses net.DialTimeout under the hood
and there is no possibility to use a proxy
when running command like `flux create source git`

so we use almost all internal implementation of ssh.Dial
except net.DialTimeout is replaced with proxy.Dial
like it is done in go-git

Signed-off-by: Artem Nistratov <[email protected]>
@adone adone force-pushed the proxy-for-host-check branch from d0fe81e to 63c6588 Compare November 27, 2024 14:00
Copy link
Member

@makkes makkes left a comment

Choose a reason for hiding this comment

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

I'm generally in favor of this change. What I can't judge right now is which Flux CLI commands are affected by this. I couldn't find any controller code affected by this.

It will be very important to document this change with respect to the affected commands.

ssh/host_key.go Outdated
defer client.Close()
ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()
// support for ALL_PROXY ENV varaible
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// support for ALL_PROXY ENV varaible
// this reads the ALL_PROXY environment variable

@adone
Copy link
Author

adone commented Nov 27, 2024

I've tried to improve flux create source git, but flux bootstrap git with SSH protocol is affected too

as I understand another PR should be made to document ALL_PROXY variable support in CLI (with pkg version update also)

P.S. and tests for go-git's clone are broken, I'm trying to understand why

@adone
Copy link
Author

adone commented Nov 27, 2024

ah! ScanHostKey ignores any SSH/network errors if it manages to get host keys

@adone adone force-pushed the proxy-for-host-check branch from 0c5c378 to 8c10c5e Compare November 27, 2024 16:23
previously ScanHostKey ignored any SSH/network errors
in case it managed to get host keys

to make it more obvious we imitate `ssh.Dial` with `sshDial` func

Signed-off-by: Artem Nistratov <[email protected]>
@adone adone force-pushed the proxy-for-host-check branch from 8c10c5e to ddb3fd8 Compare November 27, 2024 16:39
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.

2 participants