Skip to content

Commit

Permalink
Make changes based on review feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
@thisisnotashwin
thisisnotashwin committed Sep 11, 2023
1 parent a75c62a commit 80fb3e0
Show file tree
Hide file tree
Showing 9 changed files with 144 additions and 152 deletions.
15 changes: 9 additions & 6 deletions control-plane/connect-inject/constants/annotations_and_labels.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ package constants
const (
// KeyInjectStatus is the key of the annotation that is added to
// a pod after an injection is done.
KeyInjectStatus = "consul.hashicorp.com/connect-inject-status"
KeyInjectStatusV2 = "consul.hashicorp.com/mesh-inject-status"
KeyInjectStatus = "consul.hashicorp.com/connect-inject-status"

// KeyTransparentProxyStatus is the key of the annotation that is added to
// a pod when transparent proxy is done.
Expand All @@ -22,8 +21,7 @@ const (
// AnnotationInject is the key of the annotation that controls whether
// injection is explicitly enabled or disabled for a pod. This should
// be set to a truthy or falsy value, as parseable by strconv.ParseBool.
AnnotationInject = "consul.hashicorp.com/connect-inject"
AnnotationInjectV2 = "consul.hashicorp.com/mesh-inject"
AnnotationInject = "consul.hashicorp.com/connect-inject"

// AnnotationGatewayKind is the key of the annotation that indicates pods
// that represent Consul Connect Gateways. This should be set to a
Expand Down Expand Up @@ -80,8 +78,7 @@ const (
// service name should map to a Consul service namd and the local port
// is the local port in the pod that the listener will bind to. It can
// be a named port.
AnnotationUpstreams = "consul.hashicorp.com/connect-service-upstreams"
AnnotationUpstreamsV2 = "consul.hashicorp.com/mesh-service-destinations"
AnnotationUpstreams = "consul.hashicorp.com/connect-service-upstreams"

// AnnotationTags is a list of tags to register with the service
// this is specified as a comma separated list e.g. abc,123.
Expand Down Expand Up @@ -232,6 +229,12 @@ const (
// ManagedByPodValue is used in Consul metadata to identify the manager
// of resources.
ManagedByPodValue = "consul-k8s-pod-controller"

// AnnotationMeshDestinations is a list of upstreams to register with the
// proxy. The service name should map to a Consul service namd and the local
// port is the local port in the pod that the listener will bind to. It can
// be a named port.
AnnotationMeshDestinations = "consul.hashicorp.com/mesh-service-destinations"
)

// Annotations used by Prometheus.
Expand Down
15 changes: 6 additions & 9 deletions control-plane/connect-inject/webhook_v2/consul_dataplane_sidecar.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,6 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor
},
},
},
{
Name: "DP_SERVICE_NODE_NAME",
Value: "$(NODE_NAME)-virtual",
},
// The pod name isn't known currently, so we must rely on the environment variable to fill it in rather than using args.
{
Name: "POD_NAME",
Expand All @@ -108,6 +104,10 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor
FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.namespace"},
},
},
{
Name: "DP_PROXY_ID",
Value: "$(POD_NAME)",
},
{
Name: "DP_CREDENTIAL_LOGIN_META",
Value: "pod=$(POD_NAMESPACE)/$(POD_NAME)",
Expand Down Expand Up @@ -194,8 +194,6 @@ func (w *MeshWebhook) consulDataplaneSidecar(namespace corev1.Namespace, pod cor
}

func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, bearerTokenFile string, pod corev1.Pod) ([]string, error) {
proxyIDFileName := "/consul/mesh-inject/proxyid"

envoyConcurrency := w.DefaultEnvoyProxyConcurrency

// Check to see if the user has overriden concurrency via an annotation.
Expand All @@ -210,7 +208,6 @@ func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, bearer
args := []string{
"-addresses", w.ConsulAddress,
"-grpc-port=" + strconv.Itoa(w.ConsulConfig.GRPCPort),
"-proxy-service-id-path=" + proxyIDFileName,
"-log-level=" + w.LogLevel,
"-log-json=" + strconv.FormatBool(w.LogJSON),
"-envoy-concurrency=" + strconv.Itoa(envoyConcurrency),
Expand Down Expand Up @@ -239,10 +236,10 @@ func (w *MeshWebhook) getContainerSidecarArgs(namespace corev1.Namespace, bearer
}
}
if w.EnableNamespaces {
args = append(args, "-service-namespace="+w.consulNamespace(namespace.Name))
args = append(args, "-proxy-namespace="+w.consulNamespace(namespace.Name))
}
if w.ConsulPartition != "" {
args = append(args, "-service-partition="+w.ConsulPartition)
args = append(args, "-proxy-partition="+w.ConsulPartition)
}
if w.TLSEnabled {
if w.ConsulTLSServerName != "" {
Expand Down
25 changes: 12 additions & 13 deletions control-plane/connect-inject/webhook_v2/consul_dataplane_sidecar_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
w.EnableK8SNSMirroring = true
},
additionalExpCmdArgs: " -credential-type=login -login-auth-method=test-auth-method -login-bearer-token-path=/var/run/secrets/kubernetes.io/serviceaccount/token " +
"-login-namespace=default -service-namespace=k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
"-login-namespace=default -proxy-namespace=k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with ACLs and single destination namespace": {
webhookSetupFunc: func(w *MeshWebhook) {
Expand All @@ -61,15 +61,15 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
w.ConsulDestinationNamespace = "test-ns"
},
additionalExpCmdArgs: " -credential-type=login -login-auth-method=test-auth-method -login-bearer-token-path=/var/run/secrets/kubernetes.io/serviceaccount/token " +
"-login-namespace=test-ns -service-namespace=test-ns -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
"-login-namespace=test-ns -proxy-namespace=test-ns -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with ACLs and partitions": {
webhookSetupFunc: func(w *MeshWebhook) {
w.AuthMethod = "test-auth-method"
w.ConsulPartition = "test-part"
},
additionalExpCmdArgs: " -credential-type=login -login-auth-method=test-auth-method -login-bearer-token-path=/var/run/secrets/kubernetes.io/serviceaccount/token " +
"-login-partition=test-part -service-partition=test-part -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
"-login-partition=test-part -proxy-partition=test-part -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with TLS and CA cert provided": {
webhookSetupFunc: func(w *MeshWebhook) {
Expand All @@ -91,28 +91,28 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
w.EnableNamespaces = true
w.ConsulDestinationNamespace = "consul-namespace"
},
additionalExpCmdArgs: " -service-namespace=consul-namespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
additionalExpCmdArgs: " -proxy-namespace=consul-namespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with namespace mirroring": {
webhookSetupFunc: func(w *MeshWebhook) {
w.EnableNamespaces = true
w.EnableK8SNSMirroring = true
},
additionalExpCmdArgs: " -service-namespace=k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
additionalExpCmdArgs: " -proxy-namespace=k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with namespace mirroring prefix": {
webhookSetupFunc: func(w *MeshWebhook) {
w.EnableNamespaces = true
w.EnableK8SNSMirroring = true
w.K8SNSMirroringPrefix = "foo-"
},
additionalExpCmdArgs: " -service-namespace=foo-k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
additionalExpCmdArgs: " -proxy-namespace=foo-k8snamespace -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with partitions": {
webhookSetupFunc: func(w *MeshWebhook) {
w.ConsulPartition = "partition-1"
},
additionalExpCmdArgs: " -service-partition=partition-1 -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
additionalExpCmdArgs: " -proxy-partition=partition-1 -tls-disabled -graceful-port=20600 -telemetry-prom-scrape-path=/metrics",
},
"with different log level": {
webhookSetupFunc: func(w *MeshWebhook) {
Expand Down Expand Up @@ -186,8 +186,7 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
container, err := w.consulDataplaneSidecar(testNS, pod)
require.NoError(t, err)
expCmd := "-addresses 1.1.1.1 -grpc-port=" + strconv.Itoa(w.ConsulConfig.GRPCPort) +
" -proxy-service-id-path=/consul/mesh-inject/proxyid " +
"-log-level=" + w.LogLevel + " -log-json=" + strconv.FormatBool(w.LogJSON) + " -envoy-concurrency=0" + c.additionalExpCmdArgs
" -log-level=" + w.LogLevel + " -log-json=" + strconv.FormatBool(w.LogJSON) + " -envoy-concurrency=0" + c.additionalExpCmdArgs
require.Equal(t, expCmd, strings.Join(container.Args, " "))

if w.AuthMethod != "" {
Expand Down Expand Up @@ -223,10 +222,10 @@ func TestHandlerConsulDataplaneSidecar(t *testing.T) {
require.Len(t, container.Env, 7)
require.Equal(t, container.Env[0].Name, "TMPDIR")
require.Equal(t, container.Env[0].Value, "/consul/mesh-inject")
require.Equal(t, container.Env[2].Name, "DP_SERVICE_NODE_NAME")
require.Equal(t, container.Env[2].Value, "$(NODE_NAME)-virtual")
require.Equal(t, container.Env[3].Name, "POD_NAME")
require.Equal(t, container.Env[4].Name, "POD_NAMESPACE")
require.Equal(t, container.Env[2].Name, "POD_NAME")
require.Equal(t, container.Env[3].Name, "POD_NAMESPACE")
require.Equal(t, container.Env[4].Name, "DP_PROXY_ID")
require.Equal(t, container.Env[4].Value, "$(POD_NAME)")
require.Equal(t, container.Env[5].Name, "DP_CREDENTIAL_LOGIN_META")
require.Equal(t, container.Env[5].Value, "pod=$(POD_NAMESPACE)/$(POD_NAME)")
require.Equal(t, container.Env[6].Name, "DP_CREDENTIAL_LOGIN_META1")
Expand Down
58 changes: 26 additions & 32 deletions control-plane/connect-inject/webhook_v2/container_env.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,40 +4,34 @@
package webhook_v2

import (
"fmt"
"strconv"
"strings"

corev1 "k8s.io/api/core/v1"

"github.com/hashicorp/consul-k8s/control-plane/connect-inject/common"
"github.com/hashicorp/consul-k8s/control-plane/connect-inject/constants"
)

func (w *MeshWebhook) containerEnvVars(pod corev1.Pod) []corev1.EnvVar {
raw, ok := pod.Annotations[constants.AnnotationUpstreamsV2]
if !ok || raw == "" {
return []corev1.EnvVar{}
}

var result []corev1.EnvVar
for _, raw := range strings.Split(raw, ",") {
parts := strings.SplitN(raw, ":", 3)
port, _ := common.PortValue(pod, strings.TrimSpace(parts[1]))
if port > 0 {
name := strings.TrimSpace(parts[0])
name = strings.ToUpper(strings.Replace(name, "-", "_", -1))
portStr := strconv.Itoa(int(port))

result = append(result, corev1.EnvVar{
Name: fmt.Sprintf("%s_CONNECT_SERVICE_HOST", name),
Value: "127.0.0.1",
}, corev1.EnvVar{
Name: fmt.Sprintf("%s_CONNECT_SERVICE_PORT", name),
Value: portStr,
})
}
}

return result
// (TODO: ashwin) make this work with current upstreams
//raw, ok := pod.Annotations[constants.AnnotationMeshDestinations]
//if !ok || raw == "" {
// return []corev1.EnvVar{}
//}
//
//var result []corev1.EnvVar
//for _, raw := range strings.Split(raw, ",") {
// parts := strings.SplitN(raw, ":", 3)
// port, _ := common.PortValue(pod, strings.TrimSpace(parts[1]))
// if port > 0 {
// name := strings.TrimSpace(parts[0])
// name = strings.ToUpper(strings.Replace(name, "-", "_", -1))
// portStr := strconv.Itoa(int(port))
//
// result = append(result, corev1.EnvVar{
// Name: fmt.Sprintf("%s_CONNECT_SERVICE_HOST", name),
// Value: "127.0.0.1",
// }, corev1.EnvVar{
// Name: fmt.Sprintf("%s_CONNECT_SERVICE_PORT", name),
// Value: portStr,
// })
// }
//}

return []corev1.EnvVar{}
}
7 changes: 4 additions & 3 deletions control-plane/connect-inject/webhook_v2/container_env_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import (
)

func TestContainerEnvVars(t *testing.T) {

t.Skip()
// (TODO: ashwin) make these work once upstreams are fixed
cases := []struct {
Name string
Upstream string
Expand All @@ -37,8 +38,8 @@ func TestContainerEnvVars(t *testing.T) {
envVars := w.containerEnvVars(corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
constants.AnnotationService: "foo",
constants.AnnotationUpstreamsV2: tt.Upstream,
constants.AnnotationService: "foo",
constants.AnnotationMeshDestinations: tt.Upstream,
},
},
})
Expand Down
2 changes: 1 addition & 1 deletion control-plane/connect-inject/webhook_v2/container_init.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import (
)

const (
injectInitContainerName = "consul-connect-inject-init"
injectInitContainerName = "consul-mesh-init"
rootUserAndGroupID = 0
sidecarUserAndGroupID = 5995
initContainersUserAndGroupID = 5996
Expand Down
20 changes: 6 additions & 14 deletions control-plane/connect-inject/webhook_v2/mesh_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (

mapset "github.com/deckarep/golang-set"
"github.com/go-logr/logr"
"golang.org/x/exp/slices"
"gomodules.xyz/jsonpatch/v2"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand Down Expand Up @@ -314,7 +315,7 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi

// pod.Annotations has already been initialized by h.defaultAnnotations()
// and does not need to be checked for being a nil value.
pod.Annotations[constants.KeyInjectStatusV2] = constants.Injected
pod.Annotations[constants.KeyMeshInjectStatus] = constants.Injected

tproxyEnabled, err := common.TransparentProxyEnabled(*ns, pod, w.EnableTransparentProxy)
if err != nil {
Expand Down Expand Up @@ -350,7 +351,7 @@ func (w *MeshWebhook) Handle(ctx context.Context, req admission.Request) admissi
if pod.Labels == nil {
pod.Labels = make(map[string]string)
}
pod.Labels[constants.KeyInjectStatusV2] = constants.Injected
pod.Labels[constants.KeyMeshInjectStatus] = constants.Injected

// Consul-ENT only: Add the Consul destination namespace as an annotation to the pod.
if w.EnableNamespaces {
Expand Down Expand Up @@ -456,7 +457,7 @@ func (w *MeshWebhook) injectVolumeMount(pod corev1.Pod) {
containersToInject := splitCommaSeparatedItemsFromAnnotation(constants.AnnotationInjectMountVolumes, pod)

for index, container := range pod.Spec.Containers {
if sliceContains(containersToInject, container.Name) {
if slices.Contains(containersToInject, container.Name) {
pod.Spec.Containers[index].VolumeMounts = append(pod.Spec.Containers[index].VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: "/consul/connect-inject",
Expand All @@ -483,14 +484,14 @@ func (w *MeshWebhook) shouldInject(pod corev1.Pod, namespace string) (bool, erro
}

// If we already injected then don't inject again
if pod.Annotations[constants.KeyInjectStatusV2] != "" {
if pod.Annotations[constants.KeyMeshInjectStatus] != "" || pod.Annotations[constants.KeyInjectStatus] != "" {
return false, nil
}

// If the explicit true/false is on, then take that value. Note that
// this has to be the last check since it sets a default value after
// all other checks.
if raw, ok := pod.Annotations[constants.AnnotationInjectV2]; ok {
if raw, ok := pod.Annotations[constants.AnnotationMeshInject]; ok {
return strconv.ParseBool(raw)
}

Expand Down Expand Up @@ -569,12 +570,3 @@ func (w *MeshWebhook) InjectDecoder(d *admission.Decoder) error {
w.decoder = d
return nil
}

func sliceContains(slice []string, entry string) bool {
for _, s := range slice {
if entry == s {
return true
}
}
return false
}
Loading

0 comments on commit 80fb3e0

Please sign in to comment.