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

feat: add AutoTLS example #3103

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

feat: add AutoTLS example #3103

wants to merge 16 commits into from

Conversation

2color
Copy link
Contributor

@2color 2color commented Dec 17, 2024

This adds an example showing how to use the p2p-forge client library with the AutoTLS backend to issue a wild card certificate.

Credits to @guillaumemichel for writing the initial code in this example.

Fix before merging

  • The libp2p.direct address is not announced by the peer via identify, even though the peer has successfully minted a TLS certificate. Fixed by c16ebd9
  • Store peer identity on disk to avoid generating a new certificate on each run

examples/autotls/README.md Outdated Show resolved Hide resolved
@2color
Copy link
Contributor Author

2color commented Dec 19, 2024

@MarcoPolo Is it a problem this PR upgrades the go to 1.23 in go.mod?

I believe this happened due to dependency on p2p-forge

@2color 2color mentioned this pull request Dec 20, 2024
2 tasks
@2color 2color requested a review from lidel January 8, 2025 09:53
@guillaumemichel
Copy link
Contributor

ipshipyard/p2p-forge#29 should solve the build issue

@2color
Copy link
Contributor Author

2color commented Jan 8, 2025

When running this, occasionally it doesn't retrieve (or even request). Here are logs

this also passes correct logger so debug messages from
p2p-forge/client are printed correctly
@lidel lidel marked this pull request as draft January 8, 2025 18:07
Comment on lines +3 to +5
go 1.23

toolchain go1.22.3
toolchain go1.23.3
Copy link
Member

Choose a reason for hiding this comment

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

💭 I suspect this produces staticcheck ./... error:

In the past, the fix was to:

  1. check if go install honnef.co/go/tools/cmd/staticcheck@lateststaticcheck ./... passes locally
  2. ask IPDX to update Unified GO CI to use new staticcheck

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, staticcheck@a093f7c2d3d45d5104fb3414ae939a98be37be02 used on CI does not support 1.23 correctly, and 1.22 is also bit out of date: https://github.com/dominikh/go-tools/releases

Will ping IPDX.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked into this. In the go-check workflow, we're choosing the version of staticcheck to use based on the Go version that we're using. We already have the latest staticcheck version associated with the latest Go version. This PR changes the Go version in the go.mod file to 1.23, but it didn't change it in the go-check workflow file. I updated it here - 5da91ac

The reason why the Go version is explicitly set in the workflow file is because the maintainers of this repository preferred the version string to be present in the workflow run name, and this was the way to achieve that. You'll notice that in most other repos using Unified GitHub Workflows, the names of the workflow runs will contain things like this/next. In those, all we need to do to update the version used by the workflow is to update the go.mod.

We should also have a look at the go-test workflow. Should we remove testing with Go 1.22 in this PR from here - https://github.com/libp2p/go-libp2p/blob/add-autotls-example/.github/workflows/go-test.yml#L20 - as well?

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for figuring this out. go-check fix should be good enough.
I don't think we should remove 1.22 support, the main /go.mod still uses that. We should not change this without approval of @libp2p/go-libp2p-maintainers

@2color I think the source of problem here in that this PR is bumping go version to 1.23 in examples/go.mod but the main go.mod remains on 1.22.

Perhaps we could do go mod edit -go=1.22 to pin /examples/go.mod to 1.22 to match /go.mod?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right! We're modifying the examples/go.mod here, not the main one. Totally agree with your point then 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that the bump to go 1.23 was necessary because one of the dependencies requires it. I'll try pinning the examples go.mod to 1.22 and seeing if it still compiles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I suspected, if go.mod for the examples is updated to use go 1.22, it doesn't build.

go: updates to go.mod needed; to update it:
	go mod tidy

If I try to build with go 1.221 I get:

go: github.com/ipshipyard/[email protected] requires go >= 1.23 (running go 1.22.8; GOTOOLCHAIN=local)

lidel added a commit to ipshipyard/p2p-forge that referenced this pull request Jan 8, 2025
this implements idea from
libp2p/go-libp2p#3103 (comment)
to ensure users who set up staging endpoint for testing
are always aware fo it and never ship it to production
@2color 2color marked this pull request as ready for review January 9, 2025 09:37
@guillaumemichel guillaumemichel self-requested a review January 9, 2025 16:34
Comment on lines +109 to +110
// Configure the QUIC transport
libp2p.Transport(quic.NewTransport),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: IIUC quic isn't required for the example to work. We can remove the transport if the goal is to show a minimal working example.

@MarcoPolo
Copy link
Collaborator

@MarcoPolo Is it a problem this PR upgrades the go to 1.23 in go.mod?

I believe this happened due to dependency on p2p-forge

Yes. We can't do that until the Go team releases 1.24

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.

6 participants