feat: add idempotent route checks and container network routes
continuous-integration/drone/push Build is passing
continuous-integration/drone/push Build is passing
- Make AddRouteInContainer idempotent by checking existing routes and handling "File exists" errors - Add loop in firewall reconciler to add routes for containers to reach other networks - Update iptables checks to include port for better rule distinction
This commit is contained in:
@@ -215,13 +215,30 @@ func (c *Client) GetContainerPID(ctx context.Context, containerName string) (int
|
|||||||
return cont.State.Pid, nil
|
return cont.State.Pid, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// AddRouteInContainer adds a route inside a container's network namespace using nsenter
|
// AddRouteInContainer adds a route inside a container's network namespace using nsenter.
|
||||||
|
// Idempotent: checks if route already exists before adding.
|
||||||
func (c *Client) AddRouteInContainer(ctx context.Context, containerName, network, gateway string) error {
|
func (c *Client) AddRouteInContainer(ctx context.Context, containerName, network, gateway string) error {
|
||||||
pid, err := c.GetContainerPID(ctx, containerName)
|
pid, err := c.GetContainerPID(ctx, containerName)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return fmt.Errorf("failed to get PID for container %s: %w", containerName, err)
|
return fmt.Errorf("failed to get PID for container %s: %w", containerName, err)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// First check if the route already exists
|
||||||
|
checkArgs := []string{
|
||||||
|
"-t", fmt.Sprintf("%d", pid),
|
||||||
|
"-n", "--",
|
||||||
|
"ip", "route", "show", network,
|
||||||
|
}
|
||||||
|
checkCmd := exec.Command("nsenter", checkArgs...)
|
||||||
|
checkOutput, _ := checkCmd.CombinedOutput()
|
||||||
|
checkStr := strings.TrimSpace(string(checkOutput))
|
||||||
|
|
||||||
|
// If the route exists and points to the correct gateway, skip
|
||||||
|
if checkStr != "" && strings.Contains(checkStr, gateway) {
|
||||||
|
logger.Debug("DOCKER: route %s via %s already exists in container %q, skipping", network, gateway, containerName)
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
logger.Info("DOCKER: adding route in container %q (PID=%d): %s via %s", containerName, pid, network, gateway)
|
logger.Info("DOCKER: adding route in container %q (PID=%d): %s via %s", containerName, pid, network, gateway)
|
||||||
args := []string{
|
args := []string{
|
||||||
"-t", fmt.Sprintf("%d", pid),
|
"-t", fmt.Sprintf("%d", pid),
|
||||||
@@ -232,6 +249,11 @@ func (c *Client) AddRouteInContainer(ctx context.Context, containerName, network
|
|||||||
cmd := exec.Command("nsenter", args...)
|
cmd := exec.Command("nsenter", args...)
|
||||||
output, err := cmd.CombinedOutput()
|
output, err := cmd.CombinedOutput()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
// "File exists" means route already exists (race condition)
|
||||||
|
if strings.Contains(string(output), "File exists") {
|
||||||
|
logger.Debug("DOCKER: route %s via %s already exists in container %q (File exists), skipping", network, gateway, containerName)
|
||||||
|
return nil
|
||||||
|
}
|
||||||
return fmt.Errorf("failed to add route in container %s: %w\noutput: %s", containerName, err, string(output))
|
return fmt.Errorf("failed to add route in container %s: %w\noutput: %s", containerName, err, string(output))
|
||||||
}
|
}
|
||||||
logger.Info("DOCKER: route added in container %q: %s via %s", containerName, network, gateway)
|
logger.Info("DOCKER: route added in container %q: %s via %s", containerName, network, gateway)
|
||||||
|
|||||||
@@ -126,6 +126,25 @@ func (o *Orchestrator) reconcileIPs(ctx context.Context, cfg *config.NetworksCon
|
|||||||
logger.Info("FIREWALL: container %s connected to network %s with IP %s",
|
logger.Info("FIREWALL: container %s connected to network %s with IP %s",
|
||||||
containerName, networkName, ipStr)
|
containerName, networkName, ipStr)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Add routes inside the container so it can reach all other networks
|
||||||
|
// This mirrors the old shell script's ip_route() function
|
||||||
|
for _, otherNetCfg := range cfg.Networks {
|
||||||
|
if otherNetCfg.NetworkName == networkName {
|
||||||
|
continue // skip the network we're already on
|
||||||
|
}
|
||||||
|
route := otherNetCfg.Subnet
|
||||||
|
gw := otherNetCfg.Gateway
|
||||||
|
if route == "" || gw == "" {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
logger.Debug("FIREWALL: adding route in container %s: %s via %s", containerName, route, gw)
|
||||||
|
if err := o.dockerClient.AddRouteInContainer(ctx, containerName, route, gw); err != nil {
|
||||||
|
logger.Warn("FIREWALL: failed to add route %s via %s in container %s: %v", route, gw, containerName, err)
|
||||||
|
} else {
|
||||||
|
logger.Debug("FIREWALL: route %s via %s added in container %s", route, gw, containerName)
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -282,7 +282,8 @@ func (m *Manager) InsertPreroutingRuleOnInterface(iface, proto, sourcePort, targ
|
|||||||
iface, proto, sourcePort, targetIP, targetPort, comment)
|
iface, proto, sourcePort, targetIP, targetPort, comment)
|
||||||
|
|
||||||
// Check if rule already exists (idempotent: don't re-apply)
|
// Check if rule already exists (idempotent: don't re-apply)
|
||||||
existing := m.getLineNumbers("PREROUTING", "nat", comment, "DNAT", targetIP)
|
// Must include port in check to distinguish port 80 from port 443 rules with same comment
|
||||||
|
existing := m.getLineNumbers("PREROUTING", "nat", comment, "DNAT", targetIP, targetPort)
|
||||||
if len(existing) > 0 {
|
if len(existing) > 0 {
|
||||||
logger.Debug("IPTABLES: PREROUTING DNAT rule already exists on %s (lines=%v), skipping", iface, existing)
|
logger.Debug("IPTABLES: PREROUTING DNAT rule already exists on %s (lines=%v), skipping", iface, existing)
|
||||||
return nil
|
return nil
|
||||||
@@ -307,7 +308,8 @@ func (m *Manager) InsertPostroutingMasquerade(destCIDR, proto, destPort, comment
|
|||||||
destCIDR, proto, destPort, comment)
|
destCIDR, proto, destPort, comment)
|
||||||
|
|
||||||
// Check if rule already exists (idempotent: don't re-apply)
|
// Check if rule already exists (idempotent: don't re-apply)
|
||||||
existing := m.getLineNumbers("POSTROUTING", "nat", comment, "MASQUERADE", destCIDR)
|
// Must include port in check to distinguish port 80 from port 443 rules with same comment
|
||||||
|
existing := m.getLineNumbers("POSTROUTING", "nat", comment, "MASQUERADE", destCIDR, destPort)
|
||||||
if len(existing) > 0 {
|
if len(existing) > 0 {
|
||||||
logger.Debug("IPTABLES: POSTROUTING MASQUERADE rule already exists (lines=%v), skipping", existing)
|
logger.Debug("IPTABLES: POSTROUTING MASQUERADE rule already exists (lines=%v), skipping", existing)
|
||||||
return nil
|
return nil
|
||||||
@@ -332,7 +334,8 @@ func (m *Manager) InsertPostroutingMasqueradeForTarget(targetCIDR, proto, target
|
|||||||
targetCIDR, proto, targetPort, comment)
|
targetCIDR, proto, targetPort, comment)
|
||||||
|
|
||||||
// Idempotent: check if rule already exists
|
// Idempotent: check if rule already exists
|
||||||
existing := m.getLineNumbers("POSTROUTING", "nat", comment, "MASQUERADE", targetCIDR)
|
// Must include port in check to distinguish port 80 from port 443 rules with same comment
|
||||||
|
existing := m.getLineNumbers("POSTROUTING", "nat", comment, "MASQUERADE", targetCIDR, targetPort)
|
||||||
if len(existing) > 0 {
|
if len(existing) > 0 {
|
||||||
logger.Debug("IPTABLES: POSTROUTING MASQUERADE for target already exists (lines=%v), skipping", existing)
|
logger.Debug("IPTABLES: POSTROUTING MASQUERADE for target already exists (lines=%v), skipping", existing)
|
||||||
return nil
|
return nil
|
||||||
@@ -484,17 +487,19 @@ func (m *Manager) InsertPostroutingMasqueradeInContainer(pid int, destCIDR, prot
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Idempotent check: scan existing rules for matching patterns
|
// Idempotent check: scan existing rules for matching patterns
|
||||||
|
// Must include port to distinguish port 80 from port 443 rules with same comment
|
||||||
ruleExists := false
|
ruleExists := false
|
||||||
for _, line := range strings.Split(output, "\n") {
|
for _, line := range strings.Split(output, "\n") {
|
||||||
if strings.Contains(line, "MASQUERADE") &&
|
if strings.Contains(line, "MASQUERADE") &&
|
||||||
strings.Contains(line, comment) &&
|
strings.Contains(line, comment) &&
|
||||||
strings.Contains(line, destCIDR) {
|
strings.Contains(line, destCIDR) &&
|
||||||
|
strings.Contains(line, destPort) {
|
||||||
ruleExists = true
|
ruleExists = true
|
||||||
break
|
break
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
if ruleExists {
|
if ruleExists {
|
||||||
logger.Info("IPTABLES: POSTROUTING MASQUERADE rule already exists in container PID %d (dst=%s), skipping", pid, destCIDR)
|
logger.Info("IPTABLES: POSTROUTING MASQUERADE rule already exists in container PID %d (dst=%s dport=%s), skipping", pid, destCIDR, destPort)
|
||||||
return nil
|
return nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user