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

add proxy/bastion for connection #48

Merged
merged 1 commit into from
Oct 23, 2024

Conversation

lilyLuLiu
Copy link
Collaborator

@lilyLuLiu lilyLuLiu commented Sep 27, 2024

@@ -94,16 +103,19 @@ scp_from_cmd () {
# Generate SSH command
ssh_cmd () {
cmd=""
if [[ ! -z "${TARGET_HOST_KEY_PATH+x}" ]]; then
if [[ -n "${PROXY_HOST}" && -n "${PROXY_USERNAME}" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as before


# Generate SCP command
# $1 local path
# $2 remote path
scp_to_cmd () {
if [[ ! -z "${TARGET_HOST_KEY_PATH+x}" ]]; then
if [[ -n "${PROXY_HOST}" && -n "${PROXY_USERNAME}" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the if [[ -n "${PROXY_HOST}" && -n "${PROXY_USERNAME}" ]]; then should have its own if...

As one thing is the proxy settings which would be added on the scp part, also here I am just seeing the proxy host and username but it probably would require a private key.

The second part is the if for the target host, if it uses a private key it directly uses the scp -i option if not and it will connect though a pass it uses the sshpass.

But as you see in both cases if we set proxy those settings should be there

@@ -84,7 +91,9 @@ scp_to_cmd () {
# $1 remote path
# $2 local path
scp_from_cmd () {
if [[ ! -z "${TARGET_HOST_KEY_PATH+x}" ]]; then
if [[ -n "${PROXY_HOST}" && -n "${PROXY_USERNAME}" ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before

lib/common/remote.sh Show resolved Hide resolved
@adrianriobo
Copy link
Collaborator

Only see PROXY_USERNAME and PROXY_HOST, still missing PROXY_PRIVATE_KEY

Unfortunately do not see an easy option (i.e for target host -i) to set the private key for proxy, it seems you would need to create a config file: ~/.ssh/config

And template it with the envs:

Host proxy_host
    HostName proxy_host
    User proxy_user
    IdentityFile /path/to/private_key_for_proxy

Host target_host
    HostName target_host
    User target_user
    IdentityFile /path/to/private_key_for_target
    ProxyJump proxy_host

@adrianriobo
Copy link
Collaborator

When you add the private key, I would ask you to test this and comment on the test is successful here 🙏

If you want for testing this without the testing farm stuff as that may complicate things we can use mapt (ask me for this) to spin an airgap machine that will output all the details for both bastion (proxy) and target host so you can use them for testing this.

@lilyLuLiu
Copy link
Collaborator Author

@adrianriobo , I designed that deliverset offers two options for SSH connection:
(1) provide an SSH Config file: proxy can only be used this way.
(2) provide target username and password/key.
What do you think?

@adrianriobo
Copy link
Collaborator

I am fine with it, but still if user pass an private key for target host and a private key for proxy you probably would need to compose the conf file no?

@lilyLuLiu
Copy link
Collaborator Author

@adrianriobo my plan is the user pass the config file to deliverset, instead of deliverset compose the config file. What do you think?

@adrianriobo
Copy link
Collaborator

adrianriobo commented Oct 21, 2024

@lilyLuLiu I would prefer the other way around, here are some thoughts on why:

  • If you force the user to create the file outside that would be a breaking change non backward compatible (and this is used across several teams ...so it could create issues or even dismiss the usage of deliverest)

  • As a direct consecuence instead of having 1 place with the logic you would split it forcing tekton to have a place for creating the file, gh action another one... and so on

  • The requirement for the file is due to the underlaying ssh client ... that would force a hard dependecy on the usage of deliverst... people should not need to know the client being used ..as long as they pass basic info the setup should be made internally ... what happens if we build a new version of deliverest which contains a new version of open ssh with some change on the format of the file? As the file creation has been delegated to external users...they would need to track the version know about the change on the file and apply everywhere + deal with how they create the file based on the deliverest version they are using (and they could be use different ones depending on the component)

@lilyLuLiu lilyLuLiu force-pushed the main branch 5 times, most recently from f69d62b to 37f0453 Compare October 22, 2024 10:14
@lilyLuLiu
Copy link
Collaborator Author

@adrianriobo Thanks for the explanation.
I have re-submitted the code that supports bastion with parameters. Could you review it when you have time?

@lilyLuLiu lilyLuLiu requested a review from adrianriobo October 22, 2024 10:19
@adrianriobo
Copy link
Collaborator

Sure let me go through it

Copy link
Collaborator

@adrianriobo adrianriobo left a comment

Choose a reason for hiding this comment

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

Did you try this with mapt on airgap scenario?

lib/common/remote.sh Show resolved Hide resolved
@adrianriobo
Copy link
Collaborator

In general and just if testing was ok LGTM

@lilyLuLiu
Copy link
Collaborator Author

@adrianriobo verified with mapt arigap scenario.

@lilyLuLiu lilyLuLiu requested a review from adrianriobo October 23, 2024 08:34
@adrianriobo adrianriobo merged commit 28bbbfb into devtools-qe-incubator:main Oct 23, 2024
1 check failed
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