fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 #1044

anderson4u2 · 2021-03-23T22:40:44Z

As Pedro Arvela explained in #1043, #480 removed the validation for 'resources' field due to the inability to support float types for the validation. However we believe this is causing k8s to prune the undefined fields in the schema.

In this MR, I am re-adding the validations removed by #480 (and a couple that were probably missed) as fields that won't be pruned, will not break with float values, and so "resources" won't be pruned and will work as expected.

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

Signed-off-by: Anderson Silva <[email protected]>

codecov · 2021-03-23T22:46:37Z

Codecov Report

Merging #1044 (43c3589) into master (bff46e1) will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1044      +/-   ##
==========================================
+ Coverage   80.91%   81.00%   +0.08%     
==========================================
  Files         102      103       +1     
  Lines        9097     9166      +69     
==========================================
+ Hits         7361     7425      +64     
- Misses       1242     1246       +4     
- Partials      494      495       +1

Impacted Files	Coverage Δ
utils/unstructured/unstructured.go	`56.52% <0.00%> (-1.26%)`	⬇️
pkg/kubectl-argo-rollouts/cmd/cmd.go	`100.00% <0.00%> (ø)`
pkg/kubectl-argo-rollouts/cmd/status/status.go	`93.44% <0.00%> (ø)`
metricproviders/newrelic/newrelic.go	`92.00% <0.00%> (+0.69%)`	⬆️

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 bff46e1...43c3589. Read the comment docs.

Signed-off-by: Anderson Silva <[email protected]>

jessesuen · 2021-03-24T22:25:41Z

I'm unable to reproduce this issue using the sample gist in https://gist.github.com/PedroArvela/66dec9c51ce0872caf272f90603e28e8 and Kubernetes v1.20.2. Is this specific to kubernetes version?

jessesuen · 2021-03-24T22:30:39Z

Note that when we run controller-gen, we do not run it with preserveUnknownFields=false, meaning we are telling Kubernetes not to prune unknown fields:

	crdYamlBytes, err := exec.Command(
		"controller-gen",
		"paths=./pkg/apis/rollouts/...",
		"crd:trivialVersions=true",
		// cannot use preserveUnknownFields=false until controller-gen generates proper support for
		// resource.Quantity, which we remove validation for
		//"crd:preserveUnknownFields=false",
		"crd:crdVersions=v1beta1",
		"output:crd:stdout",
	).Output()

So I do not understand how this could happen.

jessesuen · 2021-03-24T22:53:50Z

OK I understand what's going on. From kubernetes documentation:

https://v1-18.docs.kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#preserving-unknown-fields

For CustomResourceDefinitions created in apiextensions.k8s.io/v1, structural OpenAPI v3 validation schemas are required and pruning is enabled and cannot be disabled (note that CRDs converted from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1 might lack structural schemas, and spec.preserveUnknownFields might be true).

In other words, kubernetes no longer supports the preservation of unknown fields as a top-level option, as of apiextensions.k8s.io/v1. But because Argo Rollouts still vends apiextensions.k8s.io/v1beta1 CRD definitions, we were able to get away with disabling this. So instead of relying on the deprecated spec.preserveUnknownFields ability, we must preserve unknown fields explicitly through the custom open API annotations which is being added in this PR.

I notice that when I get the v1 CRD object, i see the following condition:

status:
  conditions:
  - lastTransitionTime: "2021-03-19T11:33:45Z"
    message: 'spec.preserveUnknownFields: Invalid value: true: must be false'
    reason: Violations
    status: "True"
    type: NonStructuralSchema

So technically speaking, Argo Rollouts is vending a CRD that violates current CRD requirements and this PR is fixing that violation.

jessesuen · 2021-03-24T22:55:05Z

@anderson4u2 - this PR is valid, but I'm still curious how you wound up in this situation, given that Argo Rollouts vends the v1beta1 CRDs.

jsoref · 2021-03-24T23:48:44Z

The argo-helm repo is vending 1.0: https://github.com/argoproj/argo-helm/blob/master/charts/argo-rollouts/templates/crds/rollout-crd.yaml

And it's impossible to merge anything into that repository without vending 1.0.

jessesuen · 2021-03-25T17:44:08Z

manifests/crds/experiment-crd.yaml

-                                                    pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$
-                                                    x-kubernetes-int-or-string: true
-                                                  type: object
+                                                  x-kubernetes-preserve-unknown-fields: true


I'm still understanding the implications of dropping the resource quantity validation here, and why this only applied to ephemeral containers. Once I do that, the PR looks good to go.

I think in theory one would want to provide a full spec. But if you can't, then providing a "we don't know what belongs here, we're labeling it dragons -- here be dragons". This allows the dragons to live here.

Ideally someone will later spend the time to properly spec it out and replace the dragon gate.

But, also, when they do that, (probably in a new version,) it'll force people to revalidate the data that's hiding there instead naively trusting it (which is the reason k8s to force default omit -- to prevent pre-poisoning)

FYI I'm only setting the not-prune on values of resources/requests and resources/limits. So it limits the scope for failure to only those fields, not its subfields nor its parent fields, which limits the scope for failure significantly.

Signed-off-by: Anderson Silva <[email protected]>

anderson4u2 · 2021-03-26T16:55:06Z

Hello, I've pushed another commit to upgrade the API to v1 CRD. Let me know your thoughts. I've removed the function removeNestedItems as it seems it hasn't been run in the argo-helm templates, and we're running that without any problems. Any feedback greatly appreciated, thanks!

Signed-off-by: Anderson Silva <[email protected]>

jessesuen · 2021-03-26T22:11:45Z

Great! will take another look

jessesuen · 2021-03-26T22:18:56Z

I've removed the function removeNestedItems as it seems it hasn't been run in the argo-helm templates, and we're running that without any problems.

In order for you to notice any problems, you would have to have had a volume like:

  volumes:
    # You set volumes at the Pod level, then mount them into containers inside that Pod
    - name: config
      configMap:
        # Provide the name of the ConfigMap you want to mount.
        name: game-demo
        # An array of keys from the ConfigMap to create as files
        items:
        - key: "game.properties"
          path: "game.properties"
        - key: "user-interface.properties"
          path: "user-interface.properties"

Before removing this, can you test that this works with the new CRD?

Signed-off-by: Anderson Silva <[email protected]>

anderson4u2 · 2021-03-27T17:16:48Z

I've removed the function removeNestedItems as it seems it hasn't been run in the argo-helm templates, and we're running that without any problems.

In order for you to notice any problems, you would have to have had a volume like:
  volumes:
    # You set volumes at the Pod level, then mount them into containers inside that Pod
    - name: config
      configMap:
        # Provide the name of the ConfigMap you want to mount.
        name: game-demo
        # An array of keys from the ConfigMap to create as files
        items:
        - key: "game.properties"
          path: "game.properties"
        - key: "user-interface.properties"
          path: "user-interface.properties"
Before removing this, can you test that this works with the new CRD?

Hi, I've checked that it works with k8s 1.17+. Tested it with k8s 1.16 and got this error, the merge that fixed it was this, which was released in 1.17. I've also added a test in the test-e2e that fails in k8s 1.16 but succeeds in 1.17+.

jessesuen · 2021-03-31T22:45:00Z

Hi, I've checked that it works with k8s 1.17+. Tested it with k8s 1.16 and got this error, the merge that fixed it was this, which was released in 1.17. I've also added a test in the test-e2e that fails in k8s 1.16 but succeeds in 1.17+.

I just polled the community in slack and looks like we still have a significant portion of our users on v1.16 or below (4/10 responses at time of writing).

So I think it's better to preserve the existing behavior in order to continue to support our v1.16/v1.15 users. Could you add back removeNestedItems to support this?

Signed-off-by: Anderson Silva <[email protected]>

sonarcloud · 2021-04-02T16:51:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

anderson4u2 · 2021-04-02T18:13:27Z

Hi, I've checked that it works with k8s 1.17+. Tested it with k8s 1.16 and got this error, the merge that fixed it was this, which was released in 1.17. I've also added a test in the test-e2e that fails in k8s 1.16 but succeeds in 1.17+.

I just polled the community in slack and looks like we still have a significant portion of our users on v1.16 or below (4/10 responses at time of writing).

So I think it's better to preserve the existing behavior in order to continue to support our v1.16/v1.15 users. Could you add back removeNestedItems to support this?

Hi, I've checked with the refactored function but I still got errors when trying to deploy a configmap volume in k8s 1.16. I've unvalidated the whole volumes block and it now works. I've added this unvalidation to all CRDs. Let me know your thoughts :)

jessesuen

Great improvement!

jessesuen · 2021-03-26T22:12:48Z

hack/gen-crd-spec/main.go

+		// The only possible value is 'false' since 'apiextensions.k8s.io/v1'
+		// https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#field-pruning
+		// It is possible though to opt-out of pruning for specifc sub-trees of fields by adding x-kubernetes-preserve-unknown-fields: true
+		// by using the 'setValidationOverride' function in this file.
+		"crd:preserveUnknownFields=false",
+		"crd:crdVersions=v1",


Thanks for documenting this.

np, glad to help!

Support 'resources' field without schema enforcing its subfields

0119dbb

Signed-off-by: Anderson Silva <[email protected]>

anderson4u2 changed the title ~~fix(crd): Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043.~~ fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043. Mar 23, 2021

anderson4u2 changed the title ~~fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043.~~ fix: Fixes the regression of dropping resources from argo-rollouts crds. Mar 23, 2021

anderson4u2 changed the title ~~fix: Fixes the regression of dropping resources from argo-rollouts crds.~~ fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 Mar 23, 2021

run make manifests

3ae8cdf

Signed-off-by: Anderson Silva <[email protected]>

dbyron0 mentioned this pull request Mar 24, 2021

Status not updating despite --update-status bitnami-labs/sealed-secrets#555

Closed

jessesuen reviewed Mar 25, 2021

View reviewed changes

Update to v1

8c19a45

Signed-off-by: Anderson Silva <[email protected]>

Fix wrong indentation in existing test manifest

3fddf62

Signed-off-by: Anderson Silva <[email protected]>

anderson4u2 added 3 commits March 27, 2021 13:59

Add test for items.items api validation

18722a0

Signed-off-by: Anderson Silva <[email protected]>

Fix typo

4e7bbbc

Signed-off-by: Anderson Silva <[email protected]>

Retest

858b867

Signed-off-by: Anderson Silva <[email protected]>

anderson4u2 mentioned this pull request Mar 29, 2021

feat(argo-rollouts): Match autogenerated/tested CRDs in upstream argo-rollouts argoproj/argo-helm#638

Merged

5 tasks

Do not validate 'volumes' until only k8s 1.17+ are supported

43c3589

Signed-off-by: Anderson Silva <[email protected]>

jessesuen approved these changes Apr 2, 2021

View reviewed changes

jessesuen merged commit 01adc2e into argoproj:master Apr 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 #1044

fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 #1044

anderson4u2 commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

jessesuen commented Mar 24, 2021 •

edited

Loading

jessesuen commented Mar 24, 2021 •

edited

Loading

jessesuen commented Mar 24, 2021

jessesuen commented Mar 24, 2021

jsoref commented Mar 24, 2021

jessesuen Mar 25, 2021

jsoref Mar 25, 2021

anderson4u2 Mar 26, 2021

anderson4u2 commented Mar 26, 2021

jessesuen commented Mar 26, 2021

jessesuen commented Mar 26, 2021 •

edited

Loading

anderson4u2 commented Mar 27, 2021

jessesuen commented Mar 31, 2021

sonarcloud bot commented Apr 2, 2021

anderson4u2 commented Apr 2, 2021

jessesuen left a comment

jessesuen Mar 26, 2021

anderson4u2 Apr 2, 2021

fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 #1044

fix: Fixes the regression of dropping resources from argo-rollouts crds. Fixes #1043 #1044

Conversation

anderson4u2 commented Mar 23, 2021 • edited Loading

codecov bot commented Mar 23, 2021 • edited Loading

Codecov Report

jessesuen commented Mar 24, 2021 • edited Loading

jessesuen commented Mar 24, 2021 • edited Loading

jessesuen commented Mar 24, 2021

jessesuen commented Mar 24, 2021

jsoref commented Mar 24, 2021

jessesuen Mar 25, 2021

Choose a reason for hiding this comment

jsoref Mar 25, 2021

Choose a reason for hiding this comment

anderson4u2 Mar 26, 2021

Choose a reason for hiding this comment

anderson4u2 commented Mar 26, 2021

jessesuen commented Mar 26, 2021

jessesuen commented Mar 26, 2021 • edited Loading

anderson4u2 commented Mar 27, 2021

jessesuen commented Mar 31, 2021

sonarcloud bot commented Apr 2, 2021

anderson4u2 commented Apr 2, 2021

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen Mar 26, 2021

Choose a reason for hiding this comment

anderson4u2 Apr 2, 2021

Choose a reason for hiding this comment

anderson4u2 commented Mar 23, 2021 •

edited

Loading

codecov bot commented Mar 23, 2021 •

edited

Loading

jessesuen commented Mar 24, 2021 •

edited

Loading

jessesuen commented Mar 24, 2021 •

edited

Loading

jessesuen commented Mar 26, 2021 •

edited

Loading