feat(ec2): add aws_vpc_security_group_ingress/egress_rule resource by huynhsontung · Pull Request #685 · crossplane-contrib/provider-upjet-aws

huynhsontung · 2023-05-03T06:07:39Z

Description of your changes

Fixes #683

Generated code for aws_vpc_security_group_ingress_rule and aws_vpc_security_group_egress_rule.

I have:

Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested in a personal cluster

apiVersion: ec2.aws.upbound.io/v1beta1
kind: VPCSecurityGroupIngressRule
metadata:
  annotations:
    crossplane.io/external-create-succeeded: '2023-09-27T17:01:35-07:00'
    crossplane.io/external-name: sgr-09ce7f826ed2b32c8
    meta.upbound.io/example-id: ec2/v1beta1/vpcsecuritygroupingressrule
    upjet.crossplane.io/provider-meta: ''
  creationTimestamp: '2023-09-28T00:00:42Z'
  finalizers:
    - finalizer.managedresource.crossplane.io
  generation: 4
  name: ingress-sgr-test
  resourceVersion: '86044428'
  uid: 8efe5fe6-d2b7-4879-9fce-d798f97abeb0
spec:
  deletionPolicy: Delete
  forProvider:
    cidrIpv4: 10.22.10.0/24
    fromPort: 10250
    ipProtocol: tcp
    region: us-east-2
    securityGroupId: sg-0e0c05b477edebd15
    tags:
      crossplane-kind: vpcsecuritygroupingressrule.ec2.aws.upbound.io
      crossplane-name: ingress-sgr-test
      crossplane-providerconfig: default
    toPort: 10250
  initProvider: {}
  managementPolicies:
    - '*'
  providerConfigRef:
    name: default
status:
  atProvider:
    arn: >-
      arn:aws:ec2:us-east-2:******:security-group-rule/sgr-09ce7f826ed2b32c8
    cidrIpv4: 10.22.10.0/24
    fromPort: 10250
    id: sgr-09ce7f826ed2b32c8
    ipProtocol: tcp
    securityGroupId: sg-0e0c05b477edebd15
    securityGroupRuleId: sgr-09ce7f826ed2b32c8
    tags:
      crossplane-kind: vpcsecuritygroupingressrule.ec2.aws.upbound.io
      crossplane-name: ingress-sgr-test
      crossplane-providerconfig: default
    tagsAll:
      crossplane-kind: vpcsecuritygroupingressrule.ec2.aws.upbound.io
      crossplane-name: ingress-sgr-test
      crossplane-providerconfig: default
    toPort: 10250
  conditions:
    - lastTransitionTime: '2023-09-28T00:01:35Z'
      reason: ReconcileSuccess
      status: 'True'
      type: Synced
    - lastTransitionTime: '2023-09-28T00:01:50Z'
      reason: Available
      status: 'True'
      type: Ready
    - lastTransitionTime: '2023-09-28T00:01:38Z'
      reason: Success
      status: 'True'
      type: LastAsyncOperation
    - lastTransitionTime: '2023-09-28T00:01:38Z'
      reason: Finished
      status: 'True'
      type: AsyncOperation

Upbound-CLA · 2023-05-03T06:07:43Z

All committers have signed the CLA.

jeanduplessis · 2023-05-03T06:34:41Z

/test-examples="examples/vpc/securitygroupegressrule.yaml,examples/vpc/securitygroupingressrule.yaml"

jeanduplessis

@huynhsontung, thanks for the contribution. Two small changes please to clean up the examples.

huynhsontung · 2023-05-03T17:51:51Z

/test-examples="examples/vpc/securitygroupegressrule.yaml,examples/vpc/securitygroupingressrule.yaml"

jeanduplessis · 2023-05-03T17:53:43Z

/test-examples="examples/vpc/securitygroupegressrule.yaml,examples/vpc/securitygroupingressrule.yaml"

huynhsontung · 2023-05-03T20:27:18Z

Log

