Skip to content

Commit b0d9603

Browse files
a-hilalya-hilaly
authored
Stop defaulting non-modified fields when trying to update a Function (#41)
Fixes aws-controllers-k8s/community#1413 This patch modifies the update code path for a `Function` resource to fix a few bugs observed while updating functions created using a container image. Description of the changes: - Only try to update `Spec.Code` or `Spec.Configuration` at once. It is not correct to sequentially call `UpdateFunctionConfiguration` and `UpdateFunctionCode` because both of them can put the function in a `Pending` state. - Stop defaulting non-modified fields when trying to update a function. Signed-off-by: Amine Hilaly <[email protected]> By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent 65365f0 commit b0d9603

File tree

2 files changed

+113
-81
lines changed

2 files changed

+113
-81
lines changed

‎pkg/resource/function/hooks.go

Lines changed: 101 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -67,41 +67,18 @@ func (rm *resourceManager) customUpdateFunction(
6767
return nil, requeueWaitWhilePending
6868
}
6969

70-
if delta.DifferentAt("Spec.Code") {
71-
err = rm.updateFunctionCode(ctx, desired, delta)
72-
if err != nil {
73-
return nil, err
74-
}
75-
}
7670
if delta.DifferentAt("Spec.Tags") {
7771
err = rm.updateFunctionTags(ctx, latest, desired)
7872
if err != nil {
7973
return nil, err
8074
}
8175
}
82-
if delta.DifferentAt("Spec.Description") ||
83-
delta.DifferentAt("Spec.Handler") ||
84-
delta.DifferentAt("Spec.KMSKeyARN") ||
85-
delta.DifferentAt("Spec.MemorySize") ||
86-
delta.DifferentAt("Spec.Role") ||
87-
delta.DifferentAt("Spec.Timeout") ||
88-
delta.DifferentAt("Spec.Environement") ||
89-
delta.DifferentAt("Spec.FileSystemConfigs") ||
90-
delta.DifferentAt("Spec.ImageConfig") ||
91-
delta.DifferentAt("Spec.TracingConfig") ||
92-
delta.DifferentAt("Spec.VPCConfig") {
93-
err = rm.updateFunctionConfiguration(ctx, desired)
94-
if err != nil {
95-
return nil, err
96-
}
97-
}
9876
if delta.DifferentAt("Spec.ReservedConcurrentExecutions") {
9977
err = rm.updateFunctionConcurrency(ctx, desired)
10078
if err != nil {
10179
return nil, err
10280
}
10381
}
104-
10582
if delta.DifferentAt("Spec.CodeSigningConfigARN") {
10683
if desired.ko.Spec.PackageType != nil && *desired.ko.Spec.PackageType == "Image" &&
10784
desired.ko.Spec.CodeSigningConfigARN != nil && *desired.ko.Spec.CodeSigningConfigARN != "" {
@@ -114,6 +91,27 @@ func (rm *resourceManager) customUpdateFunction(
11491
}
11592
}
11693

94+
// Only try to update Spec.Code or Spec.Configuration at once. It is
95+
// not correct to sequentially call UpdateFunctionConfiguration and
96+
// UpdateFunctionCode because both of them can put the function in a
97+
// Pending state.
98+
switch {
99+
case delta.DifferentAt("Spec.Code"):
100+
err = rm.updateFunctionCode(ctx, desired, delta)
101+
if err != nil {
102+
return nil, err
103+
}
104+
case delta.DifferentExcept(
105+
"Spec.Code",
106+
"Spec.Tags",
107+
"Spec.ReservedConcurrentExecutions",
108+
"Spec.CodeSigningConfigARN"):
109+
err = rm.updateFunctionConfiguration(ctx, desired, delta)
110+
if err != nil {
111+
return nil, err
112+
}
113+
}
114+
117115
readOneLatest, err := rm.ReadOne(ctx, desired)
118116
if err != nil {
119117
return nil, err
@@ -126,6 +124,7 @@ func (rm *resourceManager) customUpdateFunction(
126124
func (rm *resourceManager) updateFunctionConfiguration(
127125
ctx context.Context,
128126
desired *resource,
127+
delta *ackcompare.Delta,
129128
) error {
130129
var err error
131130
rlog := ackrtlog.FromContext(ctx)
@@ -137,86 +136,107 @@ func (rm *resourceManager) updateFunctionConfiguration(
137136
FunctionName: aws.String(*dspec.Name),
138137
}
139138

140-
if dspec.Description != nil {
141-
input.Description = aws.String(*dspec.Description)
142-
} else {
143-
input.Description = aws.String("")
144-
}
145-
146-
if dspec.Handler != nil {
147-
input.Handler = aws.String(*dspec.Handler)
148-
} else {
149-
input.Handler = aws.String("")
139+
if delta.DifferentAt("Spec.Description") {
140+
if dspec.Description != nil {
141+
input.Description = aws.String(*dspec.Description)
142+
} else {
143+
input.Description = aws.String("")
144+
}
150145
}
151146

152-
if dspec.KMSKeyARN != nil {
153-
input.KMSKeyArn = aws.String(*dspec.KMSKeyARN)
154-
} else {
155-
input.KMSKeyArn = aws.String("")
147+
if delta.DifferentAt("Spec.Handler") {
148+
if dspec.Handler != nil {
149+
input.Handler = aws.String(*dspec.Handler)
150+
} else {
151+
input.Handler = aws.String("")
152+
}
156153
}
157154

158-
if dspec.Role != nil {
159-
input.Role = aws.String(*dspec.Role)
160-
} else {
161-
input.Role = aws.String("")
155+
if delta.DifferentAt("Spec.KMSKeyARN") {
156+
if dspec.KMSKeyARN != nil {
157+
input.KMSKeyArn = aws.String(*dspec.KMSKeyARN)
158+
} else {
159+
input.KMSKeyArn = aws.String("")
160+
}
162161
}
163162

164-
if dspec.MemorySize != nil {
165-
input.MemorySize = aws.Int64(*dspec.MemorySize)
166-
} else {
167-
input.MemorySize = aws.Int64(0)
163+
if delta.DifferentAt("Spec.Role") {
164+
if dspec.Role != nil {
165+
input.Role = aws.String(*dspec.Role)
166+
} else {
167+
input.Role = aws.String("")
168+
}
168169
}
169170

170-
if dspec.Timeout != nil {
171-
input.Timeout = aws.Int64(*dspec.Timeout)
172-
} else {
173-
input.Timeout = aws.Int64(0)
171+
if delta.DifferentAt("Spec.MemorySize") {
172+
if dspec.MemorySize != nil {
173+
input.MemorySize = aws.Int64(*dspec.MemorySize)
174+
} else {
175+
input.MemorySize = aws.Int64(0)
176+
}
174177
}
175178

176-
environment := &svcsdk.Environment{}
177-
if dspec.Environment != nil {
178-
environment.Variables = dspec.Environment.DeepCopy().Variables
179-
179+
if delta.DifferentAt("Spec.Timeout") {
180+
if dspec.Timeout != nil {
181+
input.Timeout = aws.Int64(*dspec.Timeout)
182+
} else {
183+
input.Timeout = aws.Int64(0)
184+
}
180185
}
181-
input.Environment = environment
182186

183-
fileSystemConfigs := []*svcsdk.FileSystemConfig{}
184-
if len(dspec.FileSystemConfigs) > 0 {
185-
for _, elem := range dspec.FileSystemConfigs {
186-
elemCopy := elem.DeepCopy()
187-
fscElem := &svcsdk.FileSystemConfig{
188-
Arn: elemCopy.ARN,
189-
LocalMountPath: elemCopy.LocalMountPath,
187+
if delta.DifferentAt("Spec.Environment") {
188+
environment := &svcsdk.Environment{}
189+
if dspec.Environment != nil {
190+
environment.Variables = dspec.Environment.DeepCopy().Variables
191+
}
192+
input.Environment = environment
193+
}
194+
195+
if delta.DifferentAt("Spec.FileSystemConfigs") {
196+
fileSystemConfigs := []*svcsdk.FileSystemConfig{}
197+
if len(dspec.FileSystemConfigs) > 0 {
198+
for _, elem := range dspec.FileSystemConfigs {
199+
elemCopy := elem.DeepCopy()
200+
fscElem := &svcsdk.FileSystemConfig{
201+
Arn: elemCopy.ARN,
202+
LocalMountPath: elemCopy.LocalMountPath,
203+
}
204+
fileSystemConfigs = append(fileSystemConfigs, fscElem)
190205
}
191-
fileSystemConfigs = append(fileSystemConfigs, fscElem)
206+
input.FileSystemConfigs = fileSystemConfigs
192207
}
193-
input.FileSystemConfigs = fileSystemConfigs
194208
}
195209

196-
if dspec.ImageConfig != nil && dspec.Code.ImageURI != nil && *dspec.Code.ImageURI != "" {
197-
imageConfig := &svcsdk.ImageConfig{}
198-
if dspec.ImageConfig != nil {
199-
imageConfigCopy := dspec.ImageConfig.DeepCopy()
200-
imageConfig.Command = imageConfigCopy.Command
201-
imageConfig.EntryPoint = imageConfigCopy.EntryPoint
202-
imageConfig.WorkingDirectory = imageConfigCopy.WorkingDirectory
210+
if delta.DifferentAt("Spec.ImageConfig") {
211+
if dspec.ImageConfig != nil && dspec.Code.ImageURI != nil && *dspec.Code.ImageURI != "" {
212+
imageConfig := &svcsdk.ImageConfig{}
213+
if dspec.ImageConfig != nil {
214+
imageConfigCopy := dspec.ImageConfig.DeepCopy()
215+
imageConfig.Command = imageConfigCopy.Command
216+
imageConfig.EntryPoint = imageConfigCopy.EntryPoint
217+
imageConfig.WorkingDirectory = imageConfigCopy.WorkingDirectory
218+
}
219+
input.ImageConfig = imageConfig
203220
}
204-
input.ImageConfig = imageConfig
205221
}
206222

207-
tracingConfig := &svcsdk.TracingConfig{}
208-
if dspec.TracingConfig != nil {
209-
tracingConfig.Mode = aws.String(*dspec.TracingConfig.Mode)
223+
if delta.DifferentAt("Spec.TracingConfig") {
224+
tracingConfig := &svcsdk.TracingConfig{}
225+
if dspec.TracingConfig != nil {
226+
tracingConfig.Mode = aws.String(*dspec.TracingConfig.Mode)
227+
}
228+
input.TracingConfig = tracingConfig
210229
}
211-
input.TracingConfig = tracingConfig
212230

213-
VPCConfig := &svcsdk.VpcConfig{}
214-
if dspec.VPCConfig != nil {
215-
vpcConfigCopy := dspec.VPCConfig.DeepCopy()
216-
VPCConfig.SubnetIds = vpcConfigCopy.SubnetIDs
217-
VPCConfig.SecurityGroupIds = vpcConfigCopy.SecurityGroupIDs
231+
if delta.DifferentAt("Spec.VPCConfig") {
232+
VPCConfig := &svcsdk.VpcConfig{}
233+
if dspec.VPCConfig != nil {
234+
vpcConfigCopy := dspec.VPCConfig.DeepCopy()
235+
VPCConfig.SubnetIds = vpcConfigCopy.SubnetIDs
236+
VPCConfig.SecurityGroupIds = vpcConfigCopy.SecurityGroupIDs
237+
}
238+
input.VpcConfig = VPCConfig
218239
}
219-
input.VpcConfig = VPCConfig
220240

221241
_, err = rm.sdkapi.UpdateFunctionConfigurationWithContext(ctx, input)
222242
rm.metrics.RecordAPICall("UPDATE", "UpdateFunctionConfiguration", err)

‎test/e2e/tests/test_function.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,7 @@ def test_smoke(self, lambda_client):
130130
"v3": "k3",
131131
}
132132
cr["spec"]["description"] = "Updated description"
133+
cr["spec"]["timeout"] = 10
133134
cr["spec"]["tags"] = update_tags
134135

135136
# Patch k8s resource
@@ -140,6 +141,7 @@ def test_smoke(self, lambda_client):
140141
function = lambda_validator.get_function(resource_name)
141142
assert function is not None
142143
assert function["Configuration"]["Description"] == "Updated description"
144+
assert function["Configuration"]["Timeout"] == 10
143145

144146
function_tags = function["Tags"]
145147
tags.assert_ack_system_tags(
@@ -324,6 +326,16 @@ def test_function_package_type_image(self, lambda_client):
324326
# Check Lambda function exists
325327
assert lambda_validator.function_exists(resource_name)
326328

329+
cr["spec"]["timeout"] = 10
330+
331+
# Patch k8s resource
332+
k8s.patch_custom_resource(ref, cr)
333+
time.sleep(UPDATE_WAIT_AFTER_SECONDS)
334+
335+
# Check function updated fields
336+
function = lambda_validator.get_function(resource_name)
337+
assert function["Configuration"]["Timeout"] == 10
338+
327339
# Delete k8s resource
328340
_, deleted = k8s.delete_custom_resource(ref)
329341
assert deleted is True

0 commit comments

Comments
 (0)