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

fix(labelling): hotfix for issues related to deploy erroring due to nested yaml field overriding in StatefulSet w/ PVC #7011

Conversation

aaron-prindle
Copy link
Contributor

@aaron-prindle aaron-prindle commented Jan 13, 2022

Hotfix for an issue that is being seen in which when skaffold attempts to redeploy manifests with nested objects and "immutable" fields, skaffold fails as it will attempt to override the label/image when doing so will result in a deployment error.

Minimal repro:

Skaffold.yaml

apiVersion: skaffold/v2beta12
kind: Config
deploy:
  kubectl:
    manifests:
      - k8-*

k8-pod.yaml:

apiVersion: apps/v1
kind: StatefulSet
metadata:
  name: web
spec:
  selector:
    matchLabels:
      app: echoserver # has to match .spec.template.metadata.labels
  serviceName: "echoserver"
  replicas: 3 # by default is 1
  template:
    metadata:
      labels:
        app: echoserver 
    spec:
      terminationGracePeriodSeconds: 10
      containers:
      - image: k8s.gcr.io/echoserver:1.4
        name: echoserver
        volumeMounts:
        - name: www
          mountPath: /usr/share/nginx/html
  volumeClaimTemplates:
  - metadata:
      name: www
    spec:
      accessModes: [ "ReadWriteOnce" ]
      resources:
        requests:
          storage: 1Gi

run "skaffold deploy" twice w/o this PR fails with:

aprindle@aprindle ~/repro-6416 $ skaffold deploy
Tags used in deployment:
Starting deploy...
 - The StatefulSet "web" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'template', 'updateStrategy' and 'minReadySeconds' are forbidden
kubectl apply: exit status 1[

works w/ this change.

A more extensible/robust approach to fixing this issue is in progress as well that will implement something similar to the proposed idea from - #6236 (found here - https://github.com/GoogleContainerTools/skaffold/blob/main/docs/design_proposals/configurable-transformable-allowlist.md)

@aaron-prindle aaron-prindle requested a review from a team as a code owner January 13, 2022 01:40
@aaron-prindle aaron-prindle requested review from briandealwis and removed request for gsquared94 January 13, 2022 01:42
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from 0677db1 to d3ddeee Compare January 13, 2022 01:43
@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #7011 (31df8c8) into main (290280e) will decrease coverage by 1.71%.
The diff coverage is 56.96%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7011      +/-   ##
==========================================
- Coverage   70.48%   68.77%   -1.72%     
==========================================
  Files         515      554      +39     
  Lines       23150    25695    +2545     
==========================================
+ Hits        16317    17671    +1354     
- Misses       5776     6822    +1046     
- Partials     1057     1202     +145     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/deploy.go 52.00% <ø> (-1.85%) ⬇️
cmd/skaffold/app/cmd/dev.go 84.61% <0.00%> (ø)
cmd/skaffold/app/cmd/render.go 36.66% <0.00%> (-4.72%) ⬇️
cmd/skaffold/skaffold.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/inspect_tests.go 62.50% <14.28%> (-1.14%) ⬇️
cmd/skaffold/app/cmd/lsp.go 28.12% <28.12%> (ø)
cmd/skaffold/app/cmd/fix.go 68.85% <40.00%> (-7.62%) ⬇️
cmd/skaffold/app/cmd/lint.go 42.85% <42.85%> (ø)
cmd/skaffold/app/cmd/find_configs.go 48.88% <50.00%> (+0.24%) ⬆️
cmd/skaffold/app/skaffold.go 76.19% <70.00%> (-8.43%) ⬇️
... and 181 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 8cd8b69...31df8c8. Read the comment docs.

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from d3ddeee to e758885 Compare January 13, 2022 02:30
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch 2 times, most recently from 9115b39 to 9f726e9 Compare January 13, 2022 02:38
@aaron-prindle aaron-prindle changed the title fix(labelling): hotfix for issues related to deploy erroring due to nested yaml overriding in StatefulSet w/ PVC fix(labelling): hotfix for issues related to deploy erroring due to nested yaml field overriding in StatefulSet w/ PVC Jan 13, 2022
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch 3 times, most recently from 905e64e to e5f3f34 Compare January 13, 2022 05:54
Copy link
Member

@briandealwis briandealwis left a comment

Choose a reason for hiding this comment

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

LGTM with one caveat/question: is jsonpath a valid JSONPath string? If not, could you rename jsonpath to navpath or something similar to avoid any confusion that this the contents is actually a JSONPath string?

pkg/skaffold/kubernetes/manifest/visitor.go Outdated Show resolved Hide resolved
pkg/skaffold/kubernetes/manifest/visitor.go Outdated Show resolved Hide resolved
@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from e5f3f34 to a3577fb Compare January 13, 2022 19:12
@aaron-prindle
Copy link
Contributor Author

LGTM with one caveat/question: is jsonpath a valid JSONPath string? If not, could you rename jsonpath to navpath or something similar to avoid any confusion that this the contents is actually a JSONPath string?

Good idea. No I don't believe it is a valid JSONPath string as we cannot get the array index values which I believe is part of that spec. I have renamed the field "navpath"

@aaron-prindle aaron-prindle force-pushed the fix-6416-update-manifest-allowlist-functionality-v2 branch from a3577fb to 23335e8 Compare January 13, 2022 19:18
…ested yaml field overriding in StatefulSet w/ PVC
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants