Introduce `spec.code.sha256` field to help with s3 code changes #88

Vandita2020 · 2023-03-27T02:18:08Z

Issue #
#1550

Description
This PR adds the feature to update the Spec.Code.S3 variables. The issue arises as Lambda API does not keep record of Spec.Code variables, because of which ACK is not being able to compare the changes in the state for Spec.Code (ex: change in Spec.Code.S3Key) and thus fails to recognize the changes.

Thus we have added a new field Spec.Code.SHA256, the user has to manually calculate the SHA256 for their code, they can do this by running this command sha256sum filename.zip |cut -f1 -d\ | xxd -r -p | base64. This field will then be compared with current SHA256 taken from Status.CodeSHA256 and will determine if the update has been made for deployment package.

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

jaypipes · 2023-03-27T12:57:19Z

@Vandita2020 needs a rebase :)

test/e2e/resources/function.yaml

test/e2e/tests/test_function.py

pkg/resource/function/hooks.go

ada.log

jaypipes

@Vandita2020 are you able to run e2e tests locally? I think it would be an easier/tighter development iteration if you did so instead of relying on the CI system. On your local workstation, check out the github.com/aws-controllers-k8s/test-infra repository, cp test_config.example.yaml test_config.yaml, edit test_config.yaml with the IAM Role ARN you can use for testing (typically an Isengard role) and do make kind-test SERVICE=lambda.

Also left a couple comments inline for you to address. The way this code currently works will result in an endless loop of controller errors...

jaypipes · 2023-04-04T13:27:01Z

generator.yaml

+      CodeS3SHA256:
+        type: String
+        compare:
+          is_ignored: true


@Vandita2020 I'm struggling to understand why there is a need for an additional Spec.CodeS3SHA256 field here when there is already a Status.CodeSHA256 field that can be used to compare checksums with?

We have added this field in our Spec to see if the code has changed. Since the Function APIs don't store the current S3.Bucket or S3.Key, there is no way to identify if the change has made when the user changes these fields, leading to no update for the deployment package. SHA256 helps identifying the update, given the user has to provide the updated SHA256 along with the new deployment package.

jaypipes · 2023-04-04T13:36:04Z

pkg/resource/function/hooks.go

@@ -416,6 +413,19 @@ func customPreCompare(
 				delta.Add("Spec.Code.ImageURI", a.ko.Spec.Code.ImageURI, b.ko.Spec.Code.ImageURI)
 			}
 		}
+		expected, _ := json.Marshal(a.ko.Spec.CodeS3SHA256)


I'm not seeing where Spec.CodeS3SHA256 is ever being set to a value. How about calculating the S3 content's SHA256 manually and comparing it to Status.CodeSHA256 to determine if the function's S3-backed code has changed?

When the user provides the updated deployment package they have to manually calculate the CodeSHA256 for their code, which they can do by running this command sha256sum filename.zip |cut -f1 -d\ | xxd -r -p | base64. We then use this provided CodeSHA256 and compare it with Status.CodeSHA256 to see the changes/update.

pkg/resource/function/hooks.go

Vandita2020 · 2023-04-04T22:08:40Z

@Vandita2020 are you able to run e2e tests locally? I think it would be an easier/tighter development iteration if you did so instead of relying on the CI system. On your local workstation, check out the github.com/aws-controllers-k8s/test-infra repository, cp test_config.example.yaml test_config.yaml, edit test_config.yaml with the IAM Role ARN you can use for testing (typically an Isengard role) and do make kind-test SERVICE=lambda.

Also left a couple comments inline for you to address. The way this code currently works will result in an endless loop of controller errors...

Hey Jay @jaypipes ,
When I try to run this locally using make kind-test SERVICE=lambda it takes almost an hour to run the test, whereas on GitHub it's a bit faster. The process that takes most of the time is RUN go mod download, is there any way I can make this process faster? From what I imagine is it creates a container image every time I do make kind-test SERVICE=lambda, is there a way I can just use the image created before the next time I run the test?

ayanevbg · 2024-04-04T13:09:59Z

Can someone give the lgmt label, please? This will be very beneficial for our use case too!

a-hilaly · 2024-04-04T21:37:52Z

@ayanevbg This is on our radar. We just extended the code-generator to help supporting this feature. @Vandita2020 and I will merge this soon

apis/v1alpha1/ack-generate-metadata.yaml

ack-prow · 2024-04-09T20:32:07Z

@Vandita2020: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
lambda-codespell	`09227fa`	link	true	`/test lambda-codespell`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

a-hilaly

This look great! Thanks @Vandita2020

pkg/resource/function/hooks.go

a-hilaly · 2024-04-10T16:03:53Z

pkg/resource/function/hooks.go

@@ -476,35 +476,44 @@ func customPreCompare(
 	if ackcompare.HasNilDifference(a.ko.Spec.Code, b.ko.Spec.Code) {
 		delta.Add("Spec.Code", a.ko.Spec.Code, b.ko.Spec.Code)
 	} else if a.ko.Spec.Code != nil && b.ko.Spec.Code != nil {


Should we consider removing this customPreCompare, generate all this code, and then rely on the custom update function to lookup *a.ko.Spec.PackageType == "Image"|"Zip"

I think it would be better to generate this code for Code.S3Bucket, Code.S3Key and Code.ImageURI, but we will still need this custom compare code for Code.SHA256, as that is not auto-generated and we need to check if it has been changes in order to let ACK know that deployment package has been changed.

a-hilaly · 2024-04-10T16:06:28Z

pkg/resource/function/hooks.go

@@ -114,24 +113,17 @@ func (rm *resourceManager) customUpdateFunction(
 	// UpdateFunctionCode because both of them can put the function in a
 	// Pending state.
 	switch {
-	case delta.DifferentAt("Spec.Code"):
+	case delta.DifferentAt("Spec.Code") || delta.DifferentAt("Spec.Code.SHA256"):
 		err = rm.updateFunctionCode(ctx, desired, delta)


Another question that I have is: what if the user provides the wrong sha256? how does the controller behave?

We consider this to be customer's responsibility to pass the correct sha256, and the reason being we cannot calculate the sha for their package to see if everything is correct, given the size of their package could be big and we would need to download it and then calculate the sha for it. However to answer your question, if the user passes wrong sha256 but correct s3.key and s3.bucket, the deployment package would be updated without any error.

pkg/resource/function/hooks.go

…llers-k8s#87) ---------- * ACK code-generator `v0.25.0` [release notes](https://github.com/aws-controllers-k8s/code-generator/releases/tag/v0.25.0) * ACK runtime `v0.25.0` [release notes](https://github.com/aws-controllers-k8s/runtime/releases/tag/v0.25.0) ---------- NOTE: This PR increments the release version of service controller from `v0.1.6` to `v0.1.7` Once this PR is merged, release `v0.1.7` will be automatically created for `lambda-controller` **Please close this PR, if you do not want the new patch release for `lambda-controller`** ---------- ``` building ack-generate ... ok. ==== building lambda-controller ==== Copying common custom resource definitions into lambda Building Kubernetes API objects for lambda Generating deepcopy code for lambda Generating custom resource definitions for lambda Building service controller for lambda Generating RBAC manifests for lambda Running gofmt against generated code for lambda Updating additional GitHub repository maintenance files ==== building lambda-controller release artifacts ==== Building release artifacts for lambda-v0.1.7 Generating common custom resource definitions Generating custom resource definitions for lambda Generating RBAC manifests for lambda ``` ---------- By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly · 2024-05-06T13:20:18Z

pkg/resource/function/hooks.go

+			if latest.ko.Spec.PackageType != nil && *latest.ko.Spec.PackageType == "Image" {
+				input.ImageUri = latest.ko.Spec.Code.ImageURI


I think we only need to update the image if the ImageURI changed

This code is for passing code values if there is change in Architecture field. To update Architecture field we need to pass the code again too, this handles case that even if there is no change in code, it will just pass the current values of it along with updated architecture field.

a-hilaly · 2024-05-07T15:32:36Z

/unhold

a-hilaly

@Vandita2020 Great work on this, thank yoou!! 🚀
/lgtm

ack-prow · 2024-05-07T17:08:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, 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]

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

a-hilaly · 2024-05-16T21:23:03Z

@ayanevbg @aszmyd @healiha This was released in 1.4.4 :)

ack-prow bot requested review from jljaco and vijtrip2 March 27, 2023 02:18

Vandita2020 marked this pull request as draft March 27, 2023 04:34

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 Mar 27, 2023

Vandita2020 force-pushed the s3_codeSHA branch from 29c5a8e to 4cb0e48 Compare March 27, 2023 16:39

a-hilaly reviewed Mar 27, 2023

View reviewed changes

test/e2e/resources/function.yaml Outdated Show resolved Hide resolved

test/e2e/tests/test_function.py Outdated Show resolved Hide resolved

pkg/resource/function/hooks.go Outdated Show resolved Hide resolved

Vandita2020 force-pushed the s3_codeSHA branch 4 times, most recently from 6be63ca to 8a16e84 Compare March 30, 2023 08:05

jaypipes reviewed Mar 30, 2023

View reviewed changes

ada.log Outdated Show resolved Hide resolved

Vandita2020 force-pushed the s3_codeSHA branch 13 times, most recently from 74a6bde to b5e00b0 Compare April 4, 2023 08:58

jaypipes suggested changes Apr 4, 2023

View reviewed changes

Vandita2020 force-pushed the s3_codeSHA branch from b5e00b0 to 217a0db Compare April 4, 2023 21:47

Vandita2020 force-pushed the s3_codeSHA branch 2 times, most recently from ed60093 to 84d5125 Compare April 4, 2023 23:08

Vandita2020 force-pushed the s3_codeSHA branch from 030204c to 4a1f03b Compare April 7, 2024 05:46

a-hilaly reviewed Apr 8, 2024

View reviewed changes

apis/v1alpha1/ack-generate-metadata.yaml Outdated Show resolved Hide resolved

Vandita2020 force-pushed the s3_codeSHA branch 2 times, most recently from 55e2af9 to 8082723 Compare April 8, 2024 23:51

Vandita2020 force-pushed the s3_codeSHA branch from edbd669 to 2c80751 Compare April 9, 2024 21:07

a-hilaly reviewed Apr 10, 2024

View reviewed changes

a-hilaly changed the title ~~Adding code and e2e tests for Code.S3 update feature~~ Introduce spec.code.sha256 field to help with s3 code changes Apr 10, 2024

a-hilaly reviewed Apr 10, 2024

View reviewed changes

a-hilaly reviewed Apr 24, 2024

View reviewed changes

pkg/resource/function/hooks.go Outdated Show resolved Hide resolved

pkg/resource/function/hooks.go Outdated Show resolved Hide resolved

ack-bot and others added 3 commits April 25, 2024 10:19

revise

3ba2ca7

E2E tests

9981248

Vandita2020 mentioned this pull request May 1, 2024

Lambda function is not updated when s3Key changed aws-controllers-k8s/community#2067

Closed

New revision with Architecture update

b955873

Vandita2020 force-pushed the s3_codeSHA branch from 2c80751 to b955873 Compare May 2, 2024 22:57

a-hilaly reviewed May 6, 2024

View reviewed changes

ack-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 7, 2024

Vandita2020 requested a review from a-hilaly May 7, 2024 15:35

Merge branch 'main' into s3_codeSHA

6024afe

a-hilaly approved these changes May 7, 2024

View reviewed changes

ack-prow bot assigned a-hilaly May 7, 2024

ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2024

ack-prow bot merged commit 7726f83 into aws-controllers-k8s:main May 7, 2024
6 checks passed

Vandita2020 mentioned this pull request May 7, 2024

Can't deploy updated deployment package for Lambda with ACK aws-controllers-k8s/community#1550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce `spec.code.sha256` field to help with s3 code changes #88

Introduce `spec.code.sha256` field to help with s3 code changes #88

Vandita2020 commented Mar 27, 2023 •

edited

Loading

jaypipes commented Mar 27, 2023

jaypipes left a comment

jaypipes Apr 4, 2023

Vandita2020 Apr 8, 2024

jaypipes Apr 4, 2023

Vandita2020 Apr 8, 2024

Vandita2020 commented Apr 4, 2023

ayanevbg commented Apr 4, 2024

a-hilaly commented Apr 4, 2024

ack-prow bot commented Apr 9, 2024 •

edited

Loading

a-hilaly left a comment

a-hilaly Apr 10, 2024

Vandita2020 May 2, 2024

a-hilaly Apr 10, 2024

Vandita2020 May 2, 2024

a-hilaly May 6, 2024

Vandita2020 May 7, 2024

a-hilaly commented May 7, 2024

a-hilaly left a comment

ack-prow bot commented May 7, 2024

a-hilaly commented May 16, 2024

		if latest.ko.Spec.PackageType != nil && *latest.ko.Spec.PackageType == "Image" {
		input.ImageUri = latest.ko.Spec.Code.ImageURI

Introduce spec.code.sha256 field to help with s3 code changes #88

Introduce spec.code.sha256 field to help with s3 code changes #88

Conversation

Vandita2020 commented Mar 27, 2023 • edited Loading

jaypipes commented Mar 27, 2023

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vandita2020 commented Apr 4, 2023

ayanevbg commented Apr 4, 2024

a-hilaly commented Apr 4, 2024

ack-prow bot commented Apr 9, 2024 • edited Loading

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly commented May 7, 2024

a-hilaly left a comment

Choose a reason for hiding this comment

ack-prow bot commented May 7, 2024

a-hilaly commented May 16, 2024

Introduce `spec.code.sha256` field to help with s3 code changes #88

Introduce `spec.code.sha256` field to help with s3 code changes #88

Vandita2020 commented Mar 27, 2023 •

edited

Loading

ack-prow bot commented Apr 9, 2024 •

edited

Loading