Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

volume structures (mountPath, persistentVolumeClaim) must be lower-case to avoid unmarshal error #4175

Closed
angry-cellophane opened this issue May 13, 2020 · 8 comments · Fixed by #5039
Labels
kind/bug Something isn't working kind/documentation priority/p3 agreed that this would be good to have, but no one is available at the moment.

Comments

@angry-cellophane
Copy link

angry-cellophane commented May 13, 2020

Follow-up to the issue reported in the comments of this question - #3883

Expected behavior

Skaffold parses the config and builds the image.

Actual behavior

Fails to parse the config

parsing skaffold config: unable to parse config: yaml: unmarshal errors:
  line 7: field mountPath not found in type v1.VolumeMount
  line 12: field persistentVolumeClaim not found in type v1.Volume

Information

  • Skaffold version: v1.9.0-25-gd7d0e8713-dirty
  • Operating system: Ubuntu 18.04.4 LTS
  • Contents of skaffold.yaml:
apiVersion: skaffold/v2beta3
kind: Config
build:
  artifacts:
  - image: gcr.io/k8s-skaffold/example
    kaniko:
      volumeMounts:
      - name: pvc1
        mountPath: /path
  cluster:
    volumes:
    - name: pvc1
      persistentVolumeClaim:
        claimName: kaniko-workspace

Steps to reproduce the behavior

  1. skaffold build

I'm not a big expert in golang and learned while debugging this issue so any help would be appreciated. So far it looks like this issue with go-yaml library.

Volume-ish structs come from the k8s api and doesn't have yaml tags.
for exampe,

type VolumeMount struct {
	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
	ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,2,opt,name=readOnly"`
	MountPath string `json:"mountPath" protobuf:"bytes,3,opt,name=mountPath"`
	SubPath string `json:"subPath,omitempty" protobuf:"bytes,4,opt,name=subPath"`
	MountPropagation *MountPropagationMode `json:"mountPropagation,omitempty" protobuf:"bytes,5,opt,name=mountPropagation,casttype=MountPropagationMode"`
	SubPathExpr string `json:"subPathExpr,omitempty" protobuf:"bytes,6,opt,name=subPathExpr"`
}

whereas all skaffold structs have the yaml tag

type KanikoArtifact struct {
	AdditionalFlags []string `yaml:"flags,omitempty"`
	DockerfilePath string `yaml:"dockerfile,omitempty"`
	Target string `yaml:"target,omitempty"`
	BuildArgs map[string]*string `yaml:"buildArgs,omitempty"`
	Env []v1.EnvVar `yaml:"env,omitempty"`
	InitImage string `yaml:"initImage,omitempty"`
	Image string `yaml:"image,omitempty"`
	Cache *KanikoCache `yaml:"cache,omitempty"`
	Reproducible bool `yaml:"reproducible,omitempty"`
	SkipTLS bool `yaml:"skipTLS,omitempty"`
	VolumeMounts []v1.VolumeMount `yaml:"volumeMounts,omitempty"`
}

go-yaml ignores external structs with the json tag only and doesn't add them to the schema.

I made a quick change to verify my finding and added yaml tags in all used sctructs in vendor/k8s.io/api/core/v1/types.go, then rebuilt skaffold and reran it against the attached config. The error was gone.

Example

type VolumeMount struct {
	Name string `yaml:"name" json:"name" protobuf:"bytes,1,opt,name=name"`
	ReadOnly bool `json:"readOnly,omitempty" protobuf:"varint,2,opt,name=readOnly"`
	MountPath string `yaml:"mountPath" json:"mountPath" protobuf:"bytes,3,opt,name=mountPath"`
	SubPath string `json:"subPath,omitempty" protobuf:"bytes,4,opt,name=subPath"`
	MountPropagation *MountPropagationMode `json:"mountPropagation,omitempty" protobuf:"bytes,5,opt,name=mountPropagation,casttype=MountPropagationMode"`
	SubPathExpr string `json:"subPathExpr,omitempty" protobuf:"bytes,6,opt,name=subPathExpr"`
}

Can anyone help me verify and confirm my finding please? Also any idea on how to fix it would be appreciated.

@angry-cellophane
Copy link
Author

There is an open PR in go-yaml to support json tags go-yaml/yaml#564

@dranes
Copy link

dranes commented May 17, 2020

I ran into this issue too but looking around found this other issue
go-yaml/yaml#320 (comment)

Basically, from the documentation, they stated:

Struct fields are only unmarshalled if they are exported (have an upper case first letter), and are unmarshalled using the field name lowercased as the default key.

so you could lowercase your fields and it should work, for example:

build:
  artifacts:
  - image: image1
    context: ./examples/app1
    kaniko:
      volumeMounts:
      - name: data
        mountpath: /data
        readonly: true

I wrote down two tests to observe this behavior that at this time I don't know if it is a bug or intended.

Failing Test:
https://travis-ci.org/github/dranes/skaffold/jobs/687835268#L325
Diff:
dranes@edd55c3

Passing test:
https://travis-ci.org/github/dranes/skaffold/jobs/687846445
Diff:
dranes@849bcbf

I hope this gives you some guidance or help you in some way

@tstromberg tstromberg added priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. triage/discuss Items for discussion kind/bug Something isn't working kind/documentation priority/p2 May take a couple of releases and removed triage/discuss Items for discussion priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. labels May 18, 2020
@tstromberg tstromberg changed the title Parse error when volumes added in skaffold.yaml volume structures (mountPath, persistentVolumeClaim) must be lower-case to avoid unmarshal error May 21, 2020
@tstromberg
Copy link
Contributor

@dranes - Thank you for the workaround and root causing!

I believe this will be considered a documentation bug rather than a code bug, but I've added the discuss label so that we can chat about it next week.

@tejal29
Copy link
Member

tejal29 commented Jun 19, 2020

Thanks @dranes.
@tstromberg i added some docs here #4362

@zoulux
Copy link

zoulux commented Jul 24, 2020

so mountPath => mountpath, persistentVolumeClaim ?

@briandealwis
Copy link
Member

It also means that object inlining, such as used in volume.volumeSource.configMap.name, doesn't work.

@MarlonGamez MarlonGamez added priority/p3 agreed that this would be good to have, but no one is available at the moment. and removed priority/p2 May take a couple of releases labels Aug 14, 2020
@nkubala nkubala added this to the Icebox milestone Sep 1, 2020
@jlewi
Copy link
Contributor

jlewi commented Nov 18, 2020

Is there any work around for dealing with configmaps?

I think one use case for configmaps is supporting ECR (#731); with ECR we want to mount in a creds.json file.

jlewi added a commit to jlewi/skaffold that referenced this issue Nov 19, 2020
…ntainerTools#4175

* As explained in GoogleContainerTools#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.
jlewi added a commit to jlewi/skaffold that referenced this issue Nov 19, 2020
…ntainerTools#4175

* As explained in GoogleContainerTools#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.
@MarlonGamez
Copy link
Contributor

thanks @jlewi for opening a PR to fix this :) We'll try to get that in quickly

jlewi added a commit to jlewi/skaffold that referenced this issue Dec 4, 2020
…ntainerTools#4175

* As explained in GoogleContainerTools#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.
MarlonGamez pushed a commit that referenced this issue Dec 9, 2020
…5039)

* Create a custom marshler for Volumes and VolumeMounts to fix #4175

* As explained in #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.

* * Add custom marshal rountines so that Go -> YAML properly serializes Volumes and VolumeMounts

* Run lint.

* Fix GCI issues.

* Fix GCI issues.

* Apply suggestions from code review

Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>

Co-authored-by: Tejal Desai <tejal29@gmail.com>
Co-authored-by: Nick Kubala <nkubala@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working kind/documentation priority/p3 agreed that this would be good to have, but no one is available at the moment.
Projects
None yet
10 participants