Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/alicloud_vswitch: Adds ipv6_cidr_block #3427

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ezio-ballarin
Copy link

No description provided.

@xiaozhu36
Copy link
Member

HI @ezio-ballarin Can you add the testcase for this feature?

@ezio-ballarin
Copy link
Author

hey @xiaozhu36, are these tests OK? I couldn't find where to enable IPv6 for the VPC in the resource_alicloud_vswitch_test.go file. I need the default VPC to be IPv6 enabled, could you help me verify or find where to edit?

@ezio-ballarin
Copy link
Author

hi again @xiaozhu36 it's been a couple months, any update here?

@xiaozhu36
Copy link
Member

xiaozhu36 commented Jun 11, 2021

HI @ezio-ballarin I am sorry for late reply. Did you run this your testcase? I think there is any OpenAPI bug and this bug can be found by running the testcase.
The bug is Ipv6CidrBlock type is int in the CreateVSwitch but in the DescribeVSwitchAttributes is string. It will fail when running d.Set(ipv6_cidr_block) and have a diff when running the next terraform command.
It is the OpenAPI. bug and I have submitted this issue to product team to fix it. You can also create a ticket to push them.

@ezio-ballarin
Copy link
Author

No worries @xiaozhu36, we are all very busy so I understand. I've read through all the docs in this repo and I am probably not understanding enough, but is "acceptance testing" what I need to do for this? It sounds like I will need to run the acceptance tests.

Also, I will look into the OpenAPI type issue you pointed out and talk with my TAMs about it.

@LiangZugeng
Copy link

@xiaozhu36 I don't think it's a bug, it's designed as is.

  1. When creating the switch, the first 56-bit of IPv6 address was inherited from the VPC, therefore it only requires a number (0-255) as the last 8-bit of the IPv6 subnet (::/64).

  2. The IPv6CidrBlock attribute in DescribeVSwitchAttribute is a string which combines the first 56-bit from the VPC and the 8-bit of the VSwitch to show the full IPv6 subnet.

I'd like to propose a solution: Add a new int type argument called ipv6_cidr_block_suffix and make ipv6_cidr_block an string attribute. This is aligned with the current design of the Aliyun API.

@xiaozhu36
Copy link
Member

xiaozhu36 commented Jun 21, 2021

@LiangZugeng Thanks your ideas. I think the ipv6_cidr_block_suffix is not good for the issue. The vswitch ipv6 count is equals to the VPC ipv6_cidr_block suffix subtracts the VSwitch ipv6_cidr_block suffix. So, I think there should describe the VPC ipv6_cidr_block and do that when the resource setting the vswitch ipv6_cidr_block.

@xiaozhu36
Copy link
Member

Also, please @ezio-ballarin adds the attribute ipv6_cidr_block description in the resource docs.

@LiangZugeng
Copy link

@xiaozhu36 Thanks for your prompt response, could you please elaborate it? I'm not sure I fully understand "I think there should describe the VPC ipv6_cidr_block and do that when the resource setting the vswitch ipv6_cidr_block."

Did you mean that there should be only a single argument called ipv6_cidr_block? If that's the case, what's the type of the argument? It will be great if a more detailed design can be suggested.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants