Repository: kumahq/kuma
Pull request #11548 (2024-09-25 07:27:19)

fix(MeshTrace): invalid sampling default values
### Checklist prior to review

<!--
Each of these sections need to be filled by the author when opening the PR.

If something doesn't apply please check the box and add a justification after the `--`
-->

- [x] [Link to relevant issue][1] as well as docs and UI issues --
- [x] This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like `syscall.Mkfifo` have equivalent implementation on the other OS --
- [x] Tests (Unit test, E2E tests, manual test on universal and k8s) --
  - Don't forget `ci/` labels to run additional/fewer tests
- [x] Do you need to update [`UPGRADE.md`](../blob/master/UPGRADE.md)? --
- [x] Does it need to be backported according to the [backporting policy](../blob/master/CONTRIBUTING.md#backporting)? ([this](https://github.com/kumahq/kuma/actions/workflows/auto-backport.yaml) GH action will add "backport" label based on these [file globs](https://github.com/kumahq/kuma/blob/master/.github/workflows/auto-backport.yaml#L6), if you want to prevent it from adding the "backport" label use [no-backport-autolabel](https://github.com/kumahq/kuma/blob/master/.github/workflows/auto-backport.yaml#L8) label) --

<!--
> Changelog: skip
-->
<!--
Uncomment the above section to explicitly set a [`> Changelog:` entry here](https://github.com/kumahq/kuma/blob/master/CONTRIBUTING.md#submitting-a-patch)?
-->

[1]: https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword


michaelbeaumont: > Don't we need to add a test to prevent re-introducing this? How did this pass validation tests the first time?

So would we have validation that sets every object field to the empty value to test if the default values of that field's fields are correct?
michaelbeaumont: > ok so the original PR never changed the tests to check if it work 👀 

In particular this only shows up if `sampling:` is set but not all the fields are set.
slonka: > So would we have validation that sets every object field to the empty value to test if the default values of that field's fields are correct?

That will also be beneficial. It's just that if we change the default from "100" to "100%" then we **NEED** to change the unit tests as well.
michaelbeaumont: > It's just that if we change the default from "100" to "100%" then we NEED to change the unit tests as well.

What exactly would the test test, just that the default values we enter aren't invalid?

Note this also only shows up with Kubernetes because of #6070 
slonka: @michaelbeaumont can you add an error case with explicit "80%"?
michaelbeaumont: > @michaelbeaumont can you add an error case with explicit "80%"?

What would that test?
slonka: > What would that test?

well exactly the problem that we hit 🤔 

Patch:
diff --git a/deployments/charts/kuma/crds/kuma.io_meshtraces.yaml b/deployments/charts/kuma/crds/kuma.io_meshtraces.yaml
index 19f58e12f..b16244ce6 100644
--- a/deployments/charts/kuma/crds/kuma.io_meshtraces.yaml
+++ b/deployments/charts/kuma/crds/kuma.io_meshtraces.yaml
@@ -142,7 +142,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -153,13 +153,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -169,7 +169,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.
diff --git a/docs/generated/openapi.yaml b/docs/generated/openapi.yaml
index 8af55e013..62a797174 100644
--- a/docs/generated/openapi.yaml
+++ b/docs/generated/openapi.yaml
@@ -10653,7 +10653,7 @@ components:
                       anyOf:
                         - type: integer
                         - type: string
-                      default: 100%
+                      default: 100
                       description: >-
                         Target percentage of requests that will be force traced
                         if the
@@ -10669,7 +10669,7 @@ components:
                       anyOf:
                         - type: integer
                         - type: string
-                      default: 100%
+                      default: 100
                       description: >-
                         Target percentage of requests will be traced
 
@@ -10679,11 +10679,11 @@ components:
                         random sampling). This field functions as an upper limit
                         on the total
 
-                        configured sampling rate. For instance, setting
-                        client_sampling to 100%
+                        configured sampling rate. For instance, setting client
+                        to 100
 
-                        but overall_sampling to 1% will result in only 1% of
-                        client requests with
+                        but overall to 1 will result in only 1% of client
+                        requests with
 
                         the appropriate headers to be force traced. Mirror of
 
@@ -10697,7 +10697,7 @@ components:
                       anyOf:
                         - type: integer
                         - type: string
-                      default: 100%
+                      default: 100
                       description: >-
                         Target percentage of requests that will be randomly
                         selected for trace
diff --git a/docs/generated/raw/crds/kuma.io_meshtraces.yaml b/docs/generated/raw/crds/kuma.io_meshtraces.yaml
index 19f58e12f..b16244ce6 100644
--- a/docs/generated/raw/crds/kuma.io_meshtraces.yaml
+++ b/docs/generated/raw/crds/kuma.io_meshtraces.yaml
@@ -142,7 +142,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -153,13 +153,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -169,7 +169,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.
diff --git a/pkg/plugins/policies/meshtrace/api/v1alpha1/meshtrace.go b/pkg/plugins/policies/meshtrace/api/v1alpha1/meshtrace.go
index 475206295..c2b01d83e 100644
--- a/pkg/plugins/policies/meshtrace/api/v1alpha1/meshtrace.go
+++ b/pkg/plugins/policies/meshtrace/api/v1alpha1/meshtrace.go
@@ -103,26 +103,26 @@ type Sampling struct {
 	// Target percentage of requests will be traced
 	// after all other sampling checks have been applied (client, force tracing,
 	// random sampling). This field functions as an upper limit on the total
-	// configured sampling rate. For instance, setting client_sampling to 100%
-	// but overall_sampling to 1% will result in only 1% of client requests with
+	// configured sampling rate. For instance, setting client to 100
+	// but overall to 1 will result in only 1% of client requests with
 	// the appropriate headers to be force traced. Mirror of
 	// overall_sampling in Envoy
 	// https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
 	// Either int or decimal represented as string.
-	// +kubebuilder:default="100%"
+	// +kubebuilder:default=100
 	Overall *intstr.IntOrString `json:"overall,omitempty"`
 	// Target percentage of requests that will be force traced if the
 	// 'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
 	// https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L127-L133
 	// Either int or decimal represented as string.
-	// +kubebuilder:default="100%"
+	// +kubebuilder:default=100
 	Client *intstr.IntOrString `json:"client,omitempty"`
 	// Target percentage of requests that will be randomly selected for trace
 	// generation, if not requested by the client or not forced.
 	// Mirror of random_sampling in Envoy
 	// https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L135-L140
 	// Either int or decimal represented as string.
-	// +kubebuilder:default="100%"
+	// +kubebuilder:default=100
 	Random *intstr.IntOrString `json:"random,omitempty"`
 }
 
diff --git a/pkg/plugins/policies/meshtrace/api/v1alpha1/schema.yaml b/pkg/plugins/policies/meshtrace/api/v1alpha1/schema.yaml
index 605d9fca6..201175a2c 100644
--- a/pkg/plugins/policies/meshtrace/api/v1alpha1/schema.yaml
+++ b/pkg/plugins/policies/meshtrace/api/v1alpha1/schema.yaml
@@ -115,7 +115,7 @@ properties:
                 anyOf:
                   - type: integer
                   - type: string
-                default: 100%
+                default: 100
                 description: |-
                   Target percentage of requests that will be force traced if the
                   'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -126,13 +126,13 @@ properties:
                 anyOf:
                   - type: integer
                   - type: string
-                default: 100%
+                default: 100
                 description: |-
                   Target percentage of requests will be traced
                   after all other sampling checks have been applied (client, force tracing,
                   random sampling). This field functions as an upper limit on the total
-                  configured sampling rate. For instance, setting client_sampling to 100%
-                  but overall_sampling to 1% will result in only 1% of client requests with
+                  configured sampling rate. For instance, setting client to 100
+                  but overall to 1 will result in only 1% of client requests with
                   the appropriate headers to be force traced. Mirror of
                   overall_sampling in Envoy
                   https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -142,7 +142,7 @@ properties:
                 anyOf:
                   - type: integer
                   - type: string
-                default: 100%
+                default: 100
                 description: |-
                   Target percentage of requests that will be randomly selected for trace
                   generation, if not requested by the client or not forced.
diff --git a/pkg/plugins/policies/meshtrace/k8s/crd/kuma.io_meshtraces.yaml b/pkg/plugins/policies/meshtrace/k8s/crd/kuma.io_meshtraces.yaml
index 19f58e12f..b16244ce6 100644
--- a/pkg/plugins/policies/meshtrace/k8s/crd/kuma.io_meshtraces.yaml
+++ b/pkg/plugins/policies/meshtrace/k8s/crd/kuma.io_meshtraces.yaml
@@ -142,7 +142,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -153,13 +153,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -169,7 +169,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.


Test patch:
diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml
index cb19c2f25..2e1a99c60 100644
--- a/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml
+++ b/app/kumactl/cmd/install/testdata/install-control-plane.defaults.golden.yaml
@@ -6913,7 +6913,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -6924,13 +6924,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -6940,7 +6940,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.
diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml
index 28d114edc..7b33cc5c3 100644
--- a/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml
+++ b/app/kumactl/cmd/install/testdata/install-control-plane.gateway-api-present.yaml
@@ -6913,7 +6913,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -6924,13 +6924,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -6940,7 +6940,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.
diff --git a/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml b/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml
index 8cfcf12e3..bc430e24e 100644
--- a/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml
+++ b/app/kumactl/cmd/install/testdata/install-control-plane.with-helm-set.yaml
@@ -6933,7 +6933,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -6944,13 +6944,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -6960,7 +6960,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.
diff --git a/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml b/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml
index c41ee65e9..f3cac3562 100644
--- a/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml
+++ b/app/kumactl/cmd/install/testdata/install-crds.all.golden.yaml
@@ -8399,7 +8399,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be force traced if the
                           'x-client-trace-id' header is set. Mirror of client_sampling in Envoy
@@ -8410,13 +8410,13 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests will be traced
                           after all other sampling checks have been applied (client, force tracing,
                           random sampling). This field functions as an upper limit on the total
-                          configured sampling rate. For instance, setting client_sampling to 100%
-                          but overall_sampling to 1% will result in only 1% of client requests with
+                          configured sampling rate. For instance, setting client to 100
+                          but overall to 1 will result in only 1% of client requests with
                           the appropriate headers to be force traced. Mirror of
                           overall_sampling in Envoy
                           https://github.com/envoyproxy/envoy/blob/v1.22.0/api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto#L142-L150
@@ -8426,7 +8426,7 @@ spec:
                         anyOf:
                         - type: integer
                         - type: string
-                        default: 100%
+                        default: 100
                         description: |-
                           Target percentage of requests that will be randomly selected for trace
                           generation, if not requested by the client or not forced.