Partial support for User resource, e2e test #32

echen-98 · 2021-06-04T17:40:55Z

Issue #, if available:

Description of changes:

Summary of the generator config/custom code:

add terminal codes
add additional Status fields for AccessString state-keeping which are only modified upon creation or modification of the User (needed because the ElastiCache API returns an expanded AccessString different than the one specified)
"custom modify" code only handles requeues (no actual modification done)
remove differences from Delta which are not meaningful
only populate update payload with fields that have actual differences between desired/latest (via ignoring most field paths in default modify payload and using a post_build_request hook)
post_set_output hook handles synced condition
e2e CRUD test

Working on this has revealed the need for a couple small improvements in common repos (see below) which could significantly simplify the code. So although we need some more custom code (e.g. secrets support for Passwords field among other things) this feels like a good stopping point until those get resolved.

aws-controllers-k8s/community#806
aws-controllers-k8s/community#808

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

RedbackThomson · 2021-06-04T21:25:31Z

/ok-to-test
/test elasticache-kind-e2e
/test elasticache-unit-test

kumargauravsharma · 2021-06-09T13:50:36Z

apis/v1alpha1/user.go

+	// Passwords used for this user. You can create up to two passwords for each
+	// user.
+	Passwords []*string `json:"passwords,omitempty"`


This needs to be of *ackv1alpha1.SecretKeyReference type.

Please refer next comment for details:

This needs to be array of K8s Secret type.

As discussed on this PR and offline, we will temporarily disable the passwords field for this PR, and I will follow up soon with another PR making use of aws-controllers-k8s/code-generator#87 to support this

kumargauravsharma · 2021-06-09T13:56:20Z

config/crd/bases/elasticache.services.k8s.aws_users.yaml

+              passwords:
+                description: Passwords used for this user. You can create up to two
+                  passwords for each user.
+                items:
+                  type: string
+                type: array


This needs to be array of K8s Secret type.
Please refer replication group yaml authToken field for reference.

@kumargauravsharma see the PR description... @echen-98 identified this as being a problem.

Since Passwords is a slice of string and *ackv1alpha1.SecretKeyReference refers to a single secret key, not multiple things, we have a mismatch here.

If you mark User.spec.passwords as is_secret, you will essentially need to write custom code for handling all of User's newCreateBuildRequest and setOutput code points. This custom code will need to translate between a single string value stored in the SecretKeyReference and the multiple strings needed in the Passwords field of the CreateUserInput shape... You will need to deal with the whole "up to two passwords" constraint manually in custom code and have the user place the passwords in a comma-delimited string in the SecretKeyReference storage.

Alternately, you could set the User.spec.passwords and User.spec.noPasswordRequired fields as is_ignored in the generator config and set noPasswordRequired to always be true in the CreateUserInput shape...

@jaypipes Thanks for pointing that out. I was initially considering something closer to the latter option and leaving support for the passwords field for later. As a result I wasn't aware of this additional challenge.

At the same time the passwords are an important part of the User APIs, so I'll look into it further

@jaypipes thanks for your inputs.
My comment was to have it as array (slice) of K8s Secret type.
Thus the generated Spec code would have this property as:

Passwords []*ackv1alpha1.SecretKeyReference `json:"passwords,omitempty"`

and the corresponding crd would look like:

passwords: description: Passwords used for this user. You can create up to two passwords for each user. type: array items: properties: key: description: Key is the key within the secret type: string name: description: Name is unique within a namespace to reference a secret resource. type: string namespace: description: Namespace defines the space within which the secret name must be unique. type: string required: - key type: object

With this, there is no need to store multiple password strings into single K8s secret field.
Rather, each password in slice will have its corresponding K8s secret field for value.

We considered not to exclude password fields from spec.
However, to not block this PR for this change, temporarily we can remove this field from the spec if at the moment, its not feasible to have

Passwords []*ackv1alpha1.SecretKeyReference `json:"passwords,omitempty"`

@kumargauravsharma sounds good, I will follow up with another revision and then keep the support for the passwords field as a follow-up.

I'll probably also rename the PR and commit to say "partial support" to convey this.

PR: aws-controllers-k8s/code-generator#87 adds support for K8s Secret for slice types.

kumargauravsharma · 2021-06-09T14:13:48Z

generator.yaml

+  CreateUser:
+    set_output_custom_method_name: CustomCreateUserSetOutput
+  ModifyUser:
+    custom_implementation: CustomModifyUser
+    set_output_custom_method_name: CustomModifyUserSetOutput


These can be removed and required logic can be added to hooks configuration that is already specified for these methods in this PR.

We need to do 2 things after receiving an API response:

set the synced condition based on the User's status.status

update the state-keeping status fields of LastRequestedAccessString and ExpandedAccessString, but only upon successful create and modify calls. The logic to handle the access string depends on LastRequestedAccessString to not be changed upon describe calls.

It's certainly possible to move the first item to the custom hooks, but then we would need two separate hook functions:

One that gets called from create and modify, that does both the items above.

One that gets called from read, that does only the second item above.

I think this approach might make first hook a bit too convoluted in terms of combining two tasks into one. Further, since it would be called from both sdkCreate and sdkUpdate, but the API response types are different, we would need to pass in both resp.Status and resp.AccessString as separate arguments.

I chose the initial approach since the task accomplished by the hook and the task accomplished by the set_output functions are separated and well-defined, but if I'm missing a simpler way to do this please let me know.

thanks for the details @echen-98
lets keep it as for now

kumargauravsharma · 2021-06-09T14:17:55Z

pkg/resource/user/post_build_request.go

+	//TODO: this field requires the use of k8s secrets
+	if delta.DifferentAt("Passwords") && r.ko.Spec.Passwords != nil {


Please update the code for this TODO.

As discussed above, will remove password-related code and keep it for the next PR

jaypipes

This is definitely going to be a tricky one due to the odd shape of the Elasticache CreateUser APIs...

jaypipes · 2021-06-09T15:42:04Z

config/crd/bases/elasticache.services.k8s.aws_users.yaml

+              passwords:
+                description: Passwords used for this user. You can create up to two
+                  passwords for each user.
+                items:
+                  type: string
+                type: array


@kumargauravsharma see the PR description... @echen-98 identified this as being a problem.

Since Passwords is a slice of string and *ackv1alpha1.SecretKeyReference refers to a single secret key, not multiple things, we have a mismatch here.

If you mark User.spec.passwords as is_secret, you will essentially need to write custom code for handling all of User's newCreateBuildRequest and setOutput code points. This custom code will need to translate between a single string value stored in the SecretKeyReference and the multiple strings needed in the Passwords field of the CreateUserInput shape... You will need to deal with the whole "up to two passwords" constraint manually in custom code and have the user place the passwords in a comma-delimited string in the SecretKeyReference storage.

Alternately, you could set the User.spec.passwords and User.spec.noPasswordRequired fields as is_ignored in the generator config and set noPasswordRequired to always be true in the CreateUserInput shape...

echen-98 · 2021-06-10T20:21:24Z

/test elasticache-kind-e2e

echen-98 · 2021-06-10T20:31:53Z

/retest

echen-98 · 2021-06-10T20:38:43Z

/retest

ack-bot · 2021-06-10T21:44:13Z

@echen-98: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
elasticache-kind-e2e	`b4e76c3`	link	`/test elasticache-kind-e2e`

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.

echen-98 · 2021-06-10T22:19:38Z

e2e test job is now running properly. Some tests are failing but the one associated with this PR is passing:

 [gw1] [ 14%] SKIPPED tests/test_replicationgroup_largecluster.py::TestReplicationGroupLargeCluster::test_rg_largecluster 
[gw0] [ 28%] ERROR tests/test_replicationgroup.py::TestReplicationGroup::test_rg_input_coverage 
tests/test_replicationgroup.py::TestReplicationGroup::test_rg_cmd_fromsnapshot 
[gw2] [ 42%] PASSED tests/test_user.py::TestUser::test_CRUD 
[gw3] [ 57%] PASSED tests/test_snapshot.py::TestSnapshot::test_snapshot_kms 
[gw0] [ 71%] FAILED tests/test_replicationgroup.py::TestReplicationGroup::test_rg_cmd_fromsnapshot 
tests/test_replicationgroup.py::TestReplicationGroup::test_rg_cmd_update 
[gw0] [ 85%] PASSED tests/test_replicationgroup.py::TestReplicationGroup::test_rg_cmd_update 
tests/test_replicationgroup.py::TestReplicationGroup::test_rg_auth_token 
[gw0] [100%] ERROR tests/test_replicationgroup.py::TestReplicationGroup::test_rg_auth_token 
[gw0] [100%] ERROR tests/test_replicationgroup.py::TestReplicationGroup::test_rg_auth_token

I will investigate the test failures and address them in another PR if needed

ack-bot · 2021-06-11T17:10:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: echen-98, kumargauravsharma

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~~ [echen-98,kumargauravsharma]

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

echen-98 requested review from jaypipes, nmvk and kumargauravsharma June 4, 2021 17:40

ack-bot requested review from a-hilaly and vijtrip2 June 4, 2021 17:40

ack-bot added the approved label Jun 4, 2021

ack-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 4, 2021

kumargauravsharma suggested changes Jun 9, 2021

View reviewed changes

jaypipes reviewed Jun 9, 2021

View reviewed changes

kumargauravsharma mentioned this pull request Jun 10, 2021

Extend ack-generate to generate slice with elements of type Kubernetes Secret aws-controllers-k8s/community#828

Closed

partial support for User resource, e2e test

b4e76c3

echen-98 force-pushed the user-support branch from 29ab8ee to b4e76c3 Compare June 10, 2021 20:17

echen-98 changed the title ~~Initial support for User resource, e2e test~~ Partial support for User resource, e2e test Jun 10, 2021

kumargauravsharma approved these changes Jun 11, 2021

View reviewed changes

kumargauravsharma merged commit d7f5006 into aws-controllers-k8s:main Jun 11, 2021

echen-98 mentioned this pull request Jun 17, 2021

address regression with User code, fix RG deletion test #35

Merged

This was referenced Jun 26, 2021

Clarifying ACK Update logic (spec/status, delta, etc.) aws-controllers-k8s/community#846

Closed

Proposal for handling late-initialization in ACK. aws-controllers-k8s/community#849

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial support for User resource, e2e test #32

Partial support for User resource, e2e test #32

echen-98 commented Jun 4, 2021

RedbackThomson commented Jun 4, 2021

kumargauravsharma Jun 9, 2021

kumargauravsharma Jun 9, 2021

echen-98 Jun 10, 2021

kumargauravsharma Jun 9, 2021

jaypipes Jun 9, 2021

echen-98 Jun 9, 2021

kumargauravsharma Jun 9, 2021

kumargauravsharma Jun 9, 2021 •

edited

Loading

echen-98 Jun 9, 2021

kumargauravsharma Jun 10, 2021

kumargauravsharma Jun 9, 2021

echen-98 Jun 10, 2021

kumargauravsharma Jun 11, 2021

kumargauravsharma Jun 9, 2021

echen-98 Jun 10, 2021

jaypipes left a comment

jaypipes Jun 9, 2021

echen-98 commented Jun 10, 2021

echen-98 commented Jun 10, 2021

echen-98 commented Jun 10, 2021

ack-bot commented Jun 10, 2021

echen-98 commented Jun 10, 2021

ack-bot commented Jun 11, 2021

		//TODO: this field requires the use of k8s secrets
		if delta.DifferentAt("Passwords") && r.ko.Spec.Passwords != nil {

Partial support for User resource, e2e test #32

Partial support for User resource, e2e test #32

Conversation

echen-98 commented Jun 4, 2021

RedbackThomson commented Jun 4, 2021

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

kumargauravsharma Jun 9, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaypipes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

echen-98 commented Jun 10, 2021

echen-98 commented Jun 10, 2021

echen-98 commented Jun 10, 2021

ack-bot commented Jun 10, 2021

echen-98 commented Jun 10, 2021

ack-bot commented Jun 11, 2021

kumargauravsharma Jun 9, 2021 •

edited

Loading