Skip to content

Commit

Permalink
adjust VerifyIptablesState variables
Browse files Browse the repository at this point in the history
  • Loading branch information
@leosarra
leosarra committed May 17, 2024
1 parent 10bd231 commit cad8700
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 43 deletions.
43 changes: 21 additions & 22 deletions tools/istio-iptables/pkg/capture/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -849,8 +849,8 @@ func (cfg *IptablesConfigurator) getStateFromSave(data string) map[string]map[st

func (cfg *IptablesConfigurator) VerifyIptablesState(iptVer, ipt6Ver *dep.IptablesVersion) (bool, bool) {
// These variables track the status of iptables installation
deltaExists := false // Flag to indicate if a difference is found between expected and current state
reconcile := false // Flag to indicate if a reconciliation via cleanup is needed
residueExists := false // Flag to indicate if iptables residues from previous executions are found
deltaExists := false // Flag to indicate if a difference is found between expected and current state

check_loop:
for _, ipCfg := range []struct {
Expand All @@ -866,55 +866,54 @@ check_loop:
currentState := cfg.getStateFromSave(output.String())
log.Debugf("Current iptables state: %s", currentState)
for _, value := range currentState {
if deltaExists {
if residueExists {
break
}
deltaExists = len(value) != 0
residueExists = len(value) != 0
}
if !deltaExists {
if !residueExists {
continue
}
expectedState := cfg.getStateFromSave(ipCfg.expected)
log.Debugf("Expected iptables state: %s", expectedState)
for table, chains := range expectedState {
_, ok := currentState[table]
if !ok {
reconcile = true
deltaExists = true
log.Debugf("Can't find expected table %s in current state", table)
break check_loop
}
for chain, rules := range chains {
currentRules, ok := currentState[table][chain]
if !ok || (strings.HasPrefix(chain, "ISTIO_") && len(rules) != len(currentRules)) {
reconcile = true
deltaExists = true
log.Debugf("Mismatching number of rules in chain %s between current and expected state", chain)
break check_loop
}
}
}
err = cfg.executeIptablesCommands(ipCfg.ver, ipCfg.checkRules)
if err != nil {
reconcile = true
deltaExists = true
log.Debugf("iptables check rules failed")
break
}
}

}

// If no delta is found, reconcile is never needed
if !deltaExists {
log.Info("Clean-state detected, no reconciliation needed")
return false, false
if !residueExists {
log.Info("Clean-state detected, new iptables are needed")
return false, true
}

if deltaExists && !reconcile {
log.Warn("Found compatible iptables rules/chains, no reconciliation needed")
} else if deltaExists {
log.Warn("Found residue of old iptables rules/chains, reconciliation is needed")
if deltaExists {
log.Warn("Found residues of old iptables rules/chains, reconciliation is needed")
} else {
log.Warn("Found compatible residues of old iptables rules/chains, reconciliation not needed")
}

return deltaExists, reconcile
return residueExists, deltaExists
}

func (cfg *IptablesConfigurator) executeCommands(iptVer, ipt6Ver *dep.IptablesVersion) error {
Expand All @@ -928,14 +927,14 @@ func (cfg *IptablesConfigurator) executeCommands(iptVer, ipt6Ver *dep.IptablesVe
}
}()

deltaExists, reconcile := cfg.VerifyIptablesState(iptVer, ipt6Ver)
if deltaExists && reconcile && cfg.cfg.NoReconcile {
residueExists, deltaExists := cfg.VerifyIptablesState(iptVer, ipt6Ver)
if residueExists && deltaExists && cfg.cfg.NoReconcile {
return fmt.Errorf("reconcile is needed but no-reconcile flag is set. Can't recover from this state")
}
// Cleanup Step
if (deltaExists && reconcile) || cfg.cfg.CleanupOnly {
if (residueExists && deltaExists) || cfg.cfg.CleanupOnly {
// Apply safety guardrails
if deltaExists {
if !cfg.cfg.CleanupOnly {
log.Info("Setting up guardrails")
guardrailsCleanup := cfg.ruleBuilder.BuildCleanupGuardrails()
guardrailsRules := cfg.ruleBuilder.BuildGuardrails()
Expand All @@ -954,7 +953,7 @@ func (cfg *IptablesConfigurator) executeCommands(iptVer, ipt6Ver *dep.IptablesVe
}

// Apply Step
if (reconcile || !deltaExists) && !cfg.cfg.CleanupOnly {
if deltaExists && !cfg.cfg.CleanupOnly {
log.Info("Applying iptables chains and rules")
if cfg.cfg.RestoreFormat {
// Execute iptables-restore
Expand Down
42 changes: 21 additions & 21 deletions tools/istio-iptables/pkg/capture/run_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,18 +387,18 @@ func TestIdempotentEquivalentRerun(t *testing.T) {
cfg.NoReconcile = false
iptConfigurator := NewIptablesConfigurator(cfg, ext)
assert.NoError(t, iptConfigurator.Run())
deltaExists, reconcile := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, deltaExists, false)
assert.Equal(t, reconcile, false)
residueExists, deltaExists := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, false)
assert.Equal(t, deltaExists, true)
}()

// First Pass
cfg.NoReconcile = true
iptConfigurator := NewIptablesConfigurator(cfg, ext)
assert.NoError(t, iptConfigurator.Run())
deltaExists, reconcile := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, deltaExists, true)
assert.Equal(t, reconcile, false)
residueExists, deltaExists := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, true)
assert.Equal(t, deltaExists, false)

// Second Pass
iptConfigurator = NewIptablesConfigurator(cfg, ext)
Expand Down Expand Up @@ -444,27 +444,27 @@ func TestIdempotentUnequaledRerun(t *testing.T) {
cfg.NoReconcile = false
iptConfigurator := NewIptablesConfigurator(cfg, ext)
assert.NoError(t, iptConfigurator.Run())
residueFound, applyRequired := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueFound, true) // residue found due to extra OUTPUT rule
assert.Equal(t, applyRequired, true)
residueExists, deltaExists := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, true) // residue found due to extra OUTPUT rule
assert.Equal(t, deltaExists, true)
// Remove additional rule
cmd := exec.Command("iptables", "-t", "nat", "-D", "OUTPUT", "-p", "tcp", "--dport", "123", "-j", "ACCEPT")
cmd.Stdout = &stdout
cmd.Stderr = &stderr
if err := cmd.Run(); err != nil {
t.Errorf("iptables cmd (%s %s) failed: %s", cmd.Path, cmd.Args, stderr.String())
}
residueFound, applyRequired = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueFound, false)
assert.Equal(t, applyRequired, false)
residueExists, deltaExists = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, false)
assert.Equal(t, deltaExists, true)
}()

// First Pass
iptConfigurator := NewIptablesConfigurator(cfg, ext)
assert.NoError(t, iptConfigurator.Run())
residueFound, applyRequired := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueFound, true)
assert.Equal(t, applyRequired, false)
residueExists, deltaExists := iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, true)
assert.Equal(t, deltaExists, false)

// Diverge from installation
cmd := exec.Command("iptables", "-t", "nat", "-A", "OUTPUT", "-p", "tcp", "--dport", "123", "-j", "ACCEPT")
Expand All @@ -475,9 +475,9 @@ func TestIdempotentUnequaledRerun(t *testing.T) {
}

// Apply not required after tainting non-ISTIO chains with extra rules
residueFound, applyRequired = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueFound, true)
assert.Equal(t, applyRequired, false)
residueExists, deltaExists = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, true)
assert.Equal(t, deltaExists, false)

cmd = exec.Command("iptables", "-t", "nat", "-A", "ISTIO_INBOUND", "-p", "tcp", "--dport", "123", "-j", "ACCEPT")
cmd.Stdout = &stdout
Expand All @@ -487,9 +487,9 @@ func TestIdempotentUnequaledRerun(t *testing.T) {
}

// Apply required after tainting ISTIO chains
residueFound, applyRequired = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueFound, true)
assert.Equal(t, applyRequired, true)
residueExists, deltaExists = iptConfigurator.VerifyIptablesState(&iptVer, &ipt6Ver)
assert.Equal(t, residueExists, true)
assert.Equal(t, deltaExists, true)

// Fail is expected if cleanup is skipped
cfg.NoReconcile = true
Expand Down

0 comments on commit cad8700

Please sign in to comment.