Merge wip/combined-codebase-staging into master #83
Comments
Maybe this is a given, but to make it explicit, I think we should make sure to always backport rather than FF merge in case of this situation:
As far as naming goes, I'm fine with copying the x repos. Personally, I'm used to |
That's my intended purpose, those branches would only be used to backport fixes that affect the integration with our Go fork release branches. Consumers using this as a library should use normal tags. IMO this reduces overhead on our side.
I'm good with using |
|
Why do we need an additional |
What I want to avoid is having to create a new major (or minor) version every time there is a Go release for the sole purpose of having a release branch where we can backport security fixes. For example, if in between go1.27 and go1.28 there hasn't been any change in this repo, then we shouldn't cut a new version before releasing go1.28, both can use the same. Yet, it is important to have an associated |
|
@derekparker any thought? |
I think everyone needs tags to update their builds to use this repo; but also branches which currently have tips set to a tagged commit for a clear place where one is supposed to cherry-pick bugfixes onto. I'm not sure golang/crypto/granches policy is sane, as that is tied to golang versions, whereas this repository is very much trying to be upstream golang agnostic, and openssl upstream version agnostic, thus it's probably best to have orthogonal naming.
Because for a given golang v1.XY we will know if release/vN.x supports it only after golang releases. And we can continuously integrate changes into master branch to match whichever changes are happening in the golang devel branch. This also eliminates the wait to create tags and release branches of this project, pending release schedule of golang toolchain upstream, and unblocks vendors migrating to this combined repo for old, existing, currently supported golang tool-chains as needed. |
Sorry was off yesterday for a holiday and buried with other things recently. I will take a look today / this week at the latest. |
Maybe I'm misinterpreting what you mean by this, but as far as I know, this isn't the case for us on microsoft/go at least. For example, https://github.com/golang/go/blob/go1.20.5/src/go.mod uses
What if an upstream security fix, say go1.21.7, contains an internally breaking change that requires a breaking change in golang-fips/openssl, and that change would make golang-fips/openssl incompatible with (or at least infeasible to use with) go1.20.12? How would we split It seems to me that to be safe, there should be a plan that we're prepared to put into action if this happens. Maybe this circumstance will never happen--but I can't quite convince myself that it's impossible. Given that we're patching Go internals, it seems to me there are things that could change upstream that wouldn't be breaking changes to the Go standard library, but could break our patched-in backends and perhaps create conflicting API demands on golang-fips/openssl that need to be resolved on a per-version basis. If keeping this open creates an impossible maintenance task, I could see ignoring it, but so far it doesn't seem too bad to me to account for this potential problem.
I guess what I'm saying above boils down to this: I don't think we can rely on this always being possible, and I think we should at least have an escape hatch in whatever plan we end up with. |
That is from golang/go upstream code, which is used verbantim, not something that is added in microsoft/go. In micosoft/go builds, across all branches, the crypto backend is always vendored by a clean tag (i.e. https://github.com/microsoft/go/blob/microsoft/main/patches/0007-Add-backend-code-gen.patch adds github.com/microsoft/go-crypto-openssl v0.2.7 by referencing a clean tag, and then locks and prevents any further toolchains updates with https://github.com/microsoft/go/blob/microsoft/main/patches/0008-Update-default-go.env.patch ) right now there is no tag, which is equivalent to go-crypto-openssl tag in this repository, to migrate microsoft/go backend to this repo, as far as I can tell.
if 1.21.7 security fix changes things, and we already have release/v3.x that is targetting 1.22 code, we will fork release/v2.x branch into release/v2.3.x (at which point v2.2.x also gets born to target 1.21.6 and lower if needed, assuming v2.2 was the last point release with fixes compatible with 1.21.6 and lower) and star doing micro-point releases off that instead of minor point releases. We always have a branch point where we can create a new maintenance branch and start cutting releases from it. As we simply need a logical place where the next tag should be fetched for stable maintenance; or when upgrading to new versions.
We always have a place to create a logical releases/vN.[N]?.x to handle any forseeable breakages in golang; openssl; new fips standards; or combinations of the above. I really dislike for example android NDK branch scheme. Where they have a branch for every ABI level. And then they have bot to auto-cherrypick every commit to every branch which is a fix without breaking an API/ABI. Meaning landing any change requires cherry-picks across dozens of branches, because every build points at a branch rigidly fixed to a given abi level. That's also a bit too much. Automatically creating golang-major-minor-openssl-major-minor branches & tags, with most of them pointing to the same subset of development is also not nice.
It is correct, but to not rely on this fact, we need to add tags and create branches to draw lines in the sand where we going to continue point releases of things. Right now, things are a mess, where tip of development is not the HEAD but a random wip/ branch, and one cannot reference it by a logical name apart from exact hash, wtihout any idea where to look for a maintainance branch related to it; or where to look for the next breaking change branch. |
As far as I know, we've done this to attach release notes to the tag and make the situation more readable, but it isn't a requirement.
I might have lost track of some numbers, but this is where I'm ending up after the fork:
I've been assuming that only the latest servicing release of each Go major version would be supported, but it sounds like that's not the case for what you're targeting. My next question would be: what if you need to support breaking changes for go1.21.5-3 that aren't compatible with other versions supported by
I don't like having individual branches for Go versions, it just seems extremely practical and easy to reason about.
I don't think anyone's under the impression we don't need to address this, but I think "copy how upstream Go handles its vendored modules" is an easier default strategy (because we're doing the same thing) rather than trying to fit the repo into its own, independent linear release structure. |
ok.
yeap.
I mean at the start of time, which is current state, everything else is supported in v1 which is the current master branch state.
i'm targeting 10 years of support, potentially 12 years. It ubuntu we are currently maintaining golang applications and tool chains for much longer than FIPS expiry dates, and upstream maintainance, for a limited scope.
So how are we going to handle OpenSSL changes? go122-openssl3.2 go122-openssl3.1 go122-openssl4 ?
The point is we have incompatible Openssl changes coming in, and incompatible golang changes comming in, and a given state of codebase supports only a subset of combinations. Openssl minor revisions often have ABI breaks. Creating the branch name after golang release, or after openssl release doesn't quite make sense. Given you will have per-golang-version branch in microsoft/go repo anyway. golang compat is only one axis of support. |
The project is actually designed to handle any of them [without a Git branch], branching the logic depending on version. It uses dlopen to load the library so there is no strong linkage to any particular OpenSSL version, and it basically has a compatibility layer that maps the API that Go needs onto the right implementation for each underlying OpenSSL version. https://github.com/golang-fips/openssl/tree/wip/combined-codebase-staging#features The project CI tests a handful of versions on each commit: openssl/.github/workflows/test.yml Line 9 in 827e0bc In the Microsoft fork, we aren't building for one particular Linux distro, and we need to support cross-building from a Windows host onto various Linux distros, so our needs put us on this path. |
Maybe that OpenSSL compat strategy changes things, but I have more questions about this strategy if it's still on the table. I prefer to focus on a steady state rather than how things are starting off, so I'll set up a hypothetical future situation to poke at instead of "now":
Now a breaking change specific to go1.31.7 has happened, in revision go1.31.7-2. How is |
|
My thoughts: Instead of merging the combined codebase into The only time we need to branch this repo is for external breaking API changes, for example |
Seems OK to me.
Agree on not creating a branch per OpenSSL release, not needed. About the branch per Go release, I'm with @dagood. This is not a theoretical problem, we have been maintaining go-crypto-openssl for a bunch of releases already, and I can assure that not having release branches has impacted the stability promise that users would expect from a Go patch release. For example. go1.20 started pointing to microsoft/go-crypto-openssl@v1.2.5, and during go1.21 development phase we have evolved go-crypto-openssl to improve OpenSSL 3 support, fix security issues and done cosmetic refactors, but without API breaking changes. We wanted to backport the security fixes to go1.20, but as we don't have a release branch for it, it's not possible to do so without also backporting all the other changes that shouldn't go into a patch update, aka upgrading to microsoft/go-crypto-openssl@v1.2.8. Having said this, I'm fine not agreeing on this specific topic, both strategies are compatible. We (the Microsoft team) can commit to following the semver approach, which is what we all seem to agree. In parallel, we (the Microsoft team) will maintain a separate set of branches, one for each Go release, in which we will backport security fixes. I still don't know if these branches will live here or in |
I am not against a semver based branching approach, and I'd rather there be one source of truth rather than continuing to maintain separate repos. I guess the question comes down to what do we consider a release? We could create a new minor version per Go minor version, e.g. |
This seems like a good compromise. Is still doesn't account for cases where upstream Go adds a new boring API in a patch release, in which case we would have to bump the minor version, but we couldn't do it because we would be breaking the one minor version per Go minor version approach. Yet, AFAIK this has never happened, and if it somehow happens, then we can always create a branch for that specific Go version and evolve the API without interfering with the normal semver tagging. What do you think @dagood? |
|
Would like your thoughts as well @dbenoit17 |
who created go1.31.7-2? is this a second build of a microsoft/go release of 1.31.7 toolchain? or is this golang upstream releasing go1.31.7-2 after go1.31.9 is already out? Surely they publish go1.31.10 in that case. They do minor revisions, but i haven't seen them do -2. Note that intention is to always have tags (fixed) and branches (to cherry-pick fixes on), and if fixes we want to cherrypick patches that are breaking compat with a previous tag we split the branch to ensure we have place with and without breaking changes which are still "stable". Aka as mentioned before - place to land bugfixes and a place to land API/ABI breaking changes, to allow any golang vendor that use that to make their own point releases, with new feature upgrades or with bugfixes only as needed. Because if we do land changes in this repo, i do hope we can resping golang toolchain 1.31.[0..11] with correct new tagged versions of golang-fips/openssl vendors from respective branches golang-fips/openssl to be able to ship respins as needed and desired. The number of branches & tags we create can handle all sources of API/ABI breakages, for as many golang toolchain builders as needed. In practice, it is a finite set, which will converge on a reasonable subset of golang branches X openssl versions X revisions of patches that integrate the two. |
|
just because golang released 1.31.3, the microsoft/go integration patches to glue in golang-fips/openssl can be different. and canonical might want to keep upgrading golang-fips/openssl with or without bumping golang release from 1.31.2, especially when change to golang-fips/openssl is required due to security-fixes in openssl breaking ABI which happens quite frequently. we ultimately have 4 lines of development that we are trying to integrate: upstream golang, boring-glue patchset, openssl bindings (golang-fips/openssl), and openssl (upstream + all distro-forks). All of which can and will have security issues, and would be desired to be bumped individually without rebuilding/upgrading the other 3 components as much as possible. |
@xnox If I understood this comment correctly, this means you are also in favor of #83 (comment). Am I right? |
I assume you're creating it (or something very similar, 1.31.7.2?), because you need to backport changes from upstream's 1.31.10 release back to 1.31.7. Whether or not you need to do this is what I was intending to ask here:
Reading it now, I guess the answer doesn't really say either way, but it seemed like a "yes" at the time. microsoft/go only supports the latest servicing release of each 1.X version, so I was trying to set up an example that fit your situation rather than microsoft/go's. Anyway, here's what the scenario would look like for microsoft/go, assuming we add support for older major versions (which might be the level of support you're really aiming for in your own fork?):
go1.29 is out of upstream support at this point, let's say it ended at go1.29.8. Now a new go1.31 security release comes out and we port it to go1.29. This means we're planning to create go1.29.8-2 in microsoft/go with the patch. Let's say the natural way to do the patch includes breaking changes that require changes in golang-fips/openssl. The patch to go1.30 requires different changes in the backend. How is What I'm trying to do is set up an example that shows how putting a group of versions onto one linear semver branch when we might need to expand the group later leads to splits that are too deep for clear naming. Ultimately, I don't anticipate this being a major practical issue. The upstream branching structure naturally avoids it and I want to make sure this reason for that structure is clear, even if it's just a factor in the decision rather than some requirement. (Or, I wanted to know if I was missing something, quite possible!)
I think this is ok (with the caveats @qmuntal mentioned). It does hide the version->Go version mapping, though, and it seems like it would be easy to forget to advance the minor version (in part due to the indirect mapping) and end up unintentionally grouping Go versions with nowhere to split them. But, as long as we're ok potentially creating a new branch that doesn't fit in semver to handle edge cases like that, that's always a way to solve it if it comes up. |
this is not what we'd be inteded to do. Or if we must, we wouldn't be doing straight backports but minimal inline patches without bumping any semver numbers, remaining abi/api compatible. When things become impossible, we force upgrade things to the new upstream versions.
support for older major verisons might be something canonical does in our own forks, but those will be more or less internal to canonical to basically continue building a few project we need to still support. It should be out of scope here, and nothing here so far limits to maintain that for us.
this will work for everyone i think.
another way to split is within the package names itself too, if we do get too burdened. But I too see this too-many-branches as a necessory evil in corner cases only as an escape hatch, which we hopefully will not need to use too often.
Indeed. If anyone wants a fork from some particular point, and happy to maintain it, we are happy to have it in this repo, for as long as they need it. With an appropriate ci test matrix. |
|
I still feel like we're all overthinking this a bit in terms of hypothetical versioning, backporting, etc... Here's how I see things: As mentioned before, we create a new branch The more we branch unnecessarily the more backport work we're going to have to do. This is a library, we should treat it as such. It doesn't have to be tied to a particular Go release. I really can't see a compelling reason to cut a new version of this library every time there's a new Go version. This library exists to call into OpenSSL, and yes it is tied to Go in that it needs to be essentially a drop in replacement for |
|
@derekparker given the engineering done to support multiple openssl abis, even with breaking golang changes we can engineer to support different major golang releases with build tags and appropriate CI test matrix, to even continue to support everything from a single code base. Anyway, your assesment is progmatic and true enough, all in favour? +1 the above comment and let's get on with it. |
|
The way I've been seeing it, overthinking is choosing something other than the scheme Go uses for the Or, the problems can be discarded because they might not come up in practice--it sounds like this is the pragmatic approach we're going for. I agree that if we can treat this module as a simple library, that would mean less maintenance. I suppose this is fine with me. We can always change the approach later if problems do turn up, and having more real examples would make it easier to talk about. |
I think this is an interesting idea, want to call it out in particular because I think it might help quite a bit. |
|
We have quorum. I'll rename |
|
@derekparker I don't have permissions to change the default branch, Could you give admin rights to @dagood and myself so we can also manage this repo in the future? Thanks. |
|
Thanks for the permissions @derekparker. Closing this issue as I've already renamed |
The Microsoft Go fork will probably start using this repo for go1.21, given that it has better support for OpenSSL3+FIPS than https://github.com/microsoft/go-crypto-openssl.
It would be good not to depend on a development branch, so I propose to take the following actions:
masteras is today namedinternal-branch.pre-go1.21. This will be used by RedHat till they don't migrate to the new API and code.wip/combined-codebase-stagingintomaster.masterasv0.1. Microsoft will reference this tag from it's fork. RedHat might also do so if they want to migrate during go1.21.masternamedinternal-branche.go1.21-vendor. This will be used by Microsoft and RedHat to backport fixes to the go1.21 fork.I've taken the branch naming convention from what Go does today for vendored repos (e.g. https://github.com/golang/crypto/branches), but I'm open to suggestions.
@ueno @derekparker @jaredpar @dagood @dbenoit17
The text was updated successfully, but these errors were encountered: