Adding `EventInvokeConfig` support for `Alias` #93

Vandita2020 · 2023-06-02T21:12:01Z

Issue #, if available:

Summary
Adding support to configure EventInvokeConfig for alias in ACK.

Description
This PR adds support for EventInvokeConfig permitted by Lambda API operations in ACK. To implement this functionality I added EventInvokeConfig as an inline property to alias. Thus the user can directly set configurations for async invocation (EventInvokeConfig) while creating alias. The user can edit the following code to create eventInvokeConfig along with alias.

apiVersion: lambda.services.k8s.aws/v1alpha1
kind: Alias
metadata:
  name: $ALIAS_NAME
  annotations:
    services.k8s.aws/region: $AWS_REGION
spec:
  name: $ALIAS_NAME
  functionName: $FUNCTION_NAME
  functionVersion: $FUNCTION_VERSION
  functionEventInvokeConfig:
    destinationConfig:
      onSuccess:
        destination: $ON_SUCCESS_DESTINATION
      onFailure:
        destination: $ON_FAILURE_DESTINATION
    maximumEventAgeInSeconds: $MAXIMUM_EVENT_AGE_IN_SECONDS
    maximumRetryAttempts: $MAXIMUM_RETRY_ATTEMPTS
  description: alias created by ACK lambda-controller e2e tests

This PR includes both implementation code and e2e tests.

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

ack-prow · 2023-06-02T21:12:05Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

a-hilaly · 2023-06-14T08:40:44Z

/retest

a-hilaly · 2023-06-14T08:52:32Z

pkg/resource/alias/hooks.go

+	svcapitypes "github.com/aws-controllers-k8s/lambda-controller/apis/v1alpha1"
+	// svcresource "github.com/aws-controllers-k8s/lambda-controller/pkg/resource"
+	ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare"
+	ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log"
+	"github.com/aws/aws-sdk-go/aws"
+	svcsdk "github.com/aws/aws-sdk-go/service/lambda"


Please reorder https://github.com/golang/go/wiki/CodeReviewComments#imports

a-hilaly · 2023-06-14T08:55:36Z

pkg/resource/alias/hooks.go

