Bug fix for `EventInvokeConfig` #107

Vandita2020 · 2023-08-02T22:51:54Z

Issue #, if available:
Related PR: #92 #93

Summary
Fixing deletion part for EventInvokeConfig field.

Description:
This PR is related to bug fixes in previous PR related to EventInvokeConfig implementation. When the user deleted the set configurations for Function/Alias resource, the configurations were not deleted from the Console side, instead it used to throw an error. Fixed the bug by introducing a new condition to check if the desired configurations are nil or not, if yes then call the delete API.

Example of changes for EventInvokeConfig in Alias resource is given below:

if r.ko.Spec.FunctionEventInvokeConfig == nil {
	input_delete := &svcsdk.DeleteFunctionEventInvokeConfigInput{
		FunctionName: aws.String(*r.ko.Spec.FunctionName),
		Qualifier:    aws.String(*r.ko.Spec.Name),
	}
	_, err = rm.sdkapi.DeleteFunctionEventInvokeConfigWithContext(ctx, input_delete)
	rm.metrics.RecordAPICall("DELETE", "DeleteFunctionEventInvokeConfig", err)
	if err != nil {
		return nil, err
	}
	return r, nil
}

This PR contains implementation code and E2E test to validate the changes.

Acknowledgment
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

valerena · 2023-08-15T17:44:22Z

pkg/resource/function/hooks.go

-// GetFunctionConcurrency calls
-func (rm *resourceManager) setResourceAdditionalFields(
+// getFunctionConcurrency will describe the fields that are not return by GetFunctionConcurrency calls
+func (rm *resourceManager) getFunctionConcurrency(


I feel like all these methods getFunctionConcurrency, getFunctionCodeSigningConfig, getFunctionEventInvokeConfig would be better called set.... (instead of get..), especially because they don't actually return those fields, they actually set them inside the ko.Spec (and conceptually they're similar and small parts of the big setResourceAdditionalFields)

Makes sense. I have made the changes. Thanks!

valerena · 2023-08-15T17:47:48Z

pkg/resource/function/hooks.go

+			cloudFunctionEventInvokeConfig := &svcapitypes.PutFunctionEventInvokeConfigInput{}
+			cloudFunctionEventInvokeConfig.DestinationConfig = &svcapitypes.DestinationConfig{}
+			cloudFunctionEventInvokeConfig.DestinationConfig.OnFailure = &svcapitypes.OnFailure{}
+			cloudFunctionEventInvokeConfig.DestinationConfig.OnSuccess = &svcapitypes.OnSuccess{}
+			cloudFunctionEventInvokeConfig.DestinationConfig.OnFailure.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure.Destination
+			cloudFunctionEventInvokeConfig.DestinationConfig.OnSuccess.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess.Destination
+			cloudFunctionEventInvokeConfig.MaximumEventAgeInSeconds = getFunctionEventInvokeConfigOutput.MaximumEventAgeInSeconds
+			cloudFunctionEventInvokeConfig.MaximumRetryAttempts = getFunctionEventInvokeConfigOutput.MaximumRetryAttempts
+			ko.Spec.FunctionEventInvokeConfig = cloudFunctionEventInvokeConfig


Do we really need to have all these empty objects? Shouldn't this be just ko.Spec.FunctionEventInvokeConfig = nil ?

Well here I am not assigning ko.Spec.FunctionEventInvokeConfig to nil, instead creating the FunctionEventInvokeConfig object and assigning it to the values returned from GetFunctionEventInvokeConfig API call. I am creating the whole FunctionEventInvokeConfig object because this is for the case where the desired spec for FunctionEventInvokeConfig is nil and Spec.ko.FunctionEventInvokeConfig doesn't exist. So we are creating the whole object.
To create the FunctionEventInvokeConfig I need to define every object properly, since the DestinationConfig field is highly nested, I had to define their respective type for each following fields. This is the reason behind this multiple lines of code. Tbh, I am not okay with so many lines either but I don't see other way around.

valerena · 2023-08-15T17:50:55Z

pkg/resource/function/hooks.go

-			if getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure != nil {
-				if getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure.Destination != nil {
-					ko.Spec.FunctionEventInvokeConfig.DestinationConfig.OnFailure.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure.Destination
+		if ko.Spec.FunctionEventInvokeConfig != nil {


Shouldn't this if be checking getFunctionEventInvokeConfigOutput != nil instead? My understanding is that you want to check what's this response, and based on that you want to modify ko.Spec

I have added an additional if condition checking getFunctionEventInvokeConfigOutput != nil after checking ko.Spec.FunctionEventInvokeConfig != nil. We newly added the ko.Spec.FunctionEventInvokeConfig check, the reason was to see if the desired spec configuration for EventInvokeConfig is there or not, as we are handling the deletion part in the else condition.

valerena · 2023-08-15T17:52:03Z

pkg/resource/function/hooks.go

+			if getFunctionEventInvokeConfigOutput.DestinationConfig != nil {
+				ko.Spec.FunctionEventInvokeConfig.MaximumRetryAttempts = getFunctionEventInvokeConfigOutput.MaximumRetryAttempts


The if is wrong (it checks DestinationConfig instead of MaximumRetryAttempts)

Oh, thanks so much. My bad, I missed that.

valerena · 2023-08-15T17:56:14Z

pkg/resource/function/hooks.go

+			if getFunctionEventInvokeConfigOutput.DestinationConfig != nil {
+				if getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure != nil {
+					if getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure.Destination != nil {
+						ko.Spec.FunctionEventInvokeConfig.DestinationConfig.OnFailure.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnFailure.Destination
+					}
 				}
-			}
-			if getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess != nil {
-				if getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess.Destination != nil {
-					ko.Spec.FunctionEventInvokeConfig.DestinationConfig.OnSuccess.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess.Destination
+				if getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess != nil {
+					if getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess.Destination != nil {
+						ko.Spec.FunctionEventInvokeConfig.DestinationConfig.OnSuccess.Destination = getFunctionEventInvokeConfigOutput.DestinationConfig.OnSuccess.Destination
+					}


I think you need the else cases here. If getFunctionEventInvokeConfigOutput has DestinationConfig but doesn't have DestinationConfig.OnFailure, then you need to set ko.Spec.FunctionEventInvokeConfig.DestinationConfig.OnFailure = nil.

Same for the onSuccess (For cases where you had both and deleted one or had one and changed for the other one)

valerena · 2023-08-15T17:59:01Z

pkg/resource/function/hooks.go

+	return nil
+}
+
+func (rm *resourceManager) setFunctionEventInvokeConfigFromResponse(


Is this basically the same function that you have in alias/hooks.go? Is it possible to actually define it once and use it in both places? (Because all my comments below apply to both)

Yes, both functions do same thing. It is possible to define a separate file with the setFunctionEventInvokeConfigFromResponse function. However I am not sure how the placement of the new file will go, as in the ack-repo every file is placed within its resource. For now, I have applied similar changes to alias.hooks.go too

Ok. We might see something to make that better in the future.

pkg/resource/function/hooks.go

valerena · 2023-08-16T23:30:06Z

pkg/resource/function/hooks.go

-// setResourceAdditionalFields will describe the fields that are not return by
-// GetFunctionConcurrency calls
-func (rm *resourceManager) setResourceAdditionalFields(
+// getFunctionConcurrency will describe the fields that are not return by GetFunctionConcurrency calls


Is this comment accurate? (First, the function name is wrong (get vs set), but also) are these the fields that are not returned by GetFunctionConcurrency? It looks to me that it's ReservedConcurrentExecutions, which is actually returned from GetFunctionConcurrency. (I guess it's a comment that was here before and you didn't change it with this PR, but we can update it)

Corrected the name and changed the description to more inline with the function property.

valerena · 2023-08-16T23:30:57Z

pkg/resource/function/hooks.go

+	return nil
+}
+
+// getFunctionCodeSigningConfig will describe the code signing


You also forgot to update the function name in this comment

valerena · 2023-08-16T23:31:36Z

pkg/resource/function/hooks.go

+	return nil
+}
+
+func (rm *resourceManager) setFunctionEventInvokeConfigFromResponse(


Ok. We might see something to make that better in the future.

pkg/resource/function/hooks.go

a-hilaly · 2023-08-21T23:03:29Z

pkg/resource/alias/hooks.go

@@ -24,6 +24,8 @@ import (
 	svcapitypes "github.com/aws-controllers-k8s/lambda-controller/apis/v1alpha1"
 )

+// syncFunctionEventInvokeConfig calls `PutFunctionEventInvokeConfig` to update the fields


Suggested change

// syncFunctionEventInvokeConfig calls `PutFunctionEventInvokeConfig` to update the fields

// syncEventInvokeConfig calls `PutFunctionEventInvokeConfig` to update the fields

in go function comments have to always start with function "exact" name. https://tip.golang.org/doc/comment

a-hilaly · 2023-08-21T23:16:45Z

pkg/resource/alias/hooks.go

 	rm.metrics.RecordAPICall("GET", "GetFunctionEventInvokeConfig", err)

+	err = rm.setFunctionEventInvokeConfigFromResponse(ko, getFunctionEventInvokeConfigOutput, err)


instead of passing this err as an argument can you please handle this case before? like a if err != nil ...

a-hilaly · 2023-08-21T23:17:41Z

pkg/resource/alias/hooks.go

+	if apiError != nil {
+		if awserr, ok := ackerr.AWSError(apiError); ok && (awserr.Code() == "EventInvokeConfigNotFoundException" || awserr.Code() == "ResourceNotFoundException") {
+			ko.Spec.FunctionEventInvokeConfig = nil
+		} else {
+			return apiError
+		}


The function is called setFunctionEventInvokeConfigFromResponse and is only supposed to set new fields, not handle errors they are not supposed to.

a-hilaly · 2023-08-21T23:18:26Z

pkg/resource/function/hooks.go

+	if dspec.FunctionEventInvokeConfig.MaximumEventAgeInSeconds != nil {
+		input.MaximumEventAgeInSeconds = aws.Int64(*dspec.FunctionEventInvokeConfig.MaximumEventAgeInSeconds)
+	}
+	if dspec.FunctionEventInvokeConfig.MaximumRetryAttempts != nil {
+		input.MaximumRetryAttempts = aws.Int64(*dspec.FunctionEventInvokeConfig.MaximumRetryAttempts)
 	}


why are these handled outside of the top if statement in L524

Because these two MaximumEventInvokeConfig and MaximumRetryAttempts are another fields in FunctionEventInvokeConfig and not a part of DestinationConfig field.

a-hilaly · 2023-08-21T23:18:51Z

pkg/resource/function/hooks.go

 			FunctionName: ko.Spec.Name,
 		},
 	)
-	rm.metrics.RecordAPICall("GET", "GetFunctionEventInvokeConfig", err)
+	rm.metrics.RecordAPICall("GET", "GetFunctionCodeSigningConfig", err)


nice catch!

a-hilaly

👍
/lgtm

ack-prow · 2023-08-23T13:28:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, valerena, Vandita2020

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Vandita2020,a-hilaly,valerena]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

a-hilaly · 2023-08-23T13:28:11Z

/retest

a-hilaly · 2023-08-23T13:28:19Z

/test all

ack-prow bot requested review from a-hilaly and jljaco August 2, 2023 22:51

ack-prow bot added the approved label Aug 2, 2023

valerena requested changes Aug 15, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_correction branch 2 times, most recently from b99efad to 329e1d6 Compare August 16, 2023 22:22

valerena reviewed Aug 16, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_correction branch 2 times, most recently from 2caeb2f to 26dab5a Compare August 17, 2023 22:09

valerena approved these changes Aug 17, 2023

View reviewed changes

Bug fix for EventInvokeConfig in Function

d0a1dd3

Vandita2020 force-pushed the event_invoke_config_correction branch from 26dab5a to 8891b06 Compare August 21, 2023 23:01

a-hilaly reviewed Aug 21, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_correction branch from 8891b06 to 17806d3 Compare August 21, 2023 23:29

Bug fix for EventInvokeConfig in Alias

07b712d

Vandita2020 force-pushed the event_invoke_config_correction branch from 17806d3 to 07b712d Compare August 22, 2023 02:04

a-hilaly approved these changes Aug 23, 2023

View reviewed changes

ack-prow bot assigned a-hilaly Aug 23, 2023

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023

ack-prow bot merged commit 166103d into aws-controllers-k8s:main Aug 23, 2023

		if getFunctionEventInvokeConfigOutput.DestinationConfig != nil {
		ko.Spec.FunctionEventInvokeConfig.MaximumRetryAttempts = getFunctionEventInvokeConfigOutput.MaximumRetryAttempts

	// syncFunctionEventInvokeConfig calls `PutFunctionEventInvokeConfig` to update the fields
	// syncEventInvokeConfig calls `PutFunctionEventInvokeConfig` to update the fields

		rm.metrics.RecordAPICall("GET", "GetFunctionEventInvokeConfig", err)

		err = rm.setFunctionEventInvokeConfigFromResponse(ko, getFunctionEventInvokeConfigOutput, err)

Bug fix for EventInvokeConfig #107

Bug fix for EventInvokeConfig #107

Uh oh!

Conversation

Vandita2020 commented Aug 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Aug 23, 2023

Uh oh!

a-hilaly commented Aug 23, 2023

Uh oh!

a-hilaly commented Aug 23, 2023

Uh oh!

Uh oh!

Bug fix for `EventInvokeConfig` #107

Bug fix for `EventInvokeConfig` #107

Vandita2020 commented Aug 2, 2023 •

edited

Loading