Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

add sameness group to source intention #2097

Merged
merged 4 commits into from
Apr 28, 2023
Merged

add sameness group to source intention #2097

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01c231a
add sameness group to source intention
Apr 28, 2023
6b8f629
add more test coverage
Apr 28, 2023
69a3dd1
add comment on metaValueMaxLength variable
Apr 28, 2023
10bce9c
fix comment lint issue
Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion charts/consul/templates/crd-exportedservices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions charts/consul/templates/crd-serviceintentions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
75 changes: 52 additions & 23 deletions control-plane/api/v1alpha1/serviceintentions_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package v1alpha1

import (
"encoding/json"
"fmt"
"net/http"
"strings"

Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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(
Expand All @@ -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 == "" {
malizz marked this conversation as resolved.
Show resolved Hide resolved
errs = append(errs, field.Required(path.Child("name"), "name is required."))
}

if strings.Contains(in.Partition, WildcardSpecifier) {
malizz marked this conversation as resolved.
Show resolved Hide resolved
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 {
malizz marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down Expand Up @@ -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,
}
}

Expand Down Expand Up @@ -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)) {
Expand Down
175 changes: 172 additions & 3 deletions control-plane/api/v1alpha1/serviceintentions_types_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package v1alpha1

import (
"strings"
"testing"
"time"

Expand Down Expand Up @@ -269,6 +270,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",
Expand Down Expand Up @@ -322,6 +330,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",
Expand Down Expand Up @@ -602,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
Expand Down Expand Up @@ -1124,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{
Expand Down Expand Up @@ -1343,7 +1408,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": {
Expand Down Expand Up @@ -1377,8 +1506,48 @@ 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.`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

`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 '*'`,
},
},
}
Expand Down
2 changes: 2 additions & 0 deletions control-plane/api/v1alpha1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import (

// This file contains structs that are shared between multiple config entries.

const metaValueMaxLength = 512
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this max length in bytes? maybe we can add the units? Also is it a common type it's only used by the intentions right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the max length of the string size. In Consul it is used by more than just intention meta, any config entry meta should not be more than this length so I added it to shared types to later use in validating other config entries.
I will add test coverage for this check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay, that's great then. Can you add a comment to that effect? Thanks!


type MeshGatewayMode string

// Expose describes HTTP paths to expose through Envoy outside of Connect.
Expand Down
3 changes: 2 additions & 1 deletion control-plane/config/crd/bases/consul.hashicorp.com_exportedservices.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 4 additions & 0 deletions control-plane/config/crd/bases/consul.hashicorp.com_serviceintentions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading