Add tests for custom number of fernet keys #492

afaranha · 2024-10-24T15:13:46Z

No description provided.

openshift-ci · 2024-10-24T15:13:52Z

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

openshift-ci · 2024-10-24T15:13:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: afaranha
Once this PR has been reviewed and has the lgtm label, please assign viroel for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

afaranha · 2024-10-24T15:15:19Z

tests/functional/keystoneapi_controller_test.go

@@ -1109,6 +1110,112 @@ var _ = Describe("Keystone controller", func() {
 		})
 	})

+	When("When the fernet keys are created with fernetMaxActiveKeys as 3", func() {


Both this test and the one below are pretty much similar, is there a way to reuse it?
(While waiting for an answer I'll try to figure it out, but it may take some time)

instead of the separate test bellow, maybe it would be better to test that when updating the keystone parameter from 3 -> 6 we see that the config and the secret got additional keys? Like have a nested when update keys from 3->6 it ...

The problem is that I still don't know how to simulate a change on the spec.
I start with this one: DeferCleanup(th.DeleteInstance, CreateKeystoneAPI(keystoneAPIName, GetKeystoneAPISpec(3)))
But even if I call this again with a different value on the It(), it doesn't seem to work.

you could do it like this

When("When the fernet keys are created", func() { BeforeEach(func() { DeferCleanup( k8sClient.Delete, ctx, CreateKeystoneMessageBusSecret(namespace, "rabbitmq-secret")) // with fernetMaxActiveKeys as 3 DeferCleanup(th.DeleteInstance, CreateKeystoneAPI(keystoneAPIName, GetKeystoneAPISpec(3))) DeferCleanup( k8sClient.Delete, ctx, CreateKeystoneAPISecret(namespace, SecretName)) DeferCleanup(infra.DeleteMemcached, infra.CreateMemcached(namespace, "memcached", memcachedSpec)) DeferCleanup( mariadb.DeleteDBService, mariadb.CreateDBService( namespace, GetKeystoneAPI(keystoneAPIName).Spec.DatabaseInstance, corev1.ServiceSpec{ Ports: []corev1.ServicePort{{Port: 3306}}, }, ), ) mariadb.SimulateMariaDBAccountCompleted(keystoneAccountName) mariadb.SimulateMariaDBDatabaseCompleted(keystoneDatabaseName) infra.SimulateTransportURLReady(types.NamespacedName{ Name: fmt.Sprintf("%s-keystone-transport", keystoneAPIName.Name), Namespace: namespace, }) infra.SimulateMemcachedReady(types.NamespacedName{ Name: "memcached", Namespace: namespace, }) th.SimulateJobSuccess(dbSyncJobName) th.SimulateJobSuccess(bootstrapJobName) th.SimulateDeploymentReplicaReady(deploymentName) }) It("creates 3 keys", func() { secret := th.GetSecret(types.NamespacedName{Namespace: keystoneAPIName.Namespace, Name: "keystone"}) Expect(secret).ToNot(BeNil()) Eventually(func(g Gomega) { Expect(maps.Keys(secret.Data)).To(ContainElements("FernetKeys0", "FernetKeys1", "FernetKeys2")) Expect(maps.Keys(secret.Data)).ToNot(ContainElement("FernetKeys3")) for i := 0; i < 3; i++ { g.Expect(secret.Data["FernetKeys"+strconv.Itoa(i)]).NotTo(BeZero()) } }, timeout, interval).Should(Succeed()) }) When("When the fernet keys get updated to 6", func() { BeforeEach(func() { keystone := GetKeystoneAPI(keystoneAPIName) Expect(*keystone.Spec.FernetMaxActiveKeys).To(Equal(int32(3))) keystone.Spec.FernetMaxActiveKeys = ptr.To(int32(6)) _, err := controllerutil.CreateOrPatch( th.Ctx, th.K8sClient, keystone, func() error { return nil }) Expect(err).ToNot(HaveOccurred()) }) It("creates 6 keys", func() { secret := th.GetSecret(types.NamespacedName{Namespace: keystoneAPIName.Namespace, Name: "keystone"}) Expect(secret).ToNot(BeNil()) Eventually(func(g Gomega) { Expect(maps.Keys(secret.Data)).To(ContainElements("FernetKeys0", "FernetKeys1", "FernetKeys2", "FernetKeys3", "FernetKeys4", "FernetKeys5")) Expect(maps.Keys(secret.Data)).ToNot(ContainElement("FernetKeys6")) for i := 0; i < 6; i++ { g.Expect(secret.Data["FernetKeys"+strconv.Itoa(i)]).NotTo(BeZero()) } }, timeout, interval).Should(Succeed()) }) }) })

when you run this it fails with

[FAILED] Expected <[]string | len:5, cap:5>: ["FernetKeys2", "CredentialKeys0", "CredentialKeys1", "FernetKeys0", "FernetKeys1"] to contain elements <[]string | len:6, cap:6>: ["FernetKeys0", "FernetKeys1", "FernetKeys2", "FernetKeys3", "FernetKeys4", "FernetKeys5"] the missing elements were <[]string | len:3, cap:3>: ["FernetKeys3", "FernetKeys4", "FernetKeys5"] In [It] at: src/github.com/openstack-k8s-operators/keystone-operator/tests/functional/keystoneapi_controller_test.go:1178 @ 10/25/24 12:34:45.138

so I think there might be a bug with the logic which adds additional keys?

Thanks for the tip on updating the spec.
Regarding the test part:

Expect(maps.Keys(secret.Data)).To(ContainElements("FernetKeys0", "FernetKeys1", "FernetKeys2")) Expect(maps.Keys(secret.Data)).ToNot(ContainElement("FernetKeys3"))

I prefer to make sure there's no leftovers, for example, if we decrease from 6 to 3 keys, if somehow we ended up in this scenario: FernetKeys0, 1, 2, 5; I wanna make sure ALL the 'FernetKeys%d' are what we expect, so I`m counting how many FK we have and that they are all what we expect. Other way to do this (That I know of) would be to get each FK and see if they are valid; to check that they are valid would be to iterate over a range of fernetMaxKeys, create a string like 'FernetKeys%d' and add it to a list.
I preferred just to count the FernetKeys keys on the secret.

I am not saying you should use from above, but you could still do the same with that, like what I did that a key must not exist. I prefer that as it is easy to read. e.g. when you go from 6 to 3, you:

expect these
Expect(maps.Keys(secret.Data)).To(ContainElements("FernetKeys0", "FernetKeys1", "FernetKeys2"))

you expect that those are gone
Expect(maps.Keys(secret.Data)).ToNot(ContainElements("FernetKeys3", "FernetKeys4", "FernetKeys5"))

What you probably want to enhance, e.g. when you go from 3 to 6 that the last key is the expected one. iiuc in this scenario, the old key[2] should become the key[5], right?

afaranha · 2024-10-24T15:18:56Z

This is a draft because I still need to add tests changing the fernetMaxActiveKeys and having it reflected on the environment.

stuggi · 2024-10-24T15:27:46Z

tests/functional/keystoneapi_controller_test.go

+				}
+
+				Expect(numberFernetKeys).Should(BeNumerically("==", 3))
+				for i := 0; i < 2; i++ {


i < 3 or i <= 2

stuggi · 2024-10-24T15:31:49Z

tests/functional/keystoneapi_controller_test.go

+
+				Expect(numberFernetKeys).Should(BeNumerically("==", 3))
+				for i := 0; i < 2; i++ {
+					g.Expect(secret.Data["FernetKeys"+strconv.Itoa(i)]).NotTo(BeNil())


this won't be nil when the key exists. the value could be the empty string, so probably we want instead of BeNil() check for BeZero()

stuggi · 2024-10-24T15:34:16Z

tests/functional/keystoneapi_controller_test.go

@@ -1109,6 +1110,112 @@ var _ = Describe("Keystone controller", func() {
 		})
 	})

+	When("When the fernet keys are created with fernetMaxActiveKeys as 3", func() {


instead of the separate test bellow, maybe it would be better to test that when updating the keystone parameter from 3 -> 6 we see that the config and the secret got additional keys? Like have a nested when update keys from 3->6 it ...

stuggi · 2024-10-24T15:36:08Z

tests/functional/keystoneapi_controller_test.go

+			secret := th.GetSecret(types.NamespacedName{Namespace: keystoneAPIName.Namespace, Name: "keystone"})
+			Expect(secret).ToNot(BeNil())
+
+			Eventually(func(g Gomega) {
+				numberFernetKeys := 0
+				for k, _ := range secret.Data {
+					if strings.HasPrefix(k, "FernetKeys") {
+						numberFernetKeys++
+					}
+				}
+
+				Expect(numberFernetKeys).Should(BeNumerically("==", 3))
+				for i := 0; i < 2; i++ {
+					g.Expect(secret.Data["FernetKeys"+strconv.Itoa(i)]).NotTo(BeNil())
+				}


Instead of the looping, could do this check for the expected Fernet keys 0-2 and not to have a key3

Expect(maps.Keys(secret.Data)).To(ContainElements("FernetKeys0", "FernetKeys1", "FernetKeys2")) Expect(maps.Keys(secret.Data)).ToNot(ContainElement("FernetKeys3")) for i := 0; i < 3; i++ { g.Expect(secret.Data["FernetKeys"+strconv.Itoa(i)]).NotTo(BeZero()) }

xek · 2024-10-24T20:08:38Z

templates/keystoneapi/config/keystone.conf

@@ -22,7 +22,7 @@ enforce_scope = {{ .enableSecureRBAC }}

 [fernet_tokens]
 key_repository=/etc/keystone/fernet-keys
-max_active_keys=2
+max_active_keys={{ .fernetMaxActiveKeys }}


This setting is only used for Fernet key rotation [1], which we implemented in go. I guess we can keep your change, to be consistent... An alternative could be to just remove it from keystone.conf.

[1] https://github.com/openstack/keystone/blob/6433f1ae1c094e58216360d191a21d956a80f658/keystone/common/fernet_utils.py#L261

afaranha · 2024-10-25T11:23:32Z

tests/functional/keystoneapi_controller_test.go

+
+			keystone := GetKeystoneAPI(keystoneAPIName)
+			keystone.Spec.FernetMaxActiveKeys = ptr.To(int32(6))
+			_, err := controllerutil.CreateOrPatch(


This is not raising an error but the test doesn't work, still keeps 6 keys, maybe that's a bug. I`ll investigate it.

why should this raise an error, with this you update the keystoneapi CR to have the value changed from 3 to 6

Im not expecting an error here, the next line is to enforce there was no issue during the update of values: Expect(err).ToNot(HaveOccurred())`

yes correct, I was reading your first message here that you'd expect an error. sorry misunderstood that.

openshift-ci · 2024-11-07T12:31:34Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/functional	`c56b14c`	link	true	`/test functional`
ci/prow/precommit-check	`c56b14c`	link	true	`/test precommit-check`

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-sigs/prow repository. I understand the commands that are listed here.

afaranha · 2024-11-07T12:47:36Z

Closing this PR because I messed up the rebase.
Opened this one instead: #497

openshift-ci bot added the do-not-merge/work-in-progress label Oct 24, 2024

afaranha mentioned this pull request Oct 24, 2024

Fix keystone template for fernet keys #491

Closed

afaranha commented Oct 24, 2024

View reviewed changes

stuggi reviewed Oct 24, 2024

View reviewed changes

xek reviewed Oct 24, 2024

View reviewed changes

afaranha commented Oct 25, 2024

View reviewed changes

afaranha force-pushed the fernet_functional_tests branch from 00ae7c7 to 05e2403 Compare November 5, 2024 11:07

afaranha marked this pull request as ready for review November 5, 2024 11:08

openshift-ci bot removed the do-not-merge/work-in-progress label Nov 5, 2024

openshift-ci bot requested review from lewisdenny and viroel November 5, 2024 11:08

Add tests for custom number of fernet keys

448471c

afaranha force-pushed the fernet_functional_tests branch from 05e2403 to 448471c Compare November 7, 2024 11:25

fix(deps): update openstack-k8s-operators

d109a7f

afaranha force-pushed the fernet_functional_tests branch from c56b14c to d109a7f Compare November 7, 2024 12:37

Merge branch 'openstack-k8s-operators:main' into fernet_functional_tests

99af0c9

afaranha closed this Nov 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for custom number of fernet keys #492

Add tests for custom number of fernet keys #492

afaranha commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

afaranha Oct 24, 2024

stuggi Oct 24, 2024

afaranha Oct 25, 2024

stuggi Oct 25, 2024 •

edited

Loading

afaranha Oct 25, 2024

stuggi Oct 25, 2024

afaranha commented Oct 24, 2024

stuggi Oct 24, 2024

stuggi Oct 24, 2024

stuggi Oct 24, 2024

stuggi Oct 24, 2024

xek Oct 24, 2024 •

edited

Loading

afaranha Oct 25, 2024

stuggi Oct 25, 2024

afaranha Oct 29, 2024

stuggi Oct 29, 2024

openshift-ci bot commented Nov 7, 2024

afaranha commented Nov 7, 2024

Add tests for custom number of fernet keys #492

Add tests for custom number of fernet keys #492

Conversation

afaranha commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

openshift-ci bot commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuggi Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afaranha commented Oct 24, 2024

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

xek Oct 24, 2024 • edited Loading

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

openshift-ci bot commented Nov 7, 2024

afaranha commented Nov 7, 2024

stuggi Oct 25, 2024 •

edited

Loading

xek Oct 24, 2024 •

edited

Loading