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.

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

Overwrite k8s HTTP probes when transparent proxy is enabled. #517

Merged
merged 5 commits into from
May 20, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
## UNRELEASED

IMPROVEMENTS:
* Connect: Overwrite Kubernetes HTTP readiness and/or liveness probes to point to Envoy proxy when
transparent proxy is enabled. [[GH-517](https://github.com/hashicorp/consul-k8s/pull/517)]

## 0.26.0-beta2 (May 06, 2021)

BREAKING CHANGES:
Expand Down
22 changes: 21 additions & 1 deletion connect-inject/annotations.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ const (
// keyTransparentProxy enables or disables transparent proxy for a given pod. It can also be set as a label
// on a namespace to define the default behaviour for connect-injected pods which do not otherwise override this setting
// with their own annotation.
// This annotation takes a boolean value (true/false).
// This annotation/label takes a boolean value (true/false).
keyTransparentProxy = "consul.hashicorp.com/transparent-proxy"

// annotationTProxyExcludeInboundPorts is a comma-separated list of inbound ports to exclude from traffic redirection.
Expand All @@ -108,6 +108,26 @@ const (
// annotationTProxyExcludeUIDs is a comma-separated list of additional user IDs to exclude from traffic redirection.
annotationTProxyExcludeUIDs = "consul.hashicorp.com/transparent-proxy-exclude-uids"

// annotationTransparentProxyOverwriteProbes controls whether the Kubernetes probes should be overwritten
// to point to the Envoy proxy when running in Transparent Proxy mode.
annotationTransparentProxyOverwriteProbes = "consul.hashicorp.com/transparent-proxy-overwrite-probes"

// annotationTransparentProxyReadinessListenerPort is the port for the readiness probe
// that we will expose through Envoy when overwrite probes is enabled.
annotationTransparentProxyReadinessListenerPort = "consul.hashicorp.com/transparent-proxy-readiness-listener-port"

// annotationTransparentProxyLivenessListenerPort is the port for the liveness probe
// that we will expose through Envoy when overwrite probes is enabled.
annotationTransparentProxyLivenessListenerPort = "consul.hashicorp.com/transparent-proxy-liveness-listener-port"

// annotationOriginalLivenessProbePort is the value of the port originally defined on the liveness probe
// of the pod before we overwrote it.
annotationOriginalLivenessProbePort = "consul.hashicorp.com/original-liveness-probe-port"

// annotationOriginalReadinessProbePort is the value of the port originally defined on the readiness probe
// of the pod before we overwrote it.
annotationOriginalReadinessProbePort = "consul.hashicorp.com/original-readiness-probe-port"

// injected is used as the annotation value for annotationInjected.
injected = "injected"

Expand Down
53 changes: 52 additions & 1 deletion connect-inject/endpoints_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"fmt"
"net"
"strconv"
"strings"

"github.com/deckarep/golang-set"
Expand Down Expand Up @@ -39,6 +40,14 @@ const (
// in Consul. Note: This value should not be changed without a corresponding change in Consul.
// TODO: change this to a constant shared with Consul to avoid accidentally changing this.
clusterIPTaggedAddressName = "virtual"

// defaultExposedPathsListenerPortLiveness is the default port that we will use as the ListenerPort
// for the Expose configuration of the proxy registration for a liveness probe.
defaultExposedPathsListenerPortLiveness = 20300

// defaultExposedPathsListenerPortReadiness is the default port that we will use as the ListenerPort
// for the Expose configuration of the proxy registration for a readiness probe.
defaultExposedPathsListenerPortReadiness = 20301
)

type EndpointsController struct {
Expand Down Expand Up @@ -82,11 +91,14 @@ type EndpointsController struct {
// EnableTransparentProxy controls whether transparent proxy should be enabled
// for all proxy service registrations.
EnableTransparentProxy bool
// TProxyOverwriteProbes controls whether the endpoints controller should expose pod's HTTP probes
// via Envoy proxy.
TProxyOverwriteProbes bool

MetricsConfig MetricsConfig
Log logr.Logger
Scheme *runtime.Scheme

Scheme *runtime.Scheme
context.Context
}

Expand Down Expand Up @@ -527,6 +539,45 @@ func (r *EndpointsController) createServiceRegistrations(pod corev1.Pod, service
} else {
r.Log.Info("skipping syncing service cluster IP to Consul", "name", k8sService.Name, "ns", k8sService.Namespace, "ip", k8sService.Spec.ClusterIP)
}

// Expose k8s probes as Envoy listeners if needed.
overwriteProbes, err := shouldOverwriteProbes(pod, r.TProxyOverwriteProbes)
if err != nil {
return nil, nil, err
}
if overwriteProbes {
if cs := pod.Spec.Containers; len(cs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know for service name and service port we would look at the first container name/port in the past, but those were also overrideable by configuration. In this case, there's no configuration to pick which container is the app container to rewrite the healthcheck. Is it alright to assume here that it should be the first container?

I think yes because we don't support multi-port services and because it's not a limiting requirement to the app dev that the first container is their main app since there isn't an order the containers will run in. But just wanted to voice it out loud in case you thought of something along these lines as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, that's a really good point. Thanks for bringing this up!

You're right that with a port you could still choose a port from any container, but now we will always overwrite probes for the first container. Thinking about it, I think we could make it work and use the same container to overwrite probes as the container that has the port from the provided port annotation. That will probably be the most accurate, but I'll do it a separate PR. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that makes sense-- if the port annotation exists, look for the container with a port that matches the annotation, else use the first container? Separate PR totally makes sense as this PR is valuable on its own!

appContainer := cs[0]
if appContainer.LivenessProbe != nil && appContainer.LivenessProbe.HTTPGet != nil {
if raw, ok := pod.Annotations[annotationOriginalLivenessProbePort]; ok {
originalLivenessPort, err := strconv.Atoi(raw)
if err != nil {
return nil, nil, err
}

proxyConfig.Expose.Paths = append(proxyConfig.Expose.Paths, api.ExposePath{
ListenerPort: appContainer.LivenessProbe.HTTPGet.Port.IntValue(),
LocalPathPort: originalLivenessPort,
Path: appContainer.LivenessProbe.HTTPGet.Path,
})
}
}
if appContainer.ReadinessProbe != nil && appContainer.ReadinessProbe.HTTPGet != nil {
if raw, ok := pod.Annotations[annotationOriginalReadinessProbePort]; ok {
originalReadinessPort, err := strconv.Atoi(raw)
if err != nil {
return nil, nil, err
}

proxyConfig.Expose.Paths = append(proxyConfig.Expose.Paths, api.ExposePath{
ListenerPort: appContainer.ReadinessProbe.HTTPGet.Port.IntValue(),
LocalPathPort: originalReadinessPort,
Path: appContainer.ReadinessProbe.HTTPGet.Path,
})
}
}
}
}
}

return service, proxyService, nil
Expand Down
Loading