Fix `AdoptedResource` functionality for `SecurityGroups`

[nnbu]

Issue # : https://github.com/aws-controllers-k8s/community/issues/1946

Description of changes: RCA: Following CR currently fails to adopt the existing SecurityGroups resource.

apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: sg1
spec:
  aws:
    nameOrID: sg-07fd54f8e16eec74a
  kubernetes:
    group: ec2.services.k8s.aws
    kind: SecurityGroup
    metadata:
      name: sg-adopted
      namespace: default

The error

"error": "SecurityGroup.ec2.services.k8s.aws \"my-resource\" is invalid: spec.name: Required value"

comes from https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/adoption_reconciler.go#L157

SetIdentifiers function needs to have code to fill up required values of the CR so as to solve this issue.

Solution: A new fields is defined as name and it is initialized in SetIdentifiers function. This field needs to be part of additionalKeys field of AdoptedResource CR for SecurityGroups.

The new CR looks like,

apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: sg1
spec:
  aws:
    nameOrID: sg-xxxxxx
    additionalKeys:
      name: <security-group-name>
  kubernetes:
    group: ec2.services.k8s.aws
    kind: SecurityGroup
    metadata:
      name: sg-adopted
      namespace: default

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

[ack-prow[bot]]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nnbu Once this PR has been reviewed and has the lgtm label, please assign a-hilaly for approval by writing /assign @a-hilaly 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.

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

[ack-prow[bot]]

Hi @nnbu. 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.

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.

[nnbu]

Hi @a-hilaly This change is similar to FlowLog change which was merged yesterday. Could you please review?

[pcolazurdo]

Hi @nnbu - have you tested this? I've tried something similar before, but in this case, if the user set the wrong Name in the adoption, the existing value in AWS is overwritten which is an undesired side effect of this. Just checking if this is not the case. I have a PR in progress that follows a different method and avoid that problem

[nnbu]

Hi @nnbu - have you tested this? I've tried something similar before, but in this case, if the user set the wrong Name in the adoption, the existing value in AWS is overwritten which is an undesired side effect of this. Just checking if this is not the case. I have a PR in progress that follows a different method and avoid that problem

Hi pcolazurdo I have tested this and I dont see the behavior you mentioned.

Here is what I have tried.

1. Created SG from aws console with security group name as test-1
2. Then created following CR

apiVersion: services.k8s.aws/v1alpha1
kind: AdoptedResource
metadata:
  name: sg1
spec:
  aws:
    nameOrID: <sg created in the previous step>
    additionalKeys:
      name: nonexisting-name
  kubernetes:
    group: ec2.services.k8s.aws
    kind: SecurityGroup
    metadata:
      name: sg-adopted1
      namespace: default

3. Verified in the console that security group name remains unchanged.
4. Of course, when this wrong name is specified in the CR, there is a mismatch between security group name in the CR and that seen from aws console. But that does not affect adoptability.

[nnbu]

In fact, there is an unrelated issue here. name change in securityGroup CR does not change the name of aws SG resource.

Here are the steps

1. Apply following CR

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
  name: sg-not-adopted1
  namespace: default
spec:
  description: not-adopted
  egressRules:
  - ipProtocol: "-1"
    ipRanges:
    - cidrIP: 0.0.0.0/0
  name: not-adopted-sg
  vpcID: vpc-xxxxxx

2. Verify in the aws console that security group name is set to not-adopted-sg.
3. change the name in the CR so that it looks like following

apiVersion: ec2.services.k8s.aws/v1alpha1
kind: SecurityGroup
metadata:
  name: sg-not-adopted1
  namespace: default
spec:
  description: not-adopted
  egressRules:
  - ipProtocol: "-1"
    ipRanges:
    - cidrIP: 0.0.0.0/0
  name: not-adopted-sg-CHANGED
  vpcID: vpc-xxxxxx

4. The security group name is not changed in aws console.

This happens because ec2 controller does not detect the name change when it calculates the diff between desired CR and existing resource.

This bug is not related to adoptResrouce functionality though.

[a-hilaly]

implemented in https://github.com/aws-controllers-k8s/ec2-controller/pull/165