Fix merging with conflict by upgrade mergo version to v0.3.12 #107837

jlsong01 · 2022-01-28T15:35:16Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

client-go pins to mergo v0.3.5, seems v0.3.5 has some bug, which can not handle merge with confilct correctly.
Since unit test Example_mergingSomeWithConflict uses mergo v0.3.5, the merge process is wrong and the corresponing ut expection is wrong as well.
We need upgrade mergo version into the latest v0.3.12 and modify the Example_mergingSomeWithConflict expection accordingly.

Which issue(s) this PR fixes:

Fixes #107499

Special notes for your reviewer:

According to my test, the mergo bug has been fixed since v0.3.8, but have not found the releated issue report.

Does this PR introduce a user-facing change?

NO

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NO

jlsong01 · 2022-01-28T15:41:04Z

/triage accepted

cblecker · 2022-02-03T02:46:59Z

/lgtm
/assign @liggitt

Changes look fine to me, @liggitt.

liggitt · 2022-02-03T15:26:36Z

staging/src/k8s.io/client-go/tools/clientcmd/loader_test.go

+	//     insecure-skip-tls-verify: true
+	//     server: http://a-different-cow.org:8080


This unit test is explicitly checking what the resulting configuration is when we merge two files. This test change demonstrates the library update is flipping merge behavior and breaking compatibility with existing kubeconfig merge order.

Revert the changes to this file. If the mergo library changed behavior, we'll need to adjust how we're calling it to preserve the existing behavior of kubeconfig merging.

liggitt · 2022-02-03T15:28:08Z

Since unit test Example_mergingSomeWithConflict uses mergo v0.3.5, the merge process is wrong and the corresponing ut expection is wrong as well.

Looking at git history, that unit test has been asserting that merge behavior since kubeconfig files were introduced in https://github.com/kubernetes/kubernetes/pull/3316/files#diff-c1a584208d3dad4c9c57651c1f67dae98842323f5753a60460979792642666ccR71-R112 in 2015, prior to Kubernetes v1.0. At the time, that was using a 0.1.4-era version of mergo.

The current behavior of merging two kubeconfig files is what we need to preserve. If we want to update the mergo library, that's ok, but we have to maintain compatibility with our existing merge behavior.

liggitt · 2022-02-03T15:35:57Z

/hold
for kubeconfig merging compatibility break

liggitt · 2022-02-23T22:43:25Z

/close

due to inactivity. an update that preserves existing kubeconfig / client-go behavior would be fine

k8s-ci-robot · 2022-02-23T22:43:44Z

@liggitt: Closed this PR.

In response to this:

/close

due to inactivity. an update that preserves existing kubeconfig / client-go behavior would be fine

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jlsong01 · 2022-02-27T15:09:48Z

/reopen

@liggitt I am very sorry for the delay. An update that preserves existing behavior has been submitted. PTAL. Thanks.

k8s-ci-robot · 2022-02-27T15:10:08Z

@jlsong01: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-02-27T15:11:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jlsong01
To complete the pull request process, please ask for approval from liggitt after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS
staging/src/k8s.io/legacy-cloud-providers/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2022-02-27T15:39:26Z

New changes are detected. LGTM label has been removed.

jlsong01 · 2022-02-27T16:46:53Z

/retest

jlsong01 · 2022-02-27T23:07:40Z

/retest

liggitt · 2022-02-28T14:31:55Z

vendor/github.com/imdario/mergo/map.go

@@ -72,6 +72,7 @@ func deepMap(dst, src reflect.Value, visited map[uintptr]*visit, depth int, conf
 	case reflect.Struct:
 		srcMap := src.Interface().(map[string]interface{})
 		for key := range srcMap {
+			config.overwriteWithEmptyValue = true


this really seems like a bug in mergo... this is mutating the merge configuration options passed in and I don't see it getting restored to it's configured value

yup, reported at darccio/mergo#187

@liggitt So, how about this PR? shall we upgrade the mergo version preserving existing behavior now? Or, wait for mergo project fixing it then upgrade mergo?

as #107499 reports: "client-go behavior changes depending on version of github.com/imdario/mergo used. Kubernetes pins to v0.3.5. Many other projects in the ecosystem, including Kubernetes projects like sigs.k8s.io/controller-runtime, use newer versions like 0.3.12.

If a consumer of client-go is not careful, they will very likely automatically use mergo. This will subtly give different behavior than client-go declares. Users of client-go must therefor use a replace for mergo (if they care about this issue), which also implies all downstream consumers must do the same and go install will not work."

maybe it's better to pick up this PR in order to unify the mergo version.

I think I'd rather see us drop mergo and implement our own merging of these fields given the repeated behavior reversals and long-standing bugs. Our usage seems very limited

liggitt · 2022-03-26T17:58:29Z

/close

I don't see us picking up a version of mergo with the bug noted in #107837 (comment)

I think the best resolution of #107499, given the number of times this dependency has reversed behavior on minor bumps, would be to eliminate use of the dependency and write our own merge of our narrow use

k8s-ci-robot · 2022-03-26T17:58:48Z

@liggitt: Closed this PR.

In response to this:

/close

I don't see us picking up a version of mergo with the bug noted in #107837 (comment)

I think the best resolution of #107499, given the number of times this dependency has reversed behavior on minor bumps, would be to eliminate use of the dependency and write our own merge of our narrow use

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

jlsong01 force-pushed the fix_merging_conflict_by_upgrade_mergo branch from b26ea0a to 84a9694 Compare January 28, 2022 15:35

k8s-ci-robot requested review from caesarxuchao, jpbetz and a team January 28, 2022 15:36

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Jan 28, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 28, 2022

jlsong01 force-pushed the fix_merging_conflict_by_upgrade_mergo branch from 84a9694 to 8ef2b00 Compare January 28, 2022 15:52

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 28, 2022

jlsong01 force-pushed the fix_merging_conflict_by_upgrade_mergo branch from 8ef2b00 to d4487f4 Compare January 28, 2022 16:22

jlsong01 changed the title ~~fix merging with conflict by upgrade mergo version to v.0.3.12~~ [WIP] fix merging with conflict by upgrade mergo version to v.0.3.12 Jan 28, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2022

jlsong01 force-pushed the fix_merging_conflict_by_upgrade_mergo branch from d4487f4 to 038152e Compare January 29, 2022 04:01

k8s-ci-robot added area/apiserver area/cloudprovider area/code-generation area/kubectl sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Jan 29, 2022

enj added this to Needs Triage in SIG Auth Old Jan 31, 2022

k8s-ci-robot assigned liggitt and cblecker Feb 3, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2022

liggitt reviewed Feb 3, 2022

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2022

k8s-ci-robot closed this Feb 23, 2022

k8s-ci-robot reopened this Feb 27, 2022

k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Feb 27, 2022

fix merging with conflict by upgrade mergo version to v.0.3.12

75a72e4

jlsong01 force-pushed the fix_merging_conflict_by_upgrade_mergo branch from 038152e to 75a72e4 Compare February 27, 2022 15:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2022

jlsong01 requested a review from liggitt February 27, 2022 16:15

liggitt reviewed Feb 28, 2022

View reviewed changes

k8s-ci-robot closed this Mar 26, 2022

ritazh removed this from Needs Triage in SIG Auth Old May 31, 2022

prologic mentioned this pull request Feb 5, 2024

remove use of github.com/imdario/mergo (currently use is incompatible with latest mergo version, latest mergo version introduces bugs) #107499

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix merging with conflict by upgrade mergo version to v0.3.12 #107837

Fix merging with conflict by upgrade mergo version to v0.3.12 #107837

jlsong01 commented Jan 28, 2022

jlsong01 commented Jan 28, 2022

cblecker commented Feb 3, 2022

liggitt Feb 3, 2022

liggitt commented Feb 3, 2022 •

edited

liggitt commented Feb 3, 2022

liggitt commented Feb 23, 2022

k8s-ci-robot commented Feb 23, 2022

jlsong01 commented Feb 27, 2022 •

edited

k8s-ci-robot commented Feb 27, 2022

k8s-ci-robot commented Feb 27, 2022

k8s-ci-robot commented Feb 27, 2022

jlsong01 commented Feb 27, 2022

jlsong01 commented Feb 27, 2022

liggitt Feb 28, 2022

liggitt Feb 28, 2022

jlsong01 Mar 2, 2022 •

edited

liggitt Mar 2, 2022

liggitt commented Mar 26, 2022

k8s-ci-robot commented Mar 26, 2022

		// insecure-skip-tls-verify: true
		// server: http://a-different-cow.org:8080

Fix merging with conflict by upgrade mergo version to v0.3.12 #107837

Fix merging with conflict by upgrade mergo version to v0.3.12 #107837

Conversation

jlsong01 commented Jan 28, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jlsong01 commented Jan 28, 2022

cblecker commented Feb 3, 2022

liggitt Feb 3, 2022

Choose a reason for hiding this comment

liggitt commented Feb 3, 2022 • edited

liggitt commented Feb 3, 2022

liggitt commented Feb 23, 2022

k8s-ci-robot commented Feb 23, 2022

jlsong01 commented Feb 27, 2022 • edited

k8s-ci-robot commented Feb 27, 2022

k8s-ci-robot commented Feb 27, 2022

k8s-ci-robot commented Feb 27, 2022

jlsong01 commented Feb 27, 2022

jlsong01 commented Feb 27, 2022

liggitt Feb 28, 2022

Choose a reason for hiding this comment

liggitt Feb 28, 2022

Choose a reason for hiding this comment

jlsong01 Mar 2, 2022 • edited

Choose a reason for hiding this comment

liggitt Mar 2, 2022

Choose a reason for hiding this comment

liggitt commented Mar 26, 2022

k8s-ci-robot commented Mar 26, 2022

liggitt commented Feb 3, 2022 •

edited

jlsong01 commented Feb 27, 2022 •

edited

jlsong01 Mar 2, 2022 •

edited