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

Add -4 and -6 flags #1984

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

Conversation

jsumners
Copy link

@jsumners jsumners commented Aug 3, 2023

This PR is a redo of #1802. Since that PR has been idle so long, the branches have diverged quite a bit and it was easier to start anew.

The work in this PR includes the work originally done by @dmke in #1802.

This PR is to resolve #1801.

@jsumners jsumners mentioned this pull request Aug 3, 2023
2 tasks
@ldez ldez changed the title Add -4 and -6 flags (redo) Add -4 and -6 flags Aug 3, 2023
@jsumners
Copy link
Author

jsumners commented Aug 4, 2023

Are there any guides for getting an environment setup for developing on Windows? I used GoLand to install Go 1.20 and setup a run configuration to run go test -cover ./... and I get errors on the master branch in the providers/dns/exec/ tests:

image

@dmke
Copy link
Member

dmke commented Aug 4, 2023

Are there any guides for getting an environment setup for developing on Windows?

No, not really.

[...] get errors on the master branch in the providers/dns/exec/ tests

These tests try to execute the echo program, which works on the CI, and the last time I tried to run them in my Windows VM (in a VSCode PowerShell console, I believe). Do you have an echo.exe in your %PATH%?

If not, you can compile this small Go program as replacement:

echo.go
package main

import (
	"fmt"
	"os"
)

func main() {
	for i := 1; i < len(os.Args); i++ {
		fmt.Print(os.Args[i], " ")
	}
}

Build:

$ go build -o echo.exe echo.go

Maybe you need to configure GoLand to expose the environment variables properly to the Go test binary, not sure.

@jsumners
Copy link
Author

jsumners commented Aug 4, 2023

Windows does not ship with an echo.exe. So I'm not clear how either of the following test constructs are supposed to work:

{
desc: "Standard mode",
config: &Config{
Program: "echo",
Mode: "",
},
expected: expected{
args: "present _acme-challenge.domain. pW9ZKG0xz_PCriK-nCMOjADy9eJcgGWIzkkj2fN4uZM",
},
},

{
desc: "Raw mode",
config: &Config{
Program: "echo",
Mode: "RAW",
},
expected: expected{
args: "present -- domain token keyAuth",
},
},

However, it doesn't matter. I will just ignore these specific local failures and work on whatever is happening in this branch.

@dmke
Copy link
Member

dmke commented Aug 4, 2023

The Windows CI machines have MinGW installed, which presumably ships an echo.exe (https://github.com/actions/runner-images/blob/main/images/win/Windows2022-Readme.md#tools).

Ignoring them on your machine is also an option, of course.

@jsumners
Copy link
Author

jsumners commented Aug 4, 2023

Using some good old fashioned println debugging, I determined that the answer to the question in #1802 (comment) is the tests that are testing non-existent DNS servers are taking too long. Basically, it boils down to the way the different OSes handle lookups to addresses that don't exist. Windows seemingly being one that will take A LONG TIME. This gets exposed in this branch because of the new consideration for the net.OpError case. As a solution, I reduced the timeout for these specific test cases to something more reasonable. The tests pass locally on Windows and my macOS system.

@jsumners
Copy link
Author

jsumners commented Aug 7, 2023

@ldez @dmke will you be able to review this soon?

@jsumners
Copy link
Author

@ldez is there anything I can do to help move this along?

@jsumners
Copy link
Author

jsumners commented Sep 4, 2023

I'm at a loss for how to proceed here. There is activity in the repo with other PRs being reviewed and acted upon, but this one continues to sit idle without any input 😢

@jsumners
Copy link
Author

@ldez @dmke what are the chances of this getting into a release?

@dmke
Copy link
Member

dmke commented Sep 20, 2023

Sorry, this flew under my radar. It is 2023 and I still am not able to configure the junk filter properly for my email app. :/

I'll have a look later this week, when I have an hour or two of quiet time.

(In the meantime I'd appreciate if you could remove the @-mention from your commit message - I've found a dozen or so message in my junk folder caused by Github identifying the rebase and force-push as a completely new commit.)

@jsumners
Copy link
Author

I'll have a look later this week, when I have an hour or two of quiet time.

Thank you.

In the meantime I'd appreciate if you could remove the @-mention from your commit message

Done. Sorry.

@jsumners
Copy link
Author

jsumners commented Oct 4, 2023

@dmke have you had an opportunity to look at this?

@dmke
Copy link
Member

dmke commented Oct 4, 2023

No, sorry. Live keeps getting in the way :/

@jsumners
Copy link
Author

@ldez @dmke is there anything I can do to help make progress toward a decision on this PR?

@jsumners
Copy link
Author

Please give this PR some attention @ldez and @dmke.

@jsumners
Copy link
Author

@dmke I'll try re-running the tests on my Windows system again. I can't see what the issue is in the CI output because there is just way too much of it. It'd be helpful if CI omitted the -v in go test -cover -v ./....

@dmke
Copy link
Member

dmke commented Nov 27, 2023

I can't see what the issue is in the CI output because there is just way too much of it.

That's an issue with the way how Github presents the log output (you can't Ctrl+F)...