+	svcsdk "github.com/aws/aws-sdk-go/service/lambda"
+)
+
+func (rm *resourceManager) customCreate(


Please name this function customCreateEventInvokeConfig

Also customCreate and update... are calling the same API PutFunctionEventInvokeConfigWithContext can't we just use one function for both?

a-hilaly

Nice! Very close!

a-hilaly · 2023-06-19T16:56:47Z

templates/hooks/alias/sdk_update_pre_build_request.go.tpl

@@ -0,0 +1 @@
+rm.customUpdateEventInvokeConfig(ctx,desired,delta)


What if a user only modified to EventInvokeConfig? We should avoid calling an update for the other fields. And vice versa, if a user only modifies the description for example, we need to avoid calling customUpdateEventInvokeConfig

Also error handling is missing

Done. Have added the error handling part and inserted an if condition that will execute only when there's difference in state for EventInvokeConfig.

a-hilaly · 2023-06-19T16:57:53Z

templates/hooks/alias/sdk_create_post_set_output.go.tpl

@@ -0,0 +1,3 @@
+if ko.Spec.FunctionEventInvokeConfig != nil {
+   rm.customCreateEventInvokeConfig(ctx,desired)


Same error handling is missing

a-hilaly · 2023-06-19T16:58:43Z

apis/v1alpha1/alias.go

@@ -26,7 +26,8 @@ import (
 type AliasSpec struct {

 	// A description of the alias.
-	Description *string `json:"description,omitempty"`
+	Description               *string                            `json:"description,omitempty"`
+	FunctionEventInvokeConfig *PutFunctionEventInvokeConfigInput `json:"functionEventInvokeConfig,omitempty"`


Nit: we have a new code-generator feature that allows you to set some documentation for custom fields

Added documentation for FunctionEventInvokeConfig for both function and alias in documentation.yaml file

a-hilaly · 2023-06-19T17:00:24Z

pkg/resource/alias/hooks.go

+	return input, nil
+}
+
+func (rm *resourceManager) syncEventInvokeConfig(


customCreateEventInvokeConfig and customUpdateEventInvokeConfig are very similar, we could just use syncEventInvokeConfig in both sdkCreate and sdkUpdate (for update you'll have to handle the delta differences in the hook)

Created one single function syncEventInvokeConfig for update and create part.

a-hilaly · 2023-06-26T18:10:36Z

pkg/resource/alias/sdk.go

+	if delta.DifferentAt("Spec.FunctionEventInvokeConfig") {
+		_, err = rm.syncEventInvokeConfig(ctx, desired)
+		if err != nil {
+			return nil, err
+		}
+	}
 	input, err := rm.newUpdateRequestPayload(ctx, desired, delta)
 	if err != nil {


After the if delta.DifferentAt gets exectued the code starting from line 307 will get executed as well (even if user only changed FunctionEventInvokeConfig. How can you prevent that from happening?

Have added !delta.DifferentExcept("Spec.FunctionEventInvokeConfig") condition in sdkUpdate to prevent execution of rest of the update part, when only EventInvokeConfig fields are changed.

a-hilaly

This looks good! I have more comment. Let's merge this right after it's fixed

a-hilaly · 2023-06-28T22:02:07Z

pkg/resource/alias/hooks.go

+		}
+	}
+	_, err = rm.sdkapi.PutFunctionEventInvokeConfigWithContext(ctx, input)
+	rm.metrics.RecordAPICall("SYNC", "SyncEventInvokeConfig", err)


This should be an UPDATE record

a-hilaly

Good job on this!

a-hilaly · 2023-06-29T18:00:43Z

/cc @RedbackThomson
/cc @jljaco

RedbackThomson

/lgtm

RedbackThomson · 2023-06-29T18:54:59Z

documentation.yaml

@@ -0,0 +1,41 @@
+resources:


This is excellent attention to detail!

ack-prow · 2023-06-29T18:57:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, RedbackThomson, 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~~ [A-Hilaly,RedbackThomson]

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

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](#97) 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.

ack-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 2, 2023

ack-prow bot requested review from RedbackThomson and vijtrip2 June 2, 2023 21:12

Vandita2020 changed the title ~~Adding EventInvokeConfig support for Alias~~ Adding EventInvokeConfig support for Alias Jun 2, 2023

Vandita2020 force-pushed the event_invoke_config_alias branch 21 times, most recently from b5c1f7c to 46e818e Compare June 11, 2023 21:37

Vandita2020 force-pushed the event_invoke_config_alias branch 4 times, most recently from c9064be to cc965c1 Compare June 13, 2023 21:00

Vandita2020 force-pushed the event_invoke_config_alias branch from cc965c1 to f4d89df Compare June 13, 2023 22:37

Vandita2020 marked this pull request as ready for review June 13, 2023 23:17

ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 13, 2023

Vandita2020 force-pushed the event_invoke_config_alias branch from f4d89df to 2b95175 Compare June 13, 2023 23:56

a-hilaly reviewed Jun 14, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_alias branch 3 times, most recently from a2a8169 to fe01416 Compare June 19, 2023 16:53

a-hilaly reviewed Jun 19, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_alias branch 2 times, most recently from 65b22fc to f6e5f5c Compare June 20, 2023 22:08

a-hilaly reviewed Jun 26, 2023

View reviewed changes

Vandita2020 force-pushed the event_invoke_config_alias branch 2 times, most recently from 5b8acd1 to dfc1fa0 Compare June 28, 2023 21:59

a-hilaly reviewed Jun 28, 2023

View reviewed changes

Adding EventInvokeConfig support for Alias

d9d99a2

Vandita2020 force-pushed the event_invoke_config_alias branch from dfc1fa0 to d9d99a2 Compare June 28, 2023 23:02

a-hilaly approved these changes Jun 29, 2023

View reviewed changes

ack-prow bot added the approved label Jun 29, 2023

RedbackThomson approved these changes Jun 29, 2023

View reviewed changes

ack-prow bot assigned RedbackThomson Jun 29, 2023

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

ack-prow bot merged commit f18794e into aws-controllers-k8s:main Jun 29, 2023

Vandita2020 mentioned this pull request Aug 2, 2023

Bug fix for EventInvokeConfig #107

Merged

		@@ -0,0 +1 @@
		rm.customUpdateEventInvokeConfig(ctx,desired,delta)

		@@ -0,0 +1,3 @@
		if ko.Spec.FunctionEventInvokeConfig != nil {
		rm.customCreateEventInvokeConfig(ctx,desired)

Adding EventInvokeConfig support for Alias #93

Adding EventInvokeConfig support for Alias #93

Uh oh!

Conversation

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

Uh oh!

Uh oh!

ack-prow bot commented Jun 2, 2023

Uh oh!

a-hilaly commented Jun 14, 2023

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!

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!

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

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!

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!

a-hilaly commented Jun 29, 2023

Uh oh!

RedbackThomson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Jun 29, 2023

Uh oh!

Uh oh!

Adding `EventInvokeConfig` support for `Alias` #93

Adding `EventInvokeConfig` support for `Alias` #93

Vandita2020 commented Jun 2, 2023 •

edited

Loading

Vandita2020 Jun 20, 2023 •

edited

Loading