Skip to content

Add support for "pod-overrides" attribute #158

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.

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

Merged
merged 4 commits into from
Jan 5, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion pkg/devfile/generator/generators.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,20 @@ func GetDeployment(devfileObj parser.DevfileObj, deployParams DeploymentParams)
Volumes: deployParams.Volumes,
}

globalAttributes, err := devfileObj.Data.GetAttributes()
if err != nil {
return nil, err
}
components, err := devfileObj.Data.GetDevfileContainerComponents(common.DevfileOptions{})
if err != nil {
return nil, err
}
podTemplateSpec, err := getPodTemplateSpec(globalAttributes, components, podTemplateSpecParams)
if err != nil {
return nil, err
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you see if you can add one test to cover the err case for this func

}
deploySpecParams := deploymentSpecParams{
PodTemplateSpec: *getPodTemplateSpec(podTemplateSpecParams),
PodTemplateSpec: *podTemplateSpec,
PodSelectorLabels: deployParams.PodSelectorLabels,
Replicas: deployParams.Replicas,
}
Expand Down
99 changes: 88 additions & 11 deletions pkg/devfile/generator/generators_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,16 @@ package generator

import (
"fmt"
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"reflect"
"strings"
"testing"

"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/pointer"
"reflect"
"strings"
"testing"

v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/devfile/api/v2/pkg/attributes"
Expand Down Expand Up @@ -1076,7 +1078,9 @@ func TestGetDeployment(t *testing.T) {
name string
containerComponents []v1.Component
deploymentParams DeploymentParams
expected appsv1.Deployment
expected *appsv1.Deployment
attributes attributes.Attributes
wantErr bool
}{
{
// Currently dedicatedPod can only filter out annotations
Expand Down Expand Up @@ -1108,7 +1112,7 @@ func TestGetDeployment(t *testing.T) {
Containers: containers,
Replicas: pointer.Int32Ptr(1),
},
expected: appsv1.Deployment{
expected: &appsv1.Deployment{
ObjectMeta: objectMetaDedicatedPod,
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
Expand Down Expand Up @@ -1154,7 +1158,7 @@ func TestGetDeployment(t *testing.T) {
},
Containers: containers,
},
expected: appsv1.Deployment{
expected: &appsv1.Deployment{
ObjectMeta: objectMeta,
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
Expand All @@ -1172,6 +1176,78 @@ func TestGetDeployment(t *testing.T) {
},
},
},
{
name: "pod should have pod-overrides attribute",
containerComponents: []v1.Component{
testingutil.GenerateDummyContainerComponent("container1", nil, []v1.Endpoint{
{
Name: "http-8080",
TargetPort: 8080,
},
}, nil, v1.Annotation{
Deployment: map[string]string{
"key1": "value1",
},
}, nil),
testingutil.GenerateDummyContainerComponent("container2", nil, nil, nil, v1.Annotation{
Deployment: map[string]string{
"key2": "value2",
},
}, nil),
},
attributes: attributes.Attributes{
PodOverridesAttribute: apiext.JSON{Raw: []byte("{\"spec\": {\"serviceAccountName\": \"new-service-account\"}}")},
},
deploymentParams: DeploymentParams{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"preserved-key": "preserved-value",
},
},
Containers: containers,
},
expected: &appsv1.Deployment{
ObjectMeta: objectMeta,
Spec: appsv1.DeploymentSpec{
Strategy: appsv1.DeploymentStrategy{
Type: appsv1.RecreateDeploymentStrategyType,
},
Selector: &metav1.LabelSelector{
MatchLabels: nil,
},
Template: corev1.PodTemplateSpec{
ObjectMeta: objectMeta,
Spec: corev1.PodSpec{
Containers: containers,
ServiceAccountName: "new-service-account",
},
},
},
},
},
{
name: "pod has an invalid pod-overrides attribute that throws error",
containerComponents: []v1.Component{
testingutil.GenerateDummyContainerComponent("container2", nil, nil, nil, v1.Annotation{
Deployment: map[string]string{
"key2": "value2",
},
}, nil),
},
attributes: attributes.Attributes{
PodOverridesAttribute: apiext.JSON{Raw: []byte("{\"spec\": \"serviceAccountName\": \"new-service-account\"}}")},
},
deploymentParams: DeploymentParams{
ObjectMeta: metav1.ObjectMeta{
Annotations: map[string]string{
"preserved-key": "preserved-value",
},
},
Containers: containers,
},
expected: nil,
wantErr: trueBool,
},
}

for _, tt := range tests {
Expand All @@ -1186,18 +1262,19 @@ func TestGetDeployment(t *testing.T) {
},
}
// set up the mock data
mockGetComponents := mockDevfileData.EXPECT().GetComponents(options)
mockGetComponents.Return(tt.containerComponents, nil).AnyTimes()
mockDevfileData.EXPECT().GetAttributes().Return(tt.attributes, nil).AnyTimes()
mockDevfileData.EXPECT().GetDevfileContainerComponents(common.DevfileOptions{}).Return(tt.containerComponents, nil).AnyTimes()
mockDevfileData.EXPECT().GetComponents(options).Return(tt.containerComponents, nil).AnyTimes()

devObj := parser.DevfileObj{
Data: mockDevfileData,
}
deploy, err := GetDeployment(devObj, tt.deploymentParams)
// Checks for unexpected error cases
if err != nil {
t.Errorf("TestGetDeployment(): unexpected error %v", err)
if !tt.wantErr == (err != nil) {
t.Errorf("TestGetDeployment(): unexpected error %v, wantErr %v", err, tt.wantErr)
}
assert.Equal(t, tt.expected, *deploy, "TestGetDeployment(): The two values should be the same.")
assert.Equal(t, tt.expected, deploy, "TestGetDeployment(): The two values should be the same.")

})
}
Expand Down
122 changes: 118 additions & 4 deletions pkg/devfile/generator/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,27 +18,33 @@ package generator
import (
"encoding/json"
"fmt"
"github.com/hashicorp/go-multierror"
"path/filepath"
"reflect"
"strings"

v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/devfile/api/v2/pkg/attributes"

"github.com/devfile/library/v2/pkg/devfile/parser"
"github.com/devfile/library/v2/pkg/devfile/parser/data/v2/common"
"github.com/hashicorp/go-multierror"
buildv1 "github.com/openshift/api/build/v1"
routev1 "github.com/openshift/api/route/v1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
extensionsv1 "k8s.io/api/extensions/v1beta1"
networkingv1 "k8s.io/api/networking/v1"
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/apimachinery/pkg/util/strategicpatch"
)

