Create a custom unmarshler for Volumes and VolumeMounts to fix #4175

[jlewi]

Fixes: #4175

Description

• As explained in volume structures (mountPath, persistentVolumeClaim) must be lower-case to avoid unmarshal error #4175 structs imported from the K8s v1 core API library
  (e.g. volumes and volumeMounts) aren't parsed correctly because
  they use json tags whereas the skaffold data structures
  use the YAML tags.

• This PR fixes that by implementing a custom marshler for the Volumes and VolumeMounts
  fields. The custom marshler removes the problematic fields and parses
  the rest of the struct using the built in YAML parser.

  For the problematic fields these are first marshled using json to take advantage of the json
  tags and then unmarshaled using json to ensure they are unmarshaled based
  on the json tags.

• Due to the fact that the custom UnMarshal methods are implemented in the latest package the
  custom marshal functions will only be applied if the skaffold config is the latest version i.e. v2beta10;
  if appropriate we could copy this to the v2beta9/config.go file to have it apply to the existing config.

[jlewi]

/assign @briandealwis

[codecov]

Codecov Report

Merging #5039 (72b5aac) into master (272f104) will decrease coverage by 0.10%.
The diff coverage is 57.44%.

@@            Coverage Diff             @@
##           master    #5039      +/-   ##
==========================================
- Coverage   72.19%   72.09%   -0.11%     
==========================================
  Files         380      382       +2     
  Lines       13303    13508     +205     
==========================================
+ Hits         9604     9738     +134     
- Misses       3006     3053      +47     
- Partials      693      717      +24     

Impacted Files	Coverage Δ
pkg/skaffold/schema/util/yaml.go	57.14% <57.14%> (ø)
pkg/skaffold/schema/latest/config.go	58.82% <57.57%> (-41.18%)	⬇️
cmd/skaffold/app/tips/tips.go	66.66% <0.00%> (-33.34%)	⬇️
cmd/skaffold/app/cmd/deploy.go	57.89% <0.00%> (-10.86%)	⬇️
pkg/skaffold/schema/validation/validation.go	89.30% <0.00%> (-6.76%)	⬇️
cmd/skaffold/app/cmd/runner.go	68.29% <0.00%> (-3.51%)	⬇️
pkg/skaffold/yamltags/tags.go	89.47% <0.00%> (-2.20%)	⬇️
pkg/skaffold/errors/errors.go	91.30% <0.00%> (-1.88%)	⬇️
pkg/skaffold/docker/image.go	79.34% <0.00%> (-0.20%)	⬇️
pkg/skaffold/util/store.go	100.00% <0.00%> (ø)
... and 11 more

---

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 272f104...72b5aac. Read the comment docs.

[jlewi]

It looks like the lint failed with

pkg/skaffold/schema/latest/config.go:20: File is not `gci`-ed with -local github.com/GoogleContainerTools/skaffold (gci)
	"encoding/json"
pkg/skaffold/schema/versions_test.go:21: File is not `gci`-ed with -local github.com/GoogleContainerTools/skaffold (gci)
	v1 "k8s.io/api/core/v1"
pkg/skaffold/schema/versions_test.go:23: File is not `gci`-ed with -local github.com/GoogleContainerTools/skaffold (gci)

Not sure what this means; any guidance?

One of the tests is also failing; something about bazel.

[IsaacPD]

@jlewi The linting is failing because of a prior commit so you can expect that until #5043 is merged and this branch is not rebased. I'm not so sure about the bazel integration test failing, that could be a flake or related to this PR, i'll re-run the test again but I would suggest running that particular integration test locally if it fails again to see if the cause is in this PR.

[MarlonGamez]

hi @jlewi, thanks for opening this PR :) It looks like the gci linter is complaining because it wants a specific import ordering/format. If you install gci you can run
gci -w -local github.com/GoogleContainerTools/skaffold pkg/skaffold/schema/versions_test.go pkg/skaffold/schema/latest/config.go

or you can just change the import section manually to

import (
	"encoding/json"

	v1 "k8s.io/api/core/v1"
	"sigs.k8s.io/kustomize/kyaml/yaml"

	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
)

for config.go

and

