Skip to content

Commit

Permalink
Fix/restrict rpaas shell volumes (#164)
Browse files Browse the repository at this point in the history 
* chore: set debug-image default as tsuru/netshoot

* fix: configure Exec command to use ephemeral containers with less resources

* ref: set 'command' as a Common Terminal Arg

* feat: load volume points on ephemeral container avoiding 'certs' mountpoint

* feat: feature flag rpaas shell on ephemeral container

* feat: use default name for debug container so we only create one ephemeral container

* chore: remove deadline from .golangci.yml

* chore: remove gover check-shadowing options

* test: set default debug image on config_test

* fix: use a more restrictive condition to filter certs volume
  • Loading branch information
@ravilock
ravilock authored Feb 19, 2025
1 parent edc5274 commit df593a4
Show file tree
Hide file tree
Showing 8 changed files with 130 additions and 67 deletions.
4 changes: 0 additions & 4 deletions .golangci.yml
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,7 @@
run:
deadline: 5m
tests: true

linters-settings:
govet:
check-shadowing: true

gofmt:
simplify: true

Expand Down
3 changes: 2 additions & 1 deletion internal/config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,8 @@ func Init() error {
viper.SetDefault("enable-cert-manager", false)
viper.SetDefault("new-instance-replicas", 1)
viper.SetDefault("forbidden-annotations-prefixes", []string{"rpaas.extensions.tsuru.io", "afh.tsuru.io"})
viper.SetDefault("debug-image", "")
viper.SetDefault("debug-image", "tsuru/netshoot")
viper.SetDefault("feature-flag-ephemeral-container-shell", false)
viper.AutomaticEnv()
err := readConfig()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions internal/config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ forbidden-annotations-prefixes:
WebSocketWriteWait: time.Second,
NewInstanceReplicas: 1,
ForbiddenAnnotationsPrefixes: []string{"rpaas.extensions.tsuru.io", "afh.tsuru.io"},
DebugImage: "tsuru/netshoot",
}
if tt.expected != nil {
expected = tt.expected(expected)
Expand Down
86 changes: 60 additions & 26 deletions internal/pkg/rpaas/k8s.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
jsonpatch "github.com/evanphx/json-patch/v5"
"github.com/hashicorp/go-multierror"
"github.com/sirupsen/logrus"
"github.com/spf13/viper"
nginxv1alpha1 "github.com/tsuru/nginx-operator/api/v1alpha1"
nginxk8s "github.com/tsuru/nginx-operator/pkg/k8s"
corev1 "k8s.io/api/core/v1"
Expand All @@ -40,7 +41,6 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/httpstream/spdy"
utilrand "k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/strategicpatch"
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/apimachinery/pkg/watch"
Expand Down Expand Up @@ -81,7 +81,6 @@ const (
)

var _ RpaasManager = &k8sRpaasManager{}
var nameSuffixFunc = utilrand.String

var podAllowedReasonsToFail = map[string]bool{
"shutdown": true,
Expand Down Expand Up @@ -148,7 +147,7 @@ func (q *fixedSizeQueue) Next() *remotecommand.TerminalSize {
}

func (m *k8sRpaasManager) Debug(ctx context.Context, instanceName string, args DebugArgs) error {
instance, debugContainerName, status, err := m.debugPodWithContainerStatus(ctx, &args, instanceName)
instance, debugContainerName, status, err := m.debugPodWithContainerStatus(ctx, &args.CommonTerminalArgs, args.Image, instanceName)
if err != nil {
return err
}
Expand Down Expand Up @@ -180,18 +179,18 @@ func (m *k8sRpaasManager) Debug(ctx context.Context, instanceName string, args D
return executorStream(args.CommonTerminalArgs, executor, ctx)
}

func (m *k8sRpaasManager) debugPodWithContainerStatus(ctx context.Context, args *DebugArgs, instanceName string) (*v1alpha1.RpaasInstance, string, *corev1.ContainerStatus, error) {
if args.Image == "" && config.Get().DebugImage == "" {
func (m *k8sRpaasManager) debugPodWithContainerStatus(ctx context.Context, args *CommonTerminalArgs, image string, instanceName string) (*v1alpha1.RpaasInstance, string, *corev1.ContainerStatus, error) {
if image == "" && config.Get().DebugImage == "" {
return nil, "", nil, ValidationError{Msg: "Debug image not set and no default image configured"}
}
if args.Image == "" {
args.Image = config.Get().DebugImage
if image == "" {
image = config.Get().DebugImage
}
instance, err := m.checkPodOnInstance(ctx, instanceName, &args.CommonTerminalArgs)
instance, err := m.checkPodOnInstance(ctx, instanceName, args)
if err != nil {
return nil, "", nil, err
}
debugContainerName, err := m.generateDebugContainer(ctx, args, instance)
debugContainerName, err := m.getDebugContainer(ctx, args, image, instance)
if err != nil {
return nil, "", nil, err
}
Expand All @@ -206,36 +205,54 @@ func (m *k8sRpaasManager) debugPodWithContainerStatus(ctx context.Context, args
return instance, debugContainerName, status, nil
}

func (m *k8sRpaasManager) generateDebugContainer(ctx context.Context, args *DebugArgs, instance *v1alpha1.RpaasInstance) (string, error) {
func removeCertVolumeMounts(volumeMounts []corev1.VolumeMount) []corev1.VolumeMount {
var result []corev1.VolumeMount
for _, vm := range volumeMounts {
if !strings.HasPrefix(vm.MountPath, "/etc/nginx/certs") {
result = append(result, vm)
}
}
return result
}

func (m *k8sRpaasManager) getDebugContainer(ctx context.Context, args *CommonTerminalArgs, image string, instance *v1alpha1.RpaasInstance) (string, error) {
instancePod := corev1.Pod{}
err := m.cli.Get(ctx, types.NamespacedName{Name: args.Pod, Namespace: instance.Namespace}, &instancePod)
if err != nil {
return "", err
}
debugContainerName := fmt.Sprintf("debugger-%s", nameSuffixFunc(5))
debugContainerName := "tsuru-debugger"
if ok := doesEphemeralContainerExist(&instancePod, debugContainerName); ok {
return debugContainerName, nil
}
rpaasInstanceVolumeMounts := removeCertVolumeMounts(instance.Spec.PodTemplate.VolumeMounts)
debugContainer := &corev1.EphemeralContainer{
EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: debugContainerName,
Command: args.Command,
Image: args.Image,
Image: image,
ImagePullPolicy: corev1.PullIfNotPresent,
Stdin: args.Stdin != nil,
TTY: args.TTY,
VolumeMounts: []corev1.VolumeMount{
{
Name: "nginx-config",
MountPath: "/etc/nginx",
ReadOnly: true,
},
},
VolumeMounts: rpaasInstanceVolumeMounts,
}, TargetContainerName: args.Container,
}
instancePodWithDebug := instancePod.DeepCopy()
instancePodWithDebug.Spec.EphemeralContainers = append(instancePod.Spec.EphemeralContainers, *debugContainer)
instancePodWithDebug.Spec.EphemeralContainers = append([]corev1.EphemeralContainer{}, instancePod.Spec.EphemeralContainers...)
instancePodWithDebug.Spec.EphemeralContainers = append([]corev1.EphemeralContainer{}, *debugContainer)
err = m.patchEphemeralContainers(ctx, instancePodWithDebug, instancePod)
return debugContainerName, err
}

func doesEphemeralContainerExist(pod *corev1.Pod, debugContainerName string) bool {
for _, ephemeralContainer := range pod.Spec.EphemeralContainers {
if ephemeralContainer.Name == debugContainerName {
return true
}
}
return false
}

func (m *k8sRpaasManager) patchEphemeralContainers(ctx context.Context, instancePodWithDebug *corev1.Pod, instancePod corev1.Pod) error {
podJS, err := json.Marshal(instancePod)
if err != nil {
Expand Down Expand Up @@ -311,9 +328,26 @@ func getContainerStatusByName(pod *corev1.Pod, containerName string) *corev1.Con
}

func (m *k8sRpaasManager) Exec(ctx context.Context, instanceName string, args ExecArgs) error {
instance, err := m.checkPodOnInstance(ctx, instanceName, &args.CommonTerminalArgs)
if err != nil {
return err
var rpaasInstance *v1alpha1.RpaasInstance
var execContainerName string
if viper.GetBool("feature-flag-ephemeral-container-shell") {
instance, debugContainerName, status, err := m.debugPodWithContainerStatus(ctx, &args.CommonTerminalArgs, "", instanceName)
if err != nil {
return err
}
if status.State.Terminated != nil {
req := m.kcs.CoreV1().Pods(instance.Namespace).GetLogs(args.Pod, &corev1.PodLogOptions{Container: execContainerName})
return logs.DefaultConsumeRequest(req, args.Stdout)
}
rpaasInstance = instance
execContainerName = debugContainerName
} else {
instance, err := m.checkPodOnInstance(ctx, instanceName, &args.CommonTerminalArgs)
if err != nil {
return err
}
rpaasInstance = instance
execContainerName = args.Container
}

req := m.kcs.
Expand All @@ -322,10 +356,10 @@ func (m *k8sRpaasManager) Exec(ctx context.Context, instanceName string, args Ex
Post().
Resource("pods").
Name(args.Pod).
Namespace(instance.Namespace).
Namespace(rpaasInstance.Namespace).
SubResource("exec").
VersionedParams(&corev1.PodExecOptions{
Container: args.Container,
Container: execContainerName,
Command: args.Command,
Stdin: args.Stdin != nil,
Stdout: true,
Expand Down Expand Up @@ -682,7 +716,7 @@ func (m *k8sRpaasManager) GetCertificates(ctx context.Context, instanceName stri
nginx = &nginxv1alpha1.Nginx{}
}

var certMap = make(map[string]clientTypes.CertificateInfo)
certMap := make(map[string]clientTypes.CertificateInfo)
allTLS := append([]nginxv1alpha1.NginxTLS{}, instance.Spec.TLS...)
allTLS = append(allTLS, nginx.Spec.TLS...)

Expand Down
92 changes: 62 additions & 30 deletions internal/pkg/rpaas/k8s_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,7 @@ x2cJyBkkBQV9WB34oGtnZzQ0nKtzsY6FVlNGSeyCJ3OD2dHXO5komJY=
}
}

func newEmptyRpaasInstance() *v1alpha1.RpaasInstance {
func newEmptyRpaasInstance(volumeMounts ...corev1.VolumeMount) *v1alpha1.RpaasInstance {
return &v1alpha1.RpaasInstance{
TypeMeta: metav1.TypeMeta{
APIVersion: "extensions.tsuru.io/v1alpha1",
Expand All @@ -1070,7 +1070,11 @@ func newEmptyRpaasInstance() *v1alpha1.RpaasInstance {
Name: "my-instance",
Namespace: getServiceName(),
},
Spec: v1alpha1.RpaasInstanceSpec{},
Spec: v1alpha1.RpaasInstanceSpec{
PodTemplate: nginxv1alpha1.NginxPodTemplateSpec{
VolumeMounts: volumeMounts,
},
},
}
}

Expand Down Expand Up @@ -4364,7 +4368,8 @@ func Test_k8sRpaasManager_GetInstanceInfo(t *testing.T) {
Name: "my-instance-service",
Namespace: "rpaasv2",
Annotations: map[string]string{
"external-dns.alpha.kubernetes.io/hostname": "my-instance.zone1.tld"},
"external-dns.alpha.kubernetes.io/hostname": "my-instance.zone1.tld",
},
},
Spec: corev1.ServiceSpec{
Type: corev1.ServiceTypeLoadBalancer,
Expand Down Expand Up @@ -5268,22 +5273,50 @@ func Test_k8sRpaasManager_GetAccessControlList(t *testing.T) {
}

func Test_k8sRpaasManager_Debug(t *testing.T) {
defer func(old func(int) string) { nameSuffixFunc = old }(nameSuffixFunc)
var suffixCounter int
nameSuffixFunc = func(int) string {
suffixCounter++
return fmt.Sprint(suffixCounter)
volumeMounts := []corev1.VolumeMount{
{
Name: "certs-test",
ReadOnly: true,
MountPath: "/etc/nginx/certs/",
},
{
Name: "extra-files-0",
MountPath: "/etc/nginx/extra_files/waf.cfg",
SubPath: "waf.cfg",
ReadOnly: true,
},
{
Name: "extra-files-1",
MountPath: "/etc/nginx/extra_files/binary.exe",
SubPath: "binary.exe",
ReadOnly: true,
},
}

instance1 := newEmptyRpaasInstance()
expectedVolumeMounts := []corev1.VolumeMount{
{
Name: "extra-files-0",
MountPath: "/etc/nginx/extra_files/waf.cfg",
SubPath: "waf.cfg",
ReadOnly: true,
},
{
Name: "extra-files-1",
MountPath: "/etc/nginx/extra_files/binary.exe",
SubPath: "binary.exe",
ReadOnly: true,
},
}

instance1 := newEmptyRpaasInstance(volumeMounts...)
instance1.ObjectMeta.Name = "instance1"
instance2 := newEmptyRpaasInstance()
instance2 := newEmptyRpaasInstance(volumeMounts...)
instance2.ObjectMeta.Name = "instance2"
instance3 := newEmptyRpaasInstance()
instance3 := newEmptyRpaasInstance(volumeMounts...)
instance3.ObjectMeta.Name = "instance3"
instance4 := newEmptyRpaasInstance()
instance4 := newEmptyRpaasInstance(volumeMounts...)
instance4.ObjectMeta.Name = "instance4"
instance5 := newEmptyRpaasInstance()
instance5 := newEmptyRpaasInstance(volumeMounts...)
instance5.ObjectMeta.Name = "instance5"

nginx1 := &nginxv1alpha1.Nginx{
Expand Down Expand Up @@ -5427,23 +5460,23 @@ func Test_k8sRpaasManager_Debug(t *testing.T) {
args: DebugArgs{CommonTerminalArgs: CommonTerminalArgs{Pod: "pod1", Stdin: &bytes.Buffer{}, Stdout: io.Discard, Stderr: io.Discard}},
pods: func() []corev1.Pod {
pod1Debug := pod1.DeepCopy()
pod1Debug.Spec.EphemeralContainers = append(pod1Debug.Spec.EphemeralContainers, corev1.EphemeralContainer{EphemeralContainerCommon: corev1.EphemeralContainerCommon{Name: "debugger-1"}, TargetContainerName: "nginx"})
pod1Debug.Status.ContainerStatuses = append(pod1Debug.Status.EphemeralContainerStatuses, corev1.ContainerStatus{Name: "debugger-1", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}})
pod1Debug.Spec.EphemeralContainers = append(pod1Debug.Spec.EphemeralContainers, corev1.EphemeralContainer{EphemeralContainerCommon: corev1.EphemeralContainerCommon{Name: "tsuru-debugger"}, TargetContainerName: "nginx"})
pod1Debug.Status.ContainerStatuses = append(pod1Debug.Status.EphemeralContainerStatuses, corev1.ContainerStatus{Name: "tsuru-debugger", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}})
return []corev1.Pod{*pod1Debug}
},
assertion: func(t *testing.T, err error, m *k8sRpaasManager, instance *v1alpha1.RpaasInstance, debugContainerName string, debugContainerStatus *corev1.ContainerStatus) {
assert.NoError(t, err)
assert.Equal(t, "debugger-1", debugContainerName)
assert.Equal(t, corev1.ContainerStatus{Name: "debugger-1", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}}, *debugContainerStatus)
assert.Equal(t, "tsuru-debugger", debugContainerName)
assert.Equal(t, corev1.ContainerStatus{Name: "tsuru-debugger", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}}, *debugContainerStatus)
instancePod := corev1.Pod{}
err = m.cli.Get(context.Background(), types.NamespacedName{Name: "pod1", Namespace: instance2.Namespace}, &instancePod)
require.NoError(t, err)
expectedEphemerals := []corev1.EphemeralContainer{{EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "debugger-1",
Image: "tsuru/debug-image",
Name: "tsuru-debugger",
Image: "tsuru/netshoot",
ImagePullPolicy: corev1.PullIfNotPresent,
Stdin: true,
VolumeMounts: []corev1.VolumeMount{{Name: "nginx-config", MountPath: "/etc/nginx", ReadOnly: true}},
VolumeMounts: expectedVolumeMounts,
}, TargetContainerName: "nginx"}}
assert.Equal(t, expectedEphemerals, instancePod.Spec.EphemeralContainers)
},
Expand All @@ -5454,23 +5487,23 @@ func Test_k8sRpaasManager_Debug(t *testing.T) {
args: DebugArgs{CommonTerminalArgs: CommonTerminalArgs{Stdin: &bytes.Buffer{}, Stdout: io.Discard, Stderr: io.Discard}},
pods: func() []corev1.Pod {
pod1Debug := pod1.DeepCopy()
pod1Debug.Spec.EphemeralContainers = append(pod1Debug.Spec.EphemeralContainers, corev1.EphemeralContainer{EphemeralContainerCommon: corev1.EphemeralContainerCommon{Name: "debugger-2"}, TargetContainerName: "nginx"})
pod1Debug.Status.ContainerStatuses = append(pod1Debug.Status.EphemeralContainerStatuses, corev1.ContainerStatus{Name: "debugger-2", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}})
pod1Debug.Spec.EphemeralContainers = append(pod1Debug.Spec.EphemeralContainers, corev1.EphemeralContainer{EphemeralContainerCommon: corev1.EphemeralContainerCommon{Name: "tsuru-debugger"}, TargetContainerName: "nginx"})
pod1Debug.Status.ContainerStatuses = append(pod1Debug.Status.EphemeralContainerStatuses, corev1.ContainerStatus{Name: "tsuru-debugger", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}})
return []corev1.Pod{*pod1Debug}
},
assertion: func(t *testing.T, err error, m *k8sRpaasManager, instance *v1alpha1.RpaasInstance, debugContainerName string, debugContainerStatus *corev1.ContainerStatus) {
assert.NoError(t, err)
assert.Equal(t, "debugger-2", debugContainerName)
assert.Equal(t, corev1.ContainerStatus{Name: "debugger-2", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}}, *debugContainerStatus)
assert.Equal(t, "tsuru-debugger", debugContainerName)
assert.Equal(t, corev1.ContainerStatus{Name: "tsuru-debugger", Ready: true, State: corev1.ContainerState{Running: &corev1.ContainerStateRunning{StartedAt: staticTimeNow}}}, *debugContainerStatus)
instancePod := corev1.Pod{}
err = m.cli.Get(context.Background(), types.NamespacedName{Name: "pod1", Namespace: instance2.Namespace}, &instancePod)
require.NoError(t, err)
expectedEphemerals := []corev1.EphemeralContainer{{EphemeralContainerCommon: corev1.EphemeralContainerCommon{
Name: "debugger-2",
Image: "tsuru/debug-image",
Name: "tsuru-debugger",
Image: "tsuru/netshoot",
ImagePullPolicy: corev1.PullIfNotPresent,
Stdin: true,
VolumeMounts: []corev1.VolumeMount{{Name: "nginx-config", MountPath: "/etc/nginx", ReadOnly: true}},
VolumeMounts: expectedVolumeMounts,
}, TargetContainerName: "nginx"}}
assert.Equal(t, expectedEphemerals, instancePod.Spec.EphemeralContainers)
},
Expand All @@ -5480,7 +5513,7 @@ func Test_k8sRpaasManager_Debug(t *testing.T) {
for _, tt := range testCases {
cfg := config.Get()
defer func() { config.Set(cfg) }()
config.Set(config.RpaasConfig{DebugImage: "tsuru/debug-image"})
config.Set(config.RpaasConfig{DebugImage: "tsuru/netshoot"})
t.Run(tt.name, func(t *testing.T) {
var wg sync.WaitGroup
manager := &k8sRpaasManager{cli: fake.NewClientBuilder().WithScheme(newScheme()).WithRuntimeObjects(resources...).Build()}
Expand All @@ -5502,7 +5535,7 @@ func Test_k8sRpaasManager_Debug(t *testing.T) {
time.Sleep(1000 * time.Millisecond)
}
}()
rpaasInstance, debugContainerName, debugContainerStatus, err := manager.debugPodWithContainerStatus(ctx, &tt.args, tt.instance)
rpaasInstance, debugContainerName, debugContainerStatus, err := manager.debugPodWithContainerStatus(ctx, &tt.args.CommonTerminalArgs, tt.args.Image, tt.instance)
wg.Wait()
if err != nil {
tt.assertion(t, err, manager, nil, "", nil)
Expand All @@ -5511,5 +5544,4 @@ func Test_k8sRpaasManager_Debug(t *testing.T) {
}
})
}

}
Loading

0 comments on commit df593a4

Please sign in to comment.