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

Fix DNS based CWNPs for network-isolated Clusters #177

Merged
merged 10 commits into from
Apr 22, 2024
Merged

Conversation

vknabel
Copy link
Contributor

@vknabel vknabel commented Mar 22, 2024

Requires:

Todo:

  • Tests to cover the new adaptations
  • Update metal-networker to release containing the PR

@vknabel vknabel marked this pull request as draft April 3, 2024 11:41
@vknabel vknabel requested a review from Gerrit91 April 17, 2024 11:43
@vknabel vknabel marked this pull request as ready for review April 17, 2024 11:43
@vknabel vknabel changed the title Draft: initial dns for proxy Fix DNS based CWNPs for network-isolated Clusters Apr 17, 2024

# Add additional DNS addresses for dnat redirection for the dns proxy
table inet nat {
set public_dns_servers {
Copy link
Contributor

@Gerrit91 Gerrit91 Apr 17, 2024

Choose a reason for hiding this comment

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

To me still the name of this set is a bit misleading because a user might provide a DNS server that is not publicly reachable. Apart from that, my expectation was that the statically defined public DNS servers are not used for resolving queries in case a dedicated DNS server was specified (of course this is problematic as this DNS server needs to resolve the shoots API server DNS but if someone sets this configuration I think it's fine to expect this complex implication).

So, from my side it would be good to duplicate the DNAT rules from the metal-networker. This enables us to remove the DNS proxy related code from the metal-networker and move it the here. Then, we can also implement that only the user-given DNS server is used and fallback to 1.0.0.1, 1.1.1.1, 8.8.4.4, 8.8.8.8 if nothing was given?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed the set in metal-stack/metal-networker#106

pkg/nftables/networkpolicy.go Outdated Show resolved Hide resolved
pkg/nftables/nftables.tpl Outdated Show resolved Hide resolved
Comment on lines -240 to -243
// do nothing if message and state already have the desired values
if cwnp.Status.Message == msg && cwnp.Status.State == state {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I will try to investigate the impact on update traffic this will produce as this essentially updates all CWNPs of a cluster very regularly. Maybe it's worth to implement an update that only occurs if necessary (by comparing contents).

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a significant difference between requests of this firewall-controller and 2.3.0 for CWNPs in API server dashboards, so I assume we can start doing this.

@vknabel vknabel requested a review from mwennrich April 18, 2024 08:52
@vknabel vknabel merged commit a9deaca into master Apr 22, 2024
2 checks passed
@vknabel vknabel deleted the fix-initial-dns branch April 22, 2024 09:33
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