feat: support JSONPatches for proxy bootstrap modifications (envoypro…

…xy#4116) * Modify the API to support expressing JSONPatches Signed-off-by: Lior Okman <lior.okman@sap.com> * Support JSONPatch in the bootstrap proxy flow Signed-off-by: Lior Okman <lior.okman@sap.com> * Accept JSONPatches in a YAML format for unit-tests to be consistent with other unit tests. Signed-off-by: Lior Okman <lior.okman@sap.com> * Reduce the runtime imports, move imports needed for tests into the test code. Signed-off-by: Lior Okman <lior.okman@sap.com> * Moved validation code that requires access to internal/xds out of the api/v1alpha1/validation package. Signed-off-by: Lior Okman <lior.okman@sap.com> * Cleanup and make the linter happy Signed-off-by: Lior Okman <lior.okman@sap.com> * Added another test for the JSONPatch code Signed-off-by: Lior Okman <lior.okman@sap.com> * Make the linter happy Signed-off-by: Lior Okman <lior.okman@sap.com> * More unit-tests. Signed-off-by: Lior Okman <lior.okman@sap.com> --------- Signed-off-by: Lior Okman <lior.okman@sap.com>
guydc · Sep 5, 2024 · 9bb1601 · 9bb1601
1 parent ccff748
commit 9bb1601
Show file tree

Hide file tree

Showing 32 changed files with 1,243 additions and 333 deletions.
diff --git a/api/v1alpha1/envoyproxy_types.go b/api/v1alpha1/envoyproxy_types.go
@@ -358,19 +358,27 @@ const (
 )
 
 // ProxyBootstrap defines Envoy Bootstrap configuration.
+// +union
+// +kubebuilder:validation:XValidation:rule="self.type == 'JSONPatch' ? self.jsonPatches.size() > 0 : has(self.value)", message="provided bootstrap patch doesn't match the configured patch type"
 type ProxyBootstrap struct {
-	// Type is the type of the bootstrap configuration, it should be either Replace or Merge.
+	// Type is the type of the bootstrap configuration, it should be either Replace,  Merge, or JSONPatch.
 	// If unspecified, it defaults to Replace.
 	// +optional
 	// +kubebuilder:default=Replace
+	// +unionDiscriminator
 	Type *BootstrapType `json:"type"`
 
 	// Value is a YAML string of the bootstrap.
-	Value string `json:"value"`
+	// +optional
+	Value *string `json:"value,omitempty"`
+
+	// JSONPatches is an array of JSONPatches to be applied to the default bootstrap. Patches are
+	// applied in the order in which they are defined.
+	JSONPatches []JSONPatchOperation `json:"jsonPatches,omitempty"`
 }
 
 // BootstrapType defines the types of bootstrap supported by Envoy Gateway.
-// +kubebuilder:validation:Enum=Merge;Replace
+// +kubebuilder:validation:Enum=Merge;Replace;JSONPatch
 type BootstrapType string
 
 const (
@@ -381,6 +389,9 @@ const (
 
 	// Replace replaces the default bootstrap with the provided one.
 	BootstrapTypeReplace BootstrapType = "Replace"
+
+	// JSONPatch applies the provided JSONPatches to the default bootstrap.
+	BootstrapTypeJSONPatch BootstrapType = "JSONPatch"
 )
 
 // EnvoyProxyStatus defines the observed state of EnvoyProxy. This type is not implemented

diff --git a/api/v1alpha1/validation/envoyproxy_validate.go b/api/v1alpha1/validation/envoyproxy_validate.go
@@ -12,16 +12,9 @@ import (
 	"net/netip"
 
 	"github.com/dominikbraun/graph"
-	bootstrapv3 "github.com/envoyproxy/go-control-plane/envoy/config/bootstrap/v3"
-	clusterv3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
-	"github.com/google/go-cmp/cmp"
-	"google.golang.org/protobuf/testing/protocmp"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
-	"github.com/envoyproxy/gateway/internal/utils/proto"
-	"github.com/envoyproxy/gateway/internal/xds/bootstrap"
-	_ "github.com/envoyproxy/gateway/internal/xds/extensions" // register the generated types to support protojson unmarshalling
 )
 
 // ValidateEnvoyProxy validates the provided EnvoyProxy.
@@ -38,6 +31,8 @@ func ValidateEnvoyProxy(proxy *egv1a1.EnvoyProxy) error {
 }
 
 // validateEnvoyProxySpec validates the provided EnvoyProxy spec.
+// This method validates everything except for the bootstrap section, because validating the bootstrap
+// section in this method would require calling into the internal apis, and would cause an import cycle.
 func validateEnvoyProxySpec(spec *egv1a1.EnvoyProxySpec) error {
 	var errs []error
 
@@ -51,13 +46,6 @@ func validateEnvoyProxySpec(spec *egv1a1.EnvoyProxySpec) error {
 		errs = append(errs, validateProviderErrs...)
 	}
 
-	// validate bootstrap
-	if spec != nil && spec.Bootstrap != nil {
-		if err := validateBootstrap(spec.Bootstrap); err != nil {
-			errs = append(errs, err)
-		}
-	}
-
 	validateProxyTelemetryErrs := validateProxyTelemetry(spec)
 	if len(validateProxyTelemetryErrs) != 0 {
 		errs = append(errs, validateProxyTelemetryErrs...)
@@ -156,62 +144,6 @@ func validateService(spec *egv1a1.EnvoyProxySpec) []error {
 	return errs
 }
 
-func validateBootstrap(boostrapConfig *egv1a1.ProxyBootstrap) error {
-	// Validate user bootstrap config
-	defaultBootstrap := &bootstrapv3.Bootstrap{}
-	// TODO: need validate when enable prometheus?
-	defaultBootstrapStr, err := bootstrap.GetRenderedBootstrapConfig(nil)
-	if err != nil {
-		return err
-	}
-	if err := proto.FromYAML([]byte(defaultBootstrapStr), defaultBootstrap); err != nil {
-		return fmt.Errorf("unable to unmarshal default bootstrap: %w", err)
-	}
-	if err := defaultBootstrap.Validate(); err != nil {
-		return fmt.Errorf("default bootstrap validation failed: %w", err)
-	}
-
-	// Validate user bootstrap config
-	userBootstrapStr, err := bootstrap.ApplyBootstrapConfig(boostrapConfig, defaultBootstrapStr)
-	if err != nil {
-		return err
-	}
-	userBootstrap := &bootstrapv3.Bootstrap{}
-	if err := proto.FromYAML([]byte(userBootstrapStr), userBootstrap); err != nil {
-		return fmt.Errorf("failed to parse default bootstrap config: %w", err)
-	}
-	if err := userBootstrap.Validate(); err != nil {
-		return fmt.Errorf("validation failed for user bootstrap: %w", err)
-	}
-
-	// Ensure dynamic resources config is same
-	if userBootstrap.DynamicResources == nil ||
-		cmp.Diff(userBootstrap.DynamicResources, defaultBootstrap.DynamicResources, protocmp.Transform()) != "" {
-		return fmt.Errorf("dynamic_resources cannot be modified")
-	}
-
-	// Ensure that the xds_cluster config is same
-	var userXdsCluster, defaultXdsCluster *clusterv3.Cluster
-	for _, cluster := range userBootstrap.StaticResources.Clusters {
-		if cluster.Name == "xds_cluster" {
-			userXdsCluster = cluster
-			break
-		}
-	}
-	for _, cluster := range defaultBootstrap.StaticResources.Clusters {
-		if cluster.Name == "xds_cluster" {
-			defaultXdsCluster = cluster
-			break
-		}
-	}
-	if userXdsCluster == nil ||
-		cmp.Diff(userXdsCluster.LoadAssignment, defaultXdsCluster.LoadAssignment, protocmp.Transform()) != "" {
-		return fmt.Errorf("xds_cluster's loadAssigntment cannot be modified")
-	}
-
-	return nil
-}
-
 func validateProxyTelemetry(spec *egv1a1.EnvoyProxySpec) []error {
 	var errs []error
 

diff --git a/api/v1alpha1/validation/envoyproxy_validate_test.go b/api/v1alpha1/validation/envoyproxy_validate_test.go
@@ -6,8 +6,6 @@
 package validation
 
 import (
-	// Register embed
-	_ "embed"
 	"reflect"
 	"testing"
 
@@ -21,19 +19,6 @@ import (
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
 )
 
-var (
-	//go:embed testdata/valid-user-bootstrap.yaml
-	validUserBootstrap string
-	//go:embed testdata/merge-user-bootstrap.yaml
-	mergeUserBootstrap string
-	//go:embed testdata/missing-admin-address-user-bootstrap.yaml
-	missingAdminAddressUserBootstrap string
-	//go:embed testdata/different-dynamic-resources-user-bootstrap.yaml
-	differentDynamicResourcesUserBootstrap string
-	//go:embed testdata/different-xds-cluster-address-bootstrap.yaml
-	differentXdsClusterAddressBootstrap string
-)
-
 func TestValidateEnvoyProxy(t *testing.T) {
 	testCases := []struct {
 		name     string
@@ -319,83 +304,6 @@ func TestValidateEnvoyProxy(t *testing.T) {
 			},
 			expected: false,
 		},
-
-		{
-			name: "valid user bootstrap replace type",
-			proxy: &egv1a1.EnvoyProxy{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "test",
-				},
-				Spec: egv1a1.EnvoyProxySpec{
-					Bootstrap: &egv1a1.ProxyBootstrap{
-						Value: validUserBootstrap,
-					},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "valid user bootstrap merge type",
-			proxy: &egv1a1.EnvoyProxy{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "test",
-				},
-				Spec: egv1a1.EnvoyProxySpec{
-					Bootstrap: &egv1a1.ProxyBootstrap{
-						Type:  ptr.To(egv1a1.BootstrapTypeMerge),
-						Value: mergeUserBootstrap,
-					},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "user bootstrap with missing admin address",
-			proxy: &egv1a1.EnvoyProxy{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "test",
-				},
-				Spec: egv1a1.EnvoyProxySpec{
-					Bootstrap: &egv1a1.ProxyBootstrap{
-						Value: missingAdminAddressUserBootstrap,
-					},
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "user bootstrap with different dynamic resources",
-			proxy: &egv1a1.EnvoyProxy{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "test",
-				},
-				Spec: egv1a1.EnvoyProxySpec{
-					Bootstrap: &egv1a1.ProxyBootstrap{
-						Value: differentDynamicResourcesUserBootstrap,
-					},
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "user bootstrap with different xds_cluster endpoint",
-			proxy: &egv1a1.EnvoyProxy{
-				ObjectMeta: metav1.ObjectMeta{
-					Namespace: "test",
-					Name:      "test",
-				},
-				Spec: egv1a1.EnvoyProxySpec{
-					Bootstrap: &egv1a1.ProxyBootstrap{
-						Value: differentXdsClusterAddressBootstrap,
-					},
-				},
-			},
-			expected: false,
-		},
 		{
 			name: "should invalid when accesslog enabled using Text format, but `text` field being empty",
 			proxy: &egv1a1.EnvoyProxy{

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_envoyproxies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoyproxies.yaml
@@ -191,21 +191,69 @@ spec:
                   We strongly recommend using `egctl x translate` to generate a `EnvoyProxy` resource with the `Bootstrap` field set to the default
                   Bootstrap configuration used. You can edit this configuration, and rerun `egctl x translate` to ensure there are no validation errors.
                 properties:
+                  jsonPatches:
+                    description: |-
+                      JSONPatches is an array of JSONPatches to be applied to the default bootstrap. Patches are
+                      applied in the order in which they are defined.
+                    items:
+                      description: |-
+                        JSONPatchOperation defines the JSON Patch Operation as defined in
+                        https://datatracker.ietf.org/doc/html/rfc6902
+                      properties:
+                        from:
+                          description: |-
+                            From is the source location of the value to be copied or moved. Only valid
+                            for move or copy operations
+                            Refer to https://datatracker.ietf.org/doc/html/rfc6901 for more details.
+                          type: string
+                        jsonPath:
+                          description: |-
+                            JSONPath specifies the locations of the target document/field where the operation will be performed
+                            Refer to https://datatracker.ietf.org/doc/rfc9535/ for more details.
+                          type: string
+                        op:
+                          description: Op is the type of operation to perform
+                          enum:
+                          - add
+                          - remove
+                          - replace
+                          - move
+                          - copy
+                          - test
+                          type: string
+                        path:
+                          description: |-
+                            Path is the location of the target document/field where the operation will be performed
+                            Refer to https://datatracker.ietf.org/doc/html/rfc6901 for more details.
+                          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
+                      type: object
+                    type: array
                   type:
                     default: Replace
                     description: |-
-                      Type is the type of the bootstrap configuration, it should be either Replace or Merge.
+                      Type is the type of the bootstrap configuration, it should be either Replace,  Merge, or JSONPatch.
                       If unspecified, it defaults to Replace.
                     enum:
                     - Merge
                     - Replace
+                    - JSONPatch
                     type: string
                   value:
                     description: Value is a YAML string of the bootstrap.
                     type: string
-                required:
-                - value
                 type: object
+                x-kubernetes-validations:
+                - message: provided bootstrap patch doesn't match the configured patch
+                    type
+                  rule: 'self.type == ''JSONPatch'' ? self.jsonPatches.size() > 0
+                    : has(self.value)'
               concurrency:
                 description: |-
                   Concurrency defines the number of worker threads to run. If unset, it defaults to

diff --git a/internal/cmd/egctl/translate.go b/internal/cmd/egctl/translate.go
@@ -326,6 +326,11 @@ func translateGatewayAPIToGatewayAPI(resources *gatewayapi.Resources) (gatewayap
 			msg := fmt.Sprintf("%s: %v", status.MsgGatewayClassInvalidParams, err)
 			status.SetGatewayClassAccepted(resources.GatewayClass, false, string(gwapiv1.GatewayClassReasonInvalidParameters), msg)
 		}
+		if err := bootstrap.Validate(resources.EnvoyProxyForGatewayClass.Spec.Bootstrap); err != nil {
+			epInvalid = true
+			msg := fmt.Sprintf("%s: %v", status.MsgGatewayClassInvalidParams, err)
+			status.SetGatewayClassAccepted(resources.GatewayClass, false, string(gwapiv1.GatewayClassReasonInvalidParameters), msg)
+		}
 		gRes.EnvoyProxyForGatewayClass = resources.EnvoyProxyForGatewayClass
 	}
 	if !epInvalid {
@@ -959,7 +964,7 @@ func addDefaultEnvoyProxy(resources *gatewayapi.Resources) error {
 		},
 		Spec: egv1a1.EnvoyProxySpec{
 			Bootstrap: &egv1a1.ProxyBootstrap{
-				Value: defaultBootstrapStr,
+				Value: &defaultBootstrapStr,
 			},
 		},
 	}

diff --git a/internal/gatewayapi/envoypatchpolicy.go b/internal/gatewayapi/envoypatchpolicy.go
@@ -112,7 +112,7 @@ func (t *Translator) ProcessEnvoyPatchPolicies(envoyPatchPolicies []*egv1a1.Envo
 			irPatch := ir.JSONPatchConfig{}
 			irPatch.Type = string(patch.Type)
 			irPatch.Name = patch.Name
-			irPatch.Operation.Op = string(patch.Operation.Op)
+			irPatch.Operation.Op = ir.JSONPatchOp(patch.Operation.Op)
 			irPatch.Operation.Path = patch.Operation.Path
 			irPatch.Operation.JSONPath = patch.Operation.JSONPath
 			irPatch.Operation.From = patch.Operation.From