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

feat: support for multiple external network #2695

Closed

Conversation

shane965
Copy link
Contributor

@shane965 shane965 commented Apr 21, 2023

What type of this PR

Examples of user facing changes:

  • Features

Which issue(s) this PR fixes:

Fixes #2679

WHAT

🤖 Generated by Copilot at d2a7462

This pull request adds support for multiple external subnets for NAT gateway and EIP scenarios. It introduces a new External field for the SubnetSpec type and a new ExternalSubnet field for the VpcNatSpec and IptablesEipSpec types. It also refactors the controller logic and the CRD schemas to use these fields instead of the hardcoded VpcExternalNet constant.

🤖 Generated by Copilot at d2a7462

ExternalSubnet
A new field for EIP, NAT
Autumn of hardcodes

HOW

🤖 Generated by Copilot at d2a7462

  • Add External field to SubnetSpec type and schema to indicate external subnets (link, link)
  • Add ExternalSubnet field to VpcNatSpec and IptablesEipSpec types and schemas to specify external subnet name for NAT gateway and EIP (link, link, link, link)
  • Use ExternalSubnet field of EIP to get IP and MAC from IPAM, set EIP CIDR, update EIP CR, and enqueue subnet for status update (link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Use ExternalSubnet field of NAT gateway to get external gateway and CIDR, set attachment network annotation, and update route rules for NAT gateway pod (link, link, link)
  • Use External field of subnet to count and list EIPs in subnet usage and status (link, link)
  • Remove VpcExternalNet constant and checkVpcExternalNet function as they are no longer needed (link, link)
  • Remove MACVLAN_NAD_PROVIDER constant as it is no longer fixed to VpcExternalNet network attachment definition (link)

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should provide a clear idea of what changes have been made in the code patch.
  • There are no potential bugs or format errors found in the code patch.
  • The performance of the code patch needs to be evaluated thoroughly to ensure that it does not impact the overall performance of the system.
  • The code patch could benefit from additional comments and documentation to make it easier for other developers to understand and maintain the code.
  • The code patch could be improved by following best practices and coding standards, such as using meaningful variable names and avoiding hard-coded values.

pkg/apis/kubeovn/v1/types.go Outdated Show resolved Hide resolved
pkg/apis/kubeovn/v1/types.go Outdated Show resolved Hide resolved
pkg/controller/init.go Show resolved Hide resolved
pkg/util/const.go Show resolved Hide resolved
@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should provide a clear idea of what changes have been made in the patch.
  • There are no potential bugs or format errors found in the patch diff.
  • It is difficult to comment on performance issues without knowing the context of the code. However, it is always good to optimize the code for better performance if possible.
  • Ways to improve the patch could include adding comments to explain complex logic or improving variable names for better readability.

@shane965 shane965 marked this pull request as ready for review April 21, 2023 08:51
yamls/crd.yaml Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should clearly state what changes were made and why.
  • There are no potential bugs or format errors in the diff.
  • It is difficult to assess performance issues without further context on the codebase and the changes made.
  • Ways to improve could include adding comments to explain complex logic, refactoring repetitive code, or optimizing algorithms for better performance.

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should clearly state what changes were made and why they were necessary.
  • There are no potential bugs or format errors in the diff.
  • It is difficult to assess performance issues without more context about the code being modified.
  • It would be helpful to include comments in the code explaining the purpose of each change and how it improves the overall functionality.
  • Consider adding unit tests to ensure that the changes do not introduce new bugs or regressions.

@bobz965
Copy link
Collaborator

bobz965 commented Apr 22, 2023

before you test this branch, you need to do the following steps:

# patch the external flag of subnet
kubectl patch subnets.kubeovn.io ovn-vpc-external-network --type merge --patch '{"spec": {"external": true}}'
# patch the externalSubnet of eip
kubectl patch iptables-eips.kubeovn.io {your eips} --type merge --patch '{"spec": {"externalSubnet": "ovn-vpc-external-network"}}'
# restart kube-ovn-controller
kubectl delete pod -n kube-system kube-ovn-controller

这些应该不需要做了吧?

@shane965
Copy link
Contributor Author

before you test this branch, you need to do the following steps:

# patch the external flag of subnet
kubectl patch subnets.kubeovn.io ovn-vpc-external-network --type merge --patch '{"spec": {"external": true}}'
# patch the externalSubnet of eip
kubectl patch iptables-eips.kubeovn.io {your eips} --type merge --patch '{"spec": {"externalSubnet": "ovn-vpc-external-network"}}'
# restart kube-ovn-controller
kubectl delete pod -n kube-system kube-ovn-controller

这些应该不需要做了吧?

已删除

@oilbeater
Copy link
Collaborator

Please rebase the master and resolve the conflict

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should clearly state what changes were made and why they were necessary.
  • There are no potential bugs or format errors in the diff.
  • It is difficult to assess performance issues without more context about the code being changed.
  • It would be helpful to include comments in the code explaining the purpose of each change and how it improves the overall functionality.
  • Consider adding unit tests to ensure that the changes do not introduce new bugs or regressions.

MacAddress string `json:"macAddress"`
NatGwDp string `json:"natGwDp"`
QoSPolicy string `json:"qosPolicy"`
ExternalSubnets []string `json:"externalSubnets"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IptablesEipSpec支持多个子网好像没有意义,这里应该用ExternalSubnet string json:"externalSubnet"就可以了

Copy link
Collaborator

Choose a reason for hiding this comment

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

可能后续需要支持单个nat gw pod 支持动态添加网卡的方式扩展到多个网段,源进源出,一个网卡加一张路由表

Copy link
Collaborator

Choose a reason for hiding this comment

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

目前这样也行,后续加那个功能的时候,我们再改

@github-actions
Copy link
Contributor

  • The commit message should be more descriptive and informative. It should clearly state what changes were made and why.
  • There are no potential bugs or format errors in the patch diff.
  • It is difficult to assess performance issues without further context on the codebase and the changes made.
  • Ways to improve could include adding comments to explain complex logic or improving variable naming for better readability.

@shane965
Copy link
Contributor Author

in order to keep the commit history clean, close the PR and create a new one. #2725

@shane965 shane965 closed this Apr 27, 2023
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.

vpc模式下是否支持多个外部网络,使用特定的外部网络绑定特定的vpc-nat-gw
3 participants