Skip to content
This repository has been archived by the owner on Feb 14, 2022. It is now read-only.

Use upstream yaml.v2 #145

Merged
merged 1 commit into from Oct 7, 2019
Merged

Conversation

chris-crone
Copy link
Member

@chris-crone chris-crone commented Oct 1, 2019

To mitigate against malicious YAML (kubernetes/kubernetes#83253), we had implemented our own patch to the yams.v2 library. Now that there's an upstream fix, this PR brings us back to using the upstream library.

EDIT: Note that this is implemented via the CLI so a PR has been opened there to implement the change. Once the CLI PR has been merged, this one will be updated to no longer use my CLI branch. This is now ready

@codecov-io
Copy link

codecov-io commented Oct 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@a81f27d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #145   +/-   ##
=========================================
  Coverage          ?   64.51%           
=========================================
  Files             ?       90           
  Lines             ?    10938           
  Branches          ?        0           
=========================================
  Hits              ?     7057           
  Misses            ?     3576           
  Partials          ?      305
Impacted Files Coverage Δ
internal/parsing/loader.go 100% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a81f27d...fc232fa. Read the comment docs.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, waiting for the CLI PR to be merged

@chris-crone chris-crone changed the title [WIP] Use upstream yaml.v2 Use upstream yaml.v2 Oct 1, 2019
@chris-crone
Copy link
Member Author

@silvin-lubecki I've updated the vendoring to use the latest docker/cli

@@ -62,11 +62,6 @@
name = "github.com/Masterminds/semver"
version = "v1.3.1"

[[override]]
name = "gopkg.in/yaml.v2"
source = "https://github.com/simonferquel/yaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW @simonferquel don't remove your branch, otherwise older release may no longer be able to check vendoring (if you want to get rid of the branch, you could tag it so that it's preserved)

Gopkg.toml Outdated
@@ -80,7 +75,7 @@

[[override]]
name = "github.com/docker/cli"
branch = "19.03"
revision = "d83cd90464377d4164c8f70248d064b979e5ca98"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open a backport to current branches; if that's merged soon, we don't have to make this change

Copy link
Contributor

@simonferquel simonferquel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@thaJeztah
Copy link
Member

@chris-crone upstream is merged into 19.03 so you can switch back to that branch; docker/cli#2119

@chris-crone
Copy link
Member Author

Moved vendoring back to 19.03.

To mitigate against malicious YAML (as described here:
kubernetes/kubernetes#83253) we used a patched
version of yaml.v2. There is now a fix upstream so we can leverage that.

Signed-off-by: Christopher Crone <christopher.crone@docker.com>
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@silvin-lubecki silvin-lubecki merged commit 1eb05cf into docker:master Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants