From 01c231a6ea8f46b572087c2852cd88db1e0914cf Mon Sep 17 00:00:00 2001 From: Maliz Date: Thu, 27 Apr 2023 23:15:43 -0700 Subject: [PATCH 1/4] add sameness group to source intention --- .../templates/crd-exportedservices.yaml | 3 +- .../templates/crd-serviceintentions.yaml | 4 + .../api/v1alpha1/serviceintentions_types.go | 75 ++++++++++++----- .../v1alpha1/serviceintentions_types_test.go | 84 ++++++++++++++++++- control-plane/api/v1alpha1/shared_types.go | 2 + ...consul.hashicorp.com_exportedservices.yaml | 3 +- ...onsul.hashicorp.com_serviceintentions.yaml | 4 + control-plane/go.sum | 2 - 8 files changed, 147 insertions(+), 30 deletions(-) diff --git a/charts/consul/templates/crd-exportedservices.yaml b/charts/consul/templates/crd-exportedservices.yaml index 073e081d0c..591500cb12 100644 --- a/charts/consul/templates/crd-exportedservices.yaml +++ b/charts/consul/templates/crd-exportedservices.yaml @@ -76,7 +76,8 @@ spec: the service to. type: string peer: - description: Peer is the name of the peer to export the service to. + description: Peer is the name of the peer to export the + service to. type: string samenessGroup: description: SamenessGroup is the name of the sameness diff --git a/charts/consul/templates/crd-serviceintentions.yaml b/charts/consul/templates/crd-serviceintentions.yaml index ede8ae09b0..67998f776c 100644 --- a/charts/consul/templates/crd-serviceintentions.yaml +++ b/charts/consul/templates/crd-serviceintentions.yaml @@ -185,6 +185,10 @@ spec: type: object type: object type: array + samenessGroup: + description: SamenessGroup is the name of the sameness group, + if applicable. + type: string type: object type: array type: object diff --git a/control-plane/api/v1alpha1/serviceintentions_types.go b/control-plane/api/v1alpha1/serviceintentions_types.go index a066b4e84f..fd18ecd3fe 100644 --- a/control-plane/api/v1alpha1/serviceintentions_types.go +++ b/control-plane/api/v1alpha1/serviceintentions_types.go @@ -5,6 +5,7 @@ package v1alpha1 import ( "encoding/json" + "fmt" "net/http" "strings" @@ -85,6 +86,8 @@ type SourceIntention struct { Peer string `json:"peer,omitempty"` // Partition is the Admin Partition for the Name parameter. Partition string `json:"partition,omitempty"` + // SamenessGroup is the name of the sameness group, if applicable. + SamenessGroup string `json:"samenessGroup,omitempty"` // Action is required for an L4 intention, and should be set to one of // "allow" or "deny" for the action that should be taken if this intention matches a request. Action IntentionAction `json:"action,omitempty"` @@ -272,10 +275,10 @@ func (in *ServiceIntentions) Validate(consulMeta common.ConsulMeta) error { } else { errs = append(errs, source.Permissions.validate(path.Child("sources").Index(i))...) } + errs = append(errs, source.validate(path.Child("sources").Index(i), consulMeta.PartitionsEnabled)...) } errs = append(errs, in.validateNamespaces(consulMeta.NamespacesEnabled)...) - errs = append(errs, in.validateSourcePeerAndPartitions(consulMeta.PartitionsEnabled)...) if len(errs) > 0 { return apierrors.NewInvalid( @@ -285,6 +288,46 @@ func (in *ServiceIntentions) Validate(consulMeta common.ConsulMeta) error { return nil } +func (in *SourceIntention) validate(path *field.Path, partitionsEnabled bool) field.ErrorList { + var errs field.ErrorList + + if in.Name == "" { + errs = append(errs, field.Required(path.Child("name"), "name is required.")) + } + + if strings.Contains(in.Partition, WildcardSpecifier) { + errs = append(errs, field.Invalid(path.Child("partition"), in.Partition, "partition cannot use or contain wildcard '*'")) + } + if strings.Contains(in.Peer, WildcardSpecifier) { + errs = append(errs, field.Invalid(path.Child("peer"), in.Peer, "peer cannot use or contain wildcard '*'")) + } + if strings.Contains(in.SamenessGroup, WildcardSpecifier) { + errs = append(errs, field.Invalid(path.Child("samenessgroup"), in.SamenessGroup, "samenessgroup cannot use or contain wildcard '*'")) + } + + if in.Partition != "" && !partitionsEnabled { + errs = append(errs, field.Invalid(path.Child("partition"), in.Partition, `Consul Enterprise Admin Partitions must be enabled to set source.partition`)) + } + + if in.Peer != "" && in.Partition != "" { + errs = append(errs, field.Invalid(path, *in, "cannot set peer and partition at the same time.")) + } + + if in.SamenessGroup != "" && in.Partition != "" { + errs = append(errs, field.Invalid(path, *in, "cannot set samenessgroup and partition at the same time.")) + } + + if in.SamenessGroup != "" && in.Peer != "" { + errs = append(errs, field.Invalid(path, *in, "cannot set samenessgroup and peer at the same time.")) + } + + if len(in.Description) > metaValueMaxLength { + errs = append(errs, field.Invalid(path, "", fmt.Sprintf("description exceeds maximum length %d", metaValueMaxLength))) + } + + return errs +} + // DefaultNamespaceFields sets the namespace field on spec.destination to their default values if namespaces are enabled. func (in *ServiceIntentions) DefaultNamespaceFields(consulMeta common.ConsulMeta) { // If namespaces are enabled we want to set the destination namespace field to it's @@ -313,13 +356,14 @@ func (in *SourceIntention) toConsul() *capi.SourceIntention { return nil } return &capi.SourceIntention{ - Name: in.Name, - Namespace: in.Namespace, - Partition: in.Partition, - Peer: in.Peer, - Action: in.Action.toConsul(), - Permissions: in.Permissions.toConsul(), - Description: in.Description, + Name: in.Name, + Namespace: in.Namespace, + Partition: in.Partition, + Peer: in.Peer, + SamenessGroup: in.SamenessGroup, + Action: in.Action.toConsul(), + Permissions: in.Permissions.toConsul(), + Description: in.Description, } } @@ -461,21 +505,6 @@ func (in *ServiceIntentions) validateNamespaces(namespacesEnabled bool) field.Er return errs } -func (in *ServiceIntentions) validateSourcePeerAndPartitions(partitionsEnabled bool) field.ErrorList { - var errs field.ErrorList - path := field.NewPath("spec") - for i, source := range in.Spec.Sources { - if source.Partition != "" && !partitionsEnabled { - errs = append(errs, field.Invalid(path.Child("sources").Index(i).Child("partition"), source.Partition, `Consul Enterprise Admin Partitions must be enabled to set source.partition`)) - } - - if source.Peer != "" && source.Partition != "" { - errs = append(errs, field.Invalid(path.Child("sources").Index(i), source, `Both source.peer and source.partition cannot be set.`)) - } - } - return errs -} - func (in IntentionAction) validate(path *field.Path) *field.Error { actions := []string{"allow", "deny"} if !sliceContains(actions, string(in)) { diff --git a/control-plane/api/v1alpha1/serviceintentions_types_test.go b/control-plane/api/v1alpha1/serviceintentions_types_test.go index 99b391039c..276c75e6d8 100644 --- a/control-plane/api/v1alpha1/serviceintentions_types_test.go +++ b/control-plane/api/v1alpha1/serviceintentions_types_test.go @@ -269,6 +269,13 @@ func TestServiceIntentions_ToConsul(t *testing.T) { Action: "deny", Description: "disallow access from namespace not-test", }, + { + Name: "*", + Namespace: "ns1", + SamenessGroup: "sg2", + Action: "deny", + Description: "disallow access from namespace ns1", + }, { Name: "svc-2", Namespace: "bar", @@ -322,6 +329,13 @@ func TestServiceIntentions_ToConsul(t *testing.T) { Action: "deny", Description: "disallow access from namespace not-test", }, + { + Name: "*", + Namespace: "ns1", + SamenessGroup: "sg2", + Action: "deny", + Description: "disallow access from namespace ns1", + }, { Name: "svc-2", Namespace: "bar", @@ -1343,7 +1357,71 @@ func TestServiceIntentions_Validate(t *testing.T) { namespacesEnabled: true, partitionsEnabled: true, expectedErrMsgs: []string{ - `spec.sources[0]: Invalid value: v1alpha1.SourceIntention{Name:"web", Namespace:"namespace-b", Peer:"peer-other", Partition:"partition-other", Action:"allow", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: Both source.peer and source.partition cannot be set.`, + `cannot set peer and partition at the same time.`, + }, + }, + "single source samenessgroup and partition specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: IntentionDestination{ + Name: "dest-service", + Namespace: "namespace-a", + }, + Sources: SourceIntentions{ + { + Name: "web", + Action: "allow", + Namespace: "namespace-b", + Partition: "partition-other", + SamenessGroup: "sg2", + }, + { + Name: "db", + Action: "deny", + Namespace: "namespace-c", + }, + }, + }, + }, + namespacesEnabled: true, + partitionsEnabled: true, + expectedErrMsgs: []string{ + `cannot set samenessgroup and partition at the same time.`, + }, + }, + "single source samenessgroup and peer specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: IntentionDestination{ + Name: "dest-service", + Namespace: "namespace-a", + }, + Sources: SourceIntentions{ + { + Name: "web", + Action: "allow", + Namespace: "namespace-b", + Peer: "p2", + SamenessGroup: "sg2", + }, + { + Name: "db", + Action: "deny", + Namespace: "namespace-c", + }, + }, + }, + }, + namespacesEnabled: true, + partitionsEnabled: true, + expectedErrMsgs: []string{ + `cannot set samenessgroup and peer at the same time.`, }, }, "multiple source peer and partition specified": { @@ -1377,8 +1455,8 @@ func TestServiceIntentions_Validate(t *testing.T) { namespacesEnabled: true, partitionsEnabled: true, expectedErrMsgs: []string{ - `spec.sources[0]: Invalid value: v1alpha1.SourceIntention{Name:"web", Namespace:"namespace-b", Peer:"peer-other", Partition:"partition-other", Action:"allow", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: Both source.peer and source.partition cannot be set.`, - `spec.sources[1]: Invalid value: v1alpha1.SourceIntention{Name:"db", Namespace:"namespace-c", Peer:"peer-2", Partition:"partition-2", Action:"deny", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: Both source.peer and source.partition cannot be set.`, + `spec.sources[0]: Invalid value: v1alpha1.SourceIntention{Name:"web", Namespace:"namespace-b", Peer:"peer-other", Partition:"partition-other", SamenessGroup:"", Action:"allow", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: cannot set peer and partition at the same time.`, + `spec.sources[1]: Invalid value: v1alpha1.SourceIntention{Name:"db", Namespace:"namespace-c", Peer:"peer-2", Partition:"partition-2", SamenessGroup:"", Action:"deny", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: cannot set peer and partition at the same time.`, }, }, } diff --git a/control-plane/api/v1alpha1/shared_types.go b/control-plane/api/v1alpha1/shared_types.go index 98eb3f1b20..5b4ec4a64d 100644 --- a/control-plane/api/v1alpha1/shared_types.go +++ b/control-plane/api/v1alpha1/shared_types.go @@ -16,6 +16,8 @@ import ( // This file contains structs that are shared between multiple config entries. +const metaValueMaxLength = 512 + type MeshGatewayMode string // Expose describes HTTP paths to expose through Envoy outside of Connect. diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml index 84e523f50c..0b6b969856 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml @@ -72,7 +72,8 @@ spec: the service to. type: string peer: - description: Peer is the name of the peer to export the service to. + description: Peer is the name of the peer to export the + service to. type: string samenessGroup: description: SamenessGroup is the name of the sameness diff --git a/control-plane/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml b/control-plane/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml index 3c80ea8933..35930a0e8a 100644 --- a/control-plane/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml +++ b/control-plane/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml @@ -181,6 +181,10 @@ spec: type: object type: object type: array + samenessGroup: + description: SamenessGroup is the name of the sameness group, + if applicable. + type: string type: object type: array type: object diff --git a/control-plane/go.sum b/control-plane/go.sum index 2ad5716991..66d96bdb84 100644 --- a/control-plane/go.sum +++ b/control-plane/go.sum @@ -353,8 +353,6 @@ github.com/hashicorp/consul-k8s/control-plane/cni v0.0.0-20220831174802-b8af6526 github.com/hashicorp/consul-server-connection-manager v0.1.0 h1:XCweGvMHzra88rYv2zxwwuUOjBUdcQmNKVrnQmt/muo= github.com/hashicorp/consul-server-connection-manager v0.1.0/go.mod h1:XVVlO+Yk7aiRpspiHZkrrFVn9BJIiOPnQIzqytPxGaU= github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q= -github.com/hashicorp/consul/api v1.10.1-0.20230424202255-e47f3216e51b h1:6JQAVFzqHzB51SP55BOLoDsw6sVD2WGkNSOoxI1hVZ4= -github.com/hashicorp/consul/api v1.10.1-0.20230424202255-e47f3216e51b/go.mod h1:tXfrC6o0yFTgAW46xd5Ic8STHc9oIBcRVBcwhX5KNCQ= github.com/hashicorp/consul/api v1.10.1-0.20230427155444-391ed069c461 h1:cbsTR88ShbvcRMqLU8K0atm4GmRr8UH4x4jX4e12RYE= github.com/hashicorp/consul/api v1.10.1-0.20230427155444-391ed069c461/go.mod h1:tXfrC6o0yFTgAW46xd5Ic8STHc9oIBcRVBcwhX5KNCQ= github.com/hashicorp/consul/proto-public v0.1.0 h1:O0LSmCqydZi363hsqc6n2v5sMz3usQMXZF6ziK3SzXU= From 6b8f629b11a018c6da5e0684a52f14040d027fc2 Mon Sep 17 00:00:00 2001 From: Maliz Date: Fri, 28 Apr 2023 12:19:44 -0700 Subject: [PATCH 2/4] add more test coverage --- .../v1alpha1/serviceintentions_types_test.go | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/control-plane/api/v1alpha1/serviceintentions_types_test.go b/control-plane/api/v1alpha1/serviceintentions_types_test.go index 276c75e6d8..df0100e75a 100644 --- a/control-plane/api/v1alpha1/serviceintentions_types_test.go +++ b/control-plane/api/v1alpha1/serviceintentions_types_test.go @@ -4,6 +4,7 @@ package v1alpha1 import ( + "strings" "testing" "time" @@ -616,6 +617,8 @@ func TestServiceIntentions_DefaultNamespaceFields(t *testing.T) { } func TestServiceIntentions_Validate(t *testing.T) { + longDescription := strings.Repeat("x", metaValueMaxLength+1) + cases := map[string]struct { input *ServiceIntentions namespacesEnabled bool @@ -1138,6 +1141,54 @@ func TestServiceIntentions_Validate(t *testing.T) { `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0]: Invalid value: "{\"name\":\"svc-2\",\"namespace\":\"bar\",\"action\":\"deny\",\"permissions\":[{\"action\":\"allow\",\"http\":{\"pathExact\":\"/bar\"}}]}": action and permissions are mutually exclusive and only one of them can be specified`, }, }, + "name not specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: IntentionDestination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Namespace: "bar", + Action: "deny", + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0].name: Required value: name is required.`, + }, + }, + "description is too long": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: IntentionDestination{ + Name: "dest-service", + Namespace: "namespace", + }, + Sources: SourceIntentions{ + { + Name: "foo", + Namespace: "bar", + Action: "deny", + Description: longDescription, + }, + }, + }, + }, + namespacesEnabled: true, + expectedErrMsgs: []string{ + `serviceintentions.consul.hashicorp.com "does-not-matter" is invalid: spec.sources[0]: Invalid value: "": description exceeds maximum length 512`, + }, + }, "namespaces disabled: destination namespace specified": { input: &ServiceIntentions{ ObjectMeta: metav1.ObjectMeta{ @@ -1459,6 +1510,46 @@ func TestServiceIntentions_Validate(t *testing.T) { `spec.sources[1]: Invalid value: v1alpha1.SourceIntention{Name:"db", Namespace:"namespace-c", Peer:"peer-2", Partition:"partition-2", SamenessGroup:"", Action:"deny", Permissions:v1alpha1.IntentionPermissions(nil), Description:""}: cannot set peer and partition at the same time.`, }, }, + "multiple errors: wildcard peer and partition and samenessgroup specified": { + input: &ServiceIntentions{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-matter", + }, + Spec: ServiceIntentionsSpec{ + Destination: IntentionDestination{ + Name: "dest-service", + Namespace: "namespace-a", + }, + Sources: SourceIntentions{ + { + Name: "web", + Action: "allow", + Namespace: "namespace-b", + Partition: "*", + }, + { + Name: "db", + Action: "deny", + Namespace: "namespace-c", + Peer: "*", + }, + { + Name: "db2", + Action: "deny", + Namespace: "namespace-d", + SamenessGroup: "*", + }, + }, + }, + }, + namespacesEnabled: true, + partitionsEnabled: true, + expectedErrMsgs: []string{ + `partition cannot use or contain wildcard '*'`, + `peer cannot use or contain wildcard '*'`, + `samenessgroup cannot use or contain wildcard '*'`, + }, + }, } for name, testCase := range cases { t.Run(name, func(t *testing.T) { From 69a3dd1698cd130a9db461f53e1ec7dd5763f4e0 Mon Sep 17 00:00:00 2001 From: Maliz Date: Fri, 28 Apr 2023 12:30:49 -0700 Subject: [PATCH 3/4] add comment on metaValueMaxLength variable --- control-plane/api/v1alpha1/shared_types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/control-plane/api/v1alpha1/shared_types.go b/control-plane/api/v1alpha1/shared_types.go index 5b4ec4a64d..08661d8ff8 100644 --- a/control-plane/api/v1alpha1/shared_types.go +++ b/control-plane/api/v1alpha1/shared_types.go @@ -16,6 +16,7 @@ import ( // This file contains structs that are shared between multiple config entries. +// metaValueMaxLength is the maximum allowed string length of a metadata value const metaValueMaxLength = 512 type MeshGatewayMode string From 10bce9c0d42282e65bf9517cb5358f0808fed791 Mon Sep 17 00:00:00 2001 From: Maliz Date: Fri, 28 Apr 2023 13:06:09 -0700 Subject: [PATCH 4/4] fix comment lint issue --- control-plane/api/v1alpha1/shared_types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/control-plane/api/v1alpha1/shared_types.go b/control-plane/api/v1alpha1/shared_types.go index 08661d8ff8..9aa5e519d4 100644 --- a/control-plane/api/v1alpha1/shared_types.go +++ b/control-plane/api/v1alpha1/shared_types.go @@ -16,7 +16,7 @@ import ( // This file contains structs that are shared between multiple config entries. -// metaValueMaxLength is the maximum allowed string length of a metadata value +// metaValueMaxLength is the maximum allowed string length of a metadata value. const metaValueMaxLength = 512 type MeshGatewayMode string