const ContainerOverridesAttribute = "container-overrides"
const (
ContainerOverridesAttribute = "container-overrides"
PodOverridesAttribute = "pod-overrides"
)

// convertEnvs converts environment variables from the devfile structure to kubernetes structure
func convertEnvs(vars []v1.EnvVar) []corev1.EnvVar {
Expand Down Expand Up @@ -235,7 +241,7 @@ type podTemplateSpecParams struct {
}

// getPodTemplateSpec gets a pod template spec that can be used to create a deployment spec
func getPodTemplateSpec(podTemplateSpecParams podTemplateSpecParams) *corev1.PodTemplateSpec {
func getPodTemplateSpec(globalAttributes attributes.Attributes, components []v1.Component, podTemplateSpecParams podTemplateSpecParams) (*corev1.PodTemplateSpec, error) {
podTemplateSpec := &corev1.PodTemplateSpec{
ObjectMeta: podTemplateSpecParams.ObjectMeta,
Spec: corev1.PodSpec{
Expand All @@ -245,7 +251,115 @@ func getPodTemplateSpec(podTemplateSpecParams podTemplateSpecParams) *corev1.Pod
},
}

return podTemplateSpec
if needsPodOverrides(globalAttributes, components) {
patchedPodTemplateSpec, err := applyPodOverrides(globalAttributes, components, podTemplateSpec)
if err != nil {
return nil, err
}
patchedPodTemplateSpec.ObjectMeta = podTemplateSpecParams.ObjectMeta
podTemplateSpec = patchedPodTemplateSpec
}

return podTemplateSpec, nil
}

// needsPodOverrides returns true if PodOverridesAttribute is present at Devfile or Container level attributes
func needsPodOverrides(globalAttributes attributes.Attributes, components []v1.Component) bool {
if globalAttributes.Exists(PodOverridesAttribute) {
return true
}
for _, component := range components {
if component.Attributes.Exists(PodOverridesAttribute) {
return true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a test case where we have only component attribute and no global attribute to cover this?

}
}
return false
}

// applyPodOverrides returns a list of all the PodOverridesAttribute set at Devfile and Container level attributes
func applyPodOverrides(globalAttributes attributes.Attributes, components []v1.Component, podTemplateSpec *corev1.PodTemplateSpec) (*corev1.PodTemplateSpec, error) {
overrides, err := getPodOverrides(globalAttributes, components)
if err != nil {
return nil, err
}
// Workaround: the definition for corev1.PodSpec does not make containers optional, so even a nil list
// will be interpreted as "delete all containers" as the serialized patch will include "containers": null.
// To avoid this, save the original containers and reset them at the end.
originalContainers := podTemplateSpec.Spec.Containers
// Save fields we do not allow to be configured in pod-overrides
originalInitContainers := podTemplateSpec.Spec.InitContainers
originalVolumes := podTemplateSpec.Spec.Volumes

patchedTemplateBytes, err := json.Marshal(podTemplateSpec)
if err != nil {
return nil, fmt.Errorf("failed to marshal deployment to yaml: %w", err)
}
for _, override := range overrides {
patchedTemplateBytes, err = strategicpatch.StrategicMergePatch(patchedTemplateBytes, override.Raw, &corev1.PodTemplateSpec{})
if err != nil {
return nil, fmt.Errorf("error applying pod overrides: %w", err)
}
}
patchedPodTemplateSpec := corev1.PodTemplateSpec{}
if err := json.Unmarshal(patchedTemplateBytes, &patchedPodTemplateSpec); err != nil {
return nil, fmt.Errorf("error applying pod overrides: %w", err)
}
patchedPodTemplateSpec.Spec.Containers = originalContainers
patchedPodTemplateSpec.Spec.InitContainers = originalInitContainers
patchedPodTemplateSpec.Spec.Volumes = originalVolumes
return &patchedPodTemplateSpec, nil
}

// getPodOverrides returns PodTemplateSpecOverrides for every instance of the pod overrides attribute
// present in the DevWorkspace. The order of elements is
// 1. Pod overrides defined on Container components, in the order they appear in the DevWorkspace
// 2. Pod overrides defined in the global attributes field (.spec.template.attributes)
func getPodOverrides(globalAttributes attributes.Attributes, components []v1.Component) ([]apiext.JSON, error) {
var allOverrides []apiext.JSON

for _, component := range components {
if component.Attributes.Exists(PodOverridesAttribute) {
override := corev1.PodTemplateSpec{}
// Check format of pod-overrides to detect errors early
if err := component.Attributes.GetInto(PodOverridesAttribute, &override); err != nil {
return nil, fmt.Errorf("failed to parse %s attribute on component %s: %w", PodOverridesAttribute, component.Name, err)
}
// Do not allow overriding containers or volumes
if override.Spec.Containers != nil {
return nil, fmt.Errorf("cannot use %s to override pod containers (component %s)", PodOverridesAttribute, component.Name)
}
if override.Spec.InitContainers != nil {
return nil, fmt.Errorf("cannot use %s to override pod initContainers (component %s)", PodOverridesAttribute, component.Name)
}
if override.Spec.Volumes != nil {
return nil, fmt.Errorf("cannot use %s to override pod volumes (component %s)", PodOverridesAttribute, component.Name)
}
patchData := component.Attributes[PodOverridesAttribute]
allOverrides = append(allOverrides, patchData)
}
}

if globalAttributes.Exists(PodOverridesAttribute) {
override := corev1.PodTemplateSpec{}
err := globalAttributes.GetInto(PodOverridesAttribute, &override)
if err != nil {
return nil, fmt.Errorf("failed to parse %s attribute for pod: %w", PodOverridesAttribute, err)
}
// Do not allow overriding containers or volumes
if override.Spec.Containers != nil {
return nil, fmt.Errorf("cannot use %s to override pod containers", PodOverridesAttribute)
}
if override.Spec.InitContainers != nil {
return nil, fmt.Errorf("cannot use %s to override pod initContainers", PodOverridesAttribute)
}
if override.Spec.Volumes != nil {
return nil, fmt.Errorf("cannot use %s to override pod volumes", PodOverridesAttribute)
}
patchData := globalAttributes[PodOverridesAttribute]
allOverrides = append(allOverrides, patchData)
}

return allOverrides, nil
}

// deploymentSpecParams is a struct that contains the required data to create a deployment spec object
Expand Down
Loading