Skip to content

Commit dfc4a73

Browse files
akurinnoyakurinnoy
committed
fixup! fixup! fixes
Signed-off-by: Oleksii Kurinnyi <[email protected]>
1 parent 8f04299 commit dfc4a73

File tree

5 files changed

+156
-28
lines changed

5 files changed

+156
-28
lines changed

‎controllers/workspace/devworkspace_controller.go‎

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package controllers
1818
import (
1919
"context"
2020
"fmt"
21+
"regexp"
2122
"strconv"
2223
"strings"
2324
"time"
@@ -127,8 +128,8 @@ func validateNoAdvancedFields(c corev1.Container) error {
127128

128129
// validateHomeInitContainer validates all aspects of the init-persistent-home container.
129130
func validateHomeInitContainer(c corev1.Container) error {
130-
if strings.ContainsAny(c.Image, "\n\r\t ") {
131-
return fmt.Errorf("invalid image reference for %s: image reference contains invalid whitespace characters", constants.HomeInitComponentName)
131+
if err := validateImageReference(c.Image); err != nil {
132+
return fmt.Errorf("invalid image reference for %s: %w", constants.HomeInitComponentName, err)
132133
}
133134

134135
if len(c.Command) != 2 || c.Command[0] != "/bin/sh" || c.Command[1] != "-c" {
@@ -150,6 +151,50 @@ func validateHomeInitContainer(c corev1.Container) error {
150151
return nil
151152
}
152153

154+
func validateImageReference(image string) error {
155+
if image == "" {
156+
return fmt.Errorf("image reference cannot be empty")
157+
}
158+
159+
// whitespace and control characters
160+
if strings.ContainsAny(image, "\n\r\t ") {
161+
return fmt.Errorf("contains invalid whitespace characters")
162+
}
163+
164+
// other control characters
165+
for _, r := range image {
166+
if r < 0x20 || r == 0x7F {
167+
return fmt.Errorf("contains invalid control characters")
168+
}
169+
}
170+
171+
// format: [registry[:port]/]repository[:tag][@digest]
172+
imagePattern := regexp.MustCompile(`^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])*|\[?[0-9a-fA-F:]+]?)(:\d{1,5})?(/[a-zA-Z0-9]([a-zA-Z0-9._/-]*[a-zA-Z0-9])*)*(:[a-zA-Z0-9_.-]+)?(@sha256:[a-f0-9]{64})?$`)
173+
if !imagePattern.MatchString(image) {
174+
return fmt.Errorf("invalid format: should match regex: %s", imagePattern.String())
175+
}
176+
177+
// port range
178+
portMatch := regexp.MustCompile(`:(\d{1,5})(/|:|@|$)`)
179+
matches := portMatch.FindStringSubmatch(image)
180+
if len(matches) > 1 {
181+
port, err := strconv.Atoi(matches[1])
182+
if err != nil {
183+
return fmt.Errorf("invalid port format: %w", err)
184+
}
185+
if port < 1 || port > 65535 {
186+
return fmt.Errorf("invalid port number: %d (must be 1-65535)", port)
187+
}
188+
}
189+
190+
// length check
191+
if len(image) > 4096 {
192+
return fmt.Errorf("length exceeds 4096 characters")
193+
}
194+
195+
return nil
196+
}
197+
153198
// ensureHomeInitContainerFields ensures that an init-persistent-home container has
154199
// the correct Command, Args, and VolumeMounts.
155200
func ensureHomeInitContainerFields(c *corev1.Container) error {

‎controllers/workspace/init_container_validation_test.go‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package controllers
1717

1818
import (
19+
"strings"
1920
"testing"
2021

2122
dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -301,3 +302,49 @@ func TestDefaultAndValidateHomeInitContainer_NoWorkspaceImage(t *testing.T) {
301302
assert.Error(t, err)
302303
assert.Contains(t, err.Error(), "unable to infer workspace image")
303304
}
305+
306+
func TestValidateImageReference(t *testing.T) {
307+
tests := []struct {
308+
name string
309+
image string
310+
expectError bool
311+
errorMsg string
312+
}{
313+
// Valid images
314+
{"simple image", "nginx", false, ""},
315+
{"image with tag", "nginx:latest", false, ""},
316+
{"image with version tag", "nginx:1.21", false, ""},
317+
{"registry image", "docker.io/nginx", false, ""},
318+
{"registry with tag", "docker.io/nginx:latest", false, ""},
319+
{"registry with port", "localhost:5000/nginx", false, ""},
320+
{"registry port with tag", "localhost:5000/nginx:latest", false, ""},
321+
{"multi-level path", "registry.example.com/team/project/app", false, ""},
322+
{"with digest", "nginx@sha256:abc123def4567890abcdef1234567890abcdef1234567890abcdef1234567890", false, ""},
323+
{"full reference", "registry.example.com:8080/team/app:v1.2.3@sha256:abc123def4567890abcdef1234567890abcdef1234567890abcdef1234567890", false, ""},
324+
325+
// Invalid images
326+
{"empty image", "", true, "cannot be empty"},
327+
{"whitespace", "nginx latest", true, "whitespace"},
328+
{"newline", "nginx\nlatest", true, "whitespace"},
329+
{"tab", "nginx\tlatest", true, "whitespace"},
330+
{"control char", "nginx\x00latest", true, "control characters"},
331+
{"invalid port 0", "registry:0/image", true, "port number"},
332+
{"invalid port 65536", "registry:65536/image", true, "port number"},
333+
{"invalid format", "-nginx", true, "invalid format: should match regex: ^([a-zA-Z0-9]([a-zA-Z0-9._-]*[a-zA-Z0-9])*|\\[?[0-9a-fA-F:]+]?)(:\\d{1,5})?(/[a-zA-Z0-9]([a-zA-Z0-9._/-]*[a-zA-Z0-9])*)*(:[a-zA-Z0-9_.-]+)?(@sha256:[a-f0-9]{64})?$"},
334+
{"too long", strings.Repeat("a", 4097), true, "exceeds 4096"},
335+
}
336+
337+
for _, tt := range tests {
338+
t.Run(tt.name, func(t *testing.T) {
339+
err := validateImageReference(tt.image)
340+
if tt.expectError {
341+
assert.Error(t, err)
342+
if tt.errorMsg != "" {
343+
assert.Contains(t, err.Error(), tt.errorMsg)
344+
}
345+
} else {
346+
assert.NoError(t, err, "Image %q should be valid", tt.image)
347+
}
348+
})
349+
}
350+
}

‎pkg/library/home/custom_init_test.go‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ import (
1919
"testing"
2020

2121
dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
22-
attributes "github.com/devfile/api/v2/pkg/attributes"
22+
"github.com/devfile/api/v2/pkg/attributes"
2323
"github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
2424
"github.com/devfile/devworkspace-operator/pkg/common"
2525
"github.com/devfile/devworkspace-operator/pkg/constants"
2626
"github.com/stretchr/testify/assert"
2727
corev1 "k8s.io/api/core/v1"
28-
"k8s.io/utils/pointer"
28+
"k8s.io/utils/ptr"
2929
)
3030

3131
func TestCustomInitPersistentHome(t *testing.T) {
@@ -61,7 +61,7 @@ func TestCustomInitPersistentHome(t *testing.T) {
6161
Config: &v1alpha1.OperatorConfiguration{
6262
Workspace: &v1alpha1.WorkspaceConfig{
6363
PersistUserHome: &v1alpha1.PersistentHomeConfig{
64-
Enabled: pointer.Bool(true),
64+
Enabled: ptr.To(true),
6565
},
6666
InitContainers: []corev1.Container{
6767
{
@@ -100,7 +100,7 @@ func TestCustomInitPersistentHome(t *testing.T) {
100100
Config: &v1alpha1.OperatorConfiguration{
101101
Workspace: &v1alpha1.WorkspaceConfig{
102102
PersistUserHome: &v1alpha1.PersistentHomeConfig{
103-
Enabled: pointer.Bool(true),
103+
Enabled: ptr.To(true),
104104
},
105105
InitContainers: []corev1.Container{
106106
{
@@ -140,8 +140,8 @@ func TestCustomInitPersistentHome(t *testing.T) {
140140
Config: &v1alpha1.OperatorConfiguration{
141141
Workspace: &v1alpha1.WorkspaceConfig{
142142
PersistUserHome: &v1alpha1.PersistentHomeConfig{
143-
Enabled: pointer.Bool(true),
144-
DisableInitContainer: pointer.Bool(false),
143+
Enabled: ptr.To(true),
144+
DisableInitContainer: ptr.To(false),
145145
},
146146
InitContainers: []corev1.Container{},
147147
},
@@ -175,8 +175,8 @@ func TestCustomInitPersistentHome(t *testing.T) {
175175
Config: &v1alpha1.OperatorConfiguration{
176176
Workspace: &v1alpha1.WorkspaceConfig{
177177
PersistUserHome: &v1alpha1.PersistentHomeConfig{
178-
Enabled: pointer.Bool(true),
179-
DisableInitContainer: pointer.Bool(true),
178+
Enabled: ptr.To(true),
179+
DisableInitContainer: ptr.To(true),
180180
},
181181
},
182182
},

‎pkg/library/initcontainers/merge_test.go‎

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ func TestMergeInitContainers(t *testing.T) {
2828
base []corev1.Container
2929
patches []corev1.Container
3030
want []corev1.Container
31-
wantErr bool
3231
}{
3332
{
3433
name: "empty base",
@@ -39,7 +38,6 @@ func TestMergeInitContainers(t *testing.T) {
3938
want: []corev1.Container{
4039
{Name: "new-container", Image: "new-image"},
4140
},
42-
wantErr: false,
4341
},
4442
{
4543
name: "empty patches",
@@ -50,7 +48,6 @@ func TestMergeInitContainers(t *testing.T) {
5048
want: []corev1.Container{
5149
{Name: "base-container", Image: "base-image"},
5250
},
53-
wantErr: false,
5451
},
5552
{
5653
name: "multiple containers",
@@ -69,7 +66,6 @@ func TestMergeInitContainers(t *testing.T) {
6966
{Name: "third", Image: "third-image"},
7067
{Name: "new-container", Image: "new-image"},
7168
},
72-
wantErr: false,
7369
},
7470
{
7571
name: "partial field merge",
@@ -97,19 +93,52 @@ func TestMergeInitContainers(t *testing.T) {
9793
Env: []corev1.EnvVar{{Name: "BASE_VAR", Value: "base-value"}},
9894
},
9995
},
100-
wantErr: false,
96+
},
97+
{
98+
name: "preserve user-configured init-persistent-home content",
99+
base: []corev1.Container{
100+
{
101+
Name: "init-persistent-home",
102+
Image: "workspace-image:latest",
103+
Command: []string{"/bin/sh", "-c"},
104+
Args: []string{"default stow script"},
105+
},
106+
},
107+
patches: []corev1.Container{
108+
{
109+
Name: "init-persistent-home",
110+
Image: "custom-image:latest",
111+
Args: []string{"echo 'custom init'"},
112+
Env: []corev1.EnvVar{
113+
{
114+
Name: "CUSTOM_VAR",
115+
Value: "custom-value",
116+
},
117+
},
118+
},
119+
},
120+
want: []corev1.Container{
121+
{
122+
Name: "init-persistent-home",
123+
Image: "custom-image:latest",
124+
Command: []string{"/bin/sh", "-c"},
125+
Args: []string{"echo 'custom init'"},
126+
Env: []corev1.EnvVar{
127+
{
128+
Name: "CUSTOM_VAR",
129+
Value: "custom-value",
130+
},
131+
},
132+
},
133+
},
101134
},
102135
}
103136

104137
for _, tt := range tests {
105138
t.Run(tt.name, func(t *testing.T) {
106139
got, err := MergeInitContainers(tt.base, tt.patches)
107-
if tt.wantErr {
108-
assert.Error(t, err, "should return error")
109-
} else {
110-
assert.NoError(t, err, "should not return error")
111-
assert.Equal(t, tt.want, got, "should return merged containers")
112-
}
140+
assert.NoError(t, err, "should not return error")
141+
assert.Equal(t, tt.want, got, "should return merged containers")
113142
})
114143
}
115144
}

‎test/e2e/pkg/client/oc.go‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,15 @@
1616
package client
1717

1818
import (
19+
"context"
1920
"fmt"
2021
"log"
2122
"os/exec"
2223
"strings"
2324
"time"
25+
26+
dw "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
27+
k8sErrors "k8s.io/apimachinery/pkg/api/errors"
2428
)
2529

2630
func (w *K8sClient) OcApplyWorkspace(namespace string, filePath string) (commandResult string, err error) {
@@ -68,12 +72,15 @@ func (w *K8sClient) GetLogsForContainer(podName string, namespace, containerName
6872
return string(outBytes), err
6973
}
7074

75+
// function returns an empty string as `commandResult` to match the existing signature.
7176
func (w *K8sClient) OcDeleteWorkspace(name, namespace string) (commandResult string, err error) {
72-
cmd := exec.Command("bash", "-c", fmt.Sprintf(
73-
"KUBECONFIG=%s oc delete devworkspace %s -n %s --ignore-not-found=true",
74-
w.kubeCfgFile,
75-
name,
76-
namespace))
77-
outBytes, err := cmd.CombinedOutput()
78-
return string(outBytes), err
77+
workspace := &dw.DevWorkspace{}
78+
workspace.ObjectMeta.Name = name
79+
workspace.ObjectMeta.Namespace = namespace
80+
81+
err = w.crClient.Delete(context.TODO(), workspace)
82+
if err != nil && !k8sErrors.IsNotFound(err) {
83+
return "", err
84+
}
85+
return "", nil
7986
}

0 commit comments

Comments
 (0)