[microsoft/release-branch.go1.21] Update 1.21 branch #982
Conversation
dagood
requested review from
a team and
microsoft-golang-review-bot
as code owners
eng/_core/cmd/build/build.go
Outdated
| before, _, _ := strings.Cut(string(data), "\n") | ||
| os.Setenv("TESTGO_TOOLCHAIN_VERSION", before) | ||
| fmt.Printf("---- VERSION file found with first line %q. Assigning TESTGO_TOOLCHAIN_VERSION for test run.\n", before) | ||
| } |
There was a problem hiding this comment.
Notes: I believe this problem showed up in this release because I moved buildutil.AppendExperimentEnv(o.Experiment) for 1.21. Now, the toolchain is built without opensslcrypto and tested with opensslcrypto. I think this mismatch is part of the problem.
In earlier releases, we're able to build Go with opensslcrypto because it's able to fall back to Go crypto. The check I added in this release makes that into a failure instead of fallback. There was no clear way to wire up the bootstrap builds to allow fallback while preventing it for "real app" builds, so building Go without opensslcrypto then testing with opensslcrypto was the solution I settled with.
As of writing, I don't know if this workaround works. Another approach is to revisit the possibility of allowing the Go build to fall back to Go crypto so we can build+test with opensslcrypto again.
There was a problem hiding this comment.
Notes: I believe this problem showed up in this release because I moved buildutil.AppendExperimentEnv(o.Experiment) for 1.21. Now, the toolchain is built without opensslcrypto and tested with opensslcrypto. I think this mismatch is part of the problem.
I guess you are talking about #965, no?
As of writing, I don't know if this workaround works. Another approach is to revisit the possibility of allowing the Go build to fall back to Go crypto so we can build+test with opensslcrypto again.
Difficult problem. The toolchain and the tests fight against the approach implemented in #965. What if we relax the check allowing the fallback to plain Go unless GOFIPS=1 or -tags=requirefips are set? We would be a little bit better than in Go 1.20, which didn't had the requirefips knob. We can try to come up with something better for go1.22.
There was a problem hiding this comment.
Falling back to plain Go has been disruptive for us, so I would expect some downstream users to also have unintentional compilation or test failures. For example, https://github.com/Azure/ClusterConfigurationAgent/pull/1683 intentionally modified all Docker files to compile Go binaries using CGO_ENABLED=0 GOEXPERIMENT=opensslcrypto meanwhile they figure out the performance impact of enabling cgo. When they move to go1.21 as is today, all they pipelines will break.
There was a problem hiding this comment.
I guess you are talking about #965, no?
Right.
What if we relax the check allowing the fallback to plain Go unless
GOFIPS=1or-tags=requirefipsare set?
Maybe, but I think a problem is the (large number of?) programs that will need to comply with internal Microsoft crypto policy, which may never have FIPS mode enabled. But maybe this isn't the right way to give them a guardrail. (More about it in Go sync chat.)
intentionally modified all Docker files to compile Go binaries using
CGO_ENABLED=0 GOEXPERIMENT=opensslcrypto
I think this is the sort of potentially misleading config that we want to prevent, although you're right, it's a breaking change and that deserves weight.
I think I applied the workaround to the wrong place and it only applied to devscript builders. I'm going to try it again just to see if it would theoretically work, but I do think reverting this change is probably what will end up making the most sense.
There was a problem hiding this comment.
The workaround still doesn't work. Good to know. 😄 Running a revert, and keeping in mind that I need to do it on microsoft/main as well.
There was a problem hiding this comment.
We decided in the Go sync chat to keep preventing accidental Go fallback. The situation changed since we started allowing fallback (pre-1.19) and it seems like now it helps without much downside.
I added a note to the fallback compile error message that directly addresses the breaking change.
Meanwhile, I figured out that if we change the allow_missing_crypto_backend_fallback tag to a allowcryptofallback GOEXPERIMENT, the complicated workarounds aren't necessary because opensslcrypto,allowcryptofallback follow each other around and take effect in the same places (and they are omitted in the same places).
There was a problem hiding this comment.
- Here's the main PR for this improvement: Go crypto fallback: use GOEXPERIMENT for opt-in #984
This 1.21 PR has the same changes (clean cherry-pick) so once the main PR goes in, I'll consider this PR ready to go.
dagood
force-pushed
the
dev/dagood/fork1.21
branch
from
e827dbf
to
ea6bdfb
Compare
Update the submodule to the current 1.21 commit and fix patches. The usual trivial conflicts in go.mod/go.sum and vendoring.
I mentioned in Go sync that I saw this branch and remembered that upstream typically branches for the first
rcrelease. This has been the case before, but as far as I can tell from https://github.com/golang/go/commits/release-branch.go1.21, the branch was just created today. (I don't know a reasonable way to tell when a branch was actually created on GitHub, but what is easy to see is that today is when the first branch-specific commit was pushed.)