Skip to content

Commit

Permalink
Fix incorrect proxy ConfigMap name
Browse files Browse the repository at this point in the history 
The mitmproxy ConfigMap implementation previously assumed that
the Deployment and Service name were identical. While this is typically
true in "simple" deployments (such as our testbed, which I will update
at some point to add a test for this), it is not true for complex
deployments.

This is a hotfix, and this fix should be revisited in a future revision
for a more complete fix, including tests and code cleanups.

This fixes #1.
  • Loading branch information
Matt Hamilton committed Jul 7, 2020
1 parent 53f3ed2 commit 3753acc
Show file tree
Hide file tree
Showing 4 changed files with 68 additions and 44 deletions.
14 changes: 7 additions & 7 deletions cmd/kubectl-tap/mitmproxy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ func (m *Mitmproxy) ReadyEnv() error {
// delete the configmap and try again.
// This is mostly here to fix development environments that become broken during
// code testing.
_ = destroyMitmproxyConfigMap(configmapsClient, m.ProxyOpts.Target)
_ = destroyMitmproxyConfigMap(configmapsClient, m.ProxyOpts.dplName)
rErr := createMitmproxyConfigMap(configmapsClient, m.ProxyOpts)
if rErr != nil {
if errors.Is(os.ErrInvalid, rErr) {
Expand All @@ -175,7 +175,7 @@ func (m *Mitmproxy) ReadyEnv() error {
// UnreadyEnv removes tap supporting configmap.
func (m *Mitmproxy) UnreadyEnv() error {
configmapsClient := m.Client.CoreV1().ConfigMaps(m.ProxyOpts.Namespace)
return destroyMitmproxyConfigMap(configmapsClient, m.ProxyOpts.Target)
return destroyMitmproxyConfigMap(configmapsClient, m.ProxyOpts.dplName)
}

// createMitmproxyConfigMap creates a mitmproxy configmap based on the proxy mode, however currently
Expand Down Expand Up @@ -210,10 +210,10 @@ func createMitmproxyConfigMap(configmapClient corev1.ConfigMapInterface, proxyOp
cmData[mitmproxyConfigFile] = mitmproxyConfig
cm := v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: kubetapConfigMapPrefix + proxyOpts.Target,
Name: kubetapConfigMapPrefix + proxyOpts.dplName,
Namespace: proxyOpts.Namespace,
Annotations: map[string]string{
annotationConfigMap: configMapAnnotationPrefix + proxyOpts.Target,
annotationConfigMap: configMapAnnotationPrefix + proxyOpts.dplName,
},
},
BinaryData: cmData,
Expand All @@ -237,8 +237,8 @@ func createMitmproxyConfigMap(configmapClient corev1.ConfigMapInterface, proxyOp
}

// destroyMitmproxyConfigMap removes a mitmproxy ConfigMap from the environment.
func destroyMitmproxyConfigMap(configmapClient corev1.ConfigMapInterface, serviceName string) error {
if serviceName == "" {
func destroyMitmproxyConfigMap(configmapClient corev1.ConfigMapInterface, deploymentName string) error {
if deploymentName == "" {
return os.ErrInvalid
}
cms, err := configmapClient.List(context.TODO(), metav1.ListOptions{})
Expand All @@ -252,7 +252,7 @@ func destroyMitmproxyConfigMap(configmapClient corev1.ConfigMapInterface, servic
continue
}
for k, v := range anns {
if k == annotationConfigMap && v == configMapAnnotationPrefix+serviceName {
if k == annotationConfigMap && v == configMapAnnotationPrefix+deploymentName {
targetConfigMapNames = append(targetConfigMapNames, cm.Name)
}
}
Expand Down
16 changes: 8 additions & 8 deletions cmd/kubectl-tap/mitmproxy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,22 @@ import (

func Test_DestroyMitmproxyConfigMap(t *testing.T) {
tests := []struct {
Name string
ServiceName string
ClientFunc func() *fake.Clientset
Err error
Name string
DeploymentName string
ClientFunc func() *fake.Clientset
Err error
}{
{"simple", "sample-service", fakeClientTappedSimple, nil},
{"untapped", "sample-service", fakeClientUntappedSimple, ErrConfigMapNoMatch},
{"simple", "sample-deployment", fakeClientTappedSimple, nil},
{"untapped", "sample-deployment", fakeClientUntappedSimple, ErrConfigMapNoMatch},
{"no_svc_name", "", fakeClientTappedSimple, os.ErrInvalid},
{"missing_annotations", "sample-service", fakeClientTappedWithoutAnnotations, ErrConfigMapNoMatch},
{"missing_annotations", "sample-deployment", fakeClientTappedWithoutAnnotations, ErrConfigMapNoMatch},
}
for _, tc := range tests {
t.Run(tc.Name, func(t *testing.T) {
require := require.New(t)
fakeClient := tc.ClientFunc()
cmClient := fakeClient.CoreV1().ConfigMaps("default")
err := destroyMitmproxyConfigMap(cmClient, tc.ServiceName)
err := destroyMitmproxyConfigMap(cmClient, tc.DeploymentName)
if tc.Err != nil {
require.NotNil(err)
require.True(errors.Is(err, tc.Err))
Expand Down
69 changes: 46 additions & 23 deletions cmd/kubectl-tap/tap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,23 @@ type Tap interface {

// ProxyOptions are options used to configure the Tap implementation.
type ProxyOptions struct {
Target string `json:"target"`
Protocol Protocol `json:"protocol"`
UpstreamHTTPS bool `json:"upstream_https"`
UpstreamPort string `json:"upstream_port"`
Mode string `json:"mode"`
Namespace string `json:"namespace"`
Image string `json:"image"`
// Target is the target Service
Target string `json:"target"`
// Protocol is the protocol type, one of [http, https]
Protocol Protocol `json:"protocol"`
// UpstreamHTTPS should be set to true if the target is using HTTPS
UpstreamHTTPS bool `json:"upstream_https"`
// UpstreamPort is the listening port for the target Service
UpstreamPort string `json:"upstream_port"`
// Mode is the proxy mode. Only "reverse" is currently supported.
Mode string `json:"mode"`
// Namespace is the namespace that the Service and Deployment are in
Namespace string `json:"namespace"`
// Image is the proxy image to deploy as a sidecar
Image string `json:"image"`

// dplName tracks the current deployment target
dplName string
}

// NewListCommand lists Services that are already tapped.
Expand Down Expand Up @@ -250,13 +260,15 @@ func NewTapCommand(client kubernetes.Interface, config *rest.Config, viper *vipe
}
// if named, must determine port from deployment spec
if ports.TargetPort.Type == intstr.String {
dp, err := deploymentFromSelectors(deploymentsClient, targetService.Spec.Selector)
var err error
targetDpl, err := deploymentFromSelectors(deploymentsClient, targetService.Spec.Selector)
if err != nil {
return fmt.Errorf("error resolving Deployment from Service selectors: %w", err)
return fmt.Errorf("error resolving Deployment from Service selectors while setting proxy ports: %w", err)
}
for _, c := range dp.Spec.Template.Spec.Containers {
for _, c := range targetDpl.Spec.Template.Spec.Containers {
for _, p := range c.Ports {
if p.Name == ports.TargetPort.String() {
// Set the upstream (target) Service port
proxyOpts.UpstreamPort = strconv.Itoa(int(p.ContainerPort))
}
}
Expand All @@ -267,6 +279,14 @@ func NewTapCommand(client kubernetes.Interface, config *rest.Config, viper *vipe
}
}

targetDpl, err := deploymentFromSelectors(deploymentsClient, targetService.Spec.Selector)
if err != nil {
return fmt.Errorf("error resolving Deployment from Service selectors: %w", err)
}
proxyOpts.dplName = targetDpl.Name
// Save the target Deployment name to anchor the ConfigMap
// to the Deployment.

// Get a proxy based on the protocol type
var proxy Tap
switch Protocol(protocol) {
Expand All @@ -292,6 +312,8 @@ func NewTapCommand(client kubernetes.Interface, config *rest.Config, viper *vipe
if err != nil {
return err
}
// set the Deployment name so the configmap tracks a specific Deployment

sidecar := proxy.Sidecar(dpl.Name)
sidecar.Image = image
sidecar.Args = commandArgs
Expand Down Expand Up @@ -490,19 +512,6 @@ func NewUntapCommand(client kubernetes.Interface, viper *viper.Viper) func(*cobr
deploymentsClient := client.AppsV1().Deployments(namespace)
servicesClient := client.CoreV1().Services(namespace)

//TODO cleanup
// for the cleanup, we only need to provide the namespace. The fact that I have to write
// this probably means it needs to be refactored again later.
proxy := NewMitmproxy(client, ProxyOptions{
Namespace: namespace,
Target: targetSvcName,
})
if err := proxy.UnreadyEnv(); err != nil {
if !errors.Is(ErrConfigMapNoMatch, err) {
return err
}
}

targetService, err := client.CoreV1().Services(namespace).Get(context.TODO(), targetSvcName, metav1.GetOptions{})
if err != nil {
return err
Expand All @@ -514,6 +523,20 @@ func NewUntapCommand(client kubernetes.Interface, viper *viper.Viper) func(*cobr
if dpl.Namespace != namespace {
panic(ErrDeploymentOutsideNamespace)
}

proxy := NewMitmproxy(client, ProxyOptions{
Namespace: namespace,
Target: targetSvcName,
dplName: dpl.Name,
})

if err := proxy.UnreadyEnv(); err != nil {
// both error types below can be thrown
if !errors.Is(ErrConfigMapNoMatch, err) {
return err
}
}

retryErr := retry.RetryOnConflict(retry.DefaultRetry, func() error {
// Explicitly re-fetch the deployment to reduce the chance of having a race
deployment, getErr := deploymentsClient.Get(context.TODO(), dpl.Name, metav1.GetOptions{})
Expand Down
13 changes: 7 additions & 6 deletions cmd/kubectl-tap/tap_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ func Test_NewTapCommand(t *testing.T) {
cmd.SetOutput(ioutil.Discard)
err := NewTapCommand(fakeClient, &rest.Config{}, testViper)(cmd, []string{"sample-service"})
if tc.Err != nil {
require.NotNil(err)
require.True(errors.Is(err, tc.Err))
if !errors.Is(err, tc.Err) {
t.Fatalf("expected (%q), got (%q)", tc.Err, err)
}
} else {
// sanity checks
require.Nil(err)
Expand All @@ -90,7 +91,7 @@ func Test_NewTapCommand(t *testing.T) {
require.GreaterOrEqual(len(c.Ports), 2, "tap port was not added to the deployment")
}
// configmap checks
fakeCM, err := fakeClient.CoreV1().ConfigMaps(testViper.GetString("namespace")).Get(context.TODO(), kubetapConfigMapPrefix+"sample-service", metav1.GetOptions{})
fakeCM, err := fakeClient.CoreV1().ConfigMaps(testViper.GetString("namespace")).Get(context.TODO(), kubetapConfigMapPrefix+fakeDeployment.Name, metav1.GetOptions{})
require.Nil(err)
require.NotNil(fakeCM)
require.True(strings.Contains(fakeCM.Name, kubetapConfigMapPrefix))
Expand Down Expand Up @@ -257,7 +258,7 @@ var (
},
Volumes: []v1.Volume{
{
Name: kubetapConfigMapPrefix + "sample-service",
Name: kubetapConfigMapPrefix + "sample-deployment",
},
},
},
Expand Down Expand Up @@ -294,10 +295,10 @@ var (

simpleConfigMapTapped = v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: kubetapConfigMapPrefix + "sample-service",
Name: kubetapConfigMapPrefix + "sample-deployment",
Namespace: "default",
Annotations: map[string]string{
annotationConfigMap: configMapAnnotationPrefix + "sample-service",
annotationConfigMap: configMapAnnotationPrefix + "sample-deployment",
},
},
}
Expand Down

0 comments on commit 3753acc

Please sign in to comment.