Skip to content

Conversation

kumargauravsharma
Copy link
Contributor

@kumargauravsharma kumargauravsharma commented Jun 10, 2021

Issue: aws-controllers-k8s/community#828

Description of changes:
With this code change, when a field of type slice
is configured as secret type inside Generator config,
then the generated CRD yaml allows specifying multiple k8s secret values
for that field in input yaml and generated sdk code supplies these values
to the resource API input field as slice type.

Added unit tests to test the scenario.

Details on resultant generated code:
Taking ElastiCache User API has passwords filed as example:
ElastiCache User API has passwords filed of type []*string in the API input. Reference API docs

When this field is configured as secret type inside Generator config:

resources:
  User:
    fields:
      Passwords:
        is_secret: true

and service controller code is generated with changes from this PR then,
the generated CRD yaml for password field is:

              passwords:
                description: Passwords used for this user. You can create up to two
                  passwords for each user.
                items:
                  description: SecretKeyReference combines a k8s corev1.SecretReference
                    with a specific key within the referred-to Secret
                  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
                type: array

Observe that the password field is array type with K8s Secreret as element type

and User resource sdk.go file contain following code for password field while setting the inputs:

	if r.ko.Spec.Passwords != nil {
		f3 := []*string{}
		for _, f3iter := range r.ko.Spec.Passwords {
			var f3elem string
			if f3iter != nil {
				tmpSecret, err := rm.rr.SecretValueFromReference(ctx, f3iter)
				if err != nil {
					return nil, err
				}
				if tmpSecret != "" {
					f3elem = tmpSecret
				}
			}
			f3 = append(f3, &f3elem)
		}
		res.SetPasswords(f3)
	}

Observe that it supplies the secrets values as slice type to the input API.

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

Testing:

  1. make test passed for code-generator which contained already existing tests for secret fields code generation scenarios for MQ, ElastiCache ReplicationGroup.
  2. make test passed for ElastiCache ACK controller generated code.
  3. Generated ElastiCache controller code did not have any unrelated/unexpected changes.

Copy link
Contributor

@nmvk nmvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesomesauce, @kumargauravsharma :) I have one question and a small suggestion inline, but overall this looks great.

memberShapeRef, _ := inputShape.MemberRefs[memberName]
memberShape := memberShapeRef.Shape

if r.IsSecretField(memberName) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding the additional conditionals here, how about we move this block of code:

if r.IsSecretFieildmemberName) {
...
}

down to original line 341, right below this:

		default:

which is the code block that executes for scalar field types.

We could clean up the setSDKForSecret function to remove the if XXX != nil {` guard from here:

// if ko.Spec.MasterUserPassword != nil {
out += fmt.Sprintf(
"%sif %s != nil {\n",
indent, sourceVarName,
)

and here:

out += fmt.Sprintf("%s}\n", indent)

which would simplify both the setSDKForSecret function a bit that way and clean this conditional up a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion.
I have refactored the code in next revision.

out += fmt.Sprintf("%s\t}\n", indent)
// if tmpSecret != "" {
// res.SetMasterUserPassword(tmpSecret)
// }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the above code comment to say something like this:

//     if tmpSecret != "" {
//         res.SetMasterUserPassword(tmpSecret)
//     }
//
//    or:
//
//     if tmpSecret != "" {
//         f3elem = tmpSecret
//     }
//
// The second case is used when the SecretKeyReference field
// is a slice of `[]*string` in the original AWS API Input shape.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code comments.

sourceAttributePath := sourceFieldPath
if targetShape.MemberRef.Shape.Type == "structure" {
containerFieldName = targetFieldName
sourceAttributePath = sourceFieldPath+"."
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have a unit test that can stress this code path? I don't believe the unit test about User.Passwords will hit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This path is unit tested in TestSetSDK_MQ_Broker_Create test for Users field.

gt = "*metav1.Time"
} else if fieldCfg != nil && fieldCfg.IsSecret {
gt = "*ackv1alpha1.SecretKeyReference"
gte = "SecretKeyReference"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good fix here. ++

…s Secret

Related issue: aws-controllers-k8s/community#828

With this code change, when a field of type slice
is configured as secret type inside Generator config,
then the generated CRD yaml allows specifying multiple k8s secret values
for that field in input yaml and generated sdk code supplies these values
to the resource API input field as slice type.
@kumargauravsharma kumargauravsharma force-pushed the slice_fields_as_k8s_secret branch from ad7a8d2 to a8e7770 Compare June 10, 2021 23:22
Copy link
Collaborator

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rock on :)

@jaypipes
Copy link
Collaborator

/lgtm

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Jun 11, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jaypipes, kumargauravsharma, nmvk

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:

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

@ack-bot ack-bot merged commit 4857c76 into aws-controllers-k8s:main Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants