diff --git a/README.md b/README.md index d656c30..441a90d 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,7 @@ Supported fields include: - `labels`, `annotations` - `env` - `resources` +- `imagePullPolicy` - `volumes`, `volumeMounts` - `nodeSelector`, `affinity`, `tolerations` - `hostAliases` diff --git a/apis/browserconfig/v1/browser_config.go b/apis/browserconfig/v1/browser_config.go index 5468cba..1d12ea0 100644 --- a/apis/browserconfig/v1/browser_config.go +++ b/apis/browserconfig/v1/browser_config.go @@ -45,6 +45,9 @@ type Template struct { // Resources defines CPU/memory requests and limits for the main container. Resources *corev1.ResourceRequirements `json:"resources,omitempty"` + //ImagePullPolicy defines container image pull policy + ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` + // Volumes defines pod volumes. Volumes *[]corev1.Volume `json:"volumes,omitempty"` @@ -128,6 +131,7 @@ type BrowserVersionConfigSpec struct { Annotations *map[string]string `json:"annotations,omitempty"` Env *[]corev1.EnvVar `json:"env,omitempty"` Resources *corev1.ResourceRequirements `json:"resources,omitempty"` + ImagePullPolicy corev1.PullPolicy `json:"imagePullPolicy,omitempty"` Volumes *[]corev1.Volume `json:"volumes,omitempty"` VolumeMounts *[]corev1.VolumeMount `json:"volumeMounts,omitempty"` NodeSelector *map[string]string `json:"nodeSelector,omitempty"` @@ -185,6 +189,9 @@ func (b *BrowserVersionConfigSpec) mergeWithSpec(t *BrowserConfigSpec) { b.Annotations = mergeMapPtr(t.Template.Annotations, b.Annotations) b.Env = mergeEnvPtr(t.Template.Env, b.Env) b.Resources = firstNonNilResource(t.Template.Resources, b.Resources) + if b.ImagePullPolicy == "" { + b.ImagePullPolicy = t.Template.ImagePullPolicy + } b.Volumes = mergeVolumePtr(t.Template.Volumes, b.Volumes) b.NodeSelector = mergeMapPtr(t.Template.NodeSelector, b.NodeSelector) diff --git a/apis/browserconfig/v1/browser_config_test.go b/apis/browserconfig/v1/browser_config_test.go index 6780218..cbd7f5e 100644 --- a/apis/browserconfig/v1/browser_config_test.go +++ b/apis/browserconfig/v1/browser_config_test.go @@ -1,179 +1,495 @@ package v1 -// import ( -// "testing" -// "time" - -// corev1 "k8s.io/api/core/v1" -// "k8s.io/apimachinery/pkg/api/resource" -// metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -// ) - -// func TestMergeWithTemplate_Timeouts(t *testing.T) { -// defaultTimeout := metav1.Duration{Duration: 10 * time.Minute} -// overrideTimeout := metav1.Duration{Duration: 20 * time.Minute} - -// spec := BrowserConfigSpec{ -// Template: &Template{ -// SessionTimeout: &defaultTimeout, -// }, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "chrome": { -// "100.0": { -// Image: "chrome:100", -// Path: "/wd/hub", -// SessionTimeout: &overrideTimeout, -// }, -// }, -// }, -// } - -// spec.MergeWithTemplate() - -// cfg := spec.Browsers["chrome"]["100.0"] -// if cfg.SessionTimeout.Duration != 20*time.Minute { -// t.Errorf("expected override sessionTimeout=20m, got %v", cfg.SessionTimeout.Duration) -// } -// } - -// func TestMergeWithTemplate_Labels(t *testing.T) { -// templateLabels := map[string]string{"app": "browsers", "tier": "backend"} -// overrideLabels := map[string]string{"tier": "frontend", "env": "test"} - -// spec := BrowserConfigSpec{ -// Template: &Template{Labels: &templateLabels}, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "firefox": { -// "101.0": { -// Image: "firefox:101", -// Path: "/wd/hub", -// Labels: &overrideLabels, -// }, -// }, -// }, -// } - -// spec.MergeWithTemplate() -// cfg := spec.Browsers["firefox"]["101.0"] - -// if (*cfg.Labels)["app"] != "browsers" { -// t.Errorf("expected template label app=browsers, got %v", (*cfg.Labels)["app"]) -// } -// if (*cfg.Labels)["tier"] != "frontend" { -// t.Errorf("expected override tier=frontend, got %v", (*cfg.Labels)["tier"]) -// } -// if (*cfg.Labels)["env"] != "test" { -// t.Errorf("expected override env=test, got %v", (*cfg.Labels)["env"]) -// } -// } - -// func TestMergeWithTemplate_Env(t *testing.T) { -// templateEnv := []corev1.EnvVar{{Name: "TZ", Value: "UTC"}, {Name: "DEBUG", Value: "false"}} -// overrideEnv := []corev1.EnvVar{{Name: "DEBUG", Value: "true"}, {Name: "CUSTOM", Value: "42"}} - -// spec := BrowserConfigSpec{ -// Template: &Template{Env: &templateEnv}, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "edge": { -// "110.0": {Image: "edge:110", Path: "/wd/hub", Env: &overrideEnv}, -// }, -// }, -// } -// spec.MergeWithTemplate() -// cfg := spec.Browsers["edge"]["110.0"] - -// envs := map[string]string{} -// for _, e := range *cfg.Env { -// envs[e.Name] = e.Value -// } - -// if envs["TZ"] != "UTC" { -// t.Errorf("expected TZ=UTC from template, got %v", envs["TZ"]) -// } -// if envs["DEBUG"] != "true" { -// t.Errorf("expected DEBUG=true override, got %v", envs["DEBUG"]) -// } -// if envs["CUSTOM"] != "42" { -// t.Errorf("expected CUSTOM=42 override, got %v", envs["CUSTOM"]) -// } -// } - -// func TestMergeWithTemplate_Sidecars(t *testing.T) { -// templateSC := []Sidecar{{Name: "metrics", Image: "metrics:1.0"}, {Name: "logger", Image: "logger:1.0"}} -// overrideSC := []Sidecar{{Name: "logger", Image: "logger:2.0"}, {Name: "debug", Image: "debug:latest"}} - -// spec := BrowserConfigSpec{ -// Template: &Template{Sidecars: &templateSC}, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "safari": { -// "15.0": {Image: "safari:15", Path: "/wd/hub", Sidecars: &overrideSC}, -// }, -// }, -// } -// spec.MergeWithTemplate() -// cfg := spec.Browsers["safari"]["15.0"] - -// names := map[string]string{} -// for _, sc := range *cfg.Sidecars { -// names[sc.Name] = sc.Image -// } - -// if names["metrics"] != "metrics:1.0" { -// t.Errorf("expected template sidecar metrics:1.0, got %v", names["metrics"]) -// } -// if names["logger"] != "logger:2.0" { -// t.Errorf("expected override logger:2.0, got %v", names["logger"]) -// } -// if names["debug"] != "debug:latest" { -// t.Errorf("expected override debug:latest, got %v", names["debug"]) -// } -// } - -// func TestMergeWithTemplate_Resources(t *testing.T) { -// templateRes := corev1.ResourceRequirements{ -// Requests: corev1.ResourceList{"cpu": resource.MustParse("100m")}, -// Limits: corev1.ResourceList{"cpu": resource.MustParse("200m")}, -// } -// overrideRes := corev1.ResourceRequirements{ -// Requests: corev1.ResourceList{"cpu": resource.MustParse("300m")}, -// Limits: corev1.ResourceList{"cpu": resource.MustParse("500m")}, -// } - -// spec := BrowserConfigSpec{ -// Template: &Template{Resources: &templateRes}, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "opera": { -// "12.0": {Image: "opera:12", Path: "/wd/hub", Resources: &overrideRes}, -// }, -// }, -// } -// spec.MergeWithTemplate() -// cfg := spec.Browsers["opera"]["12.0"] - -// if cfg.Resources.Requests.Cpu().String() != "300m" { -// t.Errorf("expected override requests cpu=300m, got %v", cfg.Resources.Requests.Cpu().String()) -// } -// if cfg.Resources.Limits.Cpu().String() != "500m" { -// t.Errorf("expected override limits cpu=500m, got %v", cfg.Resources.Limits.Cpu().String()) -// } -// } - -// func TestMergeWithTemplate_SecurityContext(t *testing.T) { -// runAsNonRoot := true -// templateSC := corev1.SecurityContext{RunAsNonRoot: &runAsNonRoot} - -// spec := BrowserConfigSpec{ -// Template: &Template{SecurityContext: &templateSC}, -// Browsers: map[string]map[string]*BrowserVersionConfig{ -// "brave": { -// "1.0": {Image: "brave:1", Path: "/wd/hub"}, -// }, -// }, -// } -// spec.MergeWithTemplate() -// cfg := spec.Browsers["brave"]["1.0"] - -// if cfg.SecurityContext == nil || cfg.SecurityContext.RunAsNonRoot == nil || !*cfg.SecurityContext.RunAsNonRoot { -// t.Errorf("expected SecurityContext.RunAsNonRoot=true inherited from template, got %+v", cfg.SecurityContext) -// } -// } +import ( + "testing" + + corev1 "k8s.io/api/core/v1" +) + +func TestMergeWithTemplateInheritsImagePullPolicyFromTemplate(t *testing.T) { + templateEnv := []corev1.EnvVar{{Name: "TZ", Value: "UTC"}} + spec := BrowserConfigSpec{ + Template: &Template{ + ImagePullPolicy: corev1.PullIfNotPresent, + Env: &templateEnv, + }, + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "123.0": {Image: "chrome:123"}, + }, + }, + } + + spec.MergeWithTemplate() + + cfg := spec.Browsers["chrome"]["123.0"] + if cfg.ImagePullPolicy != corev1.PullIfNotPresent { + t.Fatalf("expected imagePullPolicy=%q, got %q", corev1.PullIfNotPresent, cfg.ImagePullPolicy) + } + if cfg.Env == nil || len(*cfg.Env) != 1 || (*cfg.Env)[0].Name != "TZ" { + t.Fatalf("expected env to be merged from template, got %+v", cfg.Env) + } +} + +func TestMergeWithTemplateDoesNotOverrideImagePullPolicy(t *testing.T) { + spec := BrowserConfigSpec{ + Template: &Template{ + ImagePullPolicy: corev1.PullIfNotPresent, + }, + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "124.0": { + Image: "chrome:124", + ImagePullPolicy: corev1.PullAlways, + }, + }, + }, + } + + spec.MergeWithTemplate() + + cfg := spec.Browsers["chrome"]["124.0"] + if cfg.ImagePullPolicy != corev1.PullAlways { + t.Fatalf("expected explicit imagePullPolicy=%q to be preserved, got %q", corev1.PullAlways, cfg.ImagePullPolicy) + } +} + +func TestMergeMapPtrOverrideWins(t *testing.T) { + template := map[string]string{ + "shared": "template", + "template": "value", + } + override := map[string]string{ + "shared": "override", + "override": "value", + } + + merged := mergeMapPtr(&template, &override) + if merged == nil { + t.Fatalf("expected merged map, got nil") + } + + if (*merged)["shared"] != "override" { + t.Fatalf("expected override value for shared key, got %q", (*merged)["shared"]) + } + if (*merged)["template"] != "value" || (*merged)["override"] != "value" { + t.Fatalf("expected both template and override unique keys, got %+v", *merged) + } +} + +func TestMergeEnvPtrOverrideWinsByName(t *testing.T) { + template := []corev1.EnvVar{ + {Name: "TZ", Value: "UTC"}, + {Name: "DEBUG", Value: "false"}, + } + override := []corev1.EnvVar{ + {Name: "DEBUG", Value: "true"}, + {Name: "CUSTOM", Value: "42"}, + } + + merged := mergeEnvPtr(&template, &override) + if merged == nil { + t.Fatalf("expected merged env, got nil") + } + + got := map[string]string{} + for _, env := range *merged { + got[env.Name] = env.Value + } + + if got["TZ"] != "UTC" || got["DEBUG"] != "true" || got["CUSTOM"] != "42" { + t.Fatalf("unexpected merged env: %+v", got) + } +} + +func TestMergeSidecarPtrMergesByName(t *testing.T) { + template := []Sidecar{ + {Name: "metrics", Image: "metrics:1.0"}, + {Name: "logger", Image: "logger:1.0"}, + } + override := []Sidecar{ + {Name: "logger", Image: "logger:2.0"}, + {Name: "debug", Image: "debug:latest"}, + } + + merged := mergeSidecarPtr(&template, &override) + if merged == nil { + t.Fatalf("expected merged sidecars, got nil") + } + + if len(*merged) != 3 { + t.Fatalf("expected 3 merged sidecars, got %d", len(*merged)) + } + + got := map[string]string{} + for _, sc := range *merged { + got[sc.Name] = sc.Image + } + + if got["metrics"] != "metrics:1.0" { + t.Fatalf("expected template-only sidecar to be present, got %+v", got) + } + if got["logger"] != "logger:2.0" { + t.Fatalf("expected override sidecar to win, got %+v", got) + } + if got["debug"] != "debug:latest" { + t.Fatalf("expected override-only sidecar to be present, got %+v", got) + } +} + +func TestMergeWithTemplateNoTemplateNoop(t *testing.T) { + spec := BrowserConfigSpec{ + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "125.0": {Image: "chrome:125"}, + }, + }, + } + + spec.MergeWithTemplate() + + if got := spec.Browsers["chrome"]["125.0"].ImagePullPolicy; got != "" { + t.Fatalf("expected no changes when template is nil, got imagePullPolicy=%q", got) + } +} + +func TestMergeWithTemplateSkipsNilVersionConfig(t *testing.T) { + spec := BrowserConfigSpec{ + Template: &Template{ + ImagePullPolicy: corev1.PullIfNotPresent, + }, + Browsers: map[string]map[string]*BrowserVersionConfigSpec{ + "chrome": { + "nil-entry": nil, + "126.0": {Image: "chrome:126"}, + }, + }, + } + + spec.MergeWithTemplate() + + if spec.Browsers["chrome"]["nil-entry"] != nil { + t.Fatalf("expected nil browser version config to stay nil") + } + if got := spec.Browsers["chrome"]["126.0"].ImagePullPolicy; got != corev1.PullIfNotPresent { + t.Fatalf("expected non-nil browser config to be merged, got imagePullPolicy=%q", got) + } +} + +func TestBrowserVersionConfigSpecMergeWithSpecMergesAllPaths(t *testing.T) { + templateLabels := map[string]string{"from-template": "1"} + overrideLabels := map[string]string{"from-override": "1"} + templateAnnotations := map[string]string{"ta": "tv"} + overrideAnnotations := map[string]string{"oa": "ov"} + templateEnv := []corev1.EnvVar{{Name: "TEMPLATE_ENV", Value: "1"}} + overrideEnv := []corev1.EnvVar{{Name: "OVERRIDE_ENV", Value: "1"}} + templateResources := corev1.ResourceRequirements{} + templateVolumes := []corev1.Volume{{Name: "template-vol"}} + overrideVolumes := []corev1.Volume{{Name: "override-vol"}} + templateNodeSelector := map[string]string{"pool": "template"} + templateAffinity := &corev1.Affinity{} + templateTolerations := []corev1.Toleration{{Key: "template"}} + overrideTolerations := []corev1.Toleration{{Key: "override"}} + templateHostAliases := []corev1.HostAlias{{IP: "10.0.0.1", Hostnames: []string{"template"}}} + overrideHostAliases := []corev1.HostAlias{{IP: "10.0.0.2", Hostnames: []string{"override"}}} + templateMounts := []corev1.VolumeMount{{Name: "template-vol", MountPath: "/template"}} + overrideMounts := []corev1.VolumeMount{{Name: "override-vol", MountPath: "/override"}} + templateCommand := []string{"tmpl-cmd"} + templateSidecarEnv := []corev1.EnvVar{{Name: "TMPL_SC_ENV", Value: "1"}} + overrideSidecarEnv := []corev1.EnvVar{{Name: "OVR_SC_ENV", Value: "1"}} + templatePorts := []corev1.ContainerPort{{ContainerPort: 8080}} + templateSidecarMounts := []corev1.VolumeMount{{Name: "template-vol", MountPath: "/sc-template"}} + templateSidecars := []Sidecar{ + { + Name: "shared-sidecar", + Image: "template-sc", + Command: &templateCommand, + WorkingDir: strPtr("/template-sidecar-workdir"), + Env: &templateSidecarEnv, + Ports: &templatePorts, + VolumeMounts: &templateSidecarMounts, + Resources: &templateResources, + }, + {Name: "template-only-sidecar", Image: "template-only"}, + } + overrideSidecars := []Sidecar{ + { + Name: "shared-sidecar", + Image: "override-sc", + Env: &overrideSidecarEnv, + }, + {Name: "override-only-sidecar", Image: "override-only"}, + } + templateInit := []Sidecar{ + { + Name: "shared-init", + Image: "template-init", + Command: &templateCommand, + WorkingDir: strPtr("/template-init-workdir"), + }, + {Name: "template-only-init", Image: "template-only-init"}, + } + overrideInit := []Sidecar{ + {Name: "shared-init", Image: "override-init"}, + } + templatePrivileged := true + templatePullSecrets := []corev1.LocalObjectReference{{Name: "template-secret"}} + overridePullSecrets := []corev1.LocalObjectReference{{Name: "override-secret"}} + templateDNSConfig := &corev1.PodDNSConfig{} + templateSecurityContext := &corev1.PodSecurityContext{} + templateWorkingDir := "/template-workdir" + + spec := BrowserConfigSpec{ + Template: &Template{ + Labels: &templateLabels, + Annotations: &templateAnnotations, + Env: &templateEnv, + Resources: &templateResources, + ImagePullPolicy: corev1.PullIfNotPresent, + Volumes: &templateVolumes, + VolumeMounts: &templateMounts, + NodeSelector: &templateNodeSelector, + Affinity: templateAffinity, + Tolerations: &templateTolerations, + HostAliases: &templateHostAliases, + Sidecars: &templateSidecars, + InitContainers: &templateInit, + Privileged: &templatePrivileged, + ImagePullSecrets: &templatePullSecrets, + DNSConfig: templateDNSConfig, + SecurityContext: templateSecurityContext, + WorkingDir: &templateWorkingDir, + }, + } + b := &BrowserVersionConfigSpec{ + Image: "browser", + Labels: &overrideLabels, + Annotations: &overrideAnnotations, + Env: &overrideEnv, + Volumes: &overrideVolumes, + VolumeMounts: &overrideMounts, + Tolerations: &overrideTolerations, + HostAliases: &overrideHostAliases, + Sidecars: &overrideSidecars, + InitContainers: &overrideInit, + ImagePullSecrets: &overridePullSecrets, + } + + b.mergeWithSpec(&spec) + + if b.ImagePullPolicy != corev1.PullIfNotPresent { + t.Fatalf("expected imagePullPolicy to be inherited, got %q", b.ImagePullPolicy) + } + if b.Resources != &templateResources { + t.Fatalf("expected resources to inherit from template") + } + if b.Affinity != templateAffinity { + t.Fatalf("expected affinity to inherit from template") + } + if b.Privileged == nil || !*b.Privileged { + t.Fatalf("expected privileged to inherit from template") + } + if b.DNSConfig != templateDNSConfig || b.SecurityContext != templateSecurityContext || b.WorkingDir == nil || *b.WorkingDir != templateWorkingDir { + t.Fatalf("expected dns/securityContext/workingDir to inherit from template") + } + + if b.Labels == nil || (*b.Labels)["from-template"] != "1" || (*b.Labels)["from-override"] != "1" { + t.Fatalf("expected labels to merge, got %+v", b.Labels) + } + if b.Annotations == nil || (*b.Annotations)["ta"] != "tv" || (*b.Annotations)["oa"] != "ov" { + t.Fatalf("expected annotations to merge, got %+v", b.Annotations) + } + if b.Env == nil || !hasEnv(*b.Env, "TEMPLATE_ENV", "1") || !hasEnv(*b.Env, "OVERRIDE_ENV", "1") { + t.Fatalf("expected env to merge, got %+v", b.Env) + } + if b.Volumes == nil || len(*b.Volumes) != 2 { + t.Fatalf("expected volumes to merge, got %+v", b.Volumes) + } + if b.VolumeMounts == nil || len(*b.VolumeMounts) != 2 { + t.Fatalf("expected volumeMounts to merge, got %+v", b.VolumeMounts) + } + if b.Tolerations == nil || len(*b.Tolerations) != 2 { + t.Fatalf("expected tolerations to merge, got %+v", b.Tolerations) + } + if b.HostAliases == nil || len(*b.HostAliases) != 2 { + t.Fatalf("expected hostAliases to merge, got %+v", b.HostAliases) + } + if b.ImagePullSecrets == nil || len(*b.ImagePullSecrets) != 2 { + t.Fatalf("expected imagePullSecrets to merge, got %+v", b.ImagePullSecrets) + } + if b.Sidecars == nil || len(*b.Sidecars) != 3 { + t.Fatalf("expected sidecars to merge, got %+v", b.Sidecars) + } + shared := getSidecarByName(*b.Sidecars, "shared-sidecar") + if shared == nil { + t.Fatalf("expected shared sidecar in merged result") + } + if shared.Command == nil || len(*shared.Command) != 1 || (*shared.Command)[0] != "tmpl-cmd" { + t.Fatalf("expected shared sidecar command from template, got %+v", shared.Command) + } + if shared.Resources != &templateResources { + t.Fatalf("expected shared sidecar resources from template") + } + if shared.Ports == nil || len(*shared.Ports) != 1 || (*shared.Ports)[0].ContainerPort != 8080 { + t.Fatalf("expected shared sidecar ports from template, got %+v", shared.Ports) + } + if shared.VolumeMounts == nil || len(*shared.VolumeMounts) != 1 { + t.Fatalf("expected shared sidecar volume mounts from template, got %+v", shared.VolumeMounts) + } + if shared.Env == nil || !hasEnv(*shared.Env, "TMPL_SC_ENV", "1") || !hasEnv(*shared.Env, "OVR_SC_ENV", "1") { + t.Fatalf("expected shared sidecar env merge, got %+v", shared.Env) + } + if b.InitContainers == nil || len(*b.InitContainers) != 2 { + t.Fatalf("expected initContainers to merge, got %+v", b.InitContainers) + } +} + +func TestMergeHelperFunctionsNilAndNonNilPaths(t *testing.T) { + if got := mergeMapPtr(nil, nil); got != nil { + t.Fatalf("expected nil map for nil inputs, got %+v", got) + } + if got := mergeEnvPtr(nil, nil); got != nil { + t.Fatalf("expected nil env for nil inputs, got %+v", got) + } + if got := mergeVolumePtr(nil, nil); got != nil { + t.Fatalf("expected nil volumes for nil inputs, got %+v", got) + } + if got := mergeTolerationPtr(nil, nil); got != nil { + t.Fatalf("expected nil tolerations for nil inputs, got %+v", got) + } + if got := mergeHostAliasPtr(nil, nil); got != nil { + t.Fatalf("expected nil hostAliases for nil inputs, got %+v", got) + } + if got := mergeVolumeMountsPtr(nil, nil); got != nil { + t.Fatalf("expected nil volumeMounts for nil inputs, got %+v", got) + } + if got := mergeLocalObjectRefPtr(nil, nil); got != nil { + t.Fatalf("expected nil imagePullSecrets for nil inputs, got %+v", got) + } + if got := mergeContainerPortPtr(nil, nil); got != nil { + t.Fatalf("expected nil containerPorts for nil inputs, got %+v", got) + } + if got := mergeVolumeMountPtr(nil, nil); got != nil { + t.Fatalf("expected nil volumeMounts for nil inputs, got %+v", got) + } + + templateRes := &corev1.ResourceRequirements{} + overrideRes := &corev1.ResourceRequirements{} + if got := firstNonNilResource(templateRes, nil); got != templateRes { + t.Fatalf("expected template resource when override is nil") + } + if got := firstNonNilResource(templateRes, overrideRes); got != overrideRes { + t.Fatalf("expected override resource when override is set") + } +} + +func TestMergeSidecarPtrNilCases(t *testing.T) { + template := []Sidecar{{Name: "template", Image: "tmpl"}} + override := []Sidecar{{Name: "override", Image: "ovr"}} + + if got := mergeSidecarPtr(nil, nil); got != nil { + t.Fatalf("expected nil when both sidecar lists are nil") + } + if got := mergeSidecarPtr(&template, nil); got == nil || len(*got) != 1 || (*got)[0].Name != "template" { + t.Fatalf("expected template sidecars when override is nil, got %+v", got) + } + if got := mergeSidecarPtr(nil, &override); got == nil || len(*got) != 1 || (*got)[0].Name != "override" { + t.Fatalf("expected override sidecars when template is nil, got %+v", got) + } +} + +func TestFindTemplateSidecar(t *testing.T) { + if got := findTemplateSidecar(nil, "x"); got != nil { + t.Fatalf("expected nil result for nil template") + } + + template := []Sidecar{{Name: "metrics", Image: "metrics:1.0"}} + if got := findTemplateSidecar(&template, "missing"); got != nil { + t.Fatalf("expected nil for missing sidecar") + } + if got := findTemplateSidecar(&template, "metrics"); got == nil || got.Name != "metrics" { + t.Fatalf("expected to find sidecar metrics, got %+v", got) + } +} + +func TestSidecarMergeWithTemplate(t *testing.T) { + templateCommand := []string{"run"} + templateEnv := []corev1.EnvVar{{Name: "TMPL", Value: "1"}} + overrideEnv := []corev1.EnvVar{{Name: "OVR", Value: "1"}} + templatePorts := []corev1.ContainerPort{{ContainerPort: 8080}} + templateMounts := []corev1.VolumeMount{{Name: "v", MountPath: "/t"}} + templateResources := corev1.ResourceRequirements{} + + s := Sidecar{ + Name: "s", + Env: &overrideEnv, + } + tmpl := Sidecar{ + Name: "s", + Command: &templateCommand, + WorkingDir: strPtr("/work"), + Env: &templateEnv, + Ports: &templatePorts, + VolumeMounts: &templateMounts, + Resources: &templateResources, + } + + s.mergeWithTemplate(&tmpl) + + if s.Command == nil || len(*s.Command) != 1 || (*s.Command)[0] != "run" { + t.Fatalf("expected command from template, got %+v", s.Command) + } + if s.WorkingDir == nil || *s.WorkingDir != "/work" { + t.Fatalf("expected workingDir from template, got %+v", s.WorkingDir) + } + if s.Env == nil || !hasEnv(*s.Env, "TMPL", "1") || !hasEnv(*s.Env, "OVR", "1") { + t.Fatalf("expected env merge, got %+v", s.Env) + } + if s.Ports == nil || len(*s.Ports) != 1 || (*s.Ports)[0].ContainerPort != 8080 { + t.Fatalf("expected ports from template, got %+v", s.Ports) + } + if s.VolumeMounts == nil || len(*s.VolumeMounts) != 1 || (*s.VolumeMounts)[0].MountPath != "/t" { + t.Fatalf("expected volumeMounts from template, got %+v", s.VolumeMounts) + } + if s.Resources != &templateResources { + t.Fatalf("expected resources from template") + } +} + +func TestMergeContainerPortPtrAndMergeVolumeMountPtr(t *testing.T) { + templatePorts := []corev1.ContainerPort{{ContainerPort: 80}} + overridePorts := []corev1.ContainerPort{{ContainerPort: 443}} + mergedPorts := mergeContainerPortPtr(&templatePorts, &overridePorts) + if mergedPorts == nil || len(*mergedPorts) != 2 { + t.Fatalf("expected merged container ports, got %+v", mergedPorts) + } + + templateMounts := []corev1.VolumeMount{{Name: "t", MountPath: "/t"}} + overrideMounts := []corev1.VolumeMount{{Name: "o", MountPath: "/o"}} + mergedMounts := mergeVolumeMountPtr(&templateMounts, &overrideMounts) + if mergedMounts == nil || len(*mergedMounts) != 2 { + t.Fatalf("expected merged volume mounts, got %+v", mergedMounts) + } +} + +func strPtr(v string) *string { + return &v +} + +func hasEnv(env []corev1.EnvVar, name, value string) bool { + for _, item := range env { + if item.Name == name && item.Value == value { + return true + } + } + return false +} + +func getSidecarByName(sidecars []Sidecar, name string) *Sidecar { + for i := range sidecars { + if sidecars[i].Name == name { + return &sidecars[i] + } + } + return nil +} diff --git a/config/crd/selenosis.io_browserconfigs.yaml b/config/crd/selenosis.io_browserconfigs.yaml index da21d61..bcf56d5 100644 --- a/config/crd/selenosis.io_browserconfigs.yaml +++ b/config/crd/selenosis.io_browserconfigs.yaml @@ -1200,6 +1200,10 @@ spec: image: description: Image is the browser container image. type: string + imagePullPolicy: + description: PullPolicy describes a policy for if/when to + pull a container image + type: string imagePullSecrets: items: description: |- @@ -5415,6 +5419,9 @@ spec: - ip type: object type: array + imagePullPolicy: + description: ImagePullPolicy defines container image pull policy + type: string imagePullSecrets: description: ImagePullSecrets specifies secrets for pulling private images. diff --git a/controllers/browser/browser_reconciler.go b/controllers/browser/browser_reconciler.go index bff1c65..65ce483 100644 --- a/controllers/browser/browser_reconciler.go +++ b/controllers/browser/browser_reconciler.go @@ -702,6 +702,10 @@ func buildBrowserPod(browser *browserv1.Browser, cfg *configv1.BrowserVersionCon Image: cfg.Image, } + if cfg.ImagePullPolicy != "" { + browserContainer.ImagePullPolicy = cfg.ImagePullPolicy + } + if cfg.Env != nil { browserContainer.Env = *cfg.Env } diff --git a/controllers/browser/browser_reconciler_test.go b/controllers/browser/browser_reconciler_test.go index d93fced..45e5142 100644 --- a/controllers/browser/browser_reconciler_test.go +++ b/controllers/browser/browser_reconciler_test.go @@ -177,6 +177,27 @@ func TestBuildBrowserPod(t *testing.T) { } } +func TestBuildBrowserPodMainContainerImagePullPolicy(t *testing.T) { + cfg := &configv1.BrowserVersionConfigSpec{ + Image: "browser", + ImagePullPolicy: corev1.PullAlways, + } + brw := &browserv1.Browser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "b1", + Namespace: "ns", + }, + } + + pod := buildBrowserPod(brw, cfg, nil) + if len(pod.Spec.Containers) == 0 { + t.Fatalf("expected at least one container") + } + if pod.Spec.Containers[0].ImagePullPolicy != corev1.PullAlways { + t.Fatalf("expected main container imagePullPolicy=%q, got %q", corev1.PullAlways, pod.Spec.Containers[0].ImagePullPolicy) + } +} + func TestParseSelenosisOptionsInvalidJSON(t *testing.T) { ann := map[string]string{ browserv1.SelenosisOptionsAnnotationKey: "{nope", diff --git a/store/browserconfig_store_test.go b/store/browserconfig_store_test.go new file mode 100644 index 0000000..e5aa9d7 --- /dev/null +++ b/store/browserconfig_store_test.go @@ -0,0 +1,389 @@ +package store + +import ( + "context" + "errors" + "testing" + "time" + + configv1 "github.com/alcounit/browser-controller/apis/browserconfig/v1" + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/client-go/tools/cache" + crcache "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +func TestKeyForLowercases(t *testing.T) { + key := keyFor("Ns", "Chrome", "RC1") + if key != "Ns/chrome:rc1" { + t.Fatalf("unexpected key: %q", key) + } +} + +func TestBrowserConfigStoreWithCache(t *testing.T) { + store := NewBrowserConfigStore() + fc := &fakeCache{} + + r := store.WithCache(fc, logr.Discard()) + if r != store { + t.Fatalf("expected WithCache to return store runnable") + } + if store.cache != fc { + t.Fatalf("expected cache to be set") + } +} + +func TestBrowserConfigStoreOnAddOrUpdateMergesAndStores(t *testing.T) { + templateEnv := []corev1.EnvVar{{Name: "TZ", Value: "UTC"}} + overrideEnv := []corev1.EnvVar{{Name: "TZ", Value: "PST"}, {Name: "CUSTOM", Value: "1"}} + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + }, + Spec: configv1.BrowserConfigSpec{ + Template: &configv1.Template{ + ImagePullPolicy: corev1.PullIfNotPresent, + Env: &templateEnv, + }, + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": { + "144.0": { + Image: "img", + Env: &overrideEnv, + }, + }, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(bc, logr.Discard()) + + cfg, ok := store.Get("ns", "CHROME", "144.0") + if !ok || cfg == nil { + t.Fatalf("expected config to be stored") + } + if cfg.ImagePullPolicy != corev1.PullIfNotPresent { + t.Fatalf("expected imagePullPolicy to be merged from template, got %q", cfg.ImagePullPolicy) + } + if cfg.Env == nil || len(*cfg.Env) != 2 { + t.Fatalf("expected env to be merged, got %+v", cfg.Env) + } +} + +func TestBrowserConfigStoreOnAddOrUpdateDeepCopyIsolated(t *testing.T) { + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + }, + Spec: configv1.BrowserConfigSpec{ + Template: &configv1.Template{ + ImagePullPolicy: corev1.PullIfNotPresent, + }, + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": { + "144.0": {Image: "img"}, + }, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(bc, logr.Discard()) + + // Mutate original after add. + bc.Spec.Browsers["Chrome"]["144.0"].ImagePullPolicy = corev1.PullNever + + cfg, ok := store.Get("ns", "chrome", "144.0") + if !ok || cfg == nil { + t.Fatalf("expected config to be stored") + } + if cfg.ImagePullPolicy != corev1.PullIfNotPresent { + t.Fatalf("expected store to keep merged value from copy, got %q", cfg.ImagePullPolicy) + } +} + +func TestBrowserConfigStoreOnAddOrUpdateDeletedFinalStateUnknown(t *testing.T) { + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Firefox": {"120.0": {Image: "img"}}, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(cache.DeletedFinalStateUnknown{Obj: bc}, logr.Discard()) + + if _, ok := store.Get("ns", "firefox", "120.0"); !ok { + t.Fatalf("expected config to be stored from DeletedFinalStateUnknown") + } +} + +func TestBrowserConfigStoreOnAddOrUpdateDeletedFinalStateUnknownNilObj(t *testing.T) { + store := NewBrowserConfigStore() + store.onAddOrUpdate(cache.DeletedFinalStateUnknown{Obj: (*configv1.BrowserConfig)(nil)}, logr.Discard()) +} + +func TestBrowserConfigStoreOnAddOrUpdateIgnoresUnknownType(t *testing.T) { + store := NewBrowserConfigStore() + store.onAddOrUpdate("not-a-config", logr.Discard()) + if _, ok := store.Get("ns", "chrome", "144.0"); ok { + t.Fatalf("expected no configs to be stored") + } +} + +func TestBrowserConfigStoreOnDeleteRemovesKeys(t *testing.T) { + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": {"144.0": {Image: "img"}}, + "Firefox": {"120.0": {Image: "img"}}, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(bc, logr.Discard()) + + store.onDelete(bc, logr.Discard()) + + if _, ok := store.Get("ns", "chrome", "144.0"); ok { + t.Fatalf("expected chrome config to be deleted") + } + if _, ok := store.Get("ns", "firefox", "120.0"); ok { + t.Fatalf("expected firefox config to be deleted") + } +} + +func TestBrowserConfigStoreOnDeleteDeletedFinalStateUnknown(t *testing.T) { + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cfg", + Namespace: "ns", + }, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Safari": {"17.0": {Image: "img"}}, + }, + }, + } + + store := NewBrowserConfigStore() + store.onAddOrUpdate(bc, logr.Discard()) + + store.onDelete(cache.DeletedFinalStateUnknown{Obj: bc}, logr.Discard()) + if _, ok := store.Get("ns", "safari", "17.0"); ok { + t.Fatalf("expected config to be deleted from DeletedFinalStateUnknown") + } +} + +func TestBrowserConfigStoreOnDeleteDeletedFinalStateUnknownNilObj(t *testing.T) { + store := NewBrowserConfigStore() + store.onDelete(cache.DeletedFinalStateUnknown{Obj: (*configv1.BrowserConfig)(nil)}, logr.Discard()) +} + +func TestBrowserConfigStoreOnDeleteIgnoresUnknownType(t *testing.T) { + store := NewBrowserConfigStore() + store.onDelete("not-a-config", logr.Discard()) +} + +func TestBrowserConfigStoreGetMissing(t *testing.T) { + store := NewBrowserConfigStore() + if _, ok := store.Get("ns", "missing", "1"); ok { + t.Fatalf("expected missing config to return false") + } +} + +func TestBrowserConfigStoreStartNoCache(t *testing.T) { + store := NewBrowserConfigStore() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + if err := store.Start(ctx); err == nil { + t.Fatalf("expected error when cache is nil") + } +} + +func TestBrowserConfigStoreStartGetInformerError(t *testing.T) { + store := NewBrowserConfigStore() + fc := &fakeCache{getInformerErr: errors.New("boom")} + store.WithCache(fc, logr.Discard()) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + if err := store.Start(ctx); err == nil { + t.Fatalf("expected error from GetInformer") + } +} + +func TestBrowserConfigStoreStartWaitForCacheSyncFails(t *testing.T) { + store := NewBrowserConfigStore() + fi := &fakeInformer{synced: false} + fc := &fakeCache{informer: fi} + store.WithCache(fc, logr.Discard()) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + if err := store.Start(ctx); err == nil { + t.Fatalf("expected error when cache sync fails") + } +} + +func TestBrowserConfigStoreStartSuccess(t *testing.T) { + store := NewBrowserConfigStore() + syncCh := make(chan struct{}) + handlerCh := make(chan struct{}) + fi := &fakeInformer{synced: true, syncedCalled: syncCh, handlerSet: handlerCh} + fc := &fakeCache{informer: fi} + store.WithCache(fc, logr.Discard()) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + done := make(chan error, 1) + go func() { + done <- store.Start(ctx) + }() + + select { + case <-syncCh: + case <-time.After(1 * time.Second): + t.Fatalf("expected informer HasSynced to be called") + } + + select { + case <-handlerCh: + case <-time.After(1 * time.Second): + t.Fatalf("expected handler to be registered") + } + + if fi.handler != nil { + bc := &configv1.BrowserConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "cfg", Namespace: "ns"}, + Spec: configv1.BrowserConfigSpec{ + Browsers: map[string]map[string]*configv1.BrowserVersionConfigSpec{ + "Chrome": {"144.0": {Image: "img"}}, + }, + }, + } + fi.handler.OnAdd(bc, false) + fi.handler.OnUpdate(nil, bc) + fi.handler.OnDelete(bc) + } + + cancel() + if err := <-done; err != nil { + t.Fatalf("expected Start to return nil, got %v", err) + } + if !fi.addHandlerCalled { + t.Fatalf("expected event handler to be registered") + } +} + +type fakeCache struct { + informer crcache.Informer + getInformerErr error +} + +func (f *fakeCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return nil +} + +func (f *fakeCache) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + return nil +} + +func (f *fakeCache) GetInformer(ctx context.Context, obj client.Object, opts ...crcache.InformerGetOption) (crcache.Informer, error) { + if f.getInformerErr != nil { + return nil, f.getInformerErr + } + return f.informer, nil +} + +func (f *fakeCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind, opts ...crcache.InformerGetOption) (crcache.Informer, error) { + return f.informer, nil +} + +func (f *fakeCache) RemoveInformer(ctx context.Context, obj client.Object) error { + return nil +} + +func (f *fakeCache) Start(ctx context.Context) error { + return nil +} + +func (f *fakeCache) WaitForCacheSync(ctx context.Context) bool { + return true +} + +func (f *fakeCache) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error { + return nil +} + +type fakeInformer struct { + synced bool + addHandlerCalled bool + syncedCalled chan struct{} + handlerSet chan struct{} + handler cache.ResourceEventHandler +} + +func (f *fakeInformer) AddEventHandler(handler cache.ResourceEventHandler) (cache.ResourceEventHandlerRegistration, error) { + f.addHandlerCalled = true + f.handler = handler + if f.handlerSet != nil { + close(f.handlerSet) + } + return fakeHandlerReg{synced: f.synced}, nil +} + +func (f *fakeInformer) AddEventHandlerWithResyncPeriod(handler cache.ResourceEventHandler, resyncPeriod time.Duration) (cache.ResourceEventHandlerRegistration, error) { + return f.AddEventHandler(handler) +} + +func (f *fakeInformer) RemoveEventHandler(cache.ResourceEventHandlerRegistration) error { + return nil +} + +func (f *fakeInformer) AddIndexers(cache.Indexers) error { + return nil +} + +func (f *fakeInformer) HasSynced() bool { + if f.syncedCalled != nil { + select { + case <-f.syncedCalled: + default: + close(f.syncedCalled) + } + } + return f.synced +} + +func (f *fakeInformer) IsStopped() bool { + return false +} + +type fakeHandlerReg struct { + synced bool +} + +func (r fakeHandlerReg) HasSynced() bool { + return r.synced +}