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

fixed release-notes ssh git checkout website pr #2421

Merged

Conversation

csantanapr
Copy link
Member

What type of PR is this?

/kind bug

Which issue(s) this PR fixes:

Fixes #2420

Fixed krel release-notes git ssh fatal error when using single flag `--create-website-pr`

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/release-eng Issues or PRs related to the Release Engineering subproject labels Feb 9, 2022
@k8s-ci-robot k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Feb 9, 2022
@csantanapr
Copy link
Member Author

/assign @puerco

Copy link
Member

@puerco puerco left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Carlos, before moving on with this one there is just one concern to discuss:

@@ -788,7 +788,7 @@ func releaseNotesJSON(repoPath, tag string) (jsonString string, err error) {

// Preclone the repo to be able to read branches and tags
logrus.Infof("Cloning %s/%s", git.DefaultGithubOrg, git.DefaultGithubRepo)
repo, err := git.CloneOrOpenDefaultGitHubRepoSSH(repoPath)
repo, err := git.CloneOrOpenGitHubRepo(repoPath, git.DefaultGithubOrg, git.DefaultGithubRepo, false)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the change @csantanapr
What this change will do is avoid cloning the repo via ssh and instead do all the copy operations via https. I'm willing to have this change because I guess members of the release team will bump into fewer problems as cloning with https is simpler.

But I'm concerned that we are not really getting to the root of the problem. If there is a problem with one of the git dependencies below it would be useful to know so that we can fix it.

Any idea what may be causing the key errors?

Copy link
Member Author

Choose a reason for hiding this comment

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

The latest version of git-go doesn't have the fix form it's dependency go-git/go-git#411

I found this on argocd issue argoproj/argo-cd#7723

the Go SSH client also doesn’t yet support RSA with SHA-2, so we recommend using an Ed25519 key there
https://github.blog/2021-09-01-improving-git-protocol-security-github/#libgit2-and-other-git-clients

I believe that's why its having issues despite not using DSA.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, @justaugustus I'm not familiar with kpromo pr is this is a tool I need to run against this PR to get it merge?

Copy link
Member

Choose a reason for hiding this comment

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

Hi, @justaugustus I'm not familiar with kpromo pr is this is a tool I need to run against this PR to get it merge?

@csantanapr -- It's a tool we use for image promotion manifest updates, which will trigger the creation of a PR. I figure you can borrow some logic from it to create the release notes PRs here. :)

Copy link
Member

Choose a reason for hiding this comment

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

That code was first borrowed from the release notes code hehe :)

@csantana, the flow that @justaugustus points out is also using https, I think we can switch krel release-notes safely.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words there is agreement to merge this PR 😉

@csantanapr
Copy link
Member Author

csantanapr commented Feb 10, 2022

@puerco
As I pointed out in the issue #2420
There is also another point of consistency if you we use the flag --create-website-pr by itself the git clone to kubernetes-sig/release is in https and kubernetes/kubernetes in ssh. if the flag --create-draft-pr is added then all the multiple git clones are always in https including kubernetes/kubernetes and no ssh is used

@csantanapr
Copy link
Member Author

csantanapr commented Feb 10, 2022

Another point to add is, for a new person helping with release-notes.
Now there are 2 requirements.

  1. create a GITHUB_TOKEN (which we document)
  2. create a SSH Key and configure the system to connect via git ssh (We don't current document)

If we drop the ssh requirement then the new person only needs to set GITHUB_TOKEN

@csantanapr
Copy link
Member Author

I did some more testing. I though it was my rsa2 ssh key maybe it was old or the keylength 2048

I tried with the 2 keys github has documented in their website https://docs.github.com/en/authentication/connecting-to-github-with-ssh/generating-a-new-ssh-key-and-adding-it-to-the-ssh-agent

ssh-keygen -t ed25519 -C "[email protected]"

and

ssh-keygen -t rsa -b 4096 -C "[email protected]"

I was able to validate they both work with

Also cloning the repo

git clone [email protected]:kubernetes/kubernetes.git

Can you try your self @puerco just in case, first remove the repo

rm -rf /tmp/k8s || rm -rf /var/folders/gl/l4j1f7bj0hlb83kk3_9wx4jm0000gn/T/k8s

then run with this

krel release-notes --fix --fork puerco --tag v1.20.5 --list-v2 --create-website-pr --log-level trace

@csantanapr
Copy link
Member Author

csantanapr commented Feb 10, 2022

It might be a MacOS thing.
I was able to get something working on Linux using a new rsa key it took forever to clone and then it died. Need to look for another linux VM

INFO Cloning kubernetes/kubernetes                 file="cmd/release_notes.go:866"
DEBU Using repository url "[email protected]:kubernetes/kubernetes"  file="git/git.go:370"
DEBU Using existing repository path "/tmp/k8s"     file="git/git.go:373"
Enumerating objects: 1304206, done.
Counting objects: 100% (211/211), done.
Compressing objects: 100% (145/145), done.
Total 1304206 (delta 89), reused 75 (delta 64), pack-reused 1303995
Killed

@csantanapr
Copy link
Member Author

@puerco
I found the root cause of the issue.
To get krel to work to gitclone with ssh, the key MUST be added to the ssh-agent on any operating system included MacOS, regardless is MacOs works to work git CLI git and ssh without keys in ssh-agent or even if the private key doesn't have a passphrase.

I was able to get it working consistently on both Linux and MacOS when the key gets added to ssh-add

ssh-add ~/.ssh/<id_ed25519 or id_rsa>

@puerco
Copy link
Member

puerco commented Feb 11, 2022

OK, let's merge this to make it easier for the release team members to submit the PRs. Thanks @csantanapr !
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csantanapr, puerco

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4a52960 into kubernetes:master Feb 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Feb 11, 2022
@csantanapr
Copy link
Member Author

Woot !

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. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

krel release-notes with --create-website-pr errors with git ssh fingerprint
4 participants