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

RSS: Reuse SNAT IPs until port range is exhausted #6037

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

jgallagher
Copy link
Contributor

Prior to this PR, RSS would step both the IP and the port range when assigning SNAT IPs to boundary NTP zones. E.g., on a4x2:

            "snat_cfg": {
              "ip": "198.51.100.25",
              "first_port": 0,
              "last_port": 16383
            }
            "snat_cfg": {
              "ip": "198.51.100.26",
              "first_port": 16384,
              "last_port": 32767
            }

After the change, we reuse the IP and only step the ports:

            "snat_cfg": {
              "ip": "198.51.100.25",
              "first_port": 0,
              "last_port": 16383
            }
            "snat_cfg": {
              "ip": "198.51.100.25",
              "first_port": 16384,
              "last_port": 32767
            }

I confirmed both boundary NTP zones still have external connectivity, as expected.

I believe the code here already intended to do this, but accidentally never assigned self.next_snat_ip, so always allocated a new IP.

@jgallagher jgallagher requested review from luqmana and smklein July 10, 2024 17:51
Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Nice catch, thanks!

@jgallagher jgallagher merged commit aa06313 into main Jul 10, 2024
19 checks passed
@jgallagher jgallagher deleted the john/rss-reuse-snat-ip branch July 10, 2024 19:37
jgallagher added a commit that referenced this pull request Jul 31, 2024
Prior to this change, the planner expected all blueprints to have
fully-exclusive external IP addresses. This isn't compatible with #6037,
where RSS now hands out SNAT IPs with distinct port ranges but the same
IP address.

A big chunk of this work is necessary to support boundary NTP zone
planning, but that isn't included in this PR, so those bits are marked
with `#[cfg(test)]`.

Fixes #6194.
jgallagher added a commit that referenced this pull request Aug 2, 2024
Prior to this change, the planner expected all blueprints to have
fully-exclusive external IP addresses. This isn't compatible with #6037,
where RSS now hands out SNAT IPs with distinct port ranges but the same
IP address.

A big chunk of this work is necessary to support boundary NTP zone
planning, but that isn't included in this PR, so those bits are marked
with `#[cfg(test)]`.

Fixes #6194.
jgallagher added a commit that referenced this pull request Aug 5, 2024
Prior to this change, the planner expected all blueprints to have
fully-exclusive external IP addresses. This isn't compatible with #6037,
where RSS now hands out SNAT IPs with distinct port ranges but the same
IP address.

A big chunk of this work is necessary to support boundary NTP zone
planning, but that isn't included in this PR, so those bits are marked
with `#[cfg(test)]`.

Fixes #6194.
jgallagher added a commit that referenced this pull request Aug 6, 2024
Prior to this change, the planner expected all blueprints to have
fully-exclusive external IP addresses. This isn't compatible with #6037,
where RSS now hands out SNAT IPs with distinct port ranges but the same
IP address.

A big chunk of this work is necessary to support boundary NTP zone
planning, but that isn't included in this PR, so those bits are marked
with `#[cfg(test)]`.

Fixes #6194.
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.

2 participants