net: lookup_windows.go uses APIs that are no longer in Nano Server #21867
Comments
odeke-em
changed the title
|
\cc @alexbrainman. I have almost zero knowledge of Windows. |
SGTM. We have couple of functions in syscall with name that starts with Load (for example syscall.LoadCancelIoEx). These were created because some versions of Windows did not have those functions - so we used syscall.Load... functions in standard library to program around that limitation. You can do something similar. If you cannot replace the functionality netapi.dll provides, it is OK to just return error / skip tests. I suspect Nano Server users do not expect that functionality anyway. Alex |
|
This can be reproduced in a microsoft/nanoserver-insider image with this example code: package main
import (
"fmt"
"os/user"
)
func main() {
fmt.Println("Hello")
currentuser, err := user.Current()
if err == nil {
fmt.Println("Current user", currentuser)
} else {
fmt.Println("Error", err)
}
}Running the binary crashes |
Thank you @StefanScherer Alex |
|
Any updates? I found so far class swarm.exe, Traefik reverse proxy and Prometheus that need the netapi32.dll to work correctly in NanoServer 1709 image. |
What exactly do these programs require? What netapi32.dll functionality do these programs use? Alex |
swarmSwarm has vendored glog which uses traefikThe vendored Kubernetes package prometheusAlso prometheus uses glog :-) |
|
@StefanScherer thank you
That uses user.Current().Username. Maybe we can get current username without netapi32.dll? @taylorb-microsoft can we do that? How? Thank you. Alex |
|
@StefanScherer I think the quickest and easiest way to fix swarm.exe, Traefik reverse proxy and Prometheus is to fix glog package. I looked at the code you have pointed to: and glog uses os/user package just to fetch current user name. Is there a way to get current user name on NanoServer without calling netapi32.dll ? Can we get current user name by reading %USERNAME% environment variable or something? If yes, we can change glog code to do that if os/user.Current fails. Alex |
|
Thanks @alexbrainman, I don't know if this would be accepted in glog package. |
Why would not they. If they want their package work on NanoServer, they will make it work somehow. I did not look at their code, but I don't see how they package could not work without knowing current user name.
Can you list all environment variables. Maybe user name is there. Then you could use that environment variable in glog package. Also maybe you have some contacts from Microsoft - they would tell you how to get current user name in NanoServer. Alex |
|
Output of So Listing all environments: We have So something like this? package main
import (
"fmt"
"os"
"os/user"
)
func currentUsernameFromEnv() string {
return fmt.Sprintf("%s\\%s", os.Getenv("USERDOMAIN"), os.Getenv("USERNAME"))
}
func currentUsername() (me string) {
defer func() {
if r := recover(); r != nil {
fmt.Println("Recovered!")
me = currentUsernameFromEnv()
}
}()
current, err := user.Current()
if err == nil {
me = current.Username
} else {
me = currentUsernameFromEnv()
}
return
}
func main() {
fmt.Println("Hello")
fmt.Printf("Current user %s\n", currentUsername())
}When run on the host or in a microsoft/nanoserver:sac2016 container it works without panic/recover When run in microsoft/nanoserver:1709 container it shows |
Sounds good to me. I would change your code to always use USERDOMAIN and USERNAME environment variables on Windows ( if runtime.GOOS == "windows" then ... else ... ) then you won't need to recover from panic. I looked at https://github.com/golang/glog and it says: "Send bug reports to golang-nuts@googlegroups.com.". But I would create new issue here on https://github.com/golang/go/issues explaining the problem, and suggested solution. Perhaps someone will submit your change. If that change is submitted, I am not sure how to update vendor folder in warm.exe, Traefik reverse proxy and Prometheus. I suppose you could always create an issue there too. Alex |
ianlancetaylor
added
the
NeedsInvestigation
|
can't we make user.Current return error instead of panic for this case? |
|
Note that if/when CL 597255 is merged (to fix #68312), then |
|
Change https://go.dev/cl/597255 mentions this issue: |
|
Change https://go.dev/cl/604395 mentions this issue: |
|
Change https://go.dev/cl/604415 mentions this issue: |
dmitshur
added
NeedsFix
|
Change https://go.dev/cl/605135 mentions this issue: |
[This is a roll-forward of CL 597255, which had to be rolled back
because it broke the windows-arm64 builder, whose current user display
name is unavailable. This new CL fixes the issue by reintroducing the
historical behavior of falling back to the user name instead of
returning an error].
user.Current is slow on Windows sessions connected to an Active
Directory domain. This is because it uses Windows APIs that do RPC
calls to the domain controller, such as TranslateAccountW and
NetUserGetInfo.
This change speeds up user.Current by using the GetUserNameEx API
instead, which is already optimized for retrieving the current user
name in different formats.
These are the improvements I see with the new implementation:
goos: windows
goarch: amd64
pkg: os/user
cpu: Intel(R) Core(TM) i7-10850H CPU @ 2.70GHz
│ old.txt │ new.txt │
│ sec/op │ sec/op vs base │
Current-12 501.8µ ± 7% 118.6µ ± 11% -76.36% (p=0.000 n=10)
│ old.txt │ new.txt │
│ B/op │ B/op vs base │
Current-12 888.0 ± 0% 832.0 ± 0% -6.31% (p=0.000 n=10)
│ old.txt │ new.txt │
│ allocs/op │ allocs/op vs base │
Current-12 15.00 ± 0% 11.00 ± 0% -26.67% (p=0.000 n=10)
Updates #5298
Fixes #21867
Fixes #68312
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64
Change-Id: Ib7f77086d389cccb9d91cb77ea688d438a0ee5fd
Reviewed-on: https://go-review.googlesource.com/c/go/+/605135
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Update #21867. Update #68312. Update #68647. Change-Id: Ic41d6747c5a54ba28c1292258aa4d318ccb9fe40 Reviewed-on: https://go-review.googlesource.com/c/go/+/604395 Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Updates #21867. Change-Id: I1eb923ef66aa0f338bfa0d683159edc1d8ae2a6c Reviewed-on: https://go-review.googlesource.com/c/go/+/604415 Reviewed-by: Dmitri Shuralyov <dmitshur@google.com> Reviewed-by: Alex Brainman <alex.brainman@gmail.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Cherry Mui <cherryyz@google.com>
|
Just hit this in 1.23. Is this going to make it back to the 1.23 series? or is this a 1.24 thing. |
I'm afraid that it won't backported given that the fix is non-trivial and that the issue has been around for +7 years. |
|
It's all good man, I'm using the env hack for now, looking forward to 1.24. Thank you for addressing this, let's hope no one else steps into this in 6 months. 😉 |
The fall release of Nano Server removed a number of APIs with the effort of reducing the size of Nano Server. One of the DLLs removed was
NetApi32.dll. Currently there are a few places that lookup_windows.go calls APIs in NetApi32.dll - specifically isDomainJoined(..); lookupFullNameDomain(..) which is in turn called by newUser(..).It may be possible to refactor a bit to remove this dependency - I will think about it but I wanted to get this logged.
Examples:
go/src/os/user/lookup_windows.go
Lines 18 to 27 in 3098cf0
go/src/syscall/zsyscall_windows.go
Lines 179 to 181 in 3098cf0
EDITED by @odeke-em to use permalinks and format the target examples
The text was updated successfully, but these errors were encountered: