Skip to content

Commit

Permalink
Address review comments to Peering Acceptor and Dialer.
Browse files Browse the repository at this point in the history
- rename exported service CRD consumer.peerName field from peerName -->
peer
- rename ConnectWebhook --> MeshWebhook
- add some missing unit test cases
- only add permissions to connect-inject service account for peering CRs
when peering is enabled
  • Loading branch information
thisisnotashwin authored and ndhanushkodi committed Jun 15, 2022
1 parent fbf1490 commit 46ba198
Show file tree
Hide file tree
Showing 33 changed files with 546 additions and 502 deletions.
4 changes: 2 additions & 2 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ jobs:
working-directory: control-plane
run: |
mkdir -p $HOME/bin
wget https://github.com/ndhanushkodi/binaries/releases/download/v3oss/consul -O consulbin && \
wget https://github.com/ndhanushkodi/binaries/releases/download/v4oss/consul -O consulbin && \
mv consulbin $HOME/bin/consul &&
chmod +x $HOME/bin/consul
Expand Down Expand Up @@ -194,7 +194,7 @@ jobs:
working-directory: control-plane
run: |
mkdir -p $HOME/bin
wget https://github.com/ndhanushkodi/binaries/releases/download/v3ent/consul -O consulbin && \
wget https://github.com/ndhanushkodi/binaries/releases/download/v4ent/consul -O consulbin && \
mv consulbin $HOME/bin/consul &&
chmod +x $HOME/bin/consul
Expand Down
6 changes: 3 additions & 3 deletions acceptance/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@ go 1.17
require (
github.com/gruntwork-io/terratest v0.31.2
github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638
github.com/hashicorp/consul/api v1.12.0
github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228
github.com/hashicorp/consul/sdk v0.9.0
github.com/hashicorp/go-uuid v1.0.3
github.com/hashicorp/go-version v1.2.0
github.com/hashicorp/vault/api v1.2.0
github.com/stretchr/testify v1.7.0
gopkg.in/yaml.v2 v2.4.0
Expand All @@ -34,7 +35,7 @@ require (
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/golang/snappy v0.0.1 // indirect
github.com/google/go-cmp v0.5.6 // indirect
github.com/google/go-cmp v0.5.7 // indirect
github.com/google/gofuzz v1.1.0 // indirect
github.com/google/uuid v1.1.2 // indirect
github.com/googleapis/gnostic v0.5.5 // indirect
Expand All @@ -50,7 +51,6 @@ require (
github.com/hashicorp/go-secure-stdlib/parseutil v0.1.1 // indirect
github.com/hashicorp/go-secure-stdlib/strutil v0.1.1 // indirect
github.com/hashicorp/go-sockaddr v1.0.2 // indirect
github.com/hashicorp/go-version v1.2.0 // indirect
github.com/hashicorp/golang-lru v0.5.3 // indirect
github.com/hashicorp/hcl v1.0.0 // indirect
github.com/hashicorp/serf v0.9.6 // indirect
Expand Down
7 changes: 4 additions & 3 deletions acceptance/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -333,8 +333,9 @@ github.com/google/go-cmp v0.5.0/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/
github.com/google/go-cmp v0.5.4/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/go-cmp v0.5.7 h1:81/ik6ipDQS2aGcBfIN5dHDB36BwrStyeAQquSYCV4o=
github.com/google/go-cmp v0.5.7/go.mod h1:n+brtR0CgQNWTVd5ZUFpTBC8YFBDLK/h/bpaJ8/DtOE=
github.com/google/go-containerregistry v0.0.0-20200110202235-f4fb41bf00a3/go.mod h1:2wIuQute9+hhWqvL3vEI7YB0EKluF4WcPzI1eAliazk=
github.com/google/go-querystring v0.0.0-20170111101155-53e6ce116135/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck=
github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck=
Expand Down Expand Up @@ -387,8 +388,8 @@ github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638
github.com/hashicorp/consul-k8s/control-plane v0.0.0-20211207212234-aea9efea5638/go.mod h1:7ZeaiADGbvJDuoWAT8UKj6KCcLsFUk+34OkUGMVtdXg=
github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBtguAZLlVdkD9Q=
github.com/hashicorp/consul/api v1.10.1-0.20211116182834-e6956893fb6f/go.mod h1:6pVBMo0ebnYdt2S3H87XhekM/HHrUoTD2XXb/VrZVy0=
github.com/hashicorp/consul/api v1.12.0 h1:k3y1FYv6nuKyNTqj6w9gXOx5r5CfLj/k/euUeBXj1OY=
github.com/hashicorp/consul/api v1.12.0/go.mod h1:6pVBMo0ebnYdt2S3H87XhekM/HHrUoTD2XXb/VrZVy0=
github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228 h1:BqzKe5O+75uYcFfJI0mJz3rhCgdVztvEj3rEs4xpPr0=
github.com/hashicorp/consul/api v1.10.1-0.20220614213650-6453375ab228/go.mod h1:ZlVrynguJKcYr54zGaDbaL3fOvKC9m72FhPvA8T35KQ=
github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8=
github.com/hashicorp/consul/sdk v0.8.0/go.mod h1:GBvyrGALthsZObzUGsfgHZQDXjg4lOjagTIwIR1vPms=
github.com/hashicorp/consul/sdk v0.9.0 h1:NGSHAU7X3yDCjo8WBUbNOtD3BSqv8u0vu3+zNxgmxQI=
Expand Down
18 changes: 10 additions & 8 deletions charts/consul/templates/connect-inject-clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,6 @@ rules:
- "get"
- "list"
- "watch"
- apiGroups: [ "" ]
resources: ["secrets"]
verbs:
- "get"
- "list"
- "watch"
- "create"
- "delete"
- apiGroups:
- coordination.k8s.io
resources:
Expand All @@ -52,6 +44,15 @@ rules:
- watch
- patch
{{- end }}
{{- if .Values.global.peering.enabled }}
- apiGroups: [ "" ]
resources: ["secrets"]
verbs:
- "get"
- "list"
- "watch"
- "create"
- "delete"
- apiGroups: ["consul.hashicorp.com"]
resources: ["peeringacceptors"]
verbs:
Expand Down Expand Up @@ -88,6 +89,7 @@ rules:
- get
- patch
- update
{{- end }}
{{- if .Values.global.enablePodSecurityPolicies }}
- apiGroups: [ "policy" ]
resources: [ "podsecuritypolicies" ]
Expand Down
6 changes: 3 additions & 3 deletions charts/consul/templates/crd-exportedservices.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ spec:
description: Partition is the admin partition to export
the service to.
type: string
peerName:
description: '[Experimental] PeerName is the name of the
peer to export the service to.'
peer:
description: '[Experimental] Peer is the name of the peer
to export the service to.'
type: string
type: object
type: array
Expand Down
21 changes: 10 additions & 11 deletions control-plane/api/v1alpha1/exportedservices_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ type ExportedService struct {
type ServiceConsumer struct {
// Partition is the admin partition to export the service to.
Partition string `json:"partition,omitempty"`
// [Experimental] PeerName is the name of the peer to export the service to.
PeerName string `json:"peerName,omitempty"`
// [Experimental] Peer is the name of the peer to export the service to.
Peer string `json:"peer,omitempty"`
}

func (in *ExportedServices) GetObjectMeta() metav1.ObjectMeta {
Expand Down Expand Up @@ -165,11 +165,10 @@ func (in *ExportedServices) ToConsul(datacenter string) api.ConfigEntry {
func (in *ExportedService) toConsul() capi.ExportedService {
var consumers []capi.ServiceConsumer
for _, consumer := range in.Consumers {
if consumer.PeerName != "" {
consumers = append(consumers, capi.ServiceConsumer{PeerName: consumer.PeerName})
} else {
consumers = append(consumers, capi.ServiceConsumer{Partition: consumer.Partition})
}
consumers = append(consumers, capi.ServiceConsumer{
Partition: consumer.Partition,
PeerName: consumer.Peer,
})
}
return capi.ExportedService{
Name: in.Name,
Expand Down Expand Up @@ -228,11 +227,11 @@ func (in *ExportedService) validate(path *field.Path, consulMeta common.ConsulMe
}

func (in *ServiceConsumer) validate(path *field.Path, consulMeta common.ConsulMeta) *field.Error {
if in.Partition != "" && in.PeerName != "" {
return field.Invalid(path, *in, "both partition and peerName cannot be specified.")
if in.Partition != "" && in.Peer != "" {
return field.Invalid(path, *in, "both partition and peer cannot be specified.")
}
if in.Partition == "" && in.PeerName == "" {
return field.Invalid(path, *in, "either partition or peerName must be specified.")
if in.Partition == "" && in.Peer == "" {
return field.Invalid(path, *in, "either partition or peer must be specified.")
}
if !consulMeta.PartitionsEnabled && in.Partition != "" {
return field.Invalid(path.Child("partitions"), in.Partition, "Consul Admin Partitions need to be enabled to specify partition.")
Expand Down
92 changes: 77 additions & 15 deletions control-plane/api/v1alpha1/exportedservices_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func TestExportedServices_MatchesConsul(t *testing.T) {
Partition: "third",
},
{
PeerName: "second-peer",
Peer: "second-peer",
},
},
},
Expand All @@ -69,7 +69,7 @@ func TestExportedServices_MatchesConsul(t *testing.T) {
Partition: "fifth",
},
{
PeerName: "third-peer",
Peer: "third-peer",
},
},
},
Expand Down Expand Up @@ -178,7 +178,7 @@ func TestExportedServices_ToConsul(t *testing.T) {
Partition: "third",
},
{
PeerName: "second-peer",
Peer: "second-peer",
},
},
},
Expand All @@ -193,7 +193,7 @@ func TestExportedServices_ToConsul(t *testing.T) {
Partition: "fifth",
},
{
PeerName: "third-peer",
Peer: "third-peer",
},
},
},
Expand Down Expand Up @@ -253,8 +253,10 @@ func TestExportedServices_ToConsul(t *testing.T) {

func TestExportedServices_Validate(t *testing.T) {
cases := map[string]struct {
input *ExportedServices
expectedErrMsgs []string
input *ExportedServices
namespaceEnabled bool
partitionsEnabled bool
expectedErrMsgs []string
}{
"valid": {
input: &ExportedServices{
Expand All @@ -271,14 +273,16 @@ func TestExportedServices_Validate(t *testing.T) {
Partition: "second",
},
{
PeerName: "second-peer",
Peer: "second-peer",
},
},
},
},
},
},
expectedErrMsgs: []string{},
namespaceEnabled: true,
partitionsEnabled: true,
expectedErrMsgs: []string{},
},
"no consumers specified": {
input: &ExportedServices{
Expand All @@ -295,6 +299,8 @@ func TestExportedServices_Validate(t *testing.T) {
},
},
},
namespaceEnabled: true,
partitionsEnabled: true,
expectedErrMsgs: []string{
`spec.services[0]: Invalid value: []v1alpha1.ServiceConsumer{}: service must have at least 1 consumer.`,
},
Expand All @@ -312,15 +318,17 @@ func TestExportedServices_Validate(t *testing.T) {
Consumers: []ServiceConsumer{
{
Partition: "second",
PeerName: "second-peer",
Peer: "second-peer",
},
},
},
},
},
},
namespaceEnabled: true,
partitionsEnabled: true,
expectedErrMsgs: []string{
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", PeerName:"second-peer"}: both partition and peerName cannot be specified.`,
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", Peer:"second-peer"}: both partition and peer cannot be specified.`,
},
},
"neither partition nor peer name specified": {
Expand All @@ -340,8 +348,60 @@ func TestExportedServices_Validate(t *testing.T) {
},
},
},
namespaceEnabled: true,
partitionsEnabled: true,
expectedErrMsgs: []string{
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", PeerName:""}: either partition or peerName must be specified.`,
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", Peer:""}: either partition or peer must be specified.`,
},
},
"partition provided when partitions are disabled": {
input: &ExportedServices{
ObjectMeta: metav1.ObjectMeta{
Name: common.DefaultConsulPartition,
},
Spec: ExportedServicesSpec{
Services: []ExportedService{
{
Name: "service-frontend",
Namespace: "frontend",
Consumers: []ServiceConsumer{
{
Partition: "test-partition",
},
},
},
},
},
},
namespaceEnabled: true,
partitionsEnabled: false,
expectedErrMsgs: []string{
`spec.services[0].consumers[0].partitions: Invalid value: "test-partition": Consul Admin Partitions need to be enabled to specify partition.`,
},
},
"namespace provided when namespaces are disabled": {
input: &ExportedServices{
ObjectMeta: metav1.ObjectMeta{
Name: common.DefaultConsulPartition,
},
Spec: ExportedServicesSpec{
Services: []ExportedService{
{
Name: "service-frontend",
Namespace: "frontend",
Consumers: []ServiceConsumer{
{
Peer: "test-peer",
},
},
},
},
},
},
namespaceEnabled: false,
partitionsEnabled: false,
expectedErrMsgs: []string{
`spec.services[0]: Invalid value: "frontend": Consul Namespaces must be enabled to specify service namespace.`,
},
},
"multiple errors": {
Expand All @@ -357,24 +417,26 @@ func TestExportedServices_Validate(t *testing.T) {
Consumers: []ServiceConsumer{
{
Partition: "second",
PeerName: "second-peer",
Peer: "second-peer",
},
{},
},
},
},
},
},
namespaceEnabled: true,
partitionsEnabled: true,
expectedErrMsgs: []string{
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", PeerName:"second-peer"}: both partition and peerName cannot be specified.`,
`spec.services[0].consumers[1]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", PeerName:""}: either partition or peerName must be specified.`,
`spec.services[0].consumers[0]: Invalid value: v1alpha1.ServiceConsumer{Partition:"second", Peer:"second-peer"}: both partition and peer cannot be specified.`,
`spec.services[0].consumers[1]: Invalid value: v1alpha1.ServiceConsumer{Partition:"", Peer:""}: either partition or peer must be specified.`,
},
},
}

for name, testCase := range cases {
t.Run(name, func(t *testing.T) {
err := testCase.input.Validate(common.ConsulMeta{NamespacesEnabled: true, PartitionsEnabled: true, Partition: common.DefaultConsulPartition})
err := testCase.input.Validate(common.ConsulMeta{NamespacesEnabled: testCase.namespaceEnabled, PartitionsEnabled: testCase.partitionsEnabled, Partition: common.DefaultConsulPartition})
if len(testCase.expectedErrMsgs) != 0 {
require.Error(t, err)
for _, s := range testCase.expectedErrMsgs {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ spec:
description: Partition is the admin partition to export
the service to.
type: string
peerName:
description: '[Experimental] PeerName is the name of the
peer to export the service to.'
peer:
description: '[Experimental] Peer is the name of the peer
to export the service to.'
type: string
type: object
type: array
Expand Down
2 changes: 1 addition & 1 deletion control-plane/connect-inject/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ const (
annotationTransparentProxyOverwriteProbes = "consul.hashicorp.com/transparent-proxy-overwrite-probes"

// annotationOriginalPod is the value of the pod before being overwritten by the consul
// webhook/handler.
// webhook/meshWebhook.
annotationOriginalPod = "consul.hashicorp.com/original-pod"

// labelServiceIgnore is a label that can be added to a service to prevent it from being
Expand Down
4 changes: 2 additions & 2 deletions control-plane/connect-inject/consul_sidecar.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
// the metrics merging server when metrics merging feature is enabled.
// It always disables service registration because for connect we no longer
// need to keep services registered as this is handled in the endpoints-controller.
func (w *ConnectWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
func (w *MeshWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error) {
metricsPorts, err := w.MetricsConfig.mergedMetricsServerConfiguration(pod)
if err != nil {
return corev1.Container{}, err
Expand Down Expand Up @@ -48,7 +48,7 @@ func (w *ConnectWebhook) consulSidecar(pod corev1.Pod) (corev1.Container, error)
}, nil
}

func (w *ConnectWebhook) consulSidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) {
func (w *MeshWebhook) consulSidecarResources(pod corev1.Pod) (corev1.ResourceRequirements, error) {
resources := corev1.ResourceRequirements{
Limits: corev1.ResourceList{},
Requests: corev1.ResourceList{},
Expand Down
Loading

0 comments on commit 46ba198

Please sign in to comment.