Compute digest of configmap and secret from its data #5115

lfabriko · 2024-01-28T18:38:00Z

Proposal of fix to #5114 - use Name, Namespace and Data/StringData to compute digest of Configmap and Secret.

squakez

Have you tested against the issued sample reported? I am not entirely sure the reason of the behavior is how we calculate digest.

lfabriko · 2024-01-29T09:20:13Z

I think it tries to compute digest of configmap from memory address: (digest.go)


	// Configmap and secret content
	for _, cm := range configmaps {
		//DEBUG
		if cm != nil {
			log.Infof(fmt.Sprintf("LFAB3 configmap cm.String: %s", cm.String()))
		} else {
			log.Infof(fmt.Sprintf("LFAB3 cm is nil"))
		}
		//END OF DEBUG
		if _, err := hash.Write([]byte(cm.String())); err != nil {
			return "", err
		}
	}

I observed (in build_kit.go) before cm.yaml configmap is loaded, secrets, configmaps := getIntegrationSecretsAndConfigmaps(ctx, action.client, integration) returns [nil] as configmaps (it doesn't change digest).
When the cm is loaded, digest.ComputeForIntegration computes hash from whole configmap object via cm.String(), it seems to me it takes also the memory address, which keeps changing, so next step in build_kit.go if hash != integration.Status.Digest { is always different.

After adding debugging notes:

{"level":"info","ts":"2024-01-29T09:12:00Z","logger":"camel-k","msg":"LFAB3 cm is nil"}
...
{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 configmap cm.String: &ConfigMap{ObjectMeta:{myconfigmap  lfabriko  6c693d68-2f0c-46b8-a5b3-a75cb5219211 60808454 0 2024-01-29 09:12:42 +0000 UTC <nil> <nil> map[] map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"bar\":\"2\",\"foo\":\"1\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"name\":\"myconfigmap\",\"namespace\":\"lfabriko\",\"ownerReferences\":[{\"apiVersion\":\"camel.apache.org/v1\",\"blockOwnerDeletion\":true,\"controller\":true,\"kind\":\"Integration\",\"name\":\"myintegration\",\"uid\":\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\"}]}}\n] [{camel.apache.org/v1 Integration myintegration 48e97e9e-6eb8-4cb2-9f9b-736521f16ee8 0xc0012acd2d 0xc0012acd2e}] [] [{kubectl-client-side-apply Update v1 2024-01-29 09:12:42 +0000 UTC FieldsV1 {\"f:data\":{\".\":{},\"f:bar\":{},\"f:foo\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:kubectl.kubernetes.io/last-applied-configuration\":{}},\"f:ownerReferences\":{\".\":{},\"k:{\\\"uid\\\":\\\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\\\"}\":{}}}} }]},Data:map[string]string{bar: 2,foo: 1,},BinaryData:map[string][]byte{},Immutable:nil,}"}
{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 digest vEQhvrHzidhCwTIGgGASlVTKlaW9-pFHuV08xkGVQxsQ"}

{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 configmap cm.String: &ConfigMap{ObjectMeta:{myconfigmap  lfabriko  6c693d68-2f0c-46b8-a5b3-a75cb5219211 60808454 0 2024-01-29 09:12:42 +0000 UTC <nil> <nil> map[] map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"bar\":\"2\",\"foo\":\"1\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"name\":\"myconfigmap\",\"namespace\":\"lfabriko\",\"ownerReferences\":[{\"apiVersion\":\"camel.apache.org/v1\",\"blockOwnerDeletion\":true,\"controller\":true,\"kind\":\"Integration\",\"name\":\"myintegration\",\"uid\":\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\"}]}}\n] [{camel.apache.org/v1 Integration myintegration 48e97e9e-6eb8-4cb2-9f9b-736521f16ee8 0xc0012ace07 0xc0012ace08}] [] [{kubectl-client-side-apply Update v1 2024-01-29 09:12:42 +0000 UTC FieldsV1 {\"f:data\":{\".\":{},\"f:bar\":{},\"f:foo\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:kubectl.kubernetes.io/last-applied-configuration\":{}},\"f:ownerReferences\":{\".\":{},\"k:{\\\"uid\\\":\\\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\\\"}\":{}}}} }]},Data:map[string]string{bar: 2,foo: 1,},BinaryData:map[string][]byte{},Immutable:nil,}"}
{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 digest vra0Ihy7JAv6YXdpCCbcno8lWWM7MP6ud38zXLdW-0J0"}

{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 configmap cm.String: &ConfigMap{ObjectMeta:{myconfigmap  lfabriko  6c693d68-2f0c-46b8-a5b3-a75cb5219211 60808454 0 2024-01-29 09:12:42 +0000 UTC <nil> <nil> map[] map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"bar\":\"2\",\"foo\":\"1\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"name\":\"myconfigmap\",\"namespace\":\"lfabriko\",\"ownerReferences\":[{\"apiVersion\":\"camel.apache.org/v1\",\"blockOwnerDeletion\":true,\"controller\":true,\"kind\":\"Integration\",\"name\":\"myintegration\",\"uid\":\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\"}]}}\n] [{camel.apache.org/v1 Integration myintegration 48e97e9e-6eb8-4cb2-9f9b-736521f16ee8 0xc001ca42e0 0xc001ca42e1}] [] [{kubectl-client-side-apply Update v1 2024-01-29 09:12:42 +0000 UTC FieldsV1 {\"f:data\":{\".\":{},\"f:bar\":{},\"f:foo\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:kubectl.kubernetes.io/last-applied-configuration\":{}},\"f:ownerReferences\":{\".\":{},\"k:{\\\"uid\\\":\\\"48e97e9e-6eb8-4cb2-9f9b-736521f16ee8\\\"}\":{}}}} }]},Data:map[string]string{bar: 2,foo: 1,},BinaryData:map[string][]byte{},Immutable:nil,}"}
{"level":"info","ts":"2024-01-29T09:13:00Z","logger":"camel-k","msg":"LFAB3 digest vzFRVYBlBtbp_8dPiB9Zx5A9gLj33R8YGOZV9xHTbCkI"}

I tried it against both 5115 and config_reload_test.go.

lfabriko · 2024-01-29T10:21:09Z

I observe if configmap is created without the ownerReference part (ie. order of creation is 1. configmap, 2. integration) there isn't the memory reference and the case 5115 doesn't appear:

{"level":"info","ts":"2024-01-29T09:28:41Z","logger":"camel-k","msg":"LFAB3 configmap cm.String: &ConfigMap{ObjectMeta:{myconfigmap  lfabriko  fe6aab0c-d21a-4628-b5f6-a2d945c4b9a6 60826770 0 2024-01-29 09:24:23 +0000 UTC <nil> <nil> map[] map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"bar\":\"2\",\"foo\":\"1\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"name\":\"myconfigmap\",\"namespace\":\"lfabriko\"}}\n] [] [] [{kubectl-client-side-apply Update v1 2024-01-29 09:24:23 +0000 UTC FieldsV1 {\"f:data\":{\".\":{},\"f:bar\":{},\"f:foo\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:kubectl.kubernetes.io/last-applied-configuration\":{}}}} }]},Data:map[string]string{bar: 2,foo: 1,},BinaryData:map[string][]byte{},Immutable:nil,}"}
{"level":"info","ts":"2024-01-29T09:28:41Z","logger":"camel-k","msg":"LFAB3 digest v7q7PJwFnLoEd4_wTiIOwkIRVaZHha7n77tCOXz2KH50"}

{"level":"info","ts":"2024-01-29T09:28:41Z","logger":"camel-k","msg":"LFAB3 configmap cm.String: &ConfigMap{ObjectMeta:{myconfigmap  lfabriko  fe6aab0c-d21a-4628-b5f6-a2d945c4b9a6 60826770 0 2024-01-29 09:24:23 +0000 UTC <nil> <nil> map[] map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"bar\":\"2\",\"foo\":\"1\"},\"kind\":\"ConfigMap\",\"metadata\":{\"annotations\":{},\"name\":\"myconfigmap\",\"namespace\":\"lfabriko\"}}\n] [] [] [{kubectl-client-side-apply Update v1 2024-01-29 09:24:23 +0000 UTC FieldsV1 {\"f:data\":{\".\":{},\"f:bar\":{},\"f:foo\":{}},\"f:metadata\":{\"f:annotations\":{\".\":{},\"f:kubectl.kubernetes.io/last-applied-configuration\":{}}}} }]},Data:map[string]string{bar: 2,foo: 1,},BinaryData:map[string][]byte{},Immutable:nil,}"}
{"level":"info","ts":"2024-01-29T09:28:41Z","logger":"camel-k","msg":"LFAB3 digest v7q7PJwFnLoEd4_wTiIOwkIRVaZHha7n77tCOXz2KH50"}

squakez · 2024-01-31T08:59:22Z

A part the validate check failure, the rest seems good. However, I think it would be convenient to have some unit test to cover the scenarios we're trying to fix.

squakez

Thanks for the test. However I was meaning more a controller test in order to verify that, when an Integration is provided with the owner reference, then, it is moving correctly among the different phases. Let me make an example. You have the Integration with the configmap in the mount trait spec, this configmap has the owner reference and you verify the controller as it happens for instance in this test:

camel-k/pkg/controller/integration/initialize_test.go

Line 35 in 80a8778

func TestCamelImportDeployment(t *testing.T) {

- if you can do something with it we'd have the coverage of the issue that was reported and we're sure we're not breaking it again. Alternatively, if you feel more comfortable with e2e, then, it would be also an option, though we should favour unit test whenever it's possible.

squakez · 2024-02-01T07:16:22Z

pkg/util/digest/digest.go

@@ -183,7 +183,7 @@ func ComputeForIntegration(integration *v1.Integration, configmaps []*corev1.Con
 			//StringData with sorted keys
 			if s.StringData != nil {


Stringdata should be avoided as Kubernetes will translate all those value in the correspondent base64 in data. You can remove this part entirely.

Thank you for info; also I am not sure if there shouldn't be also binaryData included in digest computation in case of Configmap...

BinaryData for CM would be required as well.

github-actions · 2024-02-01T07:31:40Z

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

github-actions · 2024-02-05T08:51:24Z

⚠️ Unit test coverage report - coverage decreased from 35.6% to 35.5% (-0.1%)

squakez · 2024-02-05T08:58:29Z

@lfabriko any reason why you removed the digest_test.go in the last commit? I think that was still good to make some unit test coverage. In fact, with that change, the coverage decreased. Can you include them back? Having an e2e does not exclude the presence of unit test.

lfabriko · 2024-02-05T09:05:35Z

Sure, I thought I should replace it by e2e test.

github-actions · 2024-02-05T09:39:32Z

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

github-actions · 2024-02-06T09:32:49Z

✔️ Unit test coverage report - coverage increased from 35.6% to 35.7% (+0.1%)

lfabriko · 2024-02-07T10:12:00Z

I observe failed TestRunIncrementalBuildWithDifferentBaseImages.

squakez · 2024-02-07T10:24:43Z

I've restarted the check, maybe it's a flaky one. Please, have a look at the validate error. Locally you can make lint to see what's the problem.

…ringData range

lfabriko · 2024-02-08T00:03:36Z

Validation via make lint with golangci-lint version 1.55.2 locally passed for modified files.

github-actions · 2024-02-08T07:32:07Z

✔️ Unit test coverage report - coverage increased from 35.6% to 35.8% (+0.2%)

lfabriko · 2024-02-12T07:48:32Z

I observe failed tests TestRunDevMode (in common-it-single-operator-installation) and TestKamelCLIPromote (in common-it-custom-operator-installation); I tried re-running them on lfabriko github (attempts 4-8, branch lfab-ad-5114 rebased on commit #5144) - common-it-single-operator-installation passed in 4, 5, 6, 8; common-it-custom-operator-installation passed in 4, 6.

squakez reviewed Jan 29, 2024

View reviewed changes

squakez reviewed Feb 1, 2024

View reviewed changes

squakez approved these changes Feb 5, 2024

View reviewed changes

gansheer approved these changes Feb 5, 2024

View reviewed changes

lfabriko marked this pull request as draft February 5, 2024 11:18

lfabriko marked this pull request as ready for review February 5, 2024 17:37

lfabriko marked this pull request as draft February 7, 2024 10:12

lfabriko added 6 commits February 7, 2024 19:59

Compute digest of configmap and secret from its data

1770bfd

Add unit tests for computing digest with secret and configmap, fix St…

3083151

…ringData range

Add BinaryData to Configmap digest computation, e2e test

769b2ab

Add digest_test for Configmap, Secret

d3eea11

Simplify e2e test

2e41124

Fixed via validation checks, add labels

6f0d775

lfabriko force-pushed the lfab-ad-5114 branch from adb7426 to 6f0d775 Compare February 7, 2024 23:58

lfabriko marked this pull request as ready for review February 8, 2024 00:04

squakez merged commit 25f5993 into apache:main Feb 12, 2024
15 checks passed

lfabriko mentioned this pull request Feb 13, 2024

Reduce write to hash, prepare string from secret, configmap #5155

Merged

realMartinez pushed a commit to realMartinez/camel-k that referenced this pull request Feb 29, 2024

Compute digest of configmap and secret from its data (apache#5115)

28fae6d

realMartinez pushed a commit to realMartinez/camel-k that referenced this pull request Feb 29, 2024

Compute digest of configmap and secret from its data (apache#5115)

bdd215d

realMartinez pushed a commit to realMartinez/camel-k that referenced this pull request Feb 29, 2024

Compute digest of configmap and secret from its data (apache#5115)

8fa0953

realMartinez pushed a commit to realMartinez/camel-k that referenced this pull request Feb 29, 2024

Compute digest of configmap and secret from its data (apache#5115)

3210d59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute digest of configmap and secret from its data #5115

Compute digest of configmap and secret from its data #5115

lfabriko commented Jan 28, 2024

squakez left a comment

lfabriko commented Jan 29, 2024

lfabriko commented Jan 29, 2024 •

edited

Loading

squakez commented Jan 31, 2024

squakez left a comment

squakez Feb 1, 2024

lfabriko Feb 1, 2024

squakez Feb 1, 2024

github-actions bot commented Feb 1, 2024

github-actions bot commented Feb 5, 2024

squakez commented Feb 5, 2024

lfabriko commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 6, 2024

lfabriko commented Feb 7, 2024

squakez commented Feb 7, 2024

lfabriko commented Feb 8, 2024

github-actions bot commented Feb 8, 2024

lfabriko commented Feb 12, 2024

		@@ -183,7 +183,7 @@ func ComputeForIntegration(integration v1.Integration, configmaps []corev1.Con
		//StringData with sorted keys
		if s.StringData != nil {

Compute digest of configmap and secret from its data #5115

Compute digest of configmap and secret from its data #5115

Conversation

lfabriko commented Jan 28, 2024

squakez left a comment

Choose a reason for hiding this comment

lfabriko commented Jan 29, 2024

lfabriko commented Jan 29, 2024 • edited Loading

squakez commented Jan 31, 2024

squakez left a comment

Choose a reason for hiding this comment

squakez Feb 1, 2024

Choose a reason for hiding this comment

lfabriko Feb 1, 2024

Choose a reason for hiding this comment

squakez Feb 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Feb 1, 2024

github-actions bot commented Feb 5, 2024

squakez commented Feb 5, 2024

lfabriko commented Feb 5, 2024

github-actions bot commented Feb 5, 2024

github-actions bot commented Feb 6, 2024

lfabriko commented Feb 7, 2024

squakez commented Feb 7, 2024

lfabriko commented Feb 8, 2024

github-actions bot commented Feb 8, 2024

lfabriko commented Feb 12, 2024

lfabriko commented Jan 29, 2024 •

edited

Loading