It'd be helpful if CI omitted the -v in go test -cover -v ./....

I don't agree, but can I understand. At least we can access the raw log behind the cog symbol on the top right:

image

There, I see the problem in github.com/go-acme/lego/v4/challenge/tlsalpn01:

2023-11-27T14:51:11.4606951Z === RUN   TestChallengeIPaddress
2023-11-27T14:51:11.4607484Z 2023/11/27 14:45:21 [INFO] [127.0.0.1] acme: Trying to solve TLS-ALPN-01
2023-11-27T14:51:11.4608070Z     tls_alpn_challenge_test.go:192: 
2023-11-27T14:51:11.4608850Z         	Error Trace:	D:/a/lego/lego/challenge/tlsalpn01/tls_alpn_challenge_test.go:192
2023-11-27T14:51:11.4609934Z         	            				D:/a/lego/lego/challenge/tlsalpn01/tls_alpn_challenge.go:70
2023-11-27T14:51:11.4611012Z         	            				D:/a/lego/lego/challenge/tlsalpn01/tls_alpn_challenge_test.go:247
2023-11-27T14:51:11.4611669Z         	Error:      	Received unexpected error:
2023-11-27T14:51:11.4612442Z         	            	tls: first record does not look like a TLS handshake
2023-11-27T14:51:11.4612972Z         	Test:       	TestChallengeIPaddress
2023-11-27T14:51:11.4613639Z         	Messages:   	Expected to connect to challenge server without an error
2023-11-27T14:51:11.4614232Z --- FAIL: TestChallengeIPaddress (0.20s)
2023-11-27T14:51:11.4614578Z FAIL
2023-11-27T14:51:11.4615112Z 	github.com/go-acme/lego/v4/challenge/tlsalpn01	coverage: 77.9% of statements
2023-11-27T14:51:11.4615818Z FAIL	github.com/go-acme/lego/v4/challenge/tlsalpn01	0.710s

I'm not sure whether that failure is caused by this commit. I'll also try do investigate later this week (time is still a precious commodity, and I don't know when I get access to my Windows VM again (I've moved homes and setting up my PC is far down a very long to-do-list)).

@wouterdebie
Copy link

Hey all! Is there any traction on this? I'd love to see this functionality, since I can't do ipv6 directly from my udmpro.

@ldez
Copy link
Member

ldez commented Feb 28, 2024

Even if I didn't provide feedback for now, this PR is still on my backlog.
Currently, I'm not satisfied with the implementation, so I need to take time to think about that.

@jsumners
Copy link
Author

Hey all! Is there any traction on this? I'd love to see this functionality, since I can't do ipv6 directly from my udmpro.

I keep the branch updated. But you can see how much response I've received for my inquiries about how to get it committed.

@jsumners
Copy link
Author

Even if I didn't provide feedback for now, this PR is still on my backlog.

Currently, I'm not satisfied with the implementation, so I need to take time to think about that.

This is the first time I have seen any mention of you not liking the implementation. I have asked repeatedly what I can do to get this accepted with very little response. I still want to get this added, I just need direction on how to get it there.

As for Windows, I have no idea how to setup an environment there that will work.

@ldez
Copy link
Member

ldez commented Feb 28, 2024

This is the first time I have seen any mention of you not liking the implementation. I have asked repeatedly what I can do to get this accepted with very little response.

Yes, you asked repeatedly.

I'm not forced to provide any feedback, and repeatedly asking for doesn't help.

FYI I've been facing difficulties in my life for several months that forced me to slow down.

Being a maintainer is not easy work, I do my best for me and the project.

@mia0x75
Copy link

mia0x75 commented May 29, 2024

This PR is helpful. And I can obtain certs by using jsumners' version, thanks.

This PR is a redo of go-acme#1802. Since that PR has been idle so long,
the branches have diverged quite a bit and it was easier to start
anew.

The work in this PR includes the work originally done by dmke in go-acme#1802.

This PR is to resolve go-acme#1801.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Provide -4 and -6 flags to use IPv4 and IPv6 respectively
5 participants