case.go:363: retrieving API resource for vpc.aws.upbound.io/v1beta1, Kind=SecurityGroupEgressRule failed: the server could not find the requested resource
case.go:363: retrieving API resource for vpc.aws.upbound.io/v1beta1, Kind=SecurityGroupIngressRule failed: the server could not find the requested resource

The package was built and deployed successfully but could not find the API resource. I'm not sure what is missing.

turkenf · 2023-05-04T09:29:00Z

The package was built and deployed successfully but could not find the API resource. I'm not sure what is missing.

Hi @huynhsontung thank you for this PR, for resources to be uptestable, dependent resources should be added to the example manifest. In this case ec2.SecurityGroup and ec2.VPC, e.g. securitygrouprule-self-true.yaml.

But first of all, it would be good if you have tested it successfully in your local environment and add a description under the heading How has this code been tested.

I gave it a try in my local environment but I could not figure it out and I get the following error:

      message: "observe failed: cannot run refresh: refresh failed: reading VPC Security
      Group Rule (): InvalidSecurityGroupRuleId.Malformed: The security group rule
      ID '' is malformed\n\tstatus code: 400, request id: 39c253e2-4845-4349-8774-62a1969a27b9"

Also, I'm not sure if these two resources should be in the vpc group. I think these resources may need to be in the ec2 group. What do you think about the group, @ulucinar?

huynhsontung · 2023-05-30T01:31:54Z

@turkenf Sorry for the late followup. I went back and updated some missing codegen configs to reference external resources correctly. However, I still get this error message regardless of what config I change.

cannot run refresh: refresh failed: reading VPC Security Group Rule (): InvalidSecurityGroupRuleId.Malformed: The security group rule ID '' is malformed
        status code: 400, request id: bb0840ad-3c70-4405-8c03-ffd864b68399      {"type": "Warning", "object": {"kind":"SecurityGroupEgressRule","name":"egressrule-test","uid":"1f43bf65-2d6d-4b97-af92-4e2d55b6f815","apiVersion":"vpc.aws.upbound.io/v1beta1","resourceVersion":"18950285"}, "reason": "CannotObserveExternalResource"}

terraform.tfstate

{
  "version": 4,
  "terraform_version": "1.2.1",
  "serial": 1,
  "lineage": "1f43bf65-2d6d-4b97-af92-4e2d55b6f815",
  "outputs": null,
  "resources": [
    {
      "mode": "managed",
      "type": "aws_vpc_security_group_egress_rule",
      "name": "egressrule-test",
      "provider": "provider[\"registry.terraform.io/hashicorp/aws\"]",
      "instances": [
        {
          "schema_version": 0,
          "attributes": {
            "cidr_ipv4": "10.0.0.0/8",
            "from_port": 80,
            "id": "", 
            "ip_protocol": "tcp",
            "security_group_id": "sg-0123456789abc",
            "tags": {
              "crossplane-kind": "securitygroupegressrule.vpc.aws.upbound.io",
              "crossplane-name": "egressrule-test",
              "crossplane-providerconfig": "default"
            },
            "to_port": 8080
          }
        }
      ]
    }
  ]
}

It seems that on the first reconciliation, the provider generates a stub tfstate file with an empty id field and Terraform doesn't like that. If I inject the SGR external name using crossplane.io/external-name annotation then it starts to reconcile just fine.

I am not aware of any way to customize the provider tfstate generation behaviour and am not sure where to go from here. I would appreciate some guidance on this issue.

turkenf · 2023-06-21T15:04:00Z

@huynhsontung thank you for your observations, my observation with provider-aws v0.36.0 is also the same as yours:

I keep getting the same error:

      message: "observe failed: cannot run refresh: refresh failed: reading VPC Security
      Group Rule (): InvalidSecurityGroupRuleId.Malformed: The security group rule
      ID '' is malformed\n\tstatus code: 400, request id: 39c253e2-4845-4349-8774-62a1969a27b9"

When I create the egress_rule resource in terraform with the example below:

resource "aws_vpc_security_group_egress_rule" "example" {
  security_group_id = "sg-0e625fc4bf678386f"

  cidr_ipv4   = "10.0.0.0/8"
  from_port   = 80
  ip_protocol = "tcp"
  to_port     = 8080
}

The attributes.id in the tfstate file comes as "id": "sgr-0ec075aef37950e08",:

          "schema_version": 0,
          "attributes": {
            "arn": "arn:aws:ec2:us-west-1:569674547146:security-group-rule/sgr-0ec075aef37950e08",
            "cidr_ipv4": "10.0.0.0/8",
            "cidr_ipv6": null,
            "description": null,
            "from_port": 80,
            "id": "sgr-0ec075aef37950e08",
            "ip_protocol": "tcp",
            "prefix_list_id": null,
            "referenced_security_group_id": null,
            "security_group_id": "sg-0e625fc4bf678386f",
            "security_group_rule_id": "sgr-0ec075aef37950e08",
            "tags": null,
            "tags_all": {},
            "to_port": 8080
          },
          "sensitive_attributes": []

In our example, the id in the tfstate file is somehow empty and this causes an error and the resource is not created:

                    "schema_version": 0,
                    "attributes": {
                        "cidr_ipv4": "10.0.0.0/8",
                        "from_port": 80,
                        "id": "",
                        "ip_protocol": "tcp",
                        "security_group_id": "sg-0e625fc4bf678386f",
                        "tags": {
                            "crossplane-kind": "securitygroupegressrule.vpc.aws.upbound.io",
                            "crossplane-name": "example",
                            "crossplane-providerconfig": "default"
                        },
                        "to_port": 8080

@ulucinar, what could be the reason why the id transferred empty to Terraform, do you have any suggestions about this?

jeanduplessis · 2023-06-23T10:12:58Z

/test-examples="examples/vpc/securitygroupegressrule.yaml,examples/vpc/securitygroupingressrule.yaml"

ulucinar · 2023-07-21T14:14:23Z

+)
+
+// Configure adds configurations for vpc group.
+func Configure(p *config.Provider) {


Hi @turkenf,
We had better understand whether we still need the SecurityGroupRule.ec2 resource as two separate ingress and egress rules are being introduced. Looking at https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group_rule, Terraform has not deprecated this resource but we may still mark it as deprecated if there are not configurations not covered by these new two resources (for instance, are there any other rule types other than ingress and egress, etc.)?
Please also see the following from Terraform docs:

... Both of these resource were added before AWS assigned a [security group rule unique ID](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-rules.html), and they do not work well in all scenarios using thedescription and tags attributes, which rely on the unique ID.

Could this be the reason Terraform is not deprecating aws_security_group_rule?

huynhsontung · 2023-09-28T00:15:40Z

@turkenf @ulucinar I have found a solution for the malformed ID issue. Terraform performs state validation so when the rule's ID is empty, it raises an error and exits. I found out that by providing a fake ID value that has sgr- prefix (I used sgr-stub), it was able to pass validation and reconcile successfully. Once the provider gets past the initial resource creation, the proper SGR ID is populated in the state file and the external name as expected. Tested successfully in my own cluster.

I also moved the resources into the ec2 group.

turkenf · 2023-09-28T08:36:05Z

/test-examples="examples/vpc/securitygroupegressrule.yaml"

turkenf · 2023-09-28T08:36:15Z

/test-examples="examples/vpc/securitygroupingressrule.yaml"

turkenf · 2023-09-28T10:42:43Z

/test-examples="examples/ec2/vpcsecuritygroupegressrule.yaml"

turkenf · 2023-09-28T10:43:16Z

/test-examples="examples/ec2/vpcsecuritygroupingressrule.yaml"

haarchri · 2023-10-09T11:56:07Z

@turkenf any ETA for this ?

turkenf · 2023-10-09T15:17:28Z

/test-examples="examples/ec2/vpcsecuritygroupingressrule.yaml"

turkenf

Thank you for your effort in this PR @huynhsontung, I left a few comments for you to consider.
Also could you please rebase PR to main?

turkenf · 2023-10-09T16:37:54Z


+	p.AddResourceConfigurator("aws_vpc_security_group_ingress_rule", func(r *config.Resource) {
+		r.References["security_group_id"] = config.Reference{
+			Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.SecurityGroup",


Suggested change

Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.SecurityGroup",

TerraformName: "aws_security_group",

nit: it could be better to use the above conf for all references.

I missed updating these references when moving the resources from the vpc to ec2 group. I think we can use Type: "SecurityGroup" now that they are all in the same ec2 group, similar to other configs in this file.

It can stay this way, but the safest way to set references is to use TerraformName.

Signed-off-by: Tung Huynh <tung75605@gmail.com>

turkenf · 2023-10-10T07:19:53Z

/test-examples="examples/ec2/vpcsecuritygroupingressrule.yaml"

turkenf · 2023-10-10T07:28:20Z

/test-examples="examples/ec2/securitygroupingressrule.yaml"

turkenf · 2023-10-10T08:11:07Z

/test-examples="examples/ec2/securitygroupegressrule.yaml"

huynhsontung · 2023-10-10T08:44:07Z

Apply is failing after rebase. I will debug this on my end. Not sure why import didn't pass.

Signed-off-by: Tung Huynh <tung75605@gmail.com>

huynhsontung · 2023-10-10T08:53:19Z

@turkenf To speed up the debug process, would you mind rerunning the uptest on one of the resources?

turkenf · 2023-10-10T09:06:05Z

Apply is failing after rebase. I will debug this on my end. Not sure why import didn't pass.

With the latest uptest update, update and import tests were added to automatic testing, these were not tested before. Have you done the import test of the resources locally?

turkenf · 2023-10-10T09:06:22Z

/test-examples="examples/ec2/securitygroupingressrule.yaml"

huynhsontung · 2023-10-10T09:20:31Z

With the latest uptest update, update and import tests were added to automatic testing, these were not tested before. Have you done the import test of the resources locally?

I tested import before rebase. I can see from the log that the atProvider object is populated. The test failed when trying to check the VPC ID.

turkenf · 2023-10-10T09:31:35Z

@huynhsontung, could you please rebase again after this PR merged and revert the external name change?

Signed-off-by: Fatih Türken <turkenf@gmail.com>

turkenf · 2023-10-10T12:36:21Z

/test-examples="examples/ec2/securitygroupingressrule.yaml"

turkenf · 2023-10-10T12:46:50Z

/test-examples="examples/ec2/securitygroupegressrule.yaml"

turkenf

Thank you so much for your effort in this PR @huynhsontung, LGTM.

huynhsontung requested review from sergenyalcin and ulucinar as code owners May 3, 2023 06:07

jeanduplessis requested changes May 3, 2023

View reviewed changes

Comment thread examples/vpc/securitygroupegressrule.yaml Outdated

Comment thread examples/vpc/securitygroupingressrule.yaml Outdated

turkenf mentioned this pull request May 3, 2023

Request for aws_vpc_security_group_egress_rule and aws_vpc_security_group_ingress_rule resource #683

Closed

haarchri mentioned this pull request May 4, 2023

security_group_rule import not working InvalidPermission.Duplicate #656

Closed

huynhsontung force-pushed the add-vpc-sgr branch 2 times, most recently from de897ed to 4469a40 Compare May 25, 2023 09:21

huynhsontung force-pushed the add-vpc-sgr branch from 4469a40 to 0648256 Compare June 22, 2023 05:37

jeanduplessis self-requested a review June 23, 2023 10:13

ulucinar reviewed Jul 21, 2023

View reviewed changes

huynhsontung force-pushed the add-vpc-sgr branch from 0648256 to aa620c3 Compare September 27, 2023 16:29

huynhsontung requested a review from turkenf as a code owner September 27, 2023 16:29

huynhsontung force-pushed the add-vpc-sgr branch from aa620c3 to 51fb249 Compare September 27, 2023 16:37

huynhsontung changed the title ~~feat(vpc): add aws_vpc_security_group_ingress/egress_rule resource~~ feat(ev2): add aws_vpc_security_group_ingress/egress_rule resource Sep 28, 2023

huynhsontung changed the title ~~feat(ev2): add aws_vpc_security_group_ingress/egress_rule resource~~ feat(ec2): add aws_vpc_security_group_ingress/egress_rule resource Sep 28, 2023

turkenf reviewed Oct 9, 2023

View reviewed changes

huynhsontung added 4 commits October 10, 2023 06:05

add aws_vpc_security_group_ingress/egress_rule resource

78ad3b7

Signed-off-by: Tung Huynh <tung75605@gmail.com>

use custom external name to get pass tf validation

b4b719b

Signed-off-by: Tung Huynh <tung75605@gmail.com>

rename resources to be less verbose

4e732de

Signed-off-by: Tung Huynh <tung75605@gmail.com>

simplify reference types

22f28a9

Signed-off-by: Tung Huynh <tung75605@gmail.com>

huynhsontung force-pushed the add-vpc-sgr branch from 63090d1 to ab323f4 Compare October 10, 2023 06:28

codegen

5d825b2

Signed-off-by: Tung Huynh <tung75605@gmail.com>

huynhsontung force-pushed the add-vpc-sgr branch from ab323f4 to 5d825b2 Compare October 10, 2023 06:35

revert custom external name

7f9a972

Signed-off-by: Tung Huynh <tung75605@gmail.com>

Bump uptest version and revert externalname changes

da9dbaa

Signed-off-by: Fatih Türken <turkenf@gmail.com>

turkenf approved these changes Oct 10, 2023

View reviewed changes

jeanduplessis approved these changes Oct 10, 2023

View reviewed changes

turkenf merged commit bb96f30 into crossplane-contrib:main Oct 10, 2023

huynhsontung deleted the add-vpc-sgr branch October 11, 2023 02:34

	Type: "github.com/upbound/provider-aws/apis/ec2/v1beta1.SecurityGroup",
	TerraformName: "aws_security_group",

Uh oh!

Conversation

huynhsontung commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Uh oh!

Upbound-CLA commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanduplessis commented May 3, 2023

Uh oh!

jeanduplessis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huynhsontung commented May 3, 2023

Uh oh!

jeanduplessis commented May 3, 2023

Uh oh!

huynhsontung commented May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turkenf commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

huynhsontung commented May 30, 2023

Uh oh!

turkenf commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeanduplessis commented Jun 23, 2023

Uh oh!

ulucinar Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huynhsontung commented Sep 28, 2023

Uh oh!

turkenf commented Sep 28, 2023

Uh oh!

turkenf commented Sep 28, 2023

Uh oh!

turkenf commented Sep 28, 2023

Uh oh!

turkenf commented Sep 28, 2023

Uh oh!

haarchri commented Oct 9, 2023

Uh oh!

turkenf commented Oct 9, 2023

Uh oh!

turkenf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

turkenf Oct 9, 2023

Choose a reason for hiding this comment

Uh oh!

huynhsontung Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

turkenf Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

turkenf commented Oct 10, 2023

Uh oh!

turkenf commented Oct 10, 2023

Uh oh!

turkenf commented Oct 10, 2023

Uh oh!

huynhsontung commented Oct 10, 2023

Uh oh!

huynhsontung commented Oct 10, 2023

Uh oh!

turkenf commented Oct 10, 2023

Uh oh!

turkenf commented Oct 10, 2023

Uh oh!

huynhsontung commented May 3, 2023 •

edited

Loading

Upbound-CLA commented May 3, 2023 •

edited

Loading

huynhsontung commented May 3, 2023 •

edited

Loading

turkenf commented May 4, 2023 •

edited

Loading

turkenf commented Jun 21, 2023 •

edited

Loading

ulucinar Jul 21, 2023 •

edited

Loading