Skip to content
This repository was archived by the owner on Jan 9, 2023. It is now read-only.

Conversation

@dippynark
Copy link
Contributor

This PR fixes the following:

  • when testing connection to the bastion before creating vault tunnels, we use the -N flag, which shouldn't be used as we are running a remote command (/bin/true)
  • when creating vault tunnels, if a problem occurs, there was a chance we could hang indefinitely - this change reduces the chance of a blocking channel so that this hanging doesn't occur
Improve Vault tunnel creation preventing indefinite hanging in certain situations

@jetstack-bot jetstack-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jun 5, 2018
@jetstack-bot jetstack-bot requested a review from charlieegan3 June 5, 2018 13:34
@jetstack-bot jetstack-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 5, 2018
@dippynark dippynark changed the title Fix vault tunnel creationg Improve vault tunnel creation Jun 5, 2018
@dippynark dippynark force-pushed the fix-vault-tunnel-creation branch 2 times, most recently from 41c8ef6 to 439057b Compare June 5, 2018 15:39
@dippynark dippynark force-pushed the fix-vault-tunnel-creation branch from 439057b to 50183e9 Compare June 5, 2018 15:41
return
}

if health.Standby == false && health.Sealed == false {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we have to check here for health.Initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed

VaultStateUnsealed
VaultStateUnintialised
VaultStateErr
vaultTunnelCreationTimeoutSeconds = 5
Copy link
Contributor

Choose a reason for hiding this comment

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

Please test multi/single cluster E2E, I am not too sure if that is a good value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It worked fine with 5, but have upped it to 10 to be safe

@simonswine
Copy link
Contributor

/assign @dippynark

@dippynark what is your feeling about my comments?

@dippynark dippynark force-pushed the fix-vault-tunnel-creation branch from 42032bf to d0a470b Compare June 8, 2018 12:00
@simonswine
Copy link
Contributor

Thanks luke for tracking this down

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2018
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: simonswine

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants