Add support for adoption of existing Flow Logs and Security Groups

[pcolazurdo]

Issue #, if available: aws-controllers-k8s/community#1946

Description of changes:

This proposed fix add support for additionalKeys needed to support adoption of Security Groups and FlowLogs as requested on the issue linked above.

The following adoption instructions will now work:

apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: my-flow-log
spec:
  aws:
    nameOrID: fl-xxxxxxx
    additionalKeys:
      resourceID: vpc-xxxxxx
      resourceType: VPC
  kubernetes:
    group: ec2.services.k8s.aws
    kind: FlowLog
---
apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: my-sg
spec:
  aws:
    nameOrID: sg-xxxxxx
    additionalKeys:
      name: "My GroupName"
  kubernetes:
    group: ec2.services.k8s.aws
    kind: SecurityGroup

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

[ack-prow]

Hi @pcolazurdo. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

[ack-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pcolazurdo
Once this PR has been reviewed and has the lgtm label, please assign likithavemulapalli for approval by writing /assign @likithavemulapalli in a comment. For more information see the Kubernetes Code Review Process.

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

Details

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

[a-hilaly]

/ok-to-test

[a-hilaly]

Neat, thank you @pcolazurdo ! i left few comments in-line
Also can we add some e2e tests to validate that those adoptions are working and won't break in the future?

[pcolazurdo]

Tried to find an example of an adopted check to use as a baseline, but couldn't find one. For these two resources adoption, we should create a new Security Group manually, and create a new VPC with flow log enabled. How should I proceed with this? I don't want to stand up new resources that will be hard to tear down after, and I don't fully understand the e2e workflow.

[a-hilaly]

Tried to find an example of an adopted check to use as a baseline, but couldn't find one. For these two resources adoption, we should create a new Security Group manually, and create a new VPC with flow log enabled. How should I proceed with this? I don't want to stand up new resources that will be hard to tear down after, and I don't fully understand the e2e workflow.

Instead of creating everything manually, you can use ACK it self to provision those resources... once provisioned you can annotate those resources with a delete-retain-policy and try to adopt right after. https://aws-controllers-k8s.github.io/community/docs/user-docs/deletion-policy/

[a-hilaly]

@pcolazurdo Did you test those changes locally? happy to merge if you say it worked for locally. We need a global plan to add resource adoption tests..

[pcolazurdo]

I've done some more testing and there is an issue with the Security Group adoption that I haven't been able to solve as this seems to be an issue with this particular API and how the runtime manager required fields.
The name property in the Security Group Resource is a required property for creation, but because it can be mutated later, it shouldn't be considered as an Identity property. The problem seem to be that the existing specs doesn't let you define a property as required for creation but not for reading (ReadOne). Defining the Name property as an additionalKey for adoption produce the side effect that if the Name specified in the adoption CRD is different that the existing on AWS, the property will be mutated, which is undesired side effect that can break existing environments. We need to find a way to specify properties as required for creation but optional for read.
I'll write a but report on the runtime repo with this

I'll also create a different PR for the VPC Flow Logs piece. In this case, the AWS API doesn't allow mutation so there is no side effect if the user makes a mistake when defining the values of the additionalKeys.