Skip to content

Commit

Permalink
Add suggestions from Luke.
Browse files Browse the repository at this point in the history
  • Loading branch information
thisisnotashwin committed Nov 10, 2021
1 parent f29938f commit 7a4ab00
Show file tree
Hide file tree
Showing 11 changed files with 79 additions and 160 deletions.
2 changes: 1 addition & 1 deletion charts/consul/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ to the consul agent command.
recursor_flags=""
for ip in $(cat /etc/resolv.conf | grep nameserver | cut -d' ' -f2)
do
recursor_flags=$recursor_flags" -recursor=$ip"
recursor_flags="$recursor_flags -recursor=$ip"
done
{{- end -}}

Expand Down
4 changes: 2 additions & 2 deletions charts/consul/templates/client-daemonset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ spec:
- |
CONSUL_FULLNAME="{{template "consul.fullname" . }}"
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
{{ template "consul.recursors" }}
{{- end }}
Expand Down Expand Up @@ -279,7 +279,7 @@ spec:
{{- range $value := .Values.global.recursors }}
-recursor={{ quote $value }} \
{{- end }}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
$recursor_flags \
{{- end }}
-config-file=/consul/extra-config/extra-from-values.json \
Expand Down
5 changes: 2 additions & 3 deletions charts/consul/templates/connect-inject-deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ spec:
- "/bin/sh"
- "-ec"
- |
CONSUL_FULLNAME="{{template "consul.fullname" . }}"
consul-k8s-control-plane inject-connect \
-log-level={{ default .Values.global.logLevel .Values.connectInject.logLevel }} \
-log-json={{ .Values.global.logJSON }} \
Expand All @@ -107,7 +105,8 @@ spec:
{{- else }}
-transparent-proxy-default-overwrite-probes=false \
{{- end }}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
-resource-prefix={{ template "consul.fullname" . }} \
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
-enable-consul-dns=true \
{{- end }}
{{- if .Values.global.openshift.enabled }}
Expand Down
4 changes: 2 additions & 2 deletions charts/consul/templates/server-statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ spec:
- |
CONSUL_FULLNAME="{{template "consul.fullname" . }}"
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
{{ template "consul.recursors" }}
{{- end }}
Expand Down Expand Up @@ -258,7 +258,7 @@ spec:
{{- range $value := .Values.global.recursors }}
-recursor={{ quote $value }} \
{{- end }}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection)}}
{{- if (and .Values.dns.enabled .Values.dns.enableRedirection) }}
$recursor_flags \
{{- end }}
-config-file=/consul/extra-config/extra-from-values.json \
Expand Down
12 changes: 11 additions & 1 deletion charts/consul/test/unit/connect-inject-deployment.bats
Original file line number Diff line number Diff line change
Expand Up @@ -522,7 +522,7 @@ EOF
#--------------------------------------------------------------------
# DNS

@test "connectInject/Deployment: -enable-consul-dns unset default" {
@test "connectInject/Deployment: -enable-consul-dns unset by default" {
cd `chart_dir`
local actual=$(helm template \
-s templates/connect-inject-deployment.yaml \
Expand All @@ -543,6 +543,16 @@ EOF
[ "${actual}" = "true" ]
}

@test "connectInject/Deployment: -resource-prefix always set" {
cd `chart_dir`
local actual=$(helm template \
-s templates/connect-inject-deployment.yaml \
--set 'connectInject.enabled=true' \
. | tee /dev/stderr |
yq -c -r '.spec.template.spec.containers[0].command | join(" ") | contains("-resource-prefix=RELEASE-NAME-consul")' | tee /dev/stderr)
[ "${actual}" = "true" ]
}

#--------------------------------------------------------------------
# global.tls.enabled

Expand Down
2 changes: 1 addition & 1 deletion charts/consul/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,7 @@ dns:
# @type: boolean
enabled: "-"

# Used to determine if services using Consul Connect use Consul DNS
# If true, services using Consul Connect will use Consul DNS
# for default DNS resolution. The DNS lookups fall back to the nameserver IPs
# listed in /etc/resolv.conf if not found in Consul.
# @type: boolean
Expand Down
33 changes: 22 additions & 11 deletions control-plane/connect-inject/container_init.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package connectinject

import (
"bytes"
"errors"
"os"
"strconv"
"strings"
Expand Down Expand Up @@ -69,8 +70,8 @@ type initContainerCommandData struct {
// the consul connect redirect-traffic command.
TProxyExcludeUIDs []string

// ConsulDNSIP is the IP of the Consul DNS Service.
ConsulDNSIP string
// ConsulDNSClusterIP is the IP of the Consul DNS Service.
ConsulDNSClusterIP string
}

// initCopyContainer returns the init container spec for the copy container which places
Expand Down Expand Up @@ -117,13 +118,14 @@ func (h *Handler) containerInit(namespace corev1.Namespace, pod corev1.Pod) (cor
return corev1.Container{}, err
}

var consulDNSIP string
var consulDNSClusterIP string
if dnsEnabled {
for _, e := range os.Environ() {
if strings.Contains(e, dnsServiceHostEnvSuffix) {
dnsServiceHostEnv := strings.SplitN(e, "=", 2)
consulDNSIP = dnsServiceHostEnv[1]
}
// If Consul DNS is enabled, we find the environment variable that has the value
// of the ClusterIP of the Consul DNS Service. constructDNSServiceHostName returns
// the name of the env variable whose value is the ClusterIP of the Consul DNS Service.
consulDNSClusterIP = os.Getenv(h.constructDNSServiceHostName())
if consulDNSClusterIP == "" {
return corev1.Container{}, errors.New("failed to find ClusterIP for Consul DNS Service Host")
}
}

Expand All @@ -138,7 +140,7 @@ func (h *Handler) containerInit(namespace corev1.Namespace, pod corev1.Pod) (cor
TProxyExcludeOutboundPorts: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeOutboundPorts, pod),
TProxyExcludeOutboundCIDRs: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeOutboundCIDRs, pod),
TProxyExcludeUIDs: splitCommaSeparatedItemsFromAnnotation(annotationTProxyExcludeUIDs, pod),
ConsulDNSIP: consulDNSIP,
ConsulDNSClusterIP: consulDNSClusterIP,
EnvoyUID: envoyUserAndGroupID,
}

Expand Down Expand Up @@ -244,6 +246,15 @@ func (h *Handler) containerInit(namespace corev1.Namespace, pod corev1.Pod) (cor
return container, nil
}

// constructDNSServiceHostName use the resource prefix and the DNS Service hostname suffix to construct the
// key of the env variable whose value is the cluster IP of the Consul DNS Service.
// It translates "resource-prefix" into "RESOURCE_PREFIX_DNS_SERVICE_HOST".
func (h *Handler) constructDNSServiceHostName() string {
upcaseResourcePrefix := strings.ToUpper(h.ResourcePrefix)
upcaseResourcePrefixWithUnderscores := strings.ReplaceAll(upcaseResourcePrefix, "-", "_")
return strings.Join([]string{upcaseResourcePrefixWithUnderscores, dnsServiceHostEnvSuffix}, "_")
}

// transparentProxyEnabled returns true if transparent proxy should be enabled for this pod.
// It returns an error when the annotation value cannot be parsed by strconv.ParseBool or if we are unable
// to read the pod's namespace label when it exists.
Expand Down Expand Up @@ -368,8 +379,8 @@ consul-k8s-control-plane connect-init -pod-name=${POD_NAME} -pod-namespace=${POD
{{- if .ConsulNamespace }}
-namespace="{{ .ConsulNamespace }}" \
{{- end }}
{{- if .ConsulDNSIP }}
-consul-dns-ip="{{ .ConsulDNSIP }}" \
{{- if .ConsulDNSClusterIP }}
-consul-dns-ip="{{ .ConsulDNSClusterIP }}" \
{{- end }}
{{- range .TProxyExcludeInboundPorts }}
-exclude-inbound-port="{{ . }}" \
Expand Down
33 changes: 30 additions & 3 deletions control-plane/connect-inject/container_init_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,9 @@ func TestHandlerContainerInit_consulDNS(t *testing.T) {
}
for name, c := range cases {
t.Run(name, func(t *testing.T) {
h := Handler{EnableConsulDNS: c.globalEnabled, EnableTransparentProxy: true}
os.Setenv(dnsServiceHostEnvSuffix, "10.0.34.16")
defer os.Unsetenv(dnsServiceHostEnvSuffix)
h := Handler{EnableConsulDNS: c.globalEnabled, EnableTransparentProxy: true, ResourcePrefix: "consul-consul"}
os.Setenv("CONSUL_CONSUL_DNS_SERVICE_HOST", "10.0.34.16")
defer os.Unsetenv("CONSUL_CONSUL_DNS_SERVICE_HOST")

pod := minimal()
pod.Annotations = c.annotations
Expand All @@ -393,6 +393,33 @@ func TestHandlerContainerInit_consulDNS(t *testing.T) {
}
}

func TestHandler_constructDNSServiceHostName(t *testing.T) {
cases := []struct {
prefix string
result string
}{
{
prefix: "consul-consul",
result: "CONSUL_CONSUL_DNS_SERVICE_HOST",
},
{
prefix: "release",
result: "RELEASE_DNS_SERVICE_HOST",
},
{
prefix: "consul-dc1",
result: "CONSUL_DC1_DNS_SERVICE_HOST",
},
}

for _, c := range cases {
t.Run(c.prefix, func(t *testing.T) {
h := Handler{ResourcePrefix: c.prefix}
require.Equal(t, c.result, h.constructDNSServiceHostName())
})
}
}

func TestHandlerContainerInit_namespacesAndPartitionsEnabled(t *testing.T) {
minimal := func() *corev1.Pod {
return &corev1.Pod{
Expand Down
4 changes: 4 additions & 0 deletions control-plane/connect-inject/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,10 @@ type Handler struct {
// from mesh services.
EnableConsulDNS bool

// ResourcePrefix is the prefix used for the installation which is used to determine the Service
// name of the Consul DNS service.
ResourcePrefix string

// EnableOpenShift indicates that when tproxy is enabled, the security context for the Envoy and init
// containers should not be added because OpenShift sets a random user for those and will not allow
// those containers to be created otherwise.
Expand Down
4 changes: 4 additions & 0 deletions control-plane/subcommand/inject-connect/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ type Command struct {

// Consul DNS flags.
flagEnableConsulDNS bool
flagResourcePrefix string

flagEnableOpenShift bool

Expand Down Expand Up @@ -166,6 +167,8 @@ func (c *Command) init() {
"Overwrite Kubernetes probes to point to Envoy by default when in Transparent Proxy mode.")
c.flagSet.BoolVar(&c.flagEnableConsulDNS, "enable-consul-dns", false,
"Enables Consul DNS lookup for services in the mesh.")
c.flagSet.StringVar(&c.flagResourcePrefix, "resource-prefix", "",
"Release prefix of the Consul installation used to determine Consul DNS Service name.")
c.flagSet.BoolVar(&c.flagEnableOpenShift, "enable-openshift", false,
"Indicates that the command runs in an OpenShift cluster.")
c.flagSet.StringVar(&c.flagLogLevel, "log-level", zapcore.InfoLevel.String(),
Expand Down Expand Up @@ -477,6 +480,7 @@ func (c *Command) Run(args []string) int {
EnableTransparentProxy: c.flagDefaultEnableTransparentProxy,
TProxyOverwriteProbes: c.flagTransparentProxyDefaultOverwriteProbes,
EnableConsulDNS: c.flagEnableConsulDNS,
ResourcePrefix: c.flagResourcePrefix,
EnableOpenShift: c.flagEnableOpenShift,
Log: ctrl.Log.WithName("handler").WithName("connect"),
LogLevel: c.flagLogLevel,
Expand Down
136 changes: 0 additions & 136 deletions demo.yaml

This file was deleted.

0 comments on commit 7a4ab00

Please sign in to comment.