api: making the value optional for JSONPatchOperation (#2522)

* api: making the value optional Signed-off-by: He Jie Xu <[email protected]> * address comments Signed-off-by: He Jie Xu <[email protected]> * update comment Signed-off-by: He Jie Xu <[email protected]> * generates Signed-off-by: He Jie Xu <[email protected]> * address comments Signed-off-by: He Jie Xu <[email protected]> * gen Signed-off-by: He Jie Xu <[email protected]> * fix format Signed-off-by: He Jie Xu <[email protected]> * update Signed-off-by: He Jie Xu <[email protected]> * fix format Signed-off-by: He Jie Xu <[email protected]> * gen again again Signed-off-by: He Jie Xu <[email protected]> * address comments Signed-off-by: He Jie Xu <[email protected]> * gen Signed-off-by: He Jie Xu <[email protected]> * address comment Signed-off-by: He Jie Xu <[email protected]> * fix test Signed-off-by: He Jie Xu <[email protected]> --------- Signed-off-by: He Jie Xu <[email protected]>
envoyproxy · Feb 8, 2024 · c3a2bd9 · c3a2bd9
1 parent 151ae6a
commit c3a2bd9
Show file tree

Hide file tree

Showing 20 changed files with 399 additions and 9 deletions.
diff --git a/api/v1alpha1/envoypatchpolicy_types.go b/api/v1alpha1/envoypatchpolicy_types.go
@@ -117,8 +117,10 @@ type JSONPatchOperation struct {
 	// Refer to https://datatracker.ietf.org/doc/html/rfc6901 for more details.
 	// +optional
 	From *string `json:"from,omitempty"`
-	// Value is the new value of the path location.
-	Value apiextensionsv1.JSON `json:"value"`
+	// Value is the new value of the path location. The value is only used by
+	// the `add` and `replace` operations.
+	// +optional
+	Value *apiextensionsv1.JSON `json:"value,omitempty"`
 }
 
 // EnvoyPatchPolicyStatus defines the state of EnvoyPatchPolicy

diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go
diff --git a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml
@@ -79,11 +79,11 @@ spec:
                           type: string
                         value:
                           description: Value is the new value of the path location.
+                            The value is only used by the `add` and `replace` operations.
                           x-kubernetes-preserve-unknown-fields: true
                       required:
                       - op
                       - path
-                      - value
                       type: object
                     type:
                       description: Type is the typed URL of the Envoy xDS Resource

diff --git a/internal/ir/xds.go b/internal/ir/xds.go
@@ -1252,7 +1252,7 @@ type JSONPatchOperation struct {
 	// +optional
 	From *string `json:"from,omitempty" yaml:"from,omitempty"`
 	// Value is the new value of the path location.
-	Value apiextensionsv1.JSON `json:"value" yaml:"value"`
+	Value *apiextensionsv1.JSON `json:"value,omitempty" yaml:"value,omitempty"`
 }
 
 // Tracing defines the configuration for tracing a Envoy xDS Resource

diff --git a/internal/ir/zz_generated.deepcopy.go b/internal/ir/zz_generated.deepcopy.go
diff --git a/internal/xds/translator/jsonpatch.go b/internal/xds/translator/jsonpatch.go
@@ -27,8 +27,12 @@ import (
 )
 
 const (
-	AddOperation = "add"
-	EmptyPath    = ""
+	AddOperation     = "add"
+	RemoveOperation  = "remove"
+	ReplaceOperation = "replace"
+	CopyOperation    = "copy"
+	MoveOperation    = "move"
+	EmptyPath        = ""
 )
 
 // processJSONPatches applies each JSONPatch to the Xds Resources for a specific type.
@@ -51,6 +55,21 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []*
 				err          error
 			)
 
+			switch p.Operation.Op {
+			case AddOperation, ReplaceOperation:
+				if p.Operation.Value == nil {
+					msg := fmt.Sprintf("The %s operation requires a value", p.Operation.Op)
+					status.SetEnvoyPatchPolicyInvalid(e.Status, msg)
+					continue
+				}
+			default:
+				if p.Operation.Value != nil {
+					msg := fmt.Sprintf("The value field can not be set for the %s operation", p.Operation.Op)
+					status.SetEnvoyPatchPolicyInvalid(e.Status, msg)
+					continue
+				}
+			}
+
 			// If Path is "" and op is "add", unmarshal and add the patch as a complete
 			// resource
 			if p.Operation.Op == AddOperation && p.Operation.Path == EmptyPath {

diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-add-op-without-value.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-add-op-without-value.yaml
@@ -0,0 +1,62 @@
+envoyPatchPolicies:
+- status: {}
+  name: "first-policy"
+  namespace: "default"
+  jsonPatches:
+  - type: "type.googleapis.com/envoy.config.listener.v3.Listener"
+    name: "first-listener"
+    operation:
+      op: "add"
+      path: "/filter_chains/0/filters/0/typed_config/http_filters/0"
+      value:
+        name: "envoy.filters.http.ratelimit"
+        typed_config:
+          "@type": "type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit"
+          domain: "eg-ratelimit"
+          failure_mode_deny: true
+          timeout: 1s
+          rate_limit_service:
+            grpc_service:
+              envoy_grpc:
+                cluster_name: rate-limit-cluster
+            transport_api_version: V3
+  - type: "type.googleapis.com/envoy.config.route.v3.RouteConfiguration"
+    name: "first-listener"
+    operation:
+      op: "add"
+      path: "/virtual_hosts/0/rate_limits"
+http:
+- name: "first-listener"
+  address: "0.0.0.0"
+  port: 10080
+  hostnames:
+  - "*"
+  path:
+    mergeSlashes: true
+    escapedSlashesAction: UnescapeAndRedirect
+  tls:
+    alpnProtocols:
+    - h2
+    - http/1.1
+    certificates:
+    - name: secret-1
+      # byte slice representation of "key-data"
+      serverCertificate: [99, 101, 114, 116, 45, 100, 97, 116, 97]
+      # byte slice representation of "key-data"
+      privateKey: [107, 101, 121, 45, 100, 97, 116, 97]
+    - name: secret-2
+      serverCertificate: [99, 101, 114, 116, 45, 100, 97, 116, 97]
+      privateKey: [107, 101, 121, 45, 100, 97, 116, 97]
+  routes:
+  - name: "first-route"
+    hostname: "*"
+    headerMatches:
+    - name: user
+      stringMatch:
+      exact: "jason"
+    destination:
+      name: "first-route-dest"
+      settings:
+      - endpoints:
+        - host: "1.2.3.4"
+          port: 50000
diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-move-op-with-value.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-move-op-with-value.yaml
@@ -0,0 +1,65 @@
+envoyPatchPolicies:
+- status: {}
+  name: "first-policy"
+  namespace: "default"
+  jsonPatches:
+  - type: "type.googleapis.com/envoy.config.listener.v3.Listener"
+    name: "first-listener"
+    operation:
+      op: "add"
+      path: "/filter_chains/0/filters/0/typed_config/http_filters/0"
+      value:
+        name: "envoy.filters.http.ratelimit"
+        typed_config:
+          "@type": "type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit"
+          domain: "eg-ratelimit"
+          failure_mode_deny: true
+          timeout: 1s
+          rate_limit_service:
+            grpc_service:
+              envoy_grpc:
+                cluster_name: rate-limit-cluster
+            transport_api_version: V3
+  - type: "type.googleapis.com/envoy.config.listener.v3.Listener"
+    name: "first-listener"
+    operation:
+      op: "remove"
+      from: "/filter_chains/0/filters/0/typed_config/http_filters/0"
+      path: "/filter_chains/0/filters/0/typed_config/http_filters/1"
+      value:
+        test: "abc"
+http:
+- name: "first-listener"
+  address: "0.0.0.0"
+  port: 10080
+  hostnames:
+  - "*"
+  path:
+    mergeSlashes: true
+    escapedSlashesAction: UnescapeAndRedirect
+  tls:
+    alpnProtocols:
+    - h2
+    - http/1.1
+    certificates:
+    - name: secret-1
+      # byte slice representation of "key-data"
+      serverCertificate: [99, 101, 114, 116, 45, 100, 97, 116, 97]
+      # byte slice representation of "key-data"
+      privateKey: [107, 101, 121, 45, 100, 97, 116, 97]
+    - name: secret-2
+      serverCertificate: [99, 101, 114, 116, 45, 100, 97, 116, 97]
+      privateKey: [107, 101, 121, 45, 100, 97, 116, 97]
+  routes:
+  - name: "first-route"
+    hostname: "*"
+    headerMatches:
+    - name: user
+      stringMatch:
+      exact: "jason"
+    destination:
+      name: "first-route-dest"
+      settings:
+      - endpoints:
+        - host: "1.2.3.4"
+          port: 50000
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.clusters.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.clusters.yaml
@@ -0,0 +1,14 @@
+- commonLbConfig:
+    localityWeightedLbConfig: {}
+  connectTimeout: 10s
+  dnsLookupFamily: V4_ONLY
+  edsClusterConfig:
+    edsConfig:
+      ads: {}
+      resourceApiVersion: V3
+    serviceName: first-route-dest
+  lbPolicy: LEAST_REQUEST
+  name: first-route-dest
+  outlierDetection: {}
+  perConnectionBufferLimitBytes: 32768
+  type: EDS
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.endpoints.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.endpoints.yaml
@@ -0,0 +1,12 @@
+- clusterName: first-route-dest
+  endpoints:
+  - lbEndpoints:
+    - endpoint:
+        address:
+          socketAddress:
+            address: 1.2.3.4
+            portValue: 50000
+      loadBalancingWeight: 1
+    loadBalancingWeight: 1
+    locality:
+      region: first-route-dest/backend/0
diff --git a/...xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml b/...xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml
@@ -0,0 +1,9 @@
+- name: first-policy
+  namespace: default
+  status:
+    conditions:
+    - lastTransitionTime: null
+      message: The add operation requires a value
+      reason: Invalid
+      status: "False"
+      type: Programmed
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.listeners.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.listeners.yaml
@@ -0,0 +1,61 @@
+- address:
+    socketAddress:
+      address: 0.0.0.0
+      portValue: 10080
+  filterChains:
+  - filters:
+    - name: envoy.filters.network.http_connection_manager
+      typedConfig:
+        '@type': type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
+        commonHttpProtocolOptions:
+          headersWithUnderscoresAction: REJECT_REQUEST
+        http2ProtocolOptions:
+          initialConnectionWindowSize: 1048576
+          initialStreamWindowSize: 65536
+          maxConcurrentStreams: 100
+        httpFilters:
+        - name: envoy.filters.http.ratelimit
+          typedConfig:
+            '@type': type.googleapis.com/envoy.extensions.filters.http.ratelimit.v3.RateLimit
+            domain: eg-ratelimit
+            failureModeDeny: true
+            rateLimitService:
+              grpcService:
+                envoyGrpc:
+                  clusterName: rate-limit-cluster
+              transportApiVersion: V3
+            timeout: 1s
+        - name: envoy.filters.http.router
+          typedConfig:
+            '@type': type.googleapis.com/envoy.extensions.filters.http.router.v3.Router
+        mergeSlashes: true
+        normalizePath: true
+        pathWithEscapedSlashesAction: UNESCAPE_AND_REDIRECT
+        rds:
+          configSource:
+            ads: {}
+            resourceApiVersion: V3
+          routeConfigName: first-listener
+        statPrefix: https
+        upgradeConfigs:
+        - upgradeType: websocket
+        useRemoteAddress: true
+    transportSocket:
+      name: envoy.transport_sockets.tls
+      typedConfig:
+        '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.DownstreamTlsContext
+        commonTlsContext:
+          alpnProtocols:
+          - h2
+          - http/1.1
+          tlsCertificateSdsSecretConfigs:
+          - name: secret-1
+            sdsConfig:
+              ads: {}
+              resourceApiVersion: V3
+          - name: secret-2
+            sdsConfig:
+              ads: {}
+              resourceApiVersion: V3
+  name: first-listener
+  perConnectionBufferLimitBytes: 32768
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.routes.yaml
@@ -0,0 +1,16 @@
+- ignorePortInHostMatching: true
+  name: first-listener
+  virtualHosts:
+  - domains:
+    - '*'
+    name: first-listener/*
+    routes:
+    - match:
+        headers:
+        - name: user
+          stringMatch:
+            exact: jason
+        prefix: /
+      name: first-route
+      route:
+        cluster: first-route-dest
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.clusters.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.clusters.yaml
@@ -0,0 +1,14 @@
+- commonLbConfig:
+    localityWeightedLbConfig: {}
+  connectTimeout: 10s
+  dnsLookupFamily: V4_ONLY
+  edsClusterConfig:
+    edsConfig:
+      ads: {}
+      resourceApiVersion: V3
+    serviceName: first-route-dest
+  lbPolicy: LEAST_REQUEST
+  name: first-route-dest
+  outlierDetection: {}
+  perConnectionBufferLimitBytes: 32768
+  type: EDS
diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.endpoints.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.endpoints.yaml
@@ -0,0 +1,12 @@
+- clusterName: first-route-dest
+  endpoints:
+  - lbEndpoints:
+    - endpoint:
+        address:
+          socketAddress:
+            address: 1.2.3.4
+            portValue: 50000
+      loadBalancingWeight: 1
+    loadBalancingWeight: 1
+    locality:
+      region: first-route-dest/backend/0
diff --git a/...l/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml b/...l/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml
@@ -0,0 +1,9 @@
+- name: first-policy
+  namespace: default
+  status:
+    conditions:
+    - lastTransitionTime: null
+      message: The value field can not be set for the remove operation
+      reason: Invalid
+      status: "False"
+      type: Programmed