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

Pass port to ssh-keyscan when known_hosts not provided #2803

Merged
merged 1 commit into from Jun 11, 2020
Merged

Pass port to ssh-keyscan when known_hosts not provided #2803

merged 1 commit into from Jun 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Jun 10, 2020

Changes

Fixes #2801

Creds-init performs an ssh-keyscan if no known_hosts file is
provided as part of a Secret. When the ssh server is using
a custom port ssh-keyscan expects the port to be provided
with the -p flag. Currently Tekton does not provide the flag
resulting in failure to generate known_hosts. The error for
this failure is also very opaque - manifesting as an "invalid
flag" message in the creds-init initContainer log and making
no mention of ssh-keyscan.

This commit:

  • adds the -p flag to ssh-keyscan calls when a port is specified
    in the given git URL.
  • adds an additional note to auth.md mentioning ssh-keyscan
  • wraps any error returned by ssh-keyscan to mention the utility,
    hopefully aiding future debugging

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Specifying a git ssh server url with custom port previously failed during credentials initialization if a known_hosts field was not specified in the Secret. This has been fixed.

@ghost ghost added the kind/bug Categorizes issue or PR as related to a bug. label Jun 10, 2020
@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 10, 2020
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/credentials/gitcreds/ssh.go 75.8% 77.1% 1.4

@ghost
Copy link
Author

ghost commented Jun 10, 2020

I plan to migrate this fix over to #2671 when this is merged.

@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/credentials/gitcreds/ssh.go 75.8% 76.1% 0.3

Creds-init performs an ssh-keyscan if no known_hosts file is
provided as part of a Secret. When the ssh server is using
a custom port ssh-keyscan expects the port to be provided
with the -p flag. Currently Tekton does not provide the flag
resulting in failure to generate known_hosts. The error for
this failure is also very opaque - manifesting as an "invalid
flag" message in the creds-init initContainer log and making
no mention of ssh-keyscan.

This commit:
- adds the -p flag to ssh-keyscan calls when a port is specified
in the given git URL.
- adds an additional note to auth.md mentioning ssh-keyscan
- wraps any error returned by ssh-keyscan to mention the utility,
hopefully aiding future debugging
@tekton-robot
Copy link
Collaborator

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/credentials/gitcreds/ssh.go 75.8% 73.0% -2.8

@ghost
Copy link
Author

ghost commented Jun 10, 2020

/test pull-tekton-pipeline-integration-tests

1 similar comment
@ghost
Copy link
Author

ghost commented Jun 10, 2020

/test pull-tekton-pipeline-integration-tests

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

/lgtm
/cc @afrittoli @chmouel

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2020
@chmouel
Copy link
Member

chmouel commented Jun 11, 2020

nicely done @sbwsg

/lgtm

Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

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

Nice, really handy, thank you!
/approve

@tekton-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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

@tekton-robot tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2020
@tekton-robot tekton-robot merged commit 1caf5bf into tektoncd:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git Credential Initializer doesn't include port when running ssh-keyscan
4 participants