Skip to content

Fix SSH into GCP instance always using external IP#35151

Merged
atburke merged 1 commit intomasterfrom
atburke/gcp-ssh-addr
Dec 7, 2023
Merged

Fix SSH into GCP instance always using external IP#35151
atburke merged 1 commit intomasterfrom
atburke/gcp-ssh-addr

Conversation

@atburke
Copy link
Copy Markdown
Contributor

@atburke atburke commented Nov 29, 2023

This change fixes a bug where the discovery service would always try to SSH into a GCP instance with its external IP address, which may not exist.

Changelog: Fixed GCP VM auto-discovery not using instances' internal IP address

@GavinFrazar
Copy link
Copy Markdown
Contributor

i'm unfamiliar with how GCP VM discovery works - is it always the case that the discovery service will be on the same network as the VM and can access the internal IP?

Copy link
Copy Markdown
Contributor

@avatus avatus 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 not too savy with how GCP is used with teleport. My instinct is saying that because this is just now coming up, there are some customers/users that rely on this external address and it works for them. I feel like this is a large change that might have some repercussions?

@GavinFrazar
Copy link
Copy Markdown
Contributor

GavinFrazar commented Dec 2, 2023

With trying both external and internal IP - what happens when external IP isn't reachable, e.g. because of net security rules? Does it hang until timeout before trying with the internal IP?

If it does hang, maybe we can try dialing both and use whichever one succeeds first?

@atburke atburke force-pushed the atburke/gcp-ssh-addr branch from bcbe853 to ee4301f Compare December 7, 2023 00:50
@atburke
Copy link
Copy Markdown
Contributor Author

atburke commented Dec 7, 2023

@GavinFrazar I think dialing both is going to get complicated, and running the installer isn't a super time-sensitive task. I've added a generous timeout to the dialer to keep it from hanging too long if the external IP isn't accessible.

Comment thread lib/cloud/gcp/vm.go Outdated
Comment on lines 568 to 569
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If the message being logged doesn't use any formatting directives it's more efficient to avoid the f variants.

Suggested change
logrus.Debugf(string(stdout))
logrus.Debugf(string(stderr))
logrus.Debug(string(stdout))
logrus.Debug(string(stderr))

Comment thread lib/cloud/gcp/vm.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
logrus.WithError(err).Debugf("Command exited with error.")
logrus.WithError(err).Debug("Command exited with error.")

Comment thread lib/cloud/gcp/vm.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this case also indicate that the command was successful?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes (or at least that the installer exited with code 0)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How come we only log the output in the if block below and not here too?

This change fixes a bug where the discovery service would always try
to SSH into a GCP instance with its external IP address, which may
not exist.
@atburke atburke force-pushed the atburke/gcp-ssh-addr branch from 3b5b50c to 733945c Compare December 7, 2023 19:52
@atburke atburke enabled auto-merge December 7, 2023 19:52
@atburke atburke added this pull request to the merge queue Dec 7, 2023
Merged via the queue into master with commit 7221745 Dec 7, 2023
@atburke atburke deleted the atburke/gcp-ssh-addr branch December 7, 2023 20:29
@public-teleport-github-review-bot
Copy link
Copy Markdown

@atburke See the table below for backport results.

Branch Result
branch/v13 Create PR
branch/v14 Create PR

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.

4 participants