From 87e9c397373165908c91e9694f9989d37699d8f4 Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Wed, 13 Mar 2024 15:49:41 +0800 Subject: [PATCH 1/3] add PolicyStatus for EPP Signed-off-by: shawnh2 --- api/v1alpha1/envoypatchpolicy_types.go | 15 +- api/v1alpha1/zz_generated.deepcopy.go | 22 -- ...eway.envoyproxy.io_envoypatchpolicies.yaml | 333 ++++++++++++++---- internal/cmd/egctl/status_test.go | 50 +-- internal/cmd/egctl/translate_test.go | 2 +- internal/gatewayapi/envoypatchpolicy.go | 113 ++++-- internal/gatewayapi/helpers.go | 1 + .../envoypatchpolicy-cross-ns-target.out.yaml | 21 +- ...chpolicy-invalid-feature-disabled.out.yaml | 18 +- ...nvalid-target-kind-merge-gateways.out.yaml | 22 +- ...oypatchpolicy-invalid-target-kind.out.yaml | 23 +- ...ypatchpolicy-valid-merge-gateways.out.yaml | 36 +- .../testdata/envoypatchpolicy-valid.out.yaml | 38 +- internal/ir/xds.go | 3 +- internal/ir/zz_generated.deepcopy.go | 3 +- internal/message/types.go | 2 +- internal/provider/kubernetes/controller.go | 2 +- internal/provider/kubernetes/status.go | 2 +- internal/status/envoypatchpolicy.go | 43 ++- internal/status/policy.go | 1 + internal/xds/translator/jsonpatch.go | 239 +++++++------ .../jsonpatch-add-op-without-value.yaml | 8 +- .../in/xds-ir/jsonpatch-invalid-patch.yaml | 8 +- .../in/xds-ir/jsonpatch-missing-resource.yaml | 8 +- .../xds-ir/jsonpatch-move-op-with-value.yaml | 8 +- .../testdata/in/xds-ir/jsonpatch.yaml | 8 +- ...d-op-without-value.envoypatchpolicies.yaml | 19 +- ...atch-invalid-patch.envoypatchpolicies.yaml | 21 +- ...h-missing-resource.envoypatchpolicies.yaml | 20 +- ...move-op-with-value.envoypatchpolicies.yaml | 19 +- .../xds-ir/jsonpatch.envoypatchpolicies.yaml | 19 +- internal/xds/translator/translator.go | 1 + internal/xds/translator/translator_test.go | 11 +- site/content/en/latest/api/extension_types.md | 2 - 34 files changed, 727 insertions(+), 414 deletions(-) diff --git a/api/v1alpha1/envoypatchpolicy_types.go b/api/v1alpha1/envoypatchpolicy_types.go index e646977286e..a6ae7b9db3b 100644 --- a/api/v1alpha1/envoypatchpolicy_types.go +++ b/api/v1alpha1/envoypatchpolicy_types.go @@ -32,7 +32,7 @@ type EnvoyPatchPolicy struct { Spec EnvoyPatchPolicySpec `json:"spec"` // Status defines the current status of EnvoyPatchPolicy. - Status EnvoyPatchPolicyStatus `json:"status,omitempty"` + Status gwapiv1a2.PolicyStatus `json:"status,omitempty"` } // EnvoyPatchPolicySpec defines the desired state of EnvoyPatchPolicy. @@ -123,17 +123,6 @@ type JSONPatchOperation struct { Value *apiextensionsv1.JSON `json:"value,omitempty"` } -// EnvoyPatchPolicyStatus defines the state of EnvoyPatchPolicy -type EnvoyPatchPolicyStatus struct { - // Conditions describe the current conditions of the EnvoyPatchPolicy. - // - // +optional - // +listType=map - // +listMapKey=type - // +kubebuilder:validation:MaxItems=8 - Conditions []metav1.Condition `json:"conditions,omitempty"` -} - const ( // PolicyConditionProgrammed indicates whether the policy has been translated // and ready to be programmed into the data plane. @@ -157,7 +146,7 @@ const ( // is syntactically or semantically invalid. PolicyReasonInvalid gwapiv1a2.PolicyConditionReason = "Invalid" - // PolicyReasonTargetNotFound is used with the "Programmed" condition when the + // PolicyReasonResourceNotFound is used with the "Programmed" condition when the // policy cannot find the resource type to patch to. PolicyReasonResourceNotFound gwapiv1a2.PolicyConditionReason = "ResourceNotFound" diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index 8d2300c7721..1bd56138b65 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -1137,28 +1137,6 @@ func (in *EnvoyPatchPolicySpec) DeepCopy() *EnvoyPatchPolicySpec { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *EnvoyPatchPolicyStatus) DeepCopyInto(out *EnvoyPatchPolicyStatus) { - *out = *in - if in.Conditions != nil { - in, out := &in.Conditions, &out.Conditions - *out = make([]v1.Condition, len(*in)) - for i := range *in { - (*in)[i].DeepCopyInto(&(*out)[i]) - } - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EnvoyPatchPolicyStatus. -func (in *EnvoyPatchPolicyStatus) DeepCopy() *EnvoyPatchPolicyStatus { - if in == nil { - return nil - } - out := new(EnvoyPatchPolicyStatus) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *EnvoyProxy) DeepCopyInto(out *EnvoyProxy) { *out = *in diff --git a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml index f157a84cd37..5b523698d3c 100644 --- a/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml +++ b/charts/gateway-helm/crds/generated/gateway.envoyproxy.io_envoypatchpolicies.yaml @@ -160,79 +160,280 @@ spec: status: description: Status defines the current status of EnvoyPatchPolicy. properties: - conditions: - description: Conditions describe the current conditions of the EnvoyPatchPolicy. + ancestors: + description: "Ancestors is a list of ancestor resources (usually Gateways) + that are associated with the policy, and the status of the policy + with respect to each ancestor. When this policy attaches to a parent, + the controller that manages the parent and the ancestors MUST add + an entry to this list when the controller first sees the policy + and SHOULD update the entry as appropriate when the relevant ancestor + is modified. \n Note that choosing the relevant ancestor is left + to the Policy designers; an important part of Policy design is designing + the right object level at which to namespace this status. \n Note + also that implementations MUST ONLY populate ancestor status for + the Ancestor resources they are responsible for. Implementations + MUST use the ControllerName field to uniquely identify the entries + in this list that they are responsible for. \n Note that to achieve + this, the list of PolicyAncestorStatus structs MUST be treated as + a map with a composite key, made up of the AncestorRef and ControllerName + fields combined. \n A maximum of 16 ancestors will be represented + in this list. An empty list means the Policy is not relevant for + any ancestors. \n If this slice is full, implementations MUST NOT + add further entries. Instead they MUST consider the policy unimplementable + and signal that on any related resources such as the ancestor that + would be referenced here. For example, if this list was full on + BackendTLSPolicy, no additional Gateways would be able to reference + the Service targeted by the BackendTLSPolicy." items: - description: "Condition contains details for one aspect of the current - state of this API Resource. --- This struct is intended for direct - use as an array at the field path .status.conditions. For example, - \n type FooStatus struct{ // Represents the observations of a - foo's current state. // Known .status.conditions.type are: \"Available\", - \"Progressing\", and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge - // +listType=map // +listMapKey=type Conditions []metav1.Condition - `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" - protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields }" + description: "PolicyAncestorStatus describes the status of a route + with respect to an associated Ancestor. \n Ancestors refer to + objects that are either the Target of a policy or above it in + terms of object hierarchy. For example, if a policy targets a + Service, the Policy's Ancestors are, in order, the Service, the + HTTPRoute, the Gateway, and the GatewayClass. Almost always, in + this hierarchy, the Gateway will be the most useful object to + place Policy status on, so we recommend that implementations SHOULD + use Gateway as the PolicyAncestorStatus object unless the designers + have a _very_ good reason otherwise. \n In the context of policy + attachment, the Ancestor is used to distinguish which resource + results in a distinct application of this policy. For example, + if a policy targets a Service, it may have a distinct result per + attached Gateway. \n Policies targeting the same resource may + have different effects depending on the ancestors of those resources. + For example, different Gateways targeting the same Service may + have different capabilities, especially if they have different + underlying implementations. \n For example, in BackendTLSPolicy, + the Policy attaches to a Service that is used as a backend in + a HTTPRoute that is itself attached to a Gateway. In this case, + the relevant object for status is the Gateway, and that is the + ancestor object referred to in this status. \n Note that a parent + is also an ancestor, so for objects where the parent is the relevant + object for status, this struct SHOULD still be used. \n This struct + is intended to be used in a slice that's effectively a map, with + a composite key made up of the AncestorRef and the ControllerName." properties: - lastTransitionTime: - description: lastTransitionTime is the last time the condition - transitioned from one status to another. This should be when - the underlying condition changed. If that is not known, then - using the time when the API field changed is acceptable. - format: date-time - type: string - message: - description: message is a human readable message indicating - details about the transition. This may be an empty string. - maxLength: 32768 - type: string - observedGeneration: - description: observedGeneration represents the .metadata.generation - that the condition was set based upon. For instance, if .metadata.generation - is currently 12, but the .status.conditions[x].observedGeneration - is 9, the condition is out of date with respect to the current - state of the instance. - format: int64 - minimum: 0 - type: integer - reason: - description: reason contains a programmatic identifier indicating - the reason for the condition's last transition. Producers - of specific condition types may define expected values and - meanings for this field, and whether the values are considered - a guaranteed API. The value should be a CamelCase string. - This field may not be empty. - maxLength: 1024 + ancestorRef: + description: AncestorRef corresponds with a ParentRef in the + spec that this PolicyAncestorStatus struct describes the status + of. + properties: + group: + default: gateway.networking.k8s.io + description: "Group is the group of the referent. When unspecified, + \"gateway.networking.k8s.io\" is inferred. To set the + core API group (such as for a \"Service\" kind referent), + Group must be explicitly set to \"\" (empty string). \n + Support: Core" + maxLength: 253 + pattern: ^$|^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + kind: + default: Gateway + description: "Kind is kind of the referent. \n There are + two kinds of parent resources with \"Core\" support: \n + * Gateway (Gateway conformance profile) * Service (Mesh + conformance profile, experimental, ClusterIP Services + only) \n Support for other resources is Implementation-Specific." + maxLength: 63 + minLength: 1 + pattern: ^[a-zA-Z]([-a-zA-Z0-9]*[a-zA-Z0-9])?$ + type: string + name: + description: "Name is the name of the referent. \n Support: + Core" + maxLength: 253 + minLength: 1 + type: string + namespace: + description: "Namespace is the namespace of the referent. + When unspecified, this refers to the local namespace of + the Route. \n Note that there are specific rules for ParentRefs + which cross namespace boundaries. Cross-namespace references + are only valid if they are explicitly allowed by something + in the namespace they are referring to. For example: Gateway + has the AllowedRoutes field, and ReferenceGrant provides + a generic way to enable any other kind of cross-namespace + reference. \n ParentRefs + from a Route to a Service in the same namespace are \"producer\" + routes, which apply default routing rules to inbound connections + from any namespace to the Service. \n ParentRefs from + a Route to a Service in a different namespace are \"consumer\" + routes, and these routing rules are only applied to outbound + connections originating from the same namespace as the + Route, for which the intended destination of the connections + are a Service targeted as a ParentRef of the Route. + \n Support: Core" + maxLength: 63 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?$ + type: string + port: + description: "Port is the network port this Route targets. + It can be interpreted differently based on the type of + parent resource. \n When the parent resource is a Gateway, + this targets all listeners listening on the specified + port that also support this kind of Route(and select this + Route). It's not recommended to set `Port` unless the + networking behaviors specified in a Route must apply to + a specific port as opposed to a listener(s) whose port(s) + may be changed. When both Port and SectionName are specified, + the name and port of the selected listener must match + both specified values. \n + When the parent resource is a Service, this targets a + specific port in the Service spec. When both Port (experimental) + and SectionName are specified, the name and port of the + selected port must match both specified values. + \n Implementations MAY choose to support other parent + resources. Implementations supporting other types of parent + resources MUST clearly document how/if Port is interpreted. + \n For the purpose of status, an attachment is considered + successful as long as the parent resource accepts it partially. + For example, Gateway listeners can restrict which Routes + can attach to them by Route kind, namespace, or hostname. + If 1 of 2 Gateway listeners accept attachment from the + referencing Route, the Route MUST be considered successfully + attached. If no Gateway listeners accept attachment from + this Route, the Route MUST be considered detached from + the Gateway. \n Support: Extended \n " + format: int32 + maximum: 65535 + minimum: 1 + type: integer + sectionName: + description: "SectionName is the name of a section within + the target resource. In the following resources, SectionName + is interpreted as the following: \n * Gateway: Listener + Name. When both Port (experimental) and SectionName are + specified, the name and port of the selected listener + must match both specified values. * Service: Port Name. + When both Port (experimental) and SectionName are specified, + the name and port of the selected listener must match + both specified values. Note that attaching Routes to Services + as Parents is part of experimental Mesh support and is + not supported for any other purpose. \n Implementations + MAY choose to support attaching Routes to other resources. + If that is the case, they MUST clearly document how SectionName + is interpreted. \n When unspecified (empty string), this + will reference the entire resource. For the purpose of + status, an attachment is considered successful if at least + one section in the parent resource accepts it. For example, + Gateway listeners can restrict which Routes can attach + to them by Route kind, namespace, or hostname. If 1 of + 2 Gateway listeners accept attachment from the referencing + Route, the Route MUST be considered successfully attached. + If no Gateway listeners accept attachment from this Route, + the Route MUST be considered detached from the Gateway. + \n Support: Core" + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$ + type: string + required: + - name + type: object + conditions: + description: Conditions describes the status of the Policy with + respect to the given Ancestor. + items: + description: "Condition contains details for one aspect of + the current state of this API Resource. --- This struct + is intended for direct use as an array at the field path + .status.conditions. For example, \n type FooStatus struct{ + // Represents the observations of a foo's current state. + // Known .status.conditions.type are: \"Available\", \"Progressing\", + and \"Degraded\" // +patchMergeKey=type // +patchStrategy=merge + // +listType=map // +listMapKey=type Conditions []metav1.Condition + `json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" + protobuf:\"bytes,1,rep,name=conditions\"` \n // other fields + }" + properties: + lastTransitionTime: + description: lastTransitionTime is the last time the condition + transitioned from one status to another. This should + be when the underlying condition changed. If that is + not known, then using the time when the API field changed + is acceptable. + format: date-time + type: string + message: + description: message is a human readable message indicating + details about the transition. This may be an empty string. + maxLength: 32768 + type: string + observedGeneration: + description: observedGeneration represents the .metadata.generation + that the condition was set based upon. For instance, + if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration + is 9, the condition is out of date with respect to the + current state of the instance. + format: int64 + minimum: 0 + type: integer + reason: + description: reason contains a programmatic identifier + indicating the reason for the condition's last transition. + Producers of specific condition types may define expected + values and meanings for this field, and whether the + values are considered a guaranteed API. The value should + be a CamelCase string. This field may not be empty. + maxLength: 1024 + minLength: 1 + pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ + type: string + status: + description: status of the condition, one of True, False, + Unknown. + enum: + - "True" + - "False" + - Unknown + type: string + type: + description: type of condition in CamelCase or in foo.example.com/CamelCase. + --- Many .condition.type values are consistent across + resources like Available, but because arbitrary conditions + can be useful (see .node.status.conditions), the ability + to deconflict is important. The regex it matches is + (dns1123SubdomainFmt/)?(qualifiedNameFmt) + maxLength: 316 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + type: string + required: + - lastTransitionTime + - message + - reason + - status + - type + type: object + maxItems: 8 + minItems: 1 + type: array + x-kubernetes-list-map-keys: + - type + x-kubernetes-list-type: map + controllerName: + description: "ControllerName is a domain/path string that indicates + the name of the controller that wrote this status. This corresponds + with the controllerName field on GatewayClass. \n Example: + \"example.net/gateway-controller\". \n The format of this + field is DOMAIN \"/\" PATH, where DOMAIN and PATH are valid + Kubernetes names (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names). + \n Controllers MUST populate this field when writing status. + Controllers should ensure that entries to status populated + with their ControllerName are cleaned up when they are no + longer necessary." + maxLength: 253 minLength: 1 - pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ - type: string - status: - description: status of the condition, one of True, False, Unknown. - enum: - - "True" - - "False" - - Unknown - type: string - type: - description: type of condition in CamelCase or in foo.example.com/CamelCase. - --- Many .condition.type values are consistent across resources - like Available, but because arbitrary conditions can be useful - (see .node.status.conditions), the ability to deconflict is - important. The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt) - maxLength: 316 - pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ + pattern: ^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$ type: string required: - - lastTransitionTime - - message - - reason - - status - - type + - ancestorRef + - controllerName type: object - maxItems: 8 + maxItems: 16 type: array - x-kubernetes-list-map-keys: - - type - x-kubernetes-list-type: map + required: + - ancestors type: object required: - spec diff --git a/internal/cmd/egctl/status_test.go b/internal/cmd/egctl/status_test.go index ad1fd80b8bc..9ffccabf893 100644 --- a/internal/cmd/egctl/status_test.go +++ b/internal/cmd/egctl/status_test.go @@ -15,8 +15,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" gwv1 "sigs.k8s.io/gateway-api/apis/v1" gwv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - - egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" ) func TestWriteStatus(t *testing.T) { @@ -518,53 +516,7 @@ btls foobar2 test-status-2 test reason 2 `, expect: true, }, - { - name: "egctl x status envoypatchpolicy with typed name", - resourceList: &egv1a1.EnvoyPatchPolicyList{ - Items: []egv1a1.EnvoyPatchPolicy{ - { - TypeMeta: metav1.TypeMeta{ - Kind: "EnvoyPatchPolicy", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "epp", - Namespace: "default", - }, - Status: egv1a1.EnvoyPatchPolicyStatus{ - Conditions: []metav1.Condition{ - { - Type: "foobar1", - Status: metav1.ConditionStatus("test-status-1"), - ObservedGeneration: 123456, - LastTransitionTime: metav1.NewTime(testTime), - Reason: "test reason 1", - Message: "test message 1", - }, - { - Type: "foobar2", - Status: metav1.ConditionStatus("test-status-2"), - ObservedGeneration: 123457, - LastTransitionTime: metav1.NewTime(testTime.Add(1 * time.Hour)), - Reason: "test reason 2", - Message: "test message 2", - }, - }, - }, - }, - }, - }, - resourceNamespaced: true, - resourceType: "envoypatchpolicy", - quiet: false, - verbose: false, - allNamespaces: false, - typedName: true, - outputs: `NAME TYPE STATUS REASON -envoypatchpolicy/epp foobar2 test-status-2 test reason 2 - foobar1 test-status-1 test reason 1 -`, - expect: true, - }, + // TODO(sh2): add a policy status test for egctl x status cmd } for _, tc := range testCases { diff --git a/internal/cmd/egctl/translate_test.go b/internal/cmd/egctl/translate_test.go index 001c8bd873b..6159b832b41 100644 --- a/internal/cmd/egctl/translate_test.go +++ b/internal/cmd/egctl/translate_test.go @@ -183,7 +183,7 @@ func TestTranslate(t *testing.T) { to: "xds", output: yamlOutput, resourceType: string(AllEnvoyConfigType), - expect: true, + expect: false, }, { name: "default-resources", diff --git a/internal/gatewayapi/envoypatchpolicy.go b/internal/gatewayapi/envoypatchpolicy.go index 1ec835e51a4..5be04d21972 100644 --- a/internal/gatewayapi/envoypatchpolicy.go +++ b/internal/gatewayapi/envoypatchpolicy.go @@ -9,9 +9,9 @@ import ( "fmt" "sort" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" - gwv1b1 "sigs.k8s.io/gateway-api/apis/v1" + gwv1 "sigs.k8s.io/gateway-api/apis/v1" gwv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" @@ -26,21 +26,43 @@ func (t *Translator) ProcessEnvoyPatchPolicies(envoyPatchPolicies []*egv1a1.Envo }) for _, policy := range envoyPatchPolicies { - policy := policy.DeepCopy() - targetNs := policy.Spec.TargetRef.Namespace - targetKind := KindGateway + var ( + policy = policy.DeepCopy() + ancestorRefs []gwv1a2.ParentReference + resolveErr *status.PolicyResolveError + targetKind string + irKey string + ) + targetNs := policy.Spec.TargetRef.Namespace // If empty, default to namespace of policy if targetNs == nil { - targetNs = ptr.To(gwv1b1.Namespace(policy.Namespace)) + targetNs = ptr.To(gwv1.Namespace(policy.Namespace)) } - // Get the IR - // It must exist since the gateways have already been processed - irKey := irStringKey(string(*targetNs), string(policy.Spec.TargetRef.Name)) if t.MergeGateways { - irKey = string(t.GatewayClassName) targetKind = KindGatewayClass + irKey = string(t.GatewayClassName) + + ancestorRefs = []gwv1a2.ParentReference{ + { + Group: GroupPtr(gwv1.GroupName), + Kind: KindPtr(targetKind), + Name: policy.Spec.TargetRef.Name, + }, + } + } else { + targetKind = KindGateway + gatewayNN := types.NamespacedName{ + Namespace: string(*targetNs), + Name: string(policy.Spec.TargetRef.Name), + } + // It must exist since the gateways have already been processed + irKey = irStringKey(gatewayNN.Namespace, gatewayNN.Name) + + ancestorRefs = []gwv1a2.ParentReference{ + getAncestorRefForPolicy(gatewayNN, nil), + } } gwXdsIR, ok := xdsIR[irKey] @@ -56,40 +78,58 @@ func (t *Translator) ProcessEnvoyPatchPolicies(envoyPatchPolicies []*egv1a1.Envo // Append the IR gwXdsIR.EnvoyPatchPolicies = append(gwXdsIR.EnvoyPatchPolicies, &policyIR) - if policy.Spec.TargetRef.Group != gwv1b1.GroupName || string(policy.Spec.TargetRef.Kind) != targetKind { - message := fmt.Sprintf("TargetRef.Group:%s TargetRef.Kind:%s, only TargetRef.Group:%s and TargetRef.Kind:%s is supported.", - policy.Spec.TargetRef.Group, policy.Spec.TargetRef.Kind, gwv1b1.GroupName, targetKind) - status.SetEnvoyPatchPolicyCondition(policy, - gwv1a2.PolicyConditionAccepted, - metav1.ConditionFalse, - gwv1a2.PolicyReasonInvalid, - message, + // Ensure EnvoyPatchPolicy is enabled + if !t.EnvoyPatchPolicyEnabled { + resolveErr = &status.PolicyResolveError{ + Reason: egv1a1.PolicyReasonDisabled, + Message: "EnvoyPatchPolicy is disabled in the EnvoyGateway configuration", + } + status.SetResolveErrorForPolicyAncestors(&policy.Status, + ancestorRefs, + t.GatewayControllerName, + policy.Generation, + resolveErr, + ) + + continue + } + + // Ensure EnvoyPatchPolicy is targeting to a support type + if policy.Spec.TargetRef.Group != gwv1.GroupName || string(policy.Spec.TargetRef.Kind) != targetKind { + message := fmt.Sprintf("TargetRef.Group:%s TargetRef.Kind:%s, only TargetRef.Group:%s and TargetRef.Kind:%s is supported.", + policy.Spec.TargetRef.Group, policy.Spec.TargetRef.Kind, gwv1.GroupName, targetKind) + + resolveErr = &status.PolicyResolveError{ + Reason: gwv1a2.PolicyReasonInvalid, + Message: message, + } + status.SetResolveErrorForPolicyAncestors(&policy.Status, + ancestorRefs, + t.GatewayControllerName, + policy.Generation, + resolveErr, ) + continue } - // Ensure Policy and target Gateway are in the same namespace + // Ensure EnvoyPatchPolicy and target Gateway are in the same namespace if policy.Namespace != string(*targetNs) { message := fmt.Sprintf("Namespace:%s TargetRef.Namespace:%s, EnvoyPatchPolicy can only target a %s in the same namespace.", policy.Namespace, *targetNs, targetKind) - status.SetEnvoyPatchPolicyCondition(policy, - gwv1a2.PolicyConditionAccepted, - metav1.ConditionFalse, - gwv1a2.PolicyReasonInvalid, - message, + resolveErr = &status.PolicyResolveError{ + Reason: gwv1a2.PolicyReasonInvalid, + Message: message, + } + status.SetResolveErrorForPolicyAncestors(&policy.Status, + ancestorRefs, + t.GatewayControllerName, + policy.Generation, + resolveErr, ) - continue - } - if !t.EnvoyPatchPolicyEnabled { - status.SetEnvoyPatchPolicyCondition(policy, - gwv1a2.PolicyConditionAccepted, - metav1.ConditionFalse, - egv1a1.PolicyReasonDisabled, - "EnvoyPatchPolicy is disabled in the EnvoyGateway configuration", - ) continue } @@ -107,11 +147,6 @@ func (t *Translator) ProcessEnvoyPatchPolicies(envoyPatchPolicies []*egv1a1.Envo } // Set Accepted=True - status.SetEnvoyPatchPolicyCondition(policy, - gwv1a2.PolicyConditionAccepted, - metav1.ConditionTrue, - gwv1a2.PolicyReasonAccepted, - "EnvoyPatchPolicy has been accepted.", - ) + status.SetAcceptedForPolicyAncestors(&policy.Status, ancestorRefs, t.GatewayControllerName) } } diff --git a/internal/gatewayapi/helpers.go b/internal/gatewayapi/helpers.go index 9fb98f591c7..6503d4f2f41 100644 --- a/internal/gatewayapi/helpers.go +++ b/internal/gatewayapi/helpers.go @@ -411,6 +411,7 @@ func protocolSliceToStringSlice(protocols []gwapiv1.ProtocolType) []string { return protocolStrings } +// getAncestorRefForPolicy returns Gateway as an ancestor reference for policy. func getAncestorRefForPolicy(gatewayNN types.NamespacedName, sectionName *v1alpha2.SectionName) v1alpha2.ParentReference { return v1alpha2.ParentReference{ Group: GroupPtr(gwapiv1.GroupName), diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-cross-ns-target.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-cross-ns-target.out.yaml index 1abf6f7b392..4d003c79e01 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-cross-ns-target.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-cross-ns-target.out.yaml @@ -64,13 +64,20 @@ xdsIR: - name: edit-conn-buffer-bytes namespace: envoy-gateway-2 status: - conditions: - - lastTransitionTime: null - message: Namespace:envoy-gateway-2 TargetRef.Namespace:envoy-gateway, EnvoyPatchPolicy - can only target a Gateway in the same namespace. - reason: Invalid - status: "False" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: Namespace:envoy-gateway-2 TargetRef.Namespace:envoy-gateway, + EnvoyPatchPolicy can only target a Gateway in the same namespace. + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-feature-disabled.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-feature-disabled.out.yaml index 711b21017b2..eee41351ea5 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-feature-disabled.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-feature-disabled.out.yaml @@ -74,12 +74,18 @@ xdsIR: - name: edit-conn-buffer-bytes namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: EnvoyPatchPolicy is disabled in the EnvoyGateway configuration - reason: Disabled - status: "False" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: EnvoyPatchPolicy is disabled in the EnvoyGateway configuration + reason: Disabled + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind-merge-gateways.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind-merge-gateways.out.yaml index bccd6afd479..740bfb7800f 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind-merge-gateways.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind-merge-gateways.out.yaml @@ -74,14 +74,20 @@ xdsIR: - name: edit-conn-buffer-bytes namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: TargetRef.Group:gateway.networking.k8s.io TargetRef.Kind:Gateway, - only TargetRef.Group:gateway.networking.k8s.io and TargetRef.Kind:GatewayClass - is supported. - reason: Invalid - status: "False" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: gateway-1 + conditions: + - lastTransitionTime: null + message: TargetRef.Group:gateway.networking.k8s.io TargetRef.Kind:Gateway, + only TargetRef.Group:gateway.networking.k8s.io and TargetRef.Kind:GatewayClass + is supported. + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind.out.yaml index 9db1977241c..5375d43d51a 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-invalid-target-kind.out.yaml @@ -64,14 +64,21 @@ xdsIR: - name: edit-conn-buffer-bytes namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: TargetRef.Group:gateway.networking.k8s.io TargetRef.Kind:MyGateway, - only TargetRef.Group:gateway.networking.k8s.io and TargetRef.Kind:Gateway - is supported. - reason: Invalid - status: "False" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: TargetRef.Group:gateway.networking.k8s.io TargetRef.Kind:MyGateway, + only TargetRef.Group:gateway.networking.k8s.io and TargetRef.Kind:Gateway + is supported. + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-valid-merge-gateways.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-valid-merge-gateways.out.yaml index 714ecafe2ed..69a919f4ccb 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-valid-merge-gateways.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-valid-merge-gateways.out.yaml @@ -81,12 +81,18 @@ xdsIR: name: edit-ignore-global-limit namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: EnvoyPatchPolicy has been accepted. - reason: Accepted - status: "True" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller - jsonPatches: - name: envoy-gateway-gateway-1-http operation: @@ -97,12 +103,18 @@ xdsIR: name: edit-conn-buffer-bytes namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: EnvoyPatchPolicy has been accepted. - reason: Accepted - status: "True" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: GatewayClass + name: envoy-gateway-class + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/gatewayapi/testdata/envoypatchpolicy-valid.out.yaml b/internal/gatewayapi/testdata/envoypatchpolicy-valid.out.yaml index 0b958297b20..ccba87e5c50 100644 --- a/internal/gatewayapi/testdata/envoypatchpolicy-valid.out.yaml +++ b/internal/gatewayapi/testdata/envoypatchpolicy-valid.out.yaml @@ -71,12 +71,19 @@ xdsIR: name: edit-ignore-global-limit namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: EnvoyPatchPolicy has been accepted. - reason: Accepted - status: "True" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller - jsonPatches: - name: envoy-gateway-gateway-1-http operation: @@ -87,12 +94,19 @@ xdsIR: name: edit-conn-buffer-bytes namespace: envoy-gateway status: - conditions: - - lastTransitionTime: null - message: EnvoyPatchPolicy has been accepted. - reason: Accepted - status: "True" - type: Accepted + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller http: - address: 0.0.0.0 hostnames: diff --git a/internal/ir/xds.go b/internal/ir/xds.go index 7d1af7c0602..0d7c5df03b2 100644 --- a/internal/ir/xds.go +++ b/internal/ir/xds.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation" + gwv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2" "sigs.k8s.io/yaml" egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" @@ -1283,7 +1284,7 @@ type EnvoyPatchPolicyStatus struct { Name string `json:"name,omitempty" yaml:"name"` Namespace string `json:"namespace,omitempty" yaml:"namespace"` // Status of the EnvoyPatchPolicy - Status *egv1a1.EnvoyPatchPolicyStatus `json:"status,omitempty" yaml:"status,omitempty"` + Status *gwv1a2.PolicyStatus `json:"status,omitempty" yaml:"status,omitempty"` } // JSONPatchConfig defines the configuration for patching a Envoy xDS Resource diff --git a/internal/ir/zz_generated.deepcopy.go b/internal/ir/zz_generated.deepcopy.go index 3e19c4b4602..ec9a1ca2bad 100644 --- a/internal/ir/zz_generated.deepcopy.go +++ b/internal/ir/zz_generated.deepcopy.go @@ -13,6 +13,7 @@ import ( "github.com/envoyproxy/gateway/api/v1alpha1" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/gateway-api/apis/v1alpha2" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. @@ -427,7 +428,7 @@ func (in *EnvoyPatchPolicyStatus) DeepCopyInto(out *EnvoyPatchPolicyStatus) { *out = *in if in.Status != nil { in, out := &in.Status, &out.Status - *out = new(v1alpha1.EnvoyPatchPolicyStatus) + *out = new(v1alpha2.PolicyStatus) (*in).DeepCopyInto(*out) } } diff --git a/internal/message/types.go b/internal/message/types.go index 0a936a87c5e..ab83a022150 100644 --- a/internal/message/types.go +++ b/internal/message/types.go @@ -92,7 +92,7 @@ func (s *GatewayAPIStatuses) Close() { type PolicyStatuses struct { ClientTrafficPolicyStatuses watchable.Map[types.NamespacedName, *gwapiv1a2.PolicyStatus] BackendTrafficPolicyStatuses watchable.Map[types.NamespacedName, *gwapiv1a2.PolicyStatus] - EnvoyPatchPolicyStatuses watchable.Map[types.NamespacedName, *egv1a1.EnvoyPatchPolicyStatus] + EnvoyPatchPolicyStatuses watchable.Map[types.NamespacedName, *gwapiv1a2.PolicyStatus] SecurityPolicyStatuses watchable.Map[types.NamespacedName, *egv1a1.SecurityPolicyStatus] BackendTLSPolicyStatuses watchable.Map[types.NamespacedName, *gwapiv1a2.PolicyStatus] } diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go index 9dc75b38100..37f7c631f1f 100644 --- a/internal/provider/kubernetes/controller.go +++ b/internal/provider/kubernetes/controller.go @@ -785,7 +785,7 @@ func (r *gatewayAPIReconciler) processEnvoyPatchPolicies(ctx context.Context, re policy := policy // Discard Status to reduce memory consumption in watchable // It will be recomputed by the gateway-api layer - policy.Status = egv1a1.EnvoyPatchPolicyStatus{} + policy.Status = gwapiv1a2.PolicyStatus{} resourceTree.EnvoyPatchPolicies = append(resourceTree.EnvoyPatchPolicies, &policy) } diff --git a/internal/provider/kubernetes/status.go b/internal/provider/kubernetes/status.go index e15d665c43b..5a6877810c8 100644 --- a/internal/provider/kubernetes/status.go +++ b/internal/provider/kubernetes/status.go @@ -212,7 +212,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context) { message.HandleSubscription( message.Metadata{Runner: string(v1alpha1.LogComponentProviderRunner), Message: "envoypatchpolicy-status"}, r.resources.EnvoyPatchPolicyStatuses.Subscribe(ctx), - func(update message.Update[types.NamespacedName, *v1alpha1.EnvoyPatchPolicyStatus], errChan chan error) { + func(update message.Update[types.NamespacedName, *gwapiv1a2.PolicyStatus], errChan chan error) { // skip delete updates. if update.Delete { return diff --git a/internal/status/envoypatchpolicy.go b/internal/status/envoypatchpolicy.go index 05deb89d154..8a8d1b601e4 100644 --- a/internal/status/envoypatchpolicy.go +++ b/internal/status/envoypatchpolicy.go @@ -6,6 +6,7 @@ package status import ( + "strings" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -14,32 +15,38 @@ import ( egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" ) -func SetEnvoyPatchPolicyCondition(e *egv1a1.EnvoyPatchPolicy, conditionType gwv1a2.PolicyConditionType, status metav1.ConditionStatus, reason gwv1a2.PolicyConditionReason, message string) { - cond := newCondition(string(conditionType), status, string(reason), message, time.Now(), e.Generation) - e.Status.Conditions = MergeConditions(e.Status.Conditions, cond) -} - -func SetEnvoyPatchPolicyProgrammedIfUnset(s *egv1a1.EnvoyPatchPolicyStatus, message string) { +// SetProgrammedForEnvoyPatchPolicy sets programmed conditions for each ancestor reference in policy status if it is unset. +func SetProgrammedForEnvoyPatchPolicy(s *gwv1a2.PolicyStatus) { // Return early if Programmed condition is already set - for _, c := range s.Conditions { - if c.Type == string(egv1a1.PolicyConditionProgrammed) { - return - } - if c.Type == string(gwv1a2.PolicyConditionAccepted) && c.Status == metav1.ConditionFalse { - return + for _, ancestor := range s.Ancestors { + for _, c := range ancestor.Conditions { + if c.Type == string(egv1a1.PolicyConditionProgrammed) { + return + } + if c.Type == string(gwv1a2.PolicyConditionAccepted) && c.Status == metav1.ConditionFalse { + return + } } } + message := "Patches have been successfully applied." cond := newCondition(string(egv1a1.PolicyConditionProgrammed), metav1.ConditionTrue, string(egv1a1.PolicyReasonProgrammed), message, time.Now(), 0) - s.Conditions = MergeConditions(s.Conditions, cond) + for i := range s.Ancestors { + s.Ancestors[i].Conditions = MergeConditions(s.Ancestors[i].Conditions, cond) + } } -func SetEnvoyPatchPolicyInvalid(s *egv1a1.EnvoyPatchPolicyStatus, message string) { - cond := newCondition(string(egv1a1.PolicyConditionProgrammed), metav1.ConditionFalse, string(egv1a1.PolicyReasonInvalid), message, time.Now(), 0) - s.Conditions = MergeConditions(s.Conditions, cond) +func SetTranslationErrorForEnvoyPatchPolicy(s *gwv1a2.PolicyStatus, errMsg string) { + cond := newCondition(string(egv1a1.PolicyConditionProgrammed), metav1.ConditionFalse, string(egv1a1.PolicyReasonInvalid), errMsg, time.Now(), 0) + for i := range s.Ancestors { + s.Ancestors[i].Conditions = MergeConditions(s.Ancestors[i].Conditions, cond) + } } -func SetEnvoyPatchPolicyResourceNotFound(s *egv1a1.EnvoyPatchPolicyStatus, message string) { +func SetResourceNotFoundErrorForEnvoyPatchPolicy(s *gwv1a2.PolicyStatus, notFoundResources []string) { + message := "Unable to find xds resources: " + strings.Join(notFoundResources, ",") cond := newCondition(string(egv1a1.PolicyConditionProgrammed), metav1.ConditionFalse, string(egv1a1.PolicyReasonResourceNotFound), message, time.Now(), 0) - s.Conditions = MergeConditions(s.Conditions, cond) + for i := range s.Ancestors { + s.Ancestors[i].Conditions = MergeConditions(s.Ancestors[i].Conditions, cond) + } } diff --git a/internal/status/policy.go b/internal/status/policy.go index d3d9afc5954..e60b52d40db 100644 --- a/internal/status/policy.go +++ b/internal/status/policy.go @@ -34,6 +34,7 @@ func SetTranslationErrorForPolicyAncestors(policyStatus *gwv1a2.PolicyStatus, an } } +// SetAcceptedForPolicyAncestors sets accepted conditions for each ancestor reference if it is unset. func SetAcceptedForPolicyAncestors(policyStatus *gwv1a2.PolicyStatus, ancestorRefs []gwv1a2.ParentReference, controllerName string) { for _, ancestorRef := range ancestorRefs { setAcceptedForPolicyAncestor(policyStatus, ancestorRef, controllerName) diff --git a/internal/xds/translator/jsonpatch.go b/internal/xds/translator/jsonpatch.go index 62638e78c69..113b252fa5a 100644 --- a/internal/xds/translator/jsonpatch.go +++ b/internal/xds/translator/jsonpatch.go @@ -35,6 +35,15 @@ const ( EmptyPath = "" ) +type typedName struct { + Type string + Name string +} + +func (t typedName) String() string { + return fmt.Sprintf("%s/%s", t.Type, t.Name) +} + // processJSONPatches applies each JSONPatch to the Xds Resources for a specific type. func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []*ir.EnvoyPatchPolicy) error { var errs error @@ -43,7 +52,12 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* } for _, e := range envoyPatchPolicies { - e := e + var ( + e = e + tErrs error + notFoundResources []string + ) + for _, p := range e.JSONPatches { var ( listener *listenerv3.Listener @@ -58,14 +72,14 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* 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) + tErr := fmt.Errorf("the %s operation requires a value", p.Operation.Op) + tErrs = errors.Join(tErrs, tErr) 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) + tErr := fmt.Errorf("the value field can not be set for the %s operation", p.Operation.Op) + tErrs = errors.Join(tErrs, tErr) continue } } @@ -77,78 +91,80 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* // The patch library expects an array so convert it into one y, err := yaml.Marshal(p.Operation.Value) if err != nil { - msg := fmt.Sprintf("unable to marshal patch %+v, err: %s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to marshal patch %+v, err: %s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } jsonBytes, err := yaml.YAMLToJSON(y) if err != nil { - msg := fmt.Sprintf("unable to convert patch to json %s, err: %s", string(y), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to convert patch to json %s, err: %s", string(y), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } + switch p.Type { - case string(resourcev3.ListenerType): + case resourcev3.ListenerType: temp := &listenerv3.Listener{} if err = protojson.Unmarshal(jsonBytes, temp); err != nil { - msg := unmarshalErrorMessage(err, p.Operation.Value) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, p.Operation.Value)) + tErrs = errors.Join(tErrs, tErr) continue } if err = tCtx.AddXdsResource(resourcev3.ListenerType, temp); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.RouteType): + case resourcev3.RouteType: temp := &routev3.RouteConfiguration{} if err = protojson.Unmarshal(jsonBytes, temp); err != nil { - msg := unmarshalErrorMessage(err, p.Operation.Value) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, p.Operation.Value)) + tErrs = errors.Join(tErrs, tErr) continue } if err = tCtx.AddXdsResource(resourcev3.RouteType, temp); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.ClusterType): + case resourcev3.ClusterType: temp := &clusterv3.Cluster{} if err = protojson.Unmarshal(jsonBytes, temp); err != nil { - msg := unmarshalErrorMessage(err, p.Operation.Value) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, p.Operation.Value)) + tErrs = errors.Join(tErrs, tErr) continue } if err = tCtx.AddXdsResource(resourcev3.ClusterType, temp); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.EndpointType): + case resourcev3.EndpointType: temp := &endpointv3.ClusterLoadAssignment{} if err = protojson.Unmarshal(jsonBytes, temp); err != nil { - msg := unmarshalErrorMessage(err, p.Operation.Value) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, p.Operation.Value)) + tErrs = errors.Join(tErrs, tErr) continue } if err = tCtx.AddXdsResource(resourcev3.EndpointType, temp); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.SecretType): + + case resourcev3.SecretType: temp := &tlsv3.Secret{} if err = protojson.Unmarshal(jsonBytes, temp); err != nil { - msg := unmarshalErrorMessage(err, p.Operation.Value) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, p.Operation.Value)) + tErrs = errors.Join(tErrs, tErr) continue } if err = tCtx.AddXdsResource(resourcev3.SecretType, temp); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %+v, err:%s", p.Operation.Value, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } @@ -157,66 +173,69 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* // Skip further processing continue } + // Find the resource to patch and convert it to JSON switch p.Type { - case string(resourcev3.ListenerType): + case resourcev3.ListenerType: if listener = findXdsListener(tCtx, p.Name); listener == nil { - msg := fmt.Sprintf("unable to find xds resource %s: %s", p.Type, p.Name) - status.SetEnvoyPatchPolicyResourceNotFound(e.Status, msg) + tn := typedName{p.Type, p.Name} + notFoundResources = append(notFoundResources, tn.String()) continue } if resourceJSON, err = m.Marshal(listener); err != nil { - err := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.RouteType): + case resourcev3.RouteType: if routeConfig = findXdsRouteConfig(tCtx, p.Name); routeConfig == nil { - msg := fmt.Sprintf("unable to find xds resource %s: %s", p.Type, p.Name) - status.SetEnvoyPatchPolicyResourceNotFound(e.Status, msg) + tn := typedName{p.Type, p.Name} + notFoundResources = append(notFoundResources, tn.String()) continue } if resourceJSON, err = m.Marshal(routeConfig); err != nil { - err = fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.ClusterType): + case resourcev3.ClusterType: if cluster = findXdsCluster(tCtx, p.Name); cluster == nil { - msg := fmt.Sprintf("unable to find xds resource %s: %s", p.Type, p.Name) - status.SetEnvoyPatchPolicyResourceNotFound(e.Status, msg) + tn := typedName{p.Type, p.Name} + notFoundResources = append(notFoundResources, tn.String()) continue } if resourceJSON, err = m.Marshal(cluster); err != nil { - err = fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.EndpointType): + + case resourcev3.EndpointType: if endpoint = findXdsEndpoint(tCtx, p.Name); endpoint == nil { - msg := fmt.Sprintf("unable to find xds resource %s: %s", p.Type, p.Name) - status.SetEnvoyPatchPolicyResourceNotFound(e.Status, msg) + tn := typedName{p.Type, p.Name} + notFoundResources = append(notFoundResources, tn.String()) continue } if resourceJSON, err = m.Marshal(endpoint); err != nil { - err = fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.SecretType): + + case resourcev3.SecretType: if secret = findXdsSecret(tCtx, p.Name); secret == nil { - msg := fmt.Sprintf("unable to find xds resource %s: %s", p.Type, p.Name) - status.SetEnvoyPatchPolicyResourceNotFound(e.Status, msg) + tn := typedName{p.Type, p.Name} + notFoundResources = append(notFoundResources, tn.String()) continue } if resourceJSON, err = m.Marshal(secret); err != nil { - err = fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to marshal xds resource %s: %s, err: %w", p.Type, p.Name, err) + tErrs = errors.Join(tErrs, tErr) continue } } @@ -225,21 +244,20 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* // The patch library expects an array so convert it into one y, err := yaml.Marshal([]ir.JSONPatchOperation{p.Operation}) if err != nil { - msg := fmt.Sprintf("unable to marshal patch %+v, err: %s", p.Operation, err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to marshal patch %+v, err: %s", p.Operation, err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } jsonBytes, err := yaml.YAMLToJSON(y) if err != nil { - msg := fmt.Sprintf("unable to convert patch to json %s, err: %s", string(y), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to convert patch to json %s, err: %s", string(y), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } - patchObj, err := jsonpatchv5.DecodePatch(jsonBytes) if err != nil { - msg := fmt.Sprintf("unable to decode patch %s, err: %s", string(jsonBytes), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to decode patch %s, err: %s", string(jsonBytes), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } @@ -248,8 +266,8 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* opts.EnsurePathExistsOnAdd = true modifiedJSON, err := patchObj.ApplyWithOptions(resourceJSON, opts) if err != nil { - msg := fmt.Sprintf("unable to apply patch:\n%s on resource:\n%s, err: %s", string(jsonBytes), string(resourceJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("unable to apply patch:\n%s on resource:\n%s, err: %s", string(jsonBytes), string(resourceJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } @@ -257,101 +275,112 @@ func processJSONPatches(tCtx *types.ResourceVersionTable, envoyPatchPolicies []* // Use a temp staging variable that can be marshalled // into and validated before saving it into the xds output resource switch p.Type { - case string(resourcev3.ListenerType): + case resourcev3.ListenerType: temp := &listenerv3.Listener{} if err = protojson.Unmarshal(modifiedJSON, temp); err != nil { - msg := unmarshalErrorMessage(err, string(modifiedJSON)) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, string(modifiedJSON))) + tErrs = errors.Join(tErrs, tErr) continue } if err = temp.Validate(); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } if err = deepCopyPtr(temp, listener); err != nil { - err := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.RouteType): + case resourcev3.RouteType: temp := &routev3.RouteConfiguration{} if err = protojson.Unmarshal(modifiedJSON, temp); err != nil { - msg := unmarshalErrorMessage(err, string(modifiedJSON)) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, string(modifiedJSON))) + tErrs = errors.Join(tErrs, tErr) continue } if err = temp.Validate(); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } if err = deepCopyPtr(temp, routeConfig); err != nil { - err := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.ClusterType): + case resourcev3.ClusterType: temp := &clusterv3.Cluster{} if err = protojson.Unmarshal(modifiedJSON, temp); err != nil { - msg := unmarshalErrorMessage(err, string(modifiedJSON)) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, string(modifiedJSON))) + tErrs = errors.Join(tErrs, tErr) continue } if err = temp.Validate(); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } if err = deepCopyPtr(temp, cluster); err != nil { - err := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.EndpointType): + case resourcev3.EndpointType: temp := &endpointv3.ClusterLoadAssignment{} if err = protojson.Unmarshal(modifiedJSON, temp); err != nil { - msg := unmarshalErrorMessage(err, string(modifiedJSON)) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, string(modifiedJSON))) + tErrs = errors.Join(tErrs, tErr) continue } if err = temp.Validate(); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } if err = deepCopyPtr(temp, endpoint); err != nil { - err := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) + tErrs = errors.Join(tErrs, tErr) continue } - case string(resourcev3.SecretType): + case resourcev3.SecretType: temp := &tlsv3.Secret{} if err = protojson.Unmarshal(modifiedJSON, temp); err != nil { - msg := unmarshalErrorMessage(err, string(modifiedJSON)) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf(unmarshalErrorMessage(err, string(modifiedJSON))) + tErrs = errors.Join(tErrs, tErr) continue } if err = temp.Validate(); err != nil { - msg := fmt.Sprintf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) - status.SetEnvoyPatchPolicyInvalid(e.Status, msg) + tErr := fmt.Errorf("validation failed for xds resource %s, err:%s", string(modifiedJSON), err.Error()) + tErrs = errors.Join(tErrs, tErr) continue } if err = deepCopyPtr(temp, secret); err != nil { - err := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) - errs = errors.Join(errs, err) + tErr := fmt.Errorf("unable to copy xds resource %s, err: %w", string(modifiedJSON), err) + tErrs = errors.Join(tErrs, tErr) continue } } } + // Set translation errors for every policy ancestor references + if tErrs != nil { + status.SetTranslationErrorForEnvoyPatchPolicy(e.Status, status.Error2ConditionMsg(tErrs)) + errs = errors.Join(errs, tErrs) + } + + // Set resources not found errors for every policy ancestor references + if len(notFoundResources) > 0 { + status.SetResourceNotFoundErrorForEnvoyPatchPolicy(e.Status, notFoundResources) + } + // Set Programmed condition if not yet set - status.SetEnvoyPatchPolicyProgrammedIfUnset(e.Status, "successfully applied patches.") + status.SetProgrammedForEnvoyPatchPolicy(e.Status) // Set output context - tCtx.EnvoyPatchPolicyStatuses = append(tCtx.EnvoyPatchPolicyStatuses, &e.EnvoyPatchPolicyStatus) } + return errs } 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 index 610195b2938..b4659755214 100644 --- 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 @@ -1,5 +1,11 @@ envoyPatchPolicies: -- status: {} +- status: + ancestors: + - ancestorRef: + group: "gateway.networking.k8s.io" + kind: "Gateway" + namespace: "default" + name: "foobar" name: "first-policy" namespace: "default" jsonPatches: diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-invalid-patch.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-invalid-patch.yaml index 661bc3897fc..551bdd6dda6 100644 --- a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-invalid-patch.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-invalid-patch.yaml @@ -1,5 +1,11 @@ envoyPatchPolicies: -- status: {} +- status: + ancestors: + - ancestorRef: + group: "gateway.networking.k8s.io" + kind: "Gateway" + namespace: "default" + name: "foobar" name: "first-policy" namespace: "default" jsonPatches: diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml index c285e244167..d795390537f 100644 --- a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml @@ -1,5 +1,11 @@ envoyPatchPolicies: -- status: {} +- status: + ancestors: + - ancestorRef: + group: "gateway.networking.k8s.io" + kind: "Gateway" + namespace: "default" + name: "foobar" name: "first-policy" namespace: "default" jsonPatches: 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 index 0f7612b3408..d66eaa633db 100644 --- 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 @@ -1,5 +1,11 @@ envoyPatchPolicies: -- status: {} +- status: + ancestors: + - ancestorRef: + group: "gateway.networking.k8s.io" + kind: "Gateway" + namespace: "default" + name: "foobar" name: "first-policy" namespace: "default" jsonPatches: diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch.yaml index c05e7e77c5d..e86dd9b8aeb 100644 --- a/internal/xds/translator/testdata/in/xds-ir/jsonpatch.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/jsonpatch.yaml @@ -1,5 +1,11 @@ envoyPatchPolicies: -- status: {} +- status: + ancestors: + - ancestorRef: + group: "gateway.networking.k8s.io" + kind: "Gateway" + namespace: "default" + name: "foobar" name: "first-policy" namespace: "default" jsonPatches: diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml index 55cdd5c4cb2..70d7cbb54e4 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-add-op-without-value.envoypatchpolicies.yaml @@ -1,9 +1,16 @@ - name: first-policy namespace: default status: - conditions: - - lastTransitionTime: null - message: The add operation requires a value - reason: Invalid - status: "False" - type: Programmed + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: foobar + namespace: default + conditions: + - lastTransitionTime: null + message: The add operation requires a value + reason: Invalid + status: "False" + type: Programmed + controllerName: "" diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-invalid-patch.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-invalid-patch.envoypatchpolicies.yaml index dee45a3b444..8610433d36d 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-invalid-patch.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-invalid-patch.envoypatchpolicies.yaml @@ -1,10 +1,17 @@ - name: first-policy namespace: default status: - conditions: - - lastTransitionTime: null - message: 'unable to unmarshal xds resource {"name":"first-listener","address":{"socket_address":{"address":"0.0.0.0","port_value":10080}},"default_filter_chain":{"filters":[{"name":"envoy.filters.network.http_connection_manager","typed_config":{"@type":"type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager","stat_prefix":"http","rds":{"config_source":{"ads":{},"resource_api_version":"V3"},"route_config_name":"first-listener"},"http_filters":[{"name":"envoy.filters.http.router","typed_config":{"@type":"type.googleapis.com/envoy.extensions.filters.http.router.v3.Router","suppress_envoy_headers":true}}],"common_http_protocol_options":{"headers_with_underscores_action":"REJECT_REQUEST"},"http2_protocol_options":{"max_concurrent_streams":100,"initial_stream_window_size":65536,"initial_connection_window_size":1048576},"server_header_transformation":"PASS_THROUGH","use_remote_address":true,"normalize_path":true,"merge_slashes":true,"path_with_escaped_slashes_action":"UNESCAPE_AND_REDIRECT"}}]},"per_connection_buffer_limit_bytes":32768,"drain_type":"MODIFY_ONLY","this":{"path":{"never":{"existed":{"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,"rate_limit_service":{"grpc_service":{"envoy_grpc":{"cluster_name":"rate-limit-cluster"}},"transport_api_version":"V3"},"timeout":"1s"}}}}}}, - err:proto: (line 1:1077): unknown field "this"' - reason: Invalid - status: "False" - type: Programmed + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: foobar + namespace: default + conditions: + - lastTransitionTime: null + message: 'Unable to unmarshal xds resource {"name":"first-listener","address":{"socket_address":{"address":"0.0.0.0","port_value":10080}},"default_filter_chain":{"filters":[{"name":"envoy.filters.network.http_connection_manager","typed_config":{"@type":"type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager","stat_prefix":"http","rds":{"config_source":{"ads":{},"resource_api_version":"V3"},"route_config_name":"first-listener"},"http_filters":[{"name":"envoy.filters.http.router","typed_config":{"@type":"type.googleapis.com/envoy.extensions.filters.http.router.v3.Router","suppress_envoy_headers":true}}],"common_http_protocol_options":{"headers_with_underscores_action":"REJECT_REQUEST"},"http2_protocol_options":{"max_concurrent_streams":100,"initial_stream_window_size":65536,"initial_connection_window_size":1048576},"server_header_transformation":"PASS_THROUGH","use_remote_address":true,"normalize_path":true,"merge_slashes":true,"path_with_escaped_slashes_action":"UNESCAPE_AND_REDIRECT"}}]},"per_connection_buffer_limit_bytes":32768,"drain_type":"MODIFY_ONLY","this":{"path":{"never":{"existed":{"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,"rate_limit_service":{"grpc_service":{"envoy_grpc":{"cluster_name":"rate-limit-cluster"}},"transport_api_version":"V3"},"timeout":"1s"}}}}}}, + err:proto: (line 1:1077): unknown field "this"' + reason: Invalid + status: "False" + type: Programmed + controllerName: "" diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml index 26d7c0af420..61f2babddbb 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml @@ -1,10 +1,16 @@ - name: first-policy namespace: default status: - conditions: - - lastTransitionTime: null - message: 'unable to find xds resource type.googleapis.com/envoy.config.listener.v3.Listener: - non-existant-listener' - reason: ResourceNotFound - status: "False" - type: Programmed + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: foobar + namespace: default + conditions: + - lastTransitionTime: null + message: 'Unable to find xds resources: type.googleapis.com/envoy.config.listener.v3.Listener/non-existant-listener' + reason: ResourceNotFound + status: "False" + type: Programmed + controllerName: "" diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml index fd77280814f..1fc8993fd1b 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-move-op-with-value.envoypatchpolicies.yaml @@ -1,9 +1,16 @@ - 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 + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: foobar + namespace: default + conditions: + - lastTransitionTime: null + message: The value field can not be set for the remove operation + reason: Invalid + status: "False" + type: Programmed + controllerName: "" diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch.envoypatchpolicies.yaml index 4043644d4ea..9508dd3e7b3 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch.envoypatchpolicies.yaml @@ -1,9 +1,16 @@ - name: first-policy namespace: default status: - conditions: - - lastTransitionTime: null - message: successfully applied patches. - reason: Programmed - status: "True" - type: Programmed + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: foobar + namespace: default + conditions: + - lastTransitionTime: null + message: Patches have been successfully applied. + reason: Programmed + status: "True" + type: Programmed + controllerName: "" diff --git a/internal/xds/translator/translator.go b/internal/xds/translator/translator.go index d93361337eb..bd35d41891a 100644 --- a/internal/xds/translator/translator.go +++ b/internal/xds/translator/translator.go @@ -98,6 +98,7 @@ func (t *Translator) Translate(ir *ir.Xds) (*types.ResourceVersionTable, error) if err := processClusterForAccessLog(tCtx, ir.AccessLog); err != nil { errs = errors.Join(errs, err) } + if err := processClusterForTracing(tCtx, ir.Tracing); err != nil { errs = errors.Join(errs, err) } diff --git a/internal/xds/translator/translator_test.go b/internal/xds/translator/translator_test.go index da7fef393ad..bd935e6139e 100644 --- a/internal/xds/translator/translator_test.go +++ b/internal/xds/translator/translator_test.go @@ -47,6 +47,7 @@ func TestTranslateXds(t *testing.T) { dnsDomain string requireSecrets bool requireEnvoyPatchPolicies bool + err bool }{ { name: "empty", @@ -183,22 +184,27 @@ func TestTranslateXds(t *testing.T) { name: "jsonpatch", requireEnvoyPatchPolicies: true, requireSecrets: true, + err: true, }, { name: "jsonpatch-missing-resource", requireEnvoyPatchPolicies: true, + err: true, }, { name: "jsonpatch-invalid-patch", requireEnvoyPatchPolicies: true, + err: true, }, { name: "jsonpatch-add-op-without-value", requireEnvoyPatchPolicies: true, + err: true, }, { name: "jsonpatch-move-op-with-value", requireEnvoyPatchPolicies: true, + err: true, }, { name: "listener-tcp-keepalive", @@ -304,7 +310,7 @@ func TestTranslateXds(t *testing.T) { } tCtx, err := tr.Translate(ir) - if !strings.HasSuffix(tc.name, "partial-invalid") { + if !strings.HasSuffix(tc.name, "partial-invalid") && !tc.err { require.NoError(t, err) } @@ -377,9 +383,6 @@ func TestTranslateXdsNegative(t *testing.T) { { name: "tracing-invalid", }, - { - name: "jsonpatch-invalid-listener", - }, } for _, tc := range testCases { diff --git a/site/content/en/latest/api/extension_types.md b/site/content/en/latest/api/extension_types.md index e1e68576a66..12334c0bf2d 100644 --- a/site/content/en/latest/api/extension_types.md +++ b/site/content/en/latest/api/extension_types.md @@ -800,8 +800,6 @@ _Appears in:_ | `priority` | _integer_ | true | Priority of the EnvoyPatchPolicy. If multiple EnvoyPatchPolicies are applied to the same TargetRef, they will be applied in the ascending order of the priority i.e. int32.min has the highest priority and int32.max has the lowest priority. Defaults to 0. | - - #### EnvoyPatchType _Underlying type:_ _string_ From 09749eb2a01b394102f74adcaceddce9f417f82c Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Wed, 13 Mar 2024 16:03:22 +0800 Subject: [PATCH 2/3] mv no status test case into status-condition Signed-off-by: shawnh2 --- ...tatus-for-unknown-gateway-or-route.in.yaml | 23 -------------- ...atus-for-unknown-gateway-or-route.out.yaml | 31 ------------------- .../securitypolicy-status-conditions.in.yaml | 22 +++++++++++++ .../securitypolicy-status-conditions.out.yaml | 28 +++++++++++++++++ internal/message/types.go | 1 - test/e2e/tests/utils.go | 1 - 6 files changed, 50 insertions(+), 56 deletions(-) delete mode 100644 internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml delete mode 100644 internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml diff --git a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml b/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml deleted file mode 100644 index 91d4841271b..00000000000 --- a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.in.yaml +++ /dev/null @@ -1,23 +0,0 @@ -securityPolicies: -- apiVersion: gateway.envoyproxy.io/v1alpha1 - kind: SecurityPolicy - metadata: - namespace: envoy-gateway - name: target-unknown-gateway - spec: - targetRef: - group: gateway.networking.k8s.io - kind: Gateway - name: unknown-gateway - namespace: envoy-gateway -- apiVersion: gateway.envoyproxy.io/v1alpha1 - kind: SecurityPolicy - metadata: - namespace: envoy-gateway - name: target-unknown-httproute - spec: - targetRef: - group: gateway.networking.k8s.io - kind: HTTPRoute - name: unknown-httproute - namespace: envoy-gateway diff --git a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml b/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml deleted file mode 100644 index 2c3ccb24518..00000000000 --- a/internal/gatewayapi/testdata/securitypolicy-no-status-for-unknown-gateway-or-route.out.yaml +++ /dev/null @@ -1,31 +0,0 @@ -infraIR: {} -securityPolicies: -- apiVersion: gateway.envoyproxy.io/v1alpha1 - kind: SecurityPolicy - metadata: - creationTimestamp: null - name: target-unknown-httproute - namespace: envoy-gateway - spec: - targetRef: - group: gateway.networking.k8s.io - kind: HTTPRoute - name: unknown-httproute - namespace: envoy-gateway - status: - ancestors: null -- apiVersion: gateway.envoyproxy.io/v1alpha1 - kind: SecurityPolicy - metadata: - creationTimestamp: null - name: target-unknown-gateway - namespace: envoy-gateway - spec: - targetRef: - group: gateway.networking.k8s.io - kind: Gateway - name: unknown-gateway - namespace: envoy-gateway - status: - ancestors: null -xdsIR: {} diff --git a/internal/gatewayapi/testdata/securitypolicy-status-conditions.in.yaml b/internal/gatewayapi/testdata/securitypolicy-status-conditions.in.yaml index bdc74d838b1..504deaab89d 100644 --- a/internal/gatewayapi/testdata/securitypolicy-status-conditions.in.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-status-conditions.in.yaml @@ -54,6 +54,28 @@ securityPolicies: kind: GRPCRoute name: grpcroute-1 namespace: envoy-gateway +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + namespace: envoy-gateway + name: target-unknown-gateway + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: unknown-gateway + namespace: envoy-gateway +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + namespace: envoy-gateway + name: target-unknown-httproute + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: unknown-httproute + namespace: envoy-gateway httpRoutes: - apiVersion: gateway.networking.k8s.io/v1beta1 kind: HTTPRoute diff --git a/internal/gatewayapi/testdata/securitypolicy-status-conditions.out.yaml b/internal/gatewayapi/testdata/securitypolicy-status-conditions.out.yaml index 843d5410402..853da756488 100755 --- a/internal/gatewayapi/testdata/securitypolicy-status-conditions.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-status-conditions.out.yaml @@ -320,6 +320,20 @@ securityPolicies: status: "True" type: Accepted controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + creationTimestamp: null + name: target-unknown-httproute + namespace: envoy-gateway + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: unknown-httproute + namespace: envoy-gateway + status: + ancestors: null - apiVersion: gateway.envoyproxy.io/v1alpha1 kind: SecurityPolicy metadata: @@ -379,6 +393,20 @@ securityPolicies: status: "False" type: Accepted controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: SecurityPolicy + metadata: + creationTimestamp: null + name: target-unknown-gateway + namespace: envoy-gateway + spec: + targetRef: + group: gateway.networking.k8s.io + kind: Gateway + name: unknown-gateway + namespace: envoy-gateway + status: + ancestors: null xdsIR: envoy-gateway/gateway-1: accessLog: diff --git a/internal/message/types.go b/internal/message/types.go index 86202f28079..2ce936442a6 100644 --- a/internal/message/types.go +++ b/internal/message/types.go @@ -11,7 +11,6 @@ import ( gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" gwapiv1a2 "sigs.k8s.io/gateway-api/apis/v1alpha2" - egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" "github.com/envoyproxy/gateway/internal/gatewayapi" "github.com/envoyproxy/gateway/internal/ir" xdstypes "github.com/envoyproxy/gateway/internal/xds/types" diff --git a/test/e2e/tests/utils.go b/test/e2e/tests/utils.go index 181f27113b9..b5c27d49d3e 100644 --- a/test/e2e/tests/utils.go +++ b/test/e2e/tests/utils.go @@ -89,7 +89,6 @@ func SecurityPolicyMustBeAccepted(t *testing.T, client client.Client, policyName } // BackendTrafficPolicyMustBeAccepted waits for the specified BackendTrafficPolicy to be accepted. -// TODO (sh2): make it generic for xPolicy func BackendTrafficPolicyMustBeAccepted(t *testing.T, client client.Client, policyName types.NamespacedName, controllerName string, ancestorRef gwv1a2.ParentReference) { t.Helper() From c4fae2552079bd50238d8e0a160d05a765884125 Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Thu, 14 Mar 2024 10:57:57 +0800 Subject: [PATCH 3/3] address comments and add missing test back Signed-off-by: shawnh2 --- .../testdata/in/xds-ir/jsonpatch-missing-resource.yaml | 2 +- .../xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml | 2 +- internal/xds/translator/translator_test.go | 3 +++ 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml index d795390537f..3f50ddf7aaf 100644 --- a/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml +++ b/internal/xds/translator/testdata/in/xds-ir/jsonpatch-missing-resource.yaml @@ -10,7 +10,7 @@ envoyPatchPolicies: namespace: "default" jsonPatches: - type: "type.googleapis.com/envoy.config.listener.v3.Listener" - name: "non-existant-listener" + name: "non-existing-listener" operation: op: "add" path: "/default_filter_chain/filters/0/typed_config/http_filters/0" diff --git a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml index 61f2babddbb..588e293507d 100644 --- a/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/jsonpatch-missing-resource.envoypatchpolicies.yaml @@ -9,7 +9,7 @@ namespace: default conditions: - lastTransitionTime: null - message: 'Unable to find xds resources: type.googleapis.com/envoy.config.listener.v3.Listener/non-existant-listener' + message: 'Unable to find xds resources: type.googleapis.com/envoy.config.listener.v3.Listener/non-existing-listener' reason: ResourceNotFound status: "False" type: Programmed diff --git a/internal/xds/translator/translator_test.go b/internal/xds/translator/translator_test.go index bd935e6139e..e0737a0df6f 100644 --- a/internal/xds/translator/translator_test.go +++ b/internal/xds/translator/translator_test.go @@ -377,6 +377,9 @@ func TestTranslateXdsNegative(t *testing.T) { { name: "jsonpatch-invalid", }, + { + name: "jsonpatch-invalid-listener", + }, { name: "accesslog-invalid", },