import (
	"fmt"
	"testing"

	v1 "k8s.io/api/core/v1"
	"k8s.io/client-go/tools/clientcmd/api"

	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/kaniko"
	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/defaults"
	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
	"github.com/GoogleContainerTools/skaffold/testutil"
)

for versions_test.go

[jlewi]

@MarlonGamez and @IsaacPD I'll be happy to fix the lint issues. Before I do could you give me some high level guidance on whether this PR is likely to be accepted or whether there are other substantive changes you'd like?

[MarlonGamez]

@jlewi I actually talked about it earlier today with the team, and it's very likely to be merged. I think the only thing I need to discuss a bit more is where exactly we want to keep the unmarshal functions, since we don't have much of a precedent with unmarshal functions. We've never really put logic in the config.go.

I'll get back to you soon with some updates on that!

[briandealwis]

It seems to me for this approach to work in the long term, we would need to duplicate or add corresponding UnmarshalYAML() functions for each subsequent schema versions?

[MarlonGamez]

Hey @jlewi, I think that we'd like to make a change here. I think that we should house the logic of the unmarshal functions in the pkg/skaffold/schema/util package, but keep the unmarshal functions in the config.go like how you have them now and have them call out to the util functions

This will hopefully help to keep things concise and avoid unnecessary duplication when creating new versions of config.go

[jlewi]

@MarlonGamez @briandealwis Thanks. I'll make that change.

It seems to me for this approach to work in the long term, we would need to duplicate or add corresponding UnmarshalYAML() functions for each subsequent schema versions?

I think that's true. I think one advantage of this approach is that it means the marshaling/unmarshaling logic gets versioned with the specs. This is likely a good thing as it means you can make changes in newer versions without breaking backwards compatibility.

As one example, a partial work around for #4175 was to use all lower case.
#4175 (comment)

Since the parsing logic is tied to the version we can fix it in newer versions without breaking existing versions.

[jlewi]

I've refactored out some of the unmarshal code into helper functions inside schema/util/yaml.go.

I don't think we can move all the code into util because that would create a circular dependency

• latest depends on util for marshaling functions
• util depends on latest for schemas

I ran GCI so hopefully that fixes lint.

I didn't add unittests for yaml.go because that code is covered by the unittests for the unmarshaling/marshaling unittests for the entire structure.

[briandealwis]

We still have a problem with code that marshals the Skaffold model back out to YAML (skaffold fix). You can see the effect if backport the change to v2beta9 and then try a fix to go from v2beta9 to v2beta10.

[jlewi]

Thanks @briandealwis. I believe that means we need custom marshal functions. I can take a stab at adding thos.

[jlewi]

@briandealwis PTAL I added Marshal functions that should marshal it correctly to YAML.

[jlewi]

@briandealwis @MarlonGamez any thoughts about the latest revisions?

[nkubala]

the logic in this PR looks really good, but the unfortunate side effect is that our script to create new schema versions is going to bring the block from config.go with it to every new config version we create.

I think we should be able to refactor this out into a custom type that lives in a different file, which can be referenced by the generated code, but we can do that in a follow up PR. cc @marlon-gamez to potentially take this on

[jlewi]

@nkubala Thanks for the suggestions!

[nkubala]

@jlewi thanks so much for the contribution!

[gfvirga]

Hello,

I built skaffold from this commit and I am trying to mount configMap volumes to kaniko, but I am getting the unmarshall errors:

Also trying to use mountPath is also returning unmarshal errors.

$ skaffold dev --port-forward
parsing skaffold config: unable to parse config: yaml: unmarshal errors:
  line 16: field configMap not found in type v1.Volume

This is what I tried:

apiVersion: skaffold/v2beta10
kind: Config
build:
  artifacts:
  - image: registry.prod.company.com/company-docker-temp/gabe-skaffold/git-started
    kaniko:
      cache: {}
      skipTLS: True
      skipTLSVerifyPull: True
      volumeMounts: 
        - mountpath: /kaniko/ssl/certs/
          name: cabundle
  cluster:
    pullSecretName: kaniko-secret
    pullSecretPath: kaniko-secret.json
    volumes: 
    - name: cabundle
      configMap:
        name: cabundle