-
Notifications
You must be signed in to change notification settings - Fork 643
Ipv6 daemon support #4853
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
base: master
Are you sure you want to change the base?
Ipv6 daemon support #4853
Conversation
Release 1.101.2
Extend daemon bridge network namespace configuration to support IPv6 default routes when the host is IPv6-compatible. This enables IPv4-only, IPv6-only, and dual-stack networking scenarios for daemon namespaces. Changes: - Add IPv6 fields to IPAMConfig struct (IPV6Subnet, IPV6Address, IPV6Gateway, IPV6Routes) - Add IPv6 constants (DefaultRouteDestinationIPv6, DaemonBridgeGatewayIPv6) - Update createDaemonBridgePluginConfig to accept IP compatibility and configure routes based on IPv4/IPv6/dual-stack mode - Update configureDaemonNetNS and StopDaemonNetNS to pass IP compatibility - Add comprehensive unit tests and property-based tests
Add IPv6 subnet configuration for daemon-bridge network mode: - Add ECSSubNetIPv6 (fd00:ec2::172:0/112) and DaemonBridgeGatewayIPv6 constants - Update createDaemonBridgePluginConfig to set IPV6Subnet and IPV6Gateway when host is IPv6 compatible - Update unit tests to verify IPv6 subnet configuration Add E2E tests for daemon-bridge networking: - daemon_bridge_e2e_test.go: Direct CNI plugin invocation tests - managed_linux_daemon_e2e_test.go: Platform API integration tests - Tests validate IPv4-only, IPv6-only, and dual-stack configurations - Add skipIfNotIPv6OnlyE2E helper for IPv6-only test scenarios The IPv6 subnet uses ULA address space (fd00:ec2::) which mirrors the IPv4 link-local subnet (169.254.172.0/22) for internal ECS communication.
aviral92
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 8 commits in this PR, is this expected? just wanted to confirm that we are looking at the right set of changes.
Yes. |
| // For any valid IPAMConfig with IPv6 fields populated (IPV6Subnet, IPV6Address, IPV6Gateway, IPV6Routes), | ||
| // serializing to JSON and deserializing back SHALL produce an equivalent IPAMConfig object. | ||
| // | ||
| // **Validates: Requirements 1.5** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - can we have the requirement details?
| // | ||
| // **Validates: Requirements 1.5** | ||
| func TestProperty_IPAMConfigIPv6JSONSerializationRoundTrip(t *testing.T) { | ||
| f := func( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we have a meaningful func name? or some comments?
| if err != nil { | ||
| // If marshaling fails, this is a valid failure case | ||
| // (e.g., invalid data that can't be serialized) | ||
| return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q - this comment is true but wondering if that is true for this function?
| } | ||
|
|
||
| // Verify equivalence of IPv6 fields | ||
| if original.IPV6Subnet != deserialized.IPV6Subnet { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking: is there any reason to use if for these checks below instead of assert or require
| // the daemon bridge plugin configuration SHALL always include the IPv4 route to | ||
| // the ECS agent endpoint (169.254.170.2/32) in the IPv4Routes field. | ||
| // | ||
| // **Validates: Requirements 3.2, 5.1, 5.2, 5.3** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems from AI?
| // TestProperty_ECSAgentEndpointRouteInvariant_AllConfigurations tests the property | ||
| // exhaustively for all three valid IP compatibility configurations. | ||
| // | ||
| // Feature: daemon-bridge-ipv6-support, Property 2: ECS Agent Endpoint Route Invariant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same. Looks it's good to remove these or have more details.
|
|
||
| // Also enable forwarding on specific interfaces if needed | ||
| if err := enableSysctlSetting("net.ipv6.conf.eth0.forwarding", "1"); err != nil { | ||
| logger.Warn("Failed to enable IPv6 forwarding on eth0", logger.Fields{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What could happen if we failed to enable this?
| // It checks if the primary interface has IPv4 and/or IPv6 addresses configured. | ||
| func (m *managedLinux) getIPCompatibilityFromNetNS(netNS *tasknetworkconfig.NetworkNamespace) ipcompatibility.IPCompatibility { | ||
| primaryIface := netNS.GetPrimaryInterface() | ||
| if primaryIface == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know if this can happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have a log for this special case?
Summary
Add IPv6 support for daemon-bridge network mode in managed Linux platform. This enables ECS managed daemons to operate on IPv6-only and dual-stack EC2 instances.
Implementation details
IPv6 Networking Configuration for Managed Daemon Namespaces:
fd00:ec2::172:0/112) and gateway (fd00:ec2::172:1) constants for daemon-bridgecreateDaemonBridgePluginConfigto configure IPv6 subnet and gateway when host is IPv6 compatibleIP Compatibility Detection:
E2E Tests:
daemon_bridge_e2e_test.gofor direct CNI plugin invocation testingmanaged_linux_daemon_e2e_test.gofor platform API integration testingTesting
cniconf_linux_test.goto verify IPv6 subnet configurationNew tests cover the changes: yes
Description for the changelog
netlib: Add IPv6 support for daemon-bridge network mode on managed Linux platform
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No breaking model changes.
Does this PR include the addition of new environment variables in the README?
No new environment variables.
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.