add test case support for ipv6, add test case for ipv6#4028
add test case support for ipv6, add test case for ipv6#4028adityagesh wants to merge 4 commits intomainfrom
Conversation
The field was intended to be set via connection info mechanism. The connection mechanism was removed in commit 43e6a3d
|
Even though things change a lot, if it still uses the original PR, it’s easier for me to refer back to my earlier comments. |
| param ip_service_tags object | ||
|
|
||
| @description('whether to use ipv6') | ||
| @description('whether to use ipv6 (legacy parameter for backward compatibility)') |
There was a problem hiding this comment.
Did you find any usages internally? If so, create a PR to main-ci branch to fix them.
There was a problem hiding this comment.
I could only find one reference in lisa/microsoft/runbook/azure.yml
my apologies, for this PR, do you want me to force push/ manually make these changes into the other PR? |
| # Check for IPv6 network interface requirements and set deployment flags | ||
| if ( | ||
| node.capability.network_interface | ||
| and hasattr(node.capability.network_interface, 'ip_version') |
There was a problem hiding this comment.
Why bother checking if the attribute exists? It’s supposed to be there by default.
| base_type=IpProtocol, | ||
| default_values=[IpProtocol.ipv4, IpProtocol.ipv6], | ||
| ), | ||
| required=False, |
There was a problem hiding this comment.
Can it be set to empty? And if so, what happens when it is?
| ): | ||
| # Handle both single value and SetSpace cases | ||
| ip_version = node.capability.network_interface.ip_version | ||
| if hasattr(ip_version, 'items'): |
There was a problem hiding this comment.
Instead of inspecting attributes, just verify the type directly. It'll keep things cleaner and more straightforward. Use def generate_min_capability_setspace_by_priority with ipv6 to check if ipv6 exists or not.
|
|
||
| if ipv6_required: | ||
| log.info("IPv6 network interface requirement detected, enabling IPv6 internal") | ||
| arm_parameters.use_ipv6_internal = True |
There was a problem hiding this comment.
how about use_ipv6_public? Does it needs to be set to true?
There was a problem hiding this comment.
You are right, currently I am not checking for public.
Let me recheck this logic
There was a problem hiding this comment.
I think I'll need to add the below to class NetworkInterfaceOptionSettings(FeatureSettings):
ipv6_addressing_type: Optional[
Union[search_space.SetSpace[IPv6AddressingType], IPv6AddressingType]
] = field(
default=None, # Default to None when IPv4 is used
metadata=field_metadata(
decoder=partial(
search_space.decode_nullable_set_space,
base_type=IPv6AddressingType,
default_values=[
IPv6AddressingType.internal_only,
IPv6AddressingType.public_and_internal,
IPv6AddressingType.public_only,
], # Capabilities can support all types
),
required=False,
allow_none=True,
),
)
There was a problem hiding this comment.
What's the purpose of this setting?
There was a problem hiding this comment.
I was thinking of replacing ip_version: Optional[Union[search_space.SetSpace[IpProtocol], IpProtocol]] with ipv6_addressing_type.
ipv6_addressing_type if none = ipv6 disabled
Otherwise, there is no way to determine if ipv6 is required for internal, public or both
There was a problem hiding this comment.
It looks like we need to follow the design for use_ipv6_public and use_ipv6_internal at the feature level. But in the Bicep code, they should be named enable_ipv6_public and enable_ipv6_internal instead. The Bicep layer doesn’t really know how to use those feature flags directly.
| ip_version = node.capability.network_interface.ip_version | ||
| if hasattr(ip_version, 'items'): | ||
| # SetSpace case - check if IPv6 is in the set | ||
| ipv6_required = schema.IpProtocol.ipv6 in ip_version.items |
There was a problem hiding this comment.
Why ipv6 is here, and so it would be required?
2 participants
Rewrite code based on #3957