vendor: kubernetes 1.19.0 rc.4 and dependencies by thaJeztah · Pull Request #1551 · containerd/cri

thaJeztah · 2020-08-05T13:50:14Z

No description provided.

full diff: json-iterator/go@v1.1.9...v1.1.10 - Fix 459 map keys of custom types should serialize using MarshalText when available - Fix potential panic in (*stringAny).ToInt64 and (*stringAny).ToUint64 (see 450) - Fix 449 do NOT marshal the field whose name start with underscore - Reuse stream buffer and remove flush in (*Stream).WriteMore(see 441 440) - Fix 421 simplify the error string returned by the decoder when it meets error unmarshaling anonymous structs - Fix 389 411 do NOT marshal the json.RawMessage type field whose real type is integer/float as "null" with ValidateJsonRawMessage option enabled - Fix 326 do Not marshal private field after calling extra.SetNamingStrategy() to register naming strategy extension Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/net@f3200d1...ab34263 Worth mentioning that there's a comment updated in golang.org/x/net/websocket: This package currently lacks some features found in alternative and more actively maintained WebSocket packages: https://godoc.org/github.com/gorilla/websocket https://godoc.org/nhooyr.io/websocket It's used in k8s.io/apiserver/pkg/util/wsstream/stream.go, so perhaps that should be reviewed if the alternatives are better for how it's used. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/sys@9dae0f8...ed371f2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

full diff: golang/crypto@bac4c82...75b2880 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2020-08-05T13:52:18Z

+golang.org/x/net                                    ab34263943818b32f575efc978a3d24e80b04bd7
 golang.org/x/sync                                   42b317875d0fa942474b76e1b46a6060d720ae6e
-golang.org/x/sys                                    9dae0f8f577553e0f21298e18926efc9644c281d
+golang.org/x/sys                                    ed371f2e16b4b305ee99df548828de367527b76b


Note that these are in the "containerd dependencies" section. Wondering if they should stay in that section (and thus need to be updated there first), or if we need some "generic" section, as these are used in various places, not just containerd

we'll be merging the cri repo soon into containerd .. agree once something has two major dependencies it's time to move it to a more generic list

thaJeztah · 2020-08-05T13:53:52Z

+// This package currently lacks some features found in alternative
+// and more actively maintained WebSocket packages:
 //
 //     https://godoc.org/github.com/gorilla/websocket
-//
+//     https://godoc.org/nhooyr.io/websocket


I noticed this change in the comment, which relates to golang/go#18152

It's used in k8s.io/apiserver/pkg/util/wsstream/stream.go, so perhaps that should be reviewed if the alternatives are better for how it's used.

thaJeztah · 2020-08-05T13:54:40Z

+	k8s.io/klog/v2 v2.2.0
 	k8s.io/kube-openapi v0.0.0-20200427153329-656914f816f9
-	sigs.k8s.io/structured-merge-diff/v3 v3.0.0
+	sigs.k8s.io/structured-merge-diff/v3 v3.0.1-0.20200706213357-43c19bbb7fba


I skipped the update to sigs.k8s.io/structured-merge-diff, as it would bring in a non-released version, and didn't bring changes in the vendored files

thaJeztah · 2020-08-05T14:00:17Z

 	github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96
 	github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153
-	github.com/evanphx/json-patch v4.2.0+incompatible
+	github.com/evanphx/json-patch v0.0.0-20190815234213-e83c0a1c26c8


Perhaps something for @dims - I noticed kubernetes switched to this version to get a fix/feature in. This commit is somewhere between v4.5.0 and v4.6.0:
evanphx/json-patch@e83c0a1...v4.6.0

There's also a v4.7.0 release, that fixes some bugs:
evanphx/json-patch@v4.6.0...v4.7.0

So, perhaps k/k can update to that version to get back on a released version of the dependency. I can try opening a PR to k/k (not sure if there's special magic needed to update dependencies there 😅), or perhaps someone else has time to do so

@thaJeztah we are a bit sensitive to json-patch historically i think. Will run it by @liggitt. It is ok for containerd to switch first. We will just pick it up when we rev to containerd 1.4.0

between v4.5.0 and v4.6.0, json-patch did the following things in this order:

evanphx/json-patch@v4.5.0...v4.6.0

added a go.mod file which made semver revisions unaddressable until they renamed to github.com/evanphx/json-patch/v5 (done in v5.0.0)

fixed an issue with significant performance implications

changed behavior to be RFC-compliant, but which is potentially breaking for API consumers

We are not yet comfortable picking up the behavior-breaking changes (especially not to backport to a patch release, see discussion in kubernetes/kubernetes#91622 (comment)), but did want the bugfix.

It is ok for containerd to switch first. We will just pick it up when we rev to containerd 1.4.0

Heh, well, containerd (nor containerd/cri) currently vendor it / depend on it (it was actually a facepalm moment that I looked at the versions, then realised it's not used here) 😂

changed behavior to be RFC-compliant, but which is potentially breaking for API consumers

We are not yet comfortable picking up the behavior-breaking changes (especially not to backport to a patch release, see discussion in kubernetes/kubernetes#91622 (comment), but did want the bugfix.

Ahm, gotcha, I thought it was updated to get the fix, and this commit was "latest" at that point, and it wasn't updated after.

We are not yet comfortable picking up the behavior-breaking changes

That makes perfect sense. So looking at the diff, it looks like evanphx/json-patch@a5df74a is the offending commit. This actually sounds like the exact scenario for which go modules were designed. Given that this is a (potentially) breaking change, that warrants a major version bump, so it could (/should) be considered a mistake that it was merged in a minor release.

Perhaps we should suggest a revert of that commit, and a v4.8.0 tag to be created. Given that they have a v5.x version now, this gives consumers of the package the opportunity to opt-in to the new behavior when ready, and others to stay on v4.x until they're ready for the change.

I guess Kubernetes is the most prominent consumer of that package (https://grep.app/search?q=github.com/evanphx/json-patch), and still is on v4 (kubernetes-sigs/kind looks to have made the switch to v5, so should not be affected by a revert https://grep.app/search?q=github.com/evanphx/json-patch/v5)

I went ahead and opened a pull request for consideration; evanphx/json-patch#111

The tagged v4.8.0 now has the performance fixes with no compatibility breaks compared to 4.6.0. It does still contain a go.mod file requiring it to be addressed as v0.0.0-20200808040245-162e5629780b in go.mod files

Opened evanphx/json-patch#113 to drop the go.mod file in the 4.x stream for tidyness, though it would only affect usage by go module-based consumers.

btw, json-patch v4.9.0 was just released that once again allows addressing as v4.9.0+incompatible via go modules

Perfect! Thanks for the update

thaJeztah · 2020-08-08T14:43:36Z

Ah, yes, I just was fiddling with that, trying to update the dependency in kubernetes; looks like go1.13+ no longer allows that, and refuses to get the dependency. tbh these breaking changes in Golang modules are kinda infuriating (imo even should warrant a major version bump of Golang itself)

…

On 8 Aug 2020, at 16:19, Jordan Liggitt ***@***.***> wrote: @liggitt commented on this pull request. In vendor/k8s.io/apimachinery/go.mod: > require ( github.com/davecgh/go-spew v1.1.1 github.com/docker/spdystream v0.0.0-20160310174837-449fdfce4d96 github.com/elazarl/goproxy v0.0.0-20180725130230-947c36da3153 - github.com/evanphx/json-patch v4.2.0+incompatible + github.com/evanphx/json-patch v0.0.0-20190815234213-e83c0a1c26c8 The tagged v4.8.0 is functionally correct with no breaking changes, but still contains a go.mod file requiring it to be addressed as v0.0.0-20200808040245-162e5629780b in go.mod files Opened evanphx/json-patch#113 to drop the go.mod file in the 4.x stream for tidyness, though it would only affect usage by go module-based consumers. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

liggitt · 2020-08-08T14:57:28Z

I agree it is difficult... we accidentally tagged a version of client-go with a go.mod file as v12.0.0 not realizing that was an invalid tag.

It was officially documented as disallowed, but was not enforced until go1.13. The backwards compatibility scenarios go modules added (e.g. v4.6.0+incompatible to indicate v4.6.0 but without a go.mod file or /v4 suffix) made it really hard to understand when you were doing things correctly, and when you were doing things incorrectly and go modules were masking the issue for compatibility purposes.

AkihiroSuda · 2020-08-10T07:27:02Z

Is this ready to be merged?

The json-patch issue doesn't seem directly related to containerd.

thaJeztah · 2020-08-10T08:32:09Z

The json-patch issue doesn't seem directly related to containerd.

Correct; I just noticed it in the diff, but not related to this repository (or this PR), so this should be ready for review

dims · 2020-08-10T10:07:58Z

+1 LGTM

thaJeztah added 5 commits August 5, 2020 14:27

vendor: golang.org/x/sys v0.0.0-20200622214017-ed371f2e16b4

e018d6f

full diff: golang/sys@9dae0f8...ed371f2 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9

968d44d

full diff: golang/crypto@bac4c82...75b2880 Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

vendor: update kubernetes v1.19.0-rc.4

ecbdffc

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

k8s-ci-robot added the size/XXL label Aug 5, 2020

thaJeztah commented Aug 5, 2020

View reviewed changes

thaJeztah mentioned this pull request Aug 5, 2020

Revert "Conform to RFC6902 replacement semantics" for v4.x evanphx/json-patch#111

Merged

AkihiroSuda approved these changes Aug 10, 2020

View reviewed changes

AkihiroSuda added this to the v1.4 milestone Aug 10, 2020

AkihiroSuda merged commit 4e6644c into containerd:master Aug 10, 2020

thaJeztah deleted the bump_kubernetes_1.19.0-rc.4 branch August 10, 2020 12:28

thaJeztah mentioned this pull request Aug 11, 2020

vendor: containerd/cri 4e6644c8cf7fb825f62e0007421b7d83dfeab5a1 and dependencies containerd/containerd#4466

Merged

Conversation

thaJeztah commented Aug 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt Aug 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 8, 2020 via email

Uh oh!

liggitt commented Aug 8, 2020

Uh oh!

AkihiroSuda commented Aug 10, 2020

Uh oh!

thaJeztah commented Aug 10, 2020

Uh oh!

dims commented Aug 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

liggitt Aug 8, 2020 •

edited

Loading