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

Podman IPv6 ULA addressing #19775

Merged
merged 1 commit into from
Jul 24, 2024
Merged

Podman IPv6 ULA addressing #19775

merged 1 commit into from
Jul 24, 2024

Conversation

pdostal
Copy link
Member

@pdostal pdostal commented Jul 18, 2024

By switching to ULA addresses we don't have to have subnet from CSP.
This is because podman by default creates IPv6 NAT.

Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@@ -1,7 +1,7 @@
terraform {
required_providers {
aws = {
version = "= 5.14.0"
version = "= 5.58.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

That's quiet a version bump. We will require heavy testing for this one and I would recommend to not merge this at the end of the week.

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been discarded.

@@ -352,6 +352,11 @@ sub create_instances {
# Install server's ssh publicckeys to prevent authenticity interactions
assert_script_run(sprintf('ssh-keyscan %s >> ~/.ssh/known_hosts', $instance->public_ip));
}

$instance->ssh_assert_script_run('sudo ip -6 a a ' . $instance->public_ip6 . '/128 dev eth0') if ($instance->public_ip6);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be excluded for Azure? Because podman_ipv6 is excluded for Azure I assume that IPv6 is not available there

Copy link
Member

Choose a reason for hiding this comment

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

no need for additional is_azure condition here . Call will be excluded just because $instance->public_ip6 is undefined for Azure

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been discarded.

my $instance = publiccloud::instance->new(
public_ip => @{$ips}[$i],
public_ip6 => $ipv6_address,
#ip6_subnet => @{$ipv6_prefixes}[$i]->[0],
Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover?

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been discarded.

@@ -27,6 +27,7 @@ sub _cleanup {
sub run {
my ($self, $args) = @_;

die "This will work only on GCE and EC2. I'm sorry for the inconvenience :(" unless (is_ec2 || is_gce);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
die "This will work only on GCE and EC2. I'm sorry for the inconvenience :(" unless (is_ec2 || is_gce);
die "IPv6 is not available on Azure" if (is_azure);

Careful, we're mixing different exclusion styles. In main_containers.pm we have:

loadtest 'containers/podman_ipv6' if (!is_azure && is_sle('>=15-SP5'));

And here it is is_ec2 || is_gce. I recommend to unify the exclusion settings, or even better not have them in the test run at all. The test run is already excluded, so IMHO this die can be removed as a whole.

A comment in the summary is IMHO better suited.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please check now.

assert_script_run(sprintf('podman network create --ipv6 --subnet %s podman-ipv6', $subnet));
assert_script_run("podman run --name test-ipv6 --network podman-ipv6 --ip6 $cont_addr -d -p 80:80 -p '[::]:80:80' $image sleep 999", timeout => 180);
assert_script_run("podman run --name test-ipv6 --network podman-ipv6 --ip6 $cont_addr -d $image sleep 999", timeout => 180);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use a local network here? We can enable IPv6 for the default network or create a new one and then test the IPv6 connectivity from within the container without exposing a whole subnet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

variables.md Outdated Show resolved Hide resolved
lib/main_containers.pm Outdated Show resolved Hide resolved
@pdostal pdostal changed the title Ec2 ipv6 Podman IPv6 ULA addressing Jul 22, 2024
By switching to ULA addresses we don't have to have subnet from CSP.
This is because podman by default creates IPv6 NAT.
Copy link
Member

@asmorodskyi asmorodskyi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@grisu48 grisu48 left a comment

Choose a reason for hiding this comment

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

LGTM!

@asmorodskyi asmorodskyi merged commit 9d9a5d1 into os-autoinst:master Jul 24, 2024
10 checks passed
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