Skip to content

Add host.ssh_port option and refactor gateway initialization #1090

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

Closed
wants to merge 45 commits into from

Conversation

kke
Copy link
Contributor

@kke kke commented Feb 19, 2019

Fixes #1062
Fixes #1072
Fixes #1088

Replaces #1071

Adds ssh_port option to host options:

hosts:
  - address: 10.0.0.1
    ssh_port: 2022

And bastion options:

hosts:
  - address: 10.0.0.1
    ssh_port: 22
    bastion:
      address: 10.0.0.2
      ssh_port: 20202

Refactored the Net::SSH::Gateway from SSH::Client#gateway to Transport::Gateway for better control (should benefit the disconnecting in #1003) and because bastion.host.ssh.gateway opened extra SSH connections.

Also adds the missing ssh_proxy_command attribute to bastion.

All k8s-client traffic is now tunneled through ssh.

ConfigureClient phase is replaced with WarmUpClientCache phase.

@kke kke added enhancement New feature or request chore labels Feb 19, 2019
@kke kke requested a review from jakolehm February 19, 2019 12:33
@kke kke mentioned this pull request Feb 19, 2019
@kke
Copy link
Contributor Author

kke commented Feb 26, 2019

PTAL

@kke
Copy link
Contributor Author

kke commented Feb 26, 2019

This probably still needs some way to track clients that are using the gateway, otherwise it will shutdown a gateway that is still used by some other client.

#1088 will likely need something from this.

@kke kke added bug Something isn't working and removed chore labels Feb 27, 2019
@kke
Copy link
Contributor Author

kke commented Feb 27, 2019

Files changed: 28. Seems like this grew out of proportion.

def bastion
@bastion ||= @opts.delete(:bastion)
def to_s
"SSH #{host.user}@#{host.address}:#{host.ssh_port}#{' using proxy command' if host.ssh_proxy_command}#{" via #{@gateway}->127.0.0.1:#{@port} " if @gateway}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

@kke
Copy link
Contributor Author

kke commented Mar 1, 2019

E2e now uses bastion and proxy command

@jakolehm
Copy link
Contributor

Conflict..

@kke
Copy link
Contributor Author

kke commented Mar 11, 2019

Unconflict..

def short_hostname
return nil unless hostname

hostname.split('.').first
end

# @return [Hash]
def ssh_options
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this should be under Transport (for example Transport::SshHelpers module)

end

# @return [Pharos::Transport::Gateway]
def gateway
Copy link
Contributor

Choose a reason for hiding this comment

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

Not completely happy about the idea that Host is becoming a "god" object that knows everything. For example gateway requires host and Host requires Gateway because of kube_client 🤔 /cc @jnummelin

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably all ssh specific stuff should be under Transport (somehow).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I agree. Now Host, Bastion, Gateway all entangle pretty badly and really hard to follow which is used where.

Copy link
Contributor Author

@kke kke Mar 12, 2019

Choose a reason for hiding this comment

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

I moved stuff around.

Now there's Pharos::Kube::Client that understands Configuration::Host, so that host can just do @kube_client ||= Pharos::Kube::Client.new(self).

The host ssh options are generated in Pharos::Transport::SSH.options_for(host).

The Host now has:

  • def gateway; @gateway ||= Pharos::Transport::Gateway.new(self); end
  • def transport; @transport ||= Pharos::Transport.for(self); end
  • def kube_client; @kube_client ||= Pharos::Kube::Client.new(self); end
  • def disconnect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative to having these memoized in Host I guess would be to go back to
Pharos::SshManager.clients[host.address] / Pharos::GatewayManager.gateways[host.address] type of connection collections or maybe have some sort of Pharos::Transport delegator/decorator and just have host.transport and all those kube_client / exec / file / gateway stuff would be contained there.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the latter sounds pretty good

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that gateway should be masked behind transport. Transport already gets host (like Gateway) as a parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kke @jnummelin are you fine with gateway being here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kke
Copy link
Contributor Author

kke commented Mar 19, 2019

#1191 fixes #1062
#1072 and #1088 can be fixed more simply separately now with #1185

@kke kke closed this Mar 19, 2019
@kke kke deleted the feature/gateway_refactorings branch March 19, 2019 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants