Upgrade kustomize-in-kubectl to v4.2.0

[natasha41575]

What type of PR is this?

/kind bug
/sig cli
/priority important-soon

What this PR does / why we need it:

This upgrades kustomize-in-kubectl to v4.2.0. As discussed in SIG-CLI meetings, this PR prevents a breaking user change from appearing in kubectl kustomize. The original issue is linked here. and further described in this comment.

Summary copied from the comment linked above:

In a commit on 2020-May-04, marshaling code in gopkg.in/yaml.v3 changed indenting from

favorites:
  fruit: apple
  car: ford
fruits
- apple
- orange

to

favorites:
  fruit: apple
  car: ford
fruits
  - apple
  - orange

Kustomize (via kyaml) depends on gopkg.in/yaml.v3. Hundreds of kustomize tests assume the older more compact behavior, where the hyphens don't 'consume' an indent. These tests are intentionally brittle in this fashion as downstream consumers of the YAML also compare differences in marhsalled YAML and don't want surprises.

Meanwhile,

• kubectl depends on sigs.k8s.io/yaml, which in turn depends on gopkg.in/yaml.v2, which has the compact indent. One can see this by using the command kubectl create deployment foo --dry-run -o yaml --image foo
• The k8s repo has gopkg.in/yaml.v3 dependence, but not via kubectl.

This means that without this PR:

1. kustomize-in-kubectl's kyaml library would pick up the indentation change, resulting in breaking whitespace behavior because of the kubernetes dependence on the upgraded version of gopkg.in/yaml.v3.
2. The new v3 indentation would be inconsistent with kubectl's output.

To remedy this issue, we added a PR to go-yaml in the hopes that it would be accepted in time for the k/k code freeze. The go-yaml maintainer had agreed to make the indentation style configurable upstream, but did not make the change in time for code freeze, so we forked the go-yaml library under kyaml/internal to use the old indentation. We will remove the fork when the change is available upstream.

Special notes for your reviewer:

This is a go.mod change to upgrade the kustomize
dependency from v4.1.3 to v4.2.0.

Does this PR introduce a user-facing change?

Upgrades functionality of `kubectl kustomize` as described at
https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv4.2.0

How was this PR made?

./hack/pin-dependency.sh sigs.k8s.io/kustomize/kyaml v0.11.0
./hack/pin-dependency.sh sigs.k8s.io/kustomize/cmd/config v0.9.13
./hack/pin-dependency.sh sigs.k8s.io/kustomize/api v0.8.11
./hack/pin-dependency.sh sigs.k8s.io/kustomize/kustomize/v4 v4.2.0

./hack/update-vendor.sh
./hack/update-internal-modules.sh 
./hack/lint-dependencies.sh 

[k8s-ci-robot]

@natasha41575: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[natasha41575]

/cc @liggitt

[liggitt]

Consuming a fork (even a temporary one) of a dependency like go-yaml makes the maintenance story for versions of kubectl that include this level unclear.

It's hard to tell:

1. what version is vendor/sigs.k8s.io/kustomize/kyaml/internal/forked/github.com/go-yaml/yaml based on?
2. what are the additional deltas?
3. if there are security-related fixes to github.com/go-yaml/yaml in the next ~year (lifetime of k8s 1.22) that we would backport to kubernetes release branches, will kustomize also pick those up and cut new releases in a way we can consume in release branches without picking up significant new kustomize changes?

[liggitt]

the dependency update mechanics look good, we just need clarity around the maintenance story for the lifetime of the kubernetes releases which will contain this

[natasha41575]

1. what version is vendor/sigs.k8s.io/kustomize/kyaml/internal/forked/github.com/go-yaml/yaml based on?

It is based on the most recent commit of go-yaml v3: go-yaml/yaml@496545a, which is the same commit that k/k currently depends on.

2. what are the additional deltas?

The deltas are shown in the PR that we made to go-yaml. We were hoping that the PR would be accepted, but since it wasn't accepted in time for code freeze, we made a copy of the fork in that PR under kyaml/internal. It adds a method to the Encoder type that allows us the option of using the old sequence indentation style and there are a couple of tests for it.

3. if there are security-related fixes to github.com/go-yaml/yaml in the next ~year (lifetime of k8s 1.22) that we would backport to kubernetes release branches, will kustomize also pick those up and cut new releases in a way we can consume in release branches without picking up significant new kustomize changes?

Yes. If there are any fixes at all to the yaml library, we have a script that will allow us to pull in those commits to our fork. If there are security related fixes, kustomize will also prioritize picking them up and backporting them into kustomize release branches. I will file an issue in kustomize for this.

EDIT: Filed kubernetes-sigs/kustomize#4033

[natasha41575]

/test pull-kubernetes-dependencies

[BenTheElder]

in order for kubernetes to in turn pick up whatever fixes kustomize picks up, kubernetes will have to update kustomize, potentially post-kubernetes-release? so kustomize can't have breaking changes

does it make more sense to have this fork as an actual repo so it can be updated independently?

has anyone looked at updating sigs.k8s.io/yaml to this?

[natasha41575]

in order for kubernetes to in turn pick up whatever fixes kustomize picks up, kubernetes will have to update kustomize, potentially post-kubernetes-release? so kustomize can't have breaking changes

If there are fixes that kustomize needs to pick up urgently (such as security fixes), kustomize can pick them up and backport them into its release branches so that kubernetes can pick them up without picking up any other changes to kustomize.

Also FWIW kustomize isn't planning to have breaking changes soon.

does it make more sense to have this fork as an actual repo so it can be updated independently?

We'd discussed this but we chose to make an internal fork for the following reasons:

1. This fork is temporary because we don't want to maintain it forever. It will be removed as soon as the fix is changed upstream, so we felt it didn't make sense to have a separate public repo for it. The go-yaml maintainer said he'd have a fix by the end of June but was unable to make time, but when they do get around to it we won't need this fork anymore.

2. Time constraints - we were hoping we wouldn't need this fork and created it at the last minute. I'm not sure what the logistics are of creating a new fork under kubernetes-sigs but we only had a few days before code freeze and this felt like the best option given the circumstances.

has anyone looked at updating sigs.k8s.io/yaml to this?

We have not. I think sigs.k8s.io/yaml depends on go-yaml.v2. I don't know what the story is for v3, which is what we have forked here. Maybe it makes sense to have a permanent fork of v3 in sigs.k8s.io/yaml. My personal opinion is that in the long term it'll be good to have a separate yaml repo (or to have people that can contribute to go-yaml) because the maintainer of go-yaml seems very busy. But again, we only have a couple of days before code freeze and this temporary internal fork seems like the best option to us right now.

@KnVerey do you have anything to add?

[KnVerey]

That was a great summary, Natasha. I felt particularly strongly about your first point: we intentionally made the fork internal because we expect to no longer need it in the near future, and we have no desire to maintain it any longer than absolutely necessary. I think a repo created under kubernetes-sigs would likely get usage no matter what disclaimers we put on the readme and should only be created if we intend and have the resources to maintain it. I agree that the difficulty of getting changes that Kubernetes needs into go-yaml is concerning and worth ongoing discussion, but IMO this instance alone does not justify permanent fork creation at this time. You're right that sigs.k8s.io/yaml is v2-based. There's an issue about updating to v3: kubernetes-sigs/yaml#18. Given Kustomize's experience even within v3, I expect there might need to be quite a lot of work done to the fork to make a migration possible without surfacing unacceptable behavioural changes in the various k/k consumers.

[KnVerey]

@BenTheElder @liggitt did our replies address your concerns?

what are the additional deltas?

In case it is helpful, the PR that created the internal fork also has commits that separate out everything we did in creating the internal fork. All but the prep and cleanup commits were done by the script and would be simple to repeat if we need to update the fork during the lifetime of the release. We didn't squash, so you can also see the same commits on master: kubernetes-sigs/kustomize@276d043...e583f19

Breakdown of the commits in that range

The first two commits prepare the script and the codebase:

• 2e8a3b7c4 Katrina Verey: Use the forked go-yaml module (8 days ago)
• 9e4a6397d Katrina Verey: Instructions and script for go-yaml fork (8 days ago)

The next one pulls in the code as a subtree:

• 4cadad5cf Katrina Verey: Internal copy of go-yaml at 496545a6307b2a7d7a710fd516e5e16e8ab62dbc (6
  days ago)

The next four are Natasha's changes from her go-yaml PR:

• 0ddf68cc8 Natasha Sarkar: compact sequence indentation option (6 days ago)
• 95c5b686b Natasha Sarkar: add defaultSeqIndent method (6 days ago)
• beea785ea Natasha Sarkar: tests for compactSeqIndent (6 days ago)
• e308f321d Natasha Sarkar: fix leading newline issue (6 days ago)

The next one makes the forked code internal to kyaml instead of its own module:

• f3d888304 Katrina Verey: Internalize forked code (6 days ago)

Then we have the commits from Natasha's PR to integrate the go-yaml changes into Kustomize (kubernetes-sigs/kustomize#4004):

• c8b049f57 Natasha Sarkar: point to natasha's fork (6 days ago)
• 979f03e76 Natasha Sarkar: remove serialization hack after bump (6 days ago)
• 4b6604373 Natasha Sarkar: compact sequence indent (6 days ago)
• 3ab0665c1 Natasha Sarkar: fix affected kyaml tests (6 days ago)
• 5c2c617ff Natasha Sarkar: fix affected cmd/config tests (6 days ago)

And finally two cleanup commits:

• b465c20f6 Katrina Verey: Remove pinning to external fork (6 days ago)
• e583f199b Katrina Verey: Comment out part of script that is likely only needed on first run (6 days
  ago)

[liggitt]

• @dims for nested forked licenses not getting included correctly, xref third_party/forked/golang/LICENSE is not in LICENSES #103551

[liggitt]

in this case, we happen to already include LICENSES/vendor/gopkg.in/yaml.v3 and LICENSES/vendor/gopkg.in/yaml.v2

[liggitt]

If there are fixes that kustomize needs to pick up urgently (such as security fixes), kustomize can pick them up and backport them into its release branches so that kubernetes can pick them up without picking up any other changes to kustomize.

That addresses my concern for 1.22.

[liggitt]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, natasha41575

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [liggitt]

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