Skip to content

Fix Sidecar without image specification issue #2977

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+212 −8
Open

Fix Sidecar without image specification issue #2977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 19 additions & 0 deletions pkg/cluster/k8sres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,6 +1303,9 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef
c.logger.Warningf("initContainers specified but disabled in configuration - next statefulset creation would fail")
}
initContainers = spec.InitContainers
if err := c.validateContainers(initContainers); err != nil {
return nil, fmt.Errorf("invalid init containers: %v", err)
}
}

// backward compatible check for InitContainers
Expand Down Expand Up @@ -1455,6 +1458,10 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef

sidecarContainers = patchSidecarContainers(sidecarContainers, volumeMounts, c.OpConfig.SuperUsername, c.credentialSecretName(c.OpConfig.SuperUsername))

if err := c.validateContainers(sidecarContainers); err != nil {
return nil, fmt.Errorf("invalid sidecar containers: %v", err)
}

tolerationSpec := tolerations(&spec.Tolerations, c.OpConfig.PodToleration)
effectivePodPriorityClassName := util.Coalesce(spec.PodPriorityClassName, c.OpConfig.PodPriorityClassName)

Expand Down Expand Up @@ -2592,3 +2599,15 @@ func ensurePath(file string, defaultDir string, defaultFile string) string {
}
return file
}

func (c *Cluster) validateContainers(containers []v1.Container) error {
for i, container := range containers {
if container.Name == "" {
return fmt.Errorf("container[%d]: name is required", i)
}
if container.Image == "" {
return fmt.Errorf("container '%v': image is required", container.Name)
}
}
return nil
}
201 changes: 193 additions & 8 deletions pkg/cluster/k8sres_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1935,7 +1935,8 @@ func TestAdditionalVolume(t *testing.T) {
AdditionalVolumes: additionalVolumes,
Sidecars: []acidv1.Sidecar{
{
Name: sidecarName,
Name: sidecarName,
DockerImage: "test-image",
},
},
},
Expand Down Expand Up @@ -2163,10 +2164,12 @@ func TestSidecars(t *testing.T) {
},
Sidecars: []acidv1.Sidecar{
{
Name: "cluster-specific-sidecar",
Name: "cluster-specific-sidecar",
DockerImage: "test-image",
},
{
Name: "cluster-specific-sidecar-with-resources",
Name: "cluster-specific-sidecar-with-resources",
DockerImage: "test-image",
Resources: &acidv1.Resources{
ResourceRequests: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("210m"), Memory: k8sutil.StringToPointer("0.8Gi")},
ResourceLimits: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("510m"), Memory: k8sutil.StringToPointer("1.4Gi")},
Expand Down Expand Up @@ -2201,7 +2204,8 @@ func TestSidecars(t *testing.T) {
},
SidecarContainers: []v1.Container{
{
Name: "global-sidecar",
Name: "global-sidecar",
Image: "test-image",
},
// will be replaced by a cluster specific sidecar with the same name
{
Expand Down Expand Up @@ -2271,6 +2275,7 @@ func TestSidecars(t *testing.T) {
// cluster specific sidecar
assert.Contains(t, s.Spec.Template.Spec.Containers, v1.Container{
Name: "cluster-specific-sidecar",
Image: "test-image",
Env: env,
Resources: generateKubernetesResources("200m", "500m", "0.7Gi", "1.3Gi"),
ImagePullPolicy: v1.PullIfNotPresent,
Expand All @@ -2297,6 +2302,7 @@ func TestSidecars(t *testing.T) {
// global sidecar
assert.Contains(t, s.Spec.Template.Spec.Containers, v1.Container{
Name: "global-sidecar",
Image: "test-image",
Env: env,
VolumeMounts: mounts,
})
Expand Down Expand Up @@ -2325,6 +2331,180 @@ func TestSidecars(t *testing.T) {

}

func TestContainerValidation(t *testing.T) {
testCases := []struct {
name string
spec acidv1.PostgresSpec
clusterConfig Config
expectedError string
}{
{
name: "init container without image",
spec: acidv1.PostgresSpec{
PostgresqlParam: acidv1.PostgresqlParam{
PgVersion: "17",
},
TeamID: "myapp",
NumberOfInstances: 1,
Volume: acidv1.Volume{
Size: "1G",
},
InitContainers: []v1.Container{
{
Name: "invalid-initcontainer",
},
},
},
clusterConfig: Config{
OpConfig: config.Config{
PodManagementPolicy: "ordered_ready",
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
},
},
expectedError: "image is required",
},
{
name: "sidecar without name",
spec: acidv1.PostgresSpec{
PostgresqlParam: acidv1.PostgresqlParam{
PgVersion: "17",
},
TeamID: "myapp",
NumberOfInstances: 1,
Volume: acidv1.Volume{
Size: "1G",
},
},
clusterConfig: Config{
OpConfig: config.Config{
PodManagementPolicy: "ordered_ready",
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
SidecarContainers: []v1.Container{
{
Image: "test-image",
},
},
},
},
expectedError: "name is required",
},
{
name: "sidecar without image",
spec: acidv1.PostgresSpec{
PostgresqlParam: acidv1.PostgresqlParam{
PgVersion: "17",
},
TeamID: "myapp",
NumberOfInstances: 1,
Volume: acidv1.Volume{
Size: "1G",
},
Sidecars: []acidv1.Sidecar{
{
Name: "invalid-sidecar",
},
},
},
clusterConfig: Config{
OpConfig: config.Config{
PodManagementPolicy: "ordered_ready",
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
},
},
expectedError: "image is required",
},
{
name: "valid containers pass validation",
spec: acidv1.PostgresSpec{
PostgresqlParam: acidv1.PostgresqlParam{
PgVersion: "17",
},
TeamID: "myapp",
NumberOfInstances: 1,
Volume: acidv1.Volume{
Size: "1G",
},
Sidecars: []acidv1.Sidecar{
{
Name: "valid-sidecar",
DockerImage: "busybox:latest",
},
},
InitContainers: []v1.Container{
{
Name: "valid-initcontainer",
Image: "alpine:latest",
},
},
},
clusterConfig: Config{
OpConfig: config.Config{
PodManagementPolicy: "ordered_ready",
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
},
},
expectedError: "",
},
{
name: "multiple invalid sidecars",
spec: acidv1.PostgresSpec{
Sidecars: []acidv1.Sidecar{
{
Name: "sidecar1",
},
{
Name: "sidecar2",
},
},
},
expectedError: "image is required",
},
{
name: "empty container name and image",
spec: acidv1.PostgresSpec{
InitContainers: []v1.Container{
{
Name: "",
Image: "",
},
},
},
expectedError: "name is required",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
cluster := New(tc.clusterConfig, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder)

_, err := cluster.generateStatefulSet(&tc.spec)

if tc.expectedError != "" {
assert.Error(t, err)
assert.Contains(t, err.Error(), tc.expectedError)
} else {
assert.NoError(t, err)
}
})
}
}

func TestGeneratePodDisruptionBudget(t *testing.T) {
testName := "Test PodDisruptionBudget spec generation"

Expand Down Expand Up @@ -2618,7 +2798,8 @@ func TestGenerateService(t *testing.T) {
Name: "cluster-specific-sidecar",
},
{
Name: "cluster-specific-sidecar-with-resources",
Name: "cluster-specific-sidecar-with-resources",
DockerImage: "test-image",
Resources: &acidv1.Resources{
ResourceRequests: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("210m"), Memory: k8sutil.StringToPointer("0.8Gi")},
ResourceLimits: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("510m"), Memory: k8sutil.StringToPointer("1.4Gi")},
Expand Down Expand Up @@ -2928,6 +3109,7 @@ func TestGenerateResourceRequirements(t *testing.T) {
namespace := "default"
clusterNameLabel := "cluster-name"
sidecarName := "postgres-exporter"
dockerImage := "test-image"

// enforceMinResourceLimits will be called 2 times emitting 4 events (2x cpu, 2x memory raise)
// enforceMaxResourceRequests will be called 4 times emitting 6 events (2x cpu, 4x memory cap)
Expand Down Expand Up @@ -2993,7 +3175,8 @@ func TestGenerateResourceRequirements(t *testing.T) {
Spec: acidv1.PostgresSpec{
Sidecars: []acidv1.Sidecar{
{
Name: sidecarName,
Name: sidecarName,
DockerImage: dockerImage,
},
},
TeamID: "acid",
Expand Down Expand Up @@ -3232,7 +3415,8 @@ func TestGenerateResourceRequirements(t *testing.T) {
Spec: acidv1.PostgresSpec{
Sidecars: []acidv1.Sidecar{
{
Name: sidecarName,
Name: sidecarName,
DockerImage: dockerImage,
Resources: &acidv1.Resources{
ResourceRequests: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("10m"), Memory: k8sutil.StringToPointer("10Mi")},
ResourceLimits: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("100m"), Memory: k8sutil.StringToPointer("100Mi")},
Expand Down Expand Up @@ -3321,7 +3505,8 @@ func TestGenerateResourceRequirements(t *testing.T) {
Spec: acidv1.PostgresSpec{
Sidecars: []acidv1.Sidecar{
{
Name: sidecarName,
Name: sidecarName,
DockerImage: dockerImage,
Resources: &acidv1.Resources{
ResourceRequests: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("10m"), Memory: k8sutil.StringToPointer("10Mi")},
ResourceLimits: acidv1.ResourceDescription{CPU: k8sutil.StringToPointer("100m"), Memory: k8sutil.StringToPointer("100Mi")},
Expand Down