Bump Go version to 1.25.4 #46793
Bump Go version to 1.25.4 #46793
Conversation
botelastic
bot
added
the
needs_team
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
ycombinator
added
Team:Elastic-Agent-Data-Plane
|
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
botelastic
bot
removed
the
needs_team
|
There is a new Go Vet rule: https://go.dev/doc/go1.25#vet So many builds are breaking with: |
|
The These appear to be golang/go#75148, which should be fixable when golang/go#74630 is implemented. However, in order to upgrade to Go 1.25.1 now, we'll need to find a workaround. |
ycombinator
force-pushed
the
bump-golang-1.25.1
branch
from
5fab649 to
b49d237
Compare
These errors are coming from Go downloading dependencies before executing the tests. The errors can be simulated like so: I ran into the same problem in elastic/elastic-agent#10156 and I had success with explicitly downloading the dependencies before executing the tests. I'm running into a different problem on that PR now; once it's sorted out, I will apply the same approach on this PR here. Moving this PR into draft until then. |
|
MacOS packaging steps are failing in CI like so (this example is from trying to package Agentbeat): From https://go.dev/doc/go1.25#darwin:
And it looks like we're still using either the 10.11 or 11.3 MacOS SDK in golang-crossbuild images. |
ycombinator
force-pushed
the
bump-golang-1.25.1
branch
from
efd4f16 to
8819cff
Compare
|
I think what is curious here is the failing auditbeat test is exactly the case the RemoveAll bug fix in Go 1.25.4 should correct - it explicitly creates a read only file and then automatically deleted it in the test Cleanup call fails. beats/auditbeat/module/file_integrity/fileinfo_windows_test.go Lines 35 to 55 in 474109e This test case is pretty simple as a reproducer if we wanted to ask upstream about it. |
|
The other option is we aren't actually building with Go 1.25.4 even though it looks like we should be. |
The |
|
This pull request is now in conflicts. Could you fix it? 🙏 |
|
I can confirm that on a Windows Server 2022 21H2 VM the auditbeat test failing here failed on the first try with Go 1.25.6 so there is nothing special going on in CI for this one. beats/auditbeat/module/file_integrity/fileinfo_windows_test.go Lines 35 to 55 in 474109e |
|
I have a theory based on the reference to POSIX semantics in golang/go#75922 (comment) that this is based on use of Windows ACLs to mark the file readonly instead of going through The patch below causes the test to pass to confirm this: diff --git a/auditbeat/module/file_integrity/fileinfo_windows_test.go b/auditbeat/module/file_integrity/fileinfo_windows_test.go
index a73f4642bc..209e4780fc 100644
--- a/auditbeat/module/file_integrity/fileinfo_windows_test.go
+++ b/auditbeat/module/file_integrity/fileinfo_windows_test.go
@@ -22,7 +22,6 @@ import (
"path/filepath"
"testing"
- "github.com/hectane/go-acl"
"github.com/stretchr/testify/assert"
"golang.org/x/sys/windows"
)
@@ -55,7 +54,7 @@ func TestFileInfoPermissions(t *testing.T) {
}
func makeFileNonReadable(t testing.TB, path string) {
- if err := acl.Chmod(path, 0); err != nil {
+ if err := os.Chmod(path, 0); err != nil {
t.Fatal(err)
}
}This test happens to use go-acl, which we cloned most of into https://github.com/elastic/elastic-agent/blob/cd886995e9b6a09602f5927f47bc83c2bc80b12c/internal/pkg/acl/acl_windows.go#L143-L146 in Elastic Agent instead of forking upstream. That is probably the source of the test failures in agent CI. I don't know why, it is probably that the fix upstream is incomplete somehow, but we have a lead on a way past this. |
|
Thanks @cmacknz. IIUC, the intent of the calling code here, |
|
Yes, I think we need to keep the ACL manipulation and add a call to I am still not sure of the exact mechanism in Go 1.25 that broke this, but using Chmod to set the read only attribute works around it and seems safe to do here. For the similar Elastic Agent failures we still need to investigate more since in that case we are definitely relying on the ACLs there. |
|
i think adding os.Chmod wont do a trick here, i'd focus more on getting a handle for a file you are owner but have no rights on (hence acl.Chmod(0)) on delete during test cleanup, we have good access on test temp dir, in this dir we created |
|
Thanks Michal I see it now, in golang/go@6d41809#diff-4620d8a807ea6bc818910000f177a72022dba33b44111ad14b2f3a959250dee0 a change in build tags in removeall_at.go and removeall_noat.go switched from calling os.Remove first in removeall_noat.go which does not open the file to the more complex deleteAt implementation that does open the file which is probably where the Access Is Denied comes from (would have to get out the debugger to confirm exactly). Testing this, the following change to call diff --git a/auditbeat/module/file_integrity/fileinfo_windows_test.go b/auditbeat/module/file_integrity/fileinfo_windows_test.go
index a73f4642bc..a79877988d 100644
--- a/auditbeat/module/file_integrity/fileinfo_windows_test.go
+++ b/auditbeat/module/file_integrity/fileinfo_windows_test.go
@@ -40,6 +40,9 @@ func TestFileInfoPermissions(t *testing.T) {
defer f.Close()
name := f.Name()
+ t.Cleanup(func() {
+ err := os.Remove(name)
+ })
The Elastic Agent case is going to be similar, we must be trying to delete a file we don't have permission to open, but that is odd because we have to be running as admin and I would be surprised if we strip all ACLs like we do here (maybe we remove read or just enough to cause open to fail or something). |
|
just a small update, i was playing with this and building go from source with some custom changes, and after quite some head-banging that my changes are not behaving in a way i want them i figured out something else. this is indeed regression in 1.2, not 1.25.4 but 1.25.0, well partially. same logic applies as described above, when we have DELETE CHILD on parent we can open a handle on a file we don't have permissions for in this case we have 2 options at this point
this is touching really an edgy edge scenario of file manipulation. in a meantime i have a very ugly custom go build (that i wont' share for the very same reason) with everything working as expected, new and old behavior, i just need to polish it and raise it upstream. so maybe it will get in, hopefully. in this case we could unskip test in one of future releases |
|
Awesome thanks, for Beats the Basically the only thing that seems to work is bypassing the How to proceed here I think depends on how fast an upstream fix can happen, Go 1.24 will go out of support in February when 1.26 releases. |
|
just for visibility, |
10 participants
This PR bumps up the Golang version to
1.25.4. It also:ms_tls13kdfGolang build tag when building in FIPS mode because this tag was only needed with Golang versions1.24.x.GODEBUG=tlsmlkem=0environment variable when running FIPS140-only unit tests. This prevents errors like so:Failed to connect: crypto/ecdh: use of X25519 is not allowed in FIPS 140-only mode.fmt.Sprintf("%s:%d", ip, port)code fragments withnet.JoinHostPort(ip, strconv.Itoa(int(port)))to work with the newhostportgo vetanalyzer.