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

chore: set beelocal branch to update-k3s-1.30.3 and go 1.23 upgrade #4878

Merged
merged 10 commits into from
Nov 5, 2024

Conversation

gacevicljubisa
Copy link
Contributor

@gacevicljubisa gacevicljubisa commented Oct 27, 2024

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

  • Update Go version to 1.23.
  • Upgrade K3s version to 1.30.3.
  • Update golangci-lint version to 1.61.0
  • Remove deprecated elliptic.Marshal function

Open API Spec Version Changes (if applicable)

Motivation and Context (Optional)

Related Issue (Optional)

Screenshots (if appropriate):

@istae istae changed the title chore: set beelocal branch to update-k3s-0.30.3 chore: set beelocal branch to update-k3s-0.30.3 and go 1.23 upgrade Oct 29, 2024
// TODO: check if this is the correct way to serialize the public key
// Is this the only curve we support?
// Should we have switch case for different curves?
pubBytes := crypto.S256().Marshal(key.X, key.Y)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since elliptic.Marshal was deprecated is there a reason we are not using equivalent from crypto/ecdh, similar with you have changed in pkg/keystore/file/key.go ? Reference: https://pkg.go.dev/crypto/elliptic#Marshal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here, we only have public key, and we could have something like this:

pubKey, err := key.ECDH()
if err != nil {
	return nil, fmt.Errorf("generate key: %w", err)
}
b = append(b, pubKey.Bytes()...)

But unit tests will return: unsupported curve by crypto/ecdh. Because of this I used specific curve that we are also using in NewEthereumAddress function.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this comment still relevant?

@gacevicljubisa
Copy link
Contributor Author

gacevicljubisa commented Oct 31, 2024

Please take a moment to carefully review the following commit f8dee190, which is made to remove deprecated elliptic.Marshal function.

Some comments are in the code, and some comments are in the commit itself.

The related changes in go-ethereum are done on this 28946 PR.

@gacevicljubisa gacevicljubisa added the help wanted Extra attention is needed label Oct 31, 2024
@@ -83,7 +83,11 @@ func encryptKey(k *ecdsa.PrivateKey, password string, edg keystore.EDG) ([]byte,
}
addr = a
case elliptic.P256():
addr = elliptic.Marshal(elliptic.P256(), k.PublicKey.X, k.PublicKey.Y)
privKey, err := k.ECDH()
Copy link
Member

Choose a reason for hiding this comment

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

can we add a very simple unit test that shows that the result of elliptic.Marshal(elliptic.P256(), k.PublicKey.X, k.PublicKey.Y) matches exactly the result of your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the commit with added tests... feel free to suggest any improvements

@gacevicljubisa
Copy link
Contributor Author

Also it seems that ScalarMult is also deprecated, but linter doesn't report the issue. Here is one of the the function calls

@gacevicljubisa gacevicljubisa requested a review from zelig November 3, 2024 14:23
@gacevicljubisa gacevicljubisa marked this pull request as ready for review November 3, 2024 14:26
@gacevicljubisa gacevicljubisa removed the help wanted Extra attention is needed label Nov 5, 2024
@gacevicljubisa gacevicljubisa changed the title chore: set beelocal branch to update-k3s-0.30.3 and go 1.23 upgrade chore: set beelocal branch to update-k3s-1.30.3 and go 1.23 upgrade Nov 5, 2024
@gacevicljubisa gacevicljubisa merged commit ef966b1 into master Nov 5, 2024
14 checks passed
@gacevicljubisa gacevicljubisa deleted the bump-local-cluster branch November 5, 2024 15:20
@gacevicljubisa gacevicljubisa restored the bump-local-cluster branch November 5, 2024 15:20
@gacevicljubisa gacevicljubisa deleted the bump-local-cluster branch November 7, 2024 14:25
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.

5 participants