diff --git a/cni/pkg/cmd/root.go b/cni/pkg/cmd/root.go index 93336d47c6..882a04b74b 100644 --- a/cni/pkg/cmd/root.go +++ b/cni/pkg/cmd/root.go @@ -21,6 +21,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -127,6 +128,7 @@ var rootCmd = &cobra.Command{ DNSCapture: cfg.InstallConfig.AmbientDNSCapture, EnableIPv6: cfg.InstallConfig.AmbientIPv6, ReconcilePodRulesOnStartup: cfg.InstallConfig.AmbientReconcilePodRulesOnStartup, + ReconcileHostRulesInterval: parseReconcileHostRulesInterval(cfg.InstallConfig.AmbientReconcileHostRulesInterval), NativeNftables: cfg.InstallConfig.NativeNftables, ForceIptablesBinary: cfg.InstallConfig.ForceIptablesBinary, }) @@ -266,6 +268,31 @@ func init() { "A set of field selectors in label=value format that will be added to the pod list filters") } +// defaultReconcileHostRulesInterval is the default interval of the periodic host rules +// reconciliation, matching kube-proxy's --iptables-sync-period default. +const defaultReconcileHostRulesInterval = 30 * time.Second + +// parseReconcileHostRulesInterval parses the host rules reconcile interval: +// an empty or invalid value logs a warning and falls back to the default; +// <= 0 disables the feature. +func parseReconcileHostRulesInterval(raw string) time.Duration { + if raw == "" { + return defaultReconcileHostRulesInterval + } + interval, err := time.ParseDuration(raw) + if err != nil { + log.Warnf("invalid %s value %q, falling back to default %v: %v", + constants.AmbientReconcileHostRulesInterval, raw, defaultReconcileHostRulesInterval, err) + return defaultReconcileHostRulesInterval + } + if interval <= 0 { + log.Infof("host rules periodic reconcile is disabled (%s=%q)", + constants.AmbientReconcileHostRulesInterval, raw) + return 0 + } + return interval +} + func registerStringParameter(name, value, usage string) { rootCmd.Flags().String(name, value, usage) registerEnvironment(name, value, usage) @@ -333,6 +360,7 @@ func constructConfig() (*config.Config, error) { AmbientIPv6: viper.GetBool(constants.AmbientIPv6), AmbientDisableSafeUpgrade: viper.GetBool(constants.AmbientDisableSafeUpgrade), AmbientReconcilePodRulesOnStartup: viper.GetBool(constants.AmbientReconcilePodRulesOnStartup), + AmbientReconcileHostRulesInterval: viper.GetString(constants.AmbientReconcileHostRulesInterval), EnableAmbientDetectionRetry: viper.GetBool(constants.EnableAmbientDetectionRetry), NativeNftables: viper.GetBool(constants.NativeNftables), diff --git a/cni/pkg/cmd/root_test.go b/cni/pkg/cmd/root_test.go new file mode 100644 index 0000000000..3cf47370b0 --- /dev/null +++ b/cni/pkg/cmd/root_test.go @@ -0,0 +1,43 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmd + +import ( + "testing" + "time" + + "istio.io/istio/pkg/test/util/assert" +) + +func TestParseReconcileHostRulesInterval(t *testing.T) { + cases := []struct { + name string + raw string + expected time.Duration + }{ + {"empty falls back to default", "", defaultReconcileHostRulesInterval}, + {"valid interval", "1m30s", 90 * time.Second}, + {"zero disables", "0", 0}, + {"zero seconds disables", "0s", 0}, + {"negative disables", "-5s", 0}, + {"invalid falls back to default", "banana", defaultReconcileHostRulesInterval}, + {"missing unit falls back to default", "30", defaultReconcileHostRulesInterval}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.expected, parseReconcileHostRulesInterval(tt.raw)) + }) + } +} diff --git a/cni/pkg/config/config.go b/cni/pkg/config/config.go index af1c872022..078bce8558 100644 --- a/cni/pkg/config/config.go +++ b/cni/pkg/config/config.go @@ -160,6 +160,10 @@ type InstallConfig struct { // Whether reconciliation of iptables at post startup is enabled for Ambient workloads AmbientReconcilePodRulesOnStartup bool + // The interval of the periodic runtime reconciliation of the ambient host-level + // health check rules (duration string, e.g. "30s"; "0" disables it) + AmbientReconcileHostRulesInterval string + // Whether to retry checking if a pod is ambient in the cni plugin when there are errors EnableAmbientDetectionRetry bool @@ -251,6 +255,7 @@ func (c InstallConfig) String() string { b.WriteString("AmbientIPv6: " + fmt.Sprint(c.AmbientIPv6) + "\n") b.WriteString("AmbientDisableSafeUpgrade: " + fmt.Sprint(c.AmbientDisableSafeUpgrade) + "\n") b.WriteString("AmbientReconcilePodRulesOnStartup: " + fmt.Sprint(c.AmbientReconcilePodRulesOnStartup) + "\n") + b.WriteString("AmbientReconcileHostRulesInterval: " + c.AmbientReconcileHostRulesInterval + "\n") b.WriteString("EnableAmbientDetectionRetry: " + fmt.Sprint(c.EnableAmbientDetectionRetry) + "\n") b.WriteString("NativeNftables: " + fmt.Sprint(c.NativeNftables) + "\n") diff --git a/cni/pkg/constants/constants.go b/cni/pkg/constants/constants.go index b9d49ff333..a3dfdd3406 100644 --- a/cni/pkg/constants/constants.go +++ b/cni/pkg/constants/constants.go @@ -47,6 +47,7 @@ const ( AmbientIPv6 = "ambient-ipv6" AmbientDisableSafeUpgrade = "ambient-disable-safe-upgrade" AmbientReconcilePodRulesOnStartup = "ambient-reconcile-pod-rules-on-startup" + AmbientReconcileHostRulesInterval = "ambient-reconcile-host-rules-interval" EnableAmbientDetectionRetry = "enable-ambient-detection-retry" NativeNftables = "native-nftables" diff --git a/cni/pkg/iptables/iptables.go b/cni/pkg/iptables/iptables.go index f4588a54e0..45f641b5d9 100644 --- a/cni/pkg/iptables/iptables.go +++ b/cni/pkg/iptables/iptables.go @@ -18,6 +18,9 @@ import ( "errors" "fmt" "strings" + "time" + + "github.com/cenkalti/backoff/v4" "istio.io/istio/cni/pkg/config" "istio.io/istio/cni/pkg/ipset" @@ -585,6 +588,109 @@ func (cfg *IptablesConfigurator) CreateHostRulesForHealthChecks() error { }) } +// EnsureHostRules verifies that the health check rules in the host netns are still in +// place and re-installs them when they have drifted (flushed/overwritten by an external +// actor). When everything is in place it performs no writes at all. It is idempotent +// and meant to be invoked from a periodic reconcile loop (istio/istio#60607). +// +// Notes: +// - This deliberately does NOT reuse the cfg.Reconcile cleanup path: that path sets up +// DROP guardrails in the filter table INPUT/FORWARD/OUTPUT chains. Inside a pod netns +// they prevent traffic from escaping the mesh while rules are rebuilt, but in the +// host netns they would drop the entire node's traffic, so they must never be +// enabled here. +// - The repair reuses the same retried DeleteHostRules + CreateHostRulesForHealthChecks +// delete-then-create sequence as the startup path, instead of simply replaying +// iptables-restore: the restore payload creates chains with -N, which fails when the +// chain already exists (so partial residue would never converge), and replaying +// would also duplicate the POSTROUTING jump. +func (cfg *IptablesConfigurator) EnsureHostRules() (bool, error) { + builder := cfg.AppendHostRules() + + converged := false + err := util.RunAsHost(func() error { + var err error + converged, err = cfg.hostRulesConverged(builder) + return err + }) + if err != nil { + // The state could not be read; that is not evidence that the (possibly + // healthy) rules are gone. Skip the repair instead of deleting live rules + // based on a failed read - the loop will verify again on the next tick. + return false, fmt.Errorf("failed to verify host iptables rules, skipping repair: %w", err) + } + if converged { + return false, nil + } + + log.WithLabels("component", "host"). + Warn("detected drift in host-level iptables rules (external flush/overwrite?), re-applying them") + // Once the leftovers are deleted the re-create must succeed, otherwise the node + // would be left without probe SNAT rules until the next tick; retry the sequence + // like the startup path does, but bounded well below the reconcile interval. + err = backoff.Retry(func() error { + cfg.DeleteHostRules() + return cfg.CreateHostRulesForHealthChecks() + }, backoff.WithMaxRetries(backoff.NewConstantBackOff(2*time.Second), 5)) + if err != nil { + return true, err + } + return true, nil +} + +// hostRulesConverged reports whether every host-level rule this configurator manages is +// still present, using the same read-only probes as VerifyIptablesState (an +// iptables-save snapshot for chain existence and ISTIO_* chain rule counts, plus a +// per-rule `iptables -C`). It intentionally diverges from VerifyIptablesState in two +// ways that matter for a periodic runtime loop: +// - a failed iptables-save is returned as an error instead of being reported as +// drift, so a transient read failure can never trigger a destructive repair; +// - ISTIO_* chains that are not part of the expected host state (e.g. residue from +// other istio components or versions) are ignored, since the delete+create repair +// cannot remove chains it does not own and treating them as drift would make the +// loop re-apply the rules on every tick without ever converging. +// +// The IPv6 state is only verified when IPv6 is enabled, since no v6 rules are managed +// otherwise. +func (cfg *IptablesConfigurator) hostRulesConverged(ruleBuilder *builder.IptablesRuleBuilder) (bool, error) { + log := log.WithLabels("component", "host") + type ipFamily struct { + ver *dep.IptablesVersion + expected string + checks [][]string + } + families := []ipFamily{ + {&cfg.iptV, ruleBuilder.BuildV4Restore(), ruleBuilder.BuildCheckV4()}, + } + if cfg.cfg.EnableIPv6 { + families = append(families, ipFamily{&cfg.ipt6V, ruleBuilder.BuildV6Restore(), ruleBuilder.BuildCheckV6()}) + } + for _, family := range families { + output, err := cfg.ext.Run(log, true, iptablesconstants.IPTablesSave, family.ver, nil) + if err != nil { + return false, fmt.Errorf("%s failed: %w", family.ver.CmdToString(iptablesconstants.IPTablesSave), err) + } + currentState := ruleBuilder.GetStateFromSave(output.String()) + expectedState := ruleBuilder.GetStateFromSave(family.expected) + for table, chains := range expectedState { + for chain, rules := range chains { + currentRules, ok := currentState[table][chain] + if !ok || (strings.HasPrefix(chain, "ISTIO_") && len(rules) != len(currentRules)) { + log.Debugf("host chain %s (table: %s) is missing or has an unexpected number of rules", chain, table) + return false, nil + } + } + } + for _, cmd := range family.checks { + if _, err := cfg.ext.Run(log, true, iptablesconstants.IPTables, family.ver, nil, cmd...); err != nil { + log.Debugf("host rule check %v failed", cmd) + return false, nil + } + } + } + return true, nil +} + func (cfg *IptablesConfigurator) DeleteHostRules() { log.Debug("Attempting to delete hostside iptables rules (if they exist)") builder := cfg.AppendHostRules() diff --git a/cni/pkg/iptables/iptables_e2e_linux_test.go b/cni/pkg/iptables/iptables_e2e_linux_test.go index 62de6c5c26..5889aeba2d 100644 --- a/cni/pkg/iptables/iptables_e2e_linux_test.go +++ b/cni/pkg/iptables/iptables_e2e_linux_test.go @@ -30,6 +30,7 @@ import ( "istio.io/istio/cni/pkg/scopes" "istio.io/istio/pkg/test/util/assert" iptablescapture "istio.io/istio/tools/istio-iptables/pkg/capture" + iptablesconstants "istio.io/istio/tools/istio-iptables/pkg/constants" dep "istio.io/istio/tools/istio-iptables/pkg/dependencies" ) @@ -255,6 +256,82 @@ func TestIptablesHostCleanRoundTrip(t *testing.T) { } } +// TestEnsureHostRulesRepairsExternalFlushE2E reproduces istio/istio#60607 against a +// real kernel: after an external actor flushes the ISTIO_POSTRT chain, EnsureHostRules +// must detect the drift and repair the host rules back to a converged state. +func TestEnsureHostRulesRepairsExternalFlushE2E(t *testing.T) { + setup(t) + + ext := &dep.RealDependencies{ + UsePodScopedXtablesLock: false, + NetworkNamespace: "", + } + iptVer, err := ext.DetectIptablesVersion(false) + if err != nil { + t.Fatalf("Can't detect iptables version: %v", err) + } + ipt6Ver, err := ext.DetectIptablesVersion(true) + if err != nil { + t.Fatalf("Can't detect ip6tables version") + } + + cfg := constructTestConfig() + + deps := &dep.RealDependencies{} + set, err := createHostsideProbeIpset(true) + if err != nil { + t.Fatalf("failed to create hostside probe ipset: %v", err) + } + defer func() { + assert.NoError(t, set.DestroySet()) + }() + + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, deps, deps, RealNlDeps()) + if err != nil { + t.Fatalf("failed to setup iptables configurator: %v", err) + } + builder := iptConfigurator.AppendHostRules() + defer func() { + iptConfigurator.DeleteHostRules() + }() + + assert.NoError(t, iptConfigurator.CreateHostRulesForHealthChecks()) + + // When converged, EnsureHostRules must be a read-only no-op + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, repaired, false) + + // Simulate an external flush (the issue's reproduction step): empty the + // ISTIO_POSTRT chain but keep the chain and the jump + if _, err := ext.Run(log, false, iptablesconstants.IPTables, &iptVer, nil, + "-t", "nat", "-F", ChainHostPostrouting); err != nil { + t.Fatalf("failed to simulate external flush: %v", err) + } + _, deltaExists := iptablescapture.VerifyIptablesState(log, iptConfigurator.ext, builder, &iptVer, &ipt6Ver) + assert.Equal(t, deltaExists, true) + + // EnsureHostRules must detect the drift and repair it + repaired, err = iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, repaired, true) + + residueExists, deltaExists := iptablescapture.VerifyIptablesState(log, iptConfigurator.ext, builder, &iptVer, &ipt6Ver) + assert.Equal(t, residueExists, true) + assert.Equal(t, deltaExists, false) + + // A more thorough breakage: remove the chain and the jump altogether, + // which must also be repaired + iptConfigurator.DeleteHostRules() + repaired, err = iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, repaired, true) + + residueExists, deltaExists = iptablescapture.VerifyIptablesState(log, iptConfigurator.ext, builder, &iptVer, &ipt6Ver) + assert.Equal(t, residueExists, true) + assert.Equal(t, deltaExists, false) +} + var initialized = &sync.Once{} func setup(t *testing.T) { diff --git a/cni/pkg/iptables/iptables_reconcile_test.go b/cni/pkg/iptables/iptables_reconcile_test.go new file mode 100644 index 0000000000..6b60f228c2 --- /dev/null +++ b/cni/pkg/iptables/iptables_reconcile_test.go @@ -0,0 +1,247 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iptables + +import ( + "bytes" + "errors" + "io" + "strings" + "testing" + + istiolog "istio.io/istio/pkg/log" + "istio.io/istio/pkg/slices" + "istio.io/istio/pkg/test/util/assert" + iptablesconstants "istio.io/istio/tools/istio-iptables/pkg/constants" + dep "istio.io/istio/tools/istio-iptables/pkg/dependencies" +) + +// hostStateStub builds on DependenciesStub to simulate pre-existing iptables state on +// the host: iptables-save returns a canned snapshot (or fails with saveErr), iptables -C +// succeeds or fails according to checkFails, iptables-restore fails for the first +// restoreFailures invocations, and every other command is only recorded. +type hostStateStub struct { + dep.DependenciesStub + saveOutput string + saveErr error + checkFails bool + // restoreFailures makes the first N iptables-restore invocations fail, to + // exercise the repair retry + restoreFailures int +} + +func (s *hostStateStub) Run(logger *istiolog.Scope, quietLogging bool, cmd iptablesconstants.IptablesCmd, + iptVer *dep.IptablesVersion, stdin io.ReadSeeker, args ...string, +) (*bytes.Buffer, error) { + // Record every executed command so that tests can assert on them + _, _ = s.DependenciesStub.Run(logger, quietLogging, cmd, iptVer, stdin, args...) + if cmd == iptablesconstants.IPTablesSave { + if s.saveErr != nil { + return nil, s.saveErr + } + return bytes.NewBufferString(s.saveOutput), nil + } + if cmd == iptablesconstants.IPTablesRestore && s.restoreFailures > 0 { + s.restoreFailures-- + return nil, errors.New("restore transiently failed") + } + if cmd == iptablesconstants.IPTables && slices.Contains(args, "-C") { + if s.checkFails { + return nil, errors.New("rule does not exist") + } + } + return &bytes.Buffer{}, nil +} + +// convergedHostState is an iptables-save snapshot that fully matches the desired host +// rules (the POSTROUTING jump + one SNAT rule in ISTIO_POSTRT). +const convergedHostState = `*nat +:PREROUTING ACCEPT [0:0] +:POSTROUTING ACCEPT [0:0] +:ISTIO_POSTRT - [0:0] +-A POSTROUTING -j ISTIO_POSTRT +-A ISTIO_POSTRT -m owner --socket-exists -p tcp -m set --match-set istio-inpod-probes-v4 dst -j SNAT --to-source 169.254.7.127 +COMMIT +` + +// driftedHostState simulates the state after an external flush: the ISTIO_POSTRT chain +// still exists but its rules are gone (the reproduction step of issue #60607, +// `iptables -t nat -F ISTIO_POSTRT`). +const driftedHostState = `*nat +:PREROUTING ACCEPT [0:0] +:POSTROUTING ACCEPT [0:0] +:ISTIO_POSTRT - [0:0] +-A POSTROUTING -j ISTIO_POSTRT +COMMIT +` + +func TestEnsureHostRulesNoopWhenConverged(t *testing.T) { + cfg := constructTestConfig() + ext := &hostStateStub{saveOutput: convergedHostState} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, false, repaired) + + // When converged this must be a pure read-only verification: + // no delete/recreate/restore writes are allowed + assert.Equal(t, 0, len(ext.ExecutedStdin)) + for _, cmd := range ext.ExecutedAll { + for _, forbidden := range []string{" -D ", " -F ", " -X ", "iptables-restore"} { + if strings.Contains(cmd, forbidden) { + t.Fatalf("expected no mutating command when converged, got: %v", cmd) + } + } + } + // ip6tables must not be touched when IPv6 is disabled + for _, cmd := range ext.ExecutedAll { + if strings.Contains(cmd, "ip6tables") { + t.Fatalf("expected no ip6tables invocation when IPv6 is disabled, got: %v", cmd) + } + } +} + +func TestEnsureHostRulesRepairsDrift(t *testing.T) { + cfg := constructTestConfig() + ext := &hostStateStub{saveOutput: driftedHostState} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, true, repaired) + + // The repair must mirror the startup path: delete first (-D jump / -F / -X chain), + // then re-create (iptables-restore) + joined := strings.Join(ext.ExecutedAll, "\n") + for _, expected := range []string{ + "-t nat -D POSTROUTING -j ISTIO_POSTRT", + "-t nat -F ISTIO_POSTRT", + "-t nat -X ISTIO_POSTRT", + } { + if !strings.Contains(joined, expected) { + t.Fatalf("expected delete command %q in executed commands:\n%v", expected, joined) + } + } + // The restore payload (stdin) must re-create the SNAT rule + restorePayload := strings.Join(ext.ExecutedStdin, "\n") + if !strings.Contains(restorePayload, "-j SNAT --to-source 169.254.7.127") { + t.Fatalf("expected SNAT rule in restore payload:\n%v", restorePayload) + } + + // Guardrail DROP rules must never show up in the host netns + // (they would drop the entire node's traffic) + if strings.Contains(joined, "-j DROP") { + t.Fatalf("host rules reconcile must never install guardrail DROP rules, got:\n%v", joined) + } +} + +func TestEnsureHostRulesSkipsRepairWhenStateUnreadable(t *testing.T) { + // A transient iptables-save failure means the state could not be verified, NOT + // that the rules have drifted. Deleting live, possibly-healthy rules based on a + // failed read would let the reconciler itself cause the outage it exists to + // prevent, so EnsureHostRules must surface the error and perform no writes. + cfg := constructTestConfig() + ext := &hostStateStub{saveErr: errors.New("iptables-save failed transiently")} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + repaired, err := iptConfigurator.EnsureHostRules() + assert.Equal(t, false, repaired) + if err == nil { + t.Fatal("expected an error when the iptables state cannot be read") + } + + // No mutating command may run when verification itself failed + assert.Equal(t, 0, len(ext.ExecutedStdin)) + for _, cmd := range ext.ExecutedAll { + for _, forbidden := range []string{" -D ", " -F ", " -X "} { + if strings.Contains(cmd, forbidden) { + t.Fatalf("expected no mutating command when state is unreadable, got: %v", cmd) + } + } + } +} + +// foreignChainHostState is convergedHostState plus an ISTIO_-prefixed chain that is not +// part of the expected host state (e.g. residue from another istio component or +// version). The delete+create repair cannot remove such a chain, so treating it as +// drift would make the reconcile loop re-apply the rules every tick forever. +const foreignChainHostState = `*nat +:PREROUTING ACCEPT [0:0] +:POSTROUTING ACCEPT [0:0] +:ISTIO_POSTRT - [0:0] +:ISTIO_OUTPUT - [0:0] +-A POSTROUTING -j ISTIO_POSTRT +-A ISTIO_POSTRT -m owner --socket-exists -p tcp -m set --match-set istio-inpod-probes-v4 dst -j SNAT --to-source 169.254.7.127 +-A ISTIO_OUTPUT -p tcp -j ACCEPT +COMMIT +` + +func TestEnsureHostRulesIgnoresForeignIstioChains(t *testing.T) { + cfg := constructTestConfig() + ext := &hostStateStub{saveOutput: foreignChainHostState} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + // All managed rules are present, so the foreign ISTIO_OUTPUT chain must not be + // reported as drift and no repair may run + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, false, repaired) + assert.Equal(t, 0, len(ext.ExecutedStdin)) +} + +func TestEnsureHostRulesRetriesFailedRepair(t *testing.T) { + // Once the drifted rules have been deleted, a transiently failing re-create must + // be retried immediately (mirroring the startup path), instead of leaving the + // node without any probe SNAT rules until the next reconcile tick. + cfg := constructTestConfig() + ext := &hostStateStub{saveOutput: driftedHostState, restoreFailures: 1} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, true, repaired) + + // The first restore failed, so the payload must have been re-sent at least twice + snatAttempts := 0 + for _, line := range ext.ExecutedStdin { + if strings.Contains(line, "-j SNAT --to-source 169.254.7.127") { + snatAttempts++ + } + } + if snatAttempts < 2 { + t.Fatalf("expected the repair to be retried after a failed restore, got %d restore attempts:\n%v", + snatAttempts, strings.Join(ext.ExecutedStdin, "\n")) + } +} + +func TestEnsureHostRulesRepairsMissingCheckedRule(t *testing.T) { + // Chains and rule counts look right, but the per-rule -C checks fail (e.g. the + // POSTROUTING jump was removed externally and other rules were inserted instead); + // this must also trigger a repair + cfg := constructTestConfig() + ext := &hostStateStub{saveOutput: convergedHostState, checkFails: true} + iptConfigurator, _, err := NewIptablesConfigurator(cfg, cfg, ext, ext, EmptyNlDeps()) + assert.NoError(t, err) + + repaired, err := iptConfigurator.EnsureHostRules() + assert.NoError(t, err) + assert.Equal(t, true, repaired) +} diff --git a/cni/pkg/nftables/host_sets.go b/cni/pkg/nftables/host_sets.go index 24b107be7a..d092f77b03 100644 --- a/cni/pkg/nftables/host_sets.go +++ b/cni/pkg/nftables/host_sets.go @@ -64,6 +64,29 @@ func NewHostSetManager(setPrefix string, enableIPv6 bool) (*HostNftSetManager, e return manager, nil } +// hostProbeIPSets returns the definitions of the probe IP sets referenced by the host +// health check SNAT rules. The definitions are shared between the set manager +// initialization and the host rules transaction (which re-asserts them so that a repair +// converges even after an external actor removed the whole table): both must stay in +// sync, since re-adding an existing set with a different definition is rejected by +// nftables, while re-adding it with the same definition is a no-op that preserves the +// set elements. +func hostProbeIPSets(setPrefix string, enableIPv6 bool) []knftables.Set { + sets := []knftables.Set{{ + Name: fmt.Sprintf("%s-v4", setPrefix), + Type: "ipv4_addr", + Comment: knftables.PtrTo("UUID of the pods that are part of ambient mesh"), + }} + if enableIPv6 { + sets = append(sets, knftables.Set{ + Name: fmt.Sprintf("%s-v6", setPrefix), + Type: "ipv6_addr", + Comment: knftables.PtrTo("UUID of the pods that are part of ambient mesh"), + }) + } + return sets +} + // initializeSets creates the nftables table and sets func (h *HostNftSetManager) initializeSets() error { nft, err := h.nftProvider(knftables.InetFamily, AmbientNatTable) @@ -76,20 +99,9 @@ func (h *HostNftSetManager) initializeSets() error { // Create table tx.Add(&knftables.Table{}) - // Create IPv4 set - tx.Add(&knftables.Set{ - Name: h.v4SetName, - Type: "ipv4_addr", - Comment: knftables.PtrTo("UUID of the pods that are part of ambient mesh"), - }) - - // Create IPv6 set if enabled - if h.enableIPv6 { - tx.Add(&knftables.Set{ - Name: h.v6SetName, - Type: "ipv6_addr", - Comment: knftables.PtrTo("UUID of the pods that are part of ambient mesh"), - }) + // Create the probe IP sets (the IPv6 one only if enabled) + for _, set := range hostProbeIPSets(h.prefix, h.enableIPv6) { + tx.Add(&set) } if err := nft.Run(context.TODO(), tx); err != nil { diff --git a/cni/pkg/nftables/nftables.go b/cni/pkg/nftables/nftables.go index 704efaaea4..36b2c27685 100644 --- a/cni/pkg/nftables/nftables.go +++ b/cni/pkg/nftables/nftables.go @@ -67,6 +67,15 @@ type NftablesConfigurator struct { nlDeps iptables.NetlinkDependencies cfg *config.AmbientConfig nftProvider NftProviderFunc + + // programmedKubeletUID and programmedHostRules record the input and the shape of + // the last successfully programmed host rules. The periodic reconcile compares the + // current state against them to detect drift without having to interpret kernel + // rule text. They are only accessed from the startup programming and the reconcile + // loop, which the caller sequences (the loop starts after the startup programming + // and is stopped before the host rules are deleted), so no locking is needed. + programmedKubeletUID string + programmedHostRules int } // NewNftablesConfigurator creates both host and pod nftables configurators @@ -478,7 +487,7 @@ func (cfg *NftablesConfigurator) CreateHostRulesForHealthChecks() error { rb.AppendV6RuleIfSupported(PostroutingChain, AmbientNatTable, "meta l4proto tcp", "skuid", kubeletUID, "ip6", "daddr", fmt.Sprintf("@%s-v6", config.ProbeIPSet), Counter, "snat", "to", cfg.cfg.HostProbeV6SNATAddress.String()) - return util.RunAsHost(func() error { + err = util.RunAsHost(func() error { tx, err := cfg.executeHostCommands(rb) if err != nil { log.Errorf("failed to program nftable rules in the host network namespace: %v", err) @@ -487,6 +496,93 @@ func (cfg *NftablesConfigurator) CreateHostRulesForHealthChecks() error { builder.LogNftRules(tx) return nil }) + if err != nil { + return err + } + + // Record the input and the shape of this successful programming so the periodic + // reconcile can verify convergence without re-programming (see EnsureHostRules). + cfg.programmedKubeletUID = kubeletUID + cfg.programmedHostRules = len(rb.Rules[AmbientNatTable]) + return nil +} + +// EnsureHostRules verifies the health check rules in the host netns and re-programs +// them only when drift is detected, meant to be invoked from a periodic reconcile +// loop. It returns whether a repair was performed, so that steady state stays free of +// rule writes, log churn, counter resets and nft generation bumps. +// +// Drift is detected without interpreting kernel rule text (the kernel normalizes rule +// text, and knftables ListRules does not return it at all); instead the check covers +// the two things that fully determine the desired state: +// - the rules present in the istio-owned postrouting chain: istio is the only owner +// of the istio-ambient-nat table, so it is enough to verify the chain still holds +// exactly the number of rules last programmed. External actors remove the rules +// wholesale (e.g. a firewalld reload running "flush ruleset") rather than editing +// them in place, and an absent table or chain is likewise drift; +// - the kubelet UID captured at the last programming: the rules embed it (meta +// skuid), so a kubelet restarted under a different UID makes them stale - a drift +// mode specific to the nftables backend (iptables matches on +// "-m owner --socket-exists" instead of a UID). +// +// When the check itself fails (for any reason other than the table/chain being +// absent, which IS drift), the tick is skipped and the error is returned without +// re-programming anything, matching the EnsureHostRules contract; the reconcile +// ticker provides the retry. The repair is a single atomic knftables transaction that +// also re-asserts the probe IP sets the rules reference, so it converges even after +// the whole table was removed, and a failed repair leaves the previous state +// untouched - which is why, unlike the iptables backend's delete+create sequence, it +// needs no retry loop of its own. Note that re-creating the sets cannot restore their +// elements: probe IPs of pods enrolled before a full table wipe are only restored as +// pod events are re-processed. +func (cfg *NftablesConfigurator) EnsureHostRules() (bool, error) { + converged, err := cfg.hostRulesConverged() + if err != nil { + return false, err + } + if converged { + return false, nil + } + if err := cfg.CreateHostRulesForHealthChecks(); err != nil { + return false, err + } + return true, nil +} + +// hostRulesConverged reports whether the host rules currently programmed in the +// kernel still match the last successful programming. An error means the check itself +// could not run; an absent table or chain is reported as non-converged, not as an +// error. A configurator that never programmed successfully (empty recorded kubelet +// UID) is treated as drift, which makes the reconcile self-seeding. +func (cfg *NftablesConfigurator) hostRulesConverged() (bool, error) { + kubeletUID, err := kubeletUIDProvider(filepath.Join(constants.HostMountsPath, "proc")) + if err != nil { + return false, fmt.Errorf("failed to get the kubelet uid: %w", err) + } + if kubeletUID != cfg.programmedKubeletUID { + return false, nil + } + + // The read must go through an interface scoped to the ambient NAT table, since + // knftables only supports ListRules with an associated family/table. + nft, err := cfg.nftProvider(knftables.InetFamily, AmbientNatTable) + if err != nil { + return false, err + } + var rules []*knftables.Rule + err = util.RunAsHost(func() error { + var listErr error + rules, listErr = nft.ListRules(context.TODO(), PostroutingChain) + return listErr + }) + if err != nil { + if knftables.IsNotFound(err) { + // The table or the chain is gone: that is precisely the drift to repair. + return false, nil + } + return false, err + } + return len(rules) == cfg.programmedHostRules, nil } // DeleteHostRules removes host-level nftables rules @@ -592,7 +688,12 @@ func (cfg *NftablesConfigurator) executeHostCommands(rb *builder.NftablesRuleBui } rules := rb.Rules[AmbientNatTable] - tx = cfg.addIstioTableRules(tx, AmbientNatTable, chains, rules) + // Re-assert the probe IP sets the SNAT rules reference, to keep the transaction + // self-contained: if an external actor removed the whole table (e.g. a "flush + // ruleset"), re-adding the rules alone would fail on the dangling set reference. + // When the sets already exist this is a no-op that preserves their elements. + sets := hostProbeIPSets(config.ProbeIPSet, cfg.cfg.EnableIPv6) + tx = cfg.addIstioTableRules(tx, AmbientNatTable, chains, rules, sets) if tx.NumOperations() > 0 { if err := nft.Run(context.TODO(), tx); err != nil { return tx, fmt.Errorf("nftables run failed: %w", err) @@ -631,7 +732,7 @@ func (cfg *NftablesConfigurator) addIstioNatTableRules(tx *knftables.Transaction rules := rb.Rules[AmbientNatTable] - return cfg.addIstioTableRules(tx, AmbientNatTable, chains, rules) + return cfg.addIstioTableRules(tx, AmbientNatTable, chains, rules, nil) } // addIstioMangleTableRules updates a transaction to include the nftables rules for the AmbientMangleTable table. @@ -664,7 +765,7 @@ func (cfg *NftablesConfigurator) addIstioMangleTableRules(tx *knftables.Transact rules := rb.Rules[AmbientMangleTable] - return cfg.addIstioTableRules(tx, AmbientMangleTable, chains, rules) + return cfg.addIstioTableRules(tx, AmbientMangleTable, chains, rules, nil) } // addIstioRawTableRules updates a transaction to include the nftables rules for the AmbientRawTable table. @@ -697,7 +798,7 @@ func (cfg *NftablesConfigurator) addIstioRawTableRules(tx *knftables.Transaction rules := rb.Rules[AmbientRawTable] - return cfg.addIstioTableRules(tx, AmbientRawTable, chains, rules) + return cfg.addIstioTableRules(tx, AmbientRawTable, chains, rules, nil) } func (cfg *NftablesConfigurator) addIstioTableRules( @@ -705,6 +806,7 @@ func (cfg *NftablesConfigurator) addIstioTableRules( tableName string, chains []knftables.Chain, rules []knftables.Rule, + sets []knftables.Set, ) *knftables.Transaction { // Track how many rules have been added to each chain chainRuleCount := make(map[string]int) @@ -745,6 +847,14 @@ func (cfg *NftablesConfigurator) addIstioTableRules( // Flush the table to remove all existing rules before applying new ones tx.Flush(&knftables.Table{Name: tableName, Family: knftables.InetFamily}) + // Ensure the sets referenced by the rules exist. Adding an existing set with an + // unchanged definition is a no-op that preserves its elements. + for _, set := range sets { + set.Table = tableName + set.Family = knftables.InetFamily + tx.Add(&set) + } + // Add the chains that have rules for _, chain := range chainsWithRules { tx.Add(&chain) diff --git a/cni/pkg/nftables/nftables_reconcile_test.go b/cni/pkg/nftables/nftables_reconcile_test.go new file mode 100644 index 0000000000..7600376214 --- /dev/null +++ b/cni/pkg/nftables/nftables_reconcile_test.go @@ -0,0 +1,365 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package nftables + +import ( + "context" + "errors" + "strings" + "testing" + + "sigs.k8s.io/knftables" + + "istio.io/istio/cni/pkg/config" + "istio.io/istio/cni/pkg/iptables" + dep "istio.io/istio/tools/istio-iptables/pkg/dependencies" + "istio.io/istio/tools/istio-nftables/pkg/builder" +) + +// resetCaptured clears the captured transactions so a test can assert whether a +// subsequent operation ran any transaction at all. +func (m *MockNftablesCapture) resetCaptured() { + m.lock.Lock() + defer m.lock.Unlock() + m.transactions = nil +} + +type hostReconcileEnv struct { + mock *MockNftablesCapture + host *NftablesConfigurator + // api is what the provider stub hands out; the configurator captures the stub at + // construction time, so tests swap this field (not nftProviderVar) to change the + // backend behavior mid-test. + api builder.NftablesAPI + // expectedRules is the number of SNAT rules the host postrouting chain is + // expected to hold (1 for IPv4 only, 2 when IPv6 is enabled). + expectedRules int +} + +// setupHostReconcileTest wires a capture mock scoped to the ambient NAT table (so that +// ListRules, used by the convergence check, can resolve the table like the scoped +// interface the production read path requests) and stubs the kubelet UID provider. +func setupHostReconcileTest(t *testing.T, ipv6 bool) *hostReconcileEnv { + t.Helper() + + cfg := constructTestConfig() + cfg.EnableIPv6 = ipv6 + ext := &dep.DependenciesStub{} + + mock := &MockNftablesCapture{ + MockNftables: builder.NewMockNftables(knftables.InetFamily, AmbientNatTable), + } + env := &hostReconcileEnv{mock: mock, api: mock} + originalProvider := nftProviderVar + nftProviderVar = func(_ knftables.Family, _ string) (builder.NftablesAPI, error) { + return env.api, nil + } + originalKubeletProvider := kubeletUIDProvider + kubeletUIDProvider = func(string) (string, error) { + return "1000", nil + } + t.Cleanup(func() { + nftProviderVar = originalProvider + kubeletUIDProvider = originalKubeletProvider + }) + + host, _, err := NewNftablesConfigurator(cfg, cfg, ext, ext, iptables.EmptyNlDeps()) + if err != nil { + t.Fatalf("NewNftablesConfigurator: %v", err) + } + + env.host = host + env.expectedRules = 1 + if ipv6 { + env.expectedRules = 2 + } + return env +} + +func (env *hostReconcileEnv) postroutingRules(t *testing.T) []*knftables.Rule { + t.Helper() + rules, err := env.mock.ListRules(context.Background(), PostroutingChain) + if err != nil { + t.Fatalf("listing postrouting rules: %v", err) + } + return rules +} + +// TestCreateHostRulesProgramsProbeSets verifies that the host rules transaction is +// self-contained: it (re-)creates the probe IP sets the SNAT rules reference, so that a +// repair converges even after an external actor removed the whole table (e.g. a +// "flush ruleset"), instead of failing forever on a dangling set reference. +func TestCreateHostRulesProgramsProbeSets(t *testing.T) { + for _, ipv6 := range []bool{false, true} { + t.Run(ipstr(ipv6), func(t *testing.T) { + env := setupHostReconcileTest(t, ipv6) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + if got := len(env.postroutingRules(t)); got != env.expectedRules { + t.Fatalf("expected %d postrouting rules programmed, got %d", env.expectedRules, got) + } + + env.mock.RLock() + defer env.mock.RUnlock() + if env.mock.Table == nil { + t.Fatal("ambient nat table not programmed") + } + if env.mock.Table.Sets[config.ProbeIPSet+"-v4"] == nil { + t.Fatal("v4 probe IP set missing from the host rules transaction") + } + if ipv6 && env.mock.Table.Sets[config.ProbeIPSet+"-v6"] == nil { + t.Fatal("v6 probe IP set missing from the host rules transaction") + } + }) + } +} + +// TestEnsureHostRulesConvergedIsReadOnly verifies that when the host rules are intact, +// the periodic reconcile does not run any nftables transaction (and reports no repair). +func TestEnsureHostRulesConvergedIsReadOnly(t *testing.T) { + for _, ipv6 := range []bool{false, true} { + t.Run(ipstr(ipv6), func(t *testing.T) { + env := setupHostReconcileTest(t, ipv6) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + env.mock.resetCaptured() + + repaired, err := env.host.EnsureHostRules() + if err != nil { + t.Fatalf("EnsureHostRules: %v", err) + } + if repaired { + t.Fatal("expected no repair to be reported when rules are intact") + } + if txs := env.mock.GetCapturedRules(); len(txs) != 0 { + t.Fatalf("expected no transactions in steady state, got %d:\n%s", len(txs), strings.Join(txs, "\n")) + } + }) + } +} + +// TestEnsureHostRulesRepairsExternalWipe verifies that deleting the whole ambient NAT +// table (the observable effect of e.g. a firewalld reload running "flush ruleset") is +// detected as drift and fully repaired, including the probe IP set structure the SNAT +// rules reference. +func TestEnsureHostRulesRepairsExternalWipe(t *testing.T) { + for _, ipv6 := range []bool{false, true} { + t.Run(ipstr(ipv6), func(t *testing.T) { + env := setupHostReconcileTest(t, ipv6) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + // Simulate the external actor: bypass the capture wrapper and delete the table. + tx := env.mock.NewTransaction() + tx.Delete(&knftables.Table{Name: AmbientNatTable, Family: knftables.InetFamily}) + if err := env.mock.MockNftables.Run(context.Background(), tx); err != nil { + t.Fatalf("simulating external table wipe: %v", err) + } + + repaired, err := env.host.EnsureHostRules() + if err != nil { + t.Fatalf("EnsureHostRules: %v", err) + } + if !repaired { + t.Fatal("expected drift to be detected and repaired after an external table wipe") + } + if got := len(env.postroutingRules(t)); got != env.expectedRules { + t.Fatalf("expected %d postrouting rules after repair, got %d", env.expectedRules, got) + } + + // A follow-up tick must be read-only again. + env.mock.resetCaptured() + repaired, err = env.host.EnsureHostRules() + if err != nil || repaired { + t.Fatalf("expected converged steady state after repair, got repaired=%v err=%v", repaired, err) + } + if txs := env.mock.GetCapturedRules(); len(txs) != 0 { + t.Fatalf("expected no transactions after repair converged, got %d", len(txs)) + } + }) + } +} + +// TestEnsureHostRulesRepairsRuleCountDrift verifies that a rule count mismatch in the +// istio-owned postrouting chain (a rule removed, or a foreign rule injected) is treated +// as drift and the chain is re-programmed back to the expected state. +func TestEnsureHostRulesRepairsRuleCountDrift(t *testing.T) { + env := setupHostReconcileTest(t, false) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + // Simulate a foreign rule injected into the istio-owned chain. + tx := env.mock.NewTransaction() + tx.Add(&knftables.Rule{ + Table: AmbientNatTable, + Family: knftables.InetFamily, + Chain: PostroutingChain, + Rule: "meta l4proto tcp counter accept", + }) + if err := env.mock.MockNftables.Run(context.Background(), tx); err != nil { + t.Fatalf("simulating foreign rule injection: %v", err) + } + + repaired, err := env.host.EnsureHostRules() + if err != nil { + t.Fatalf("EnsureHostRules: %v", err) + } + if !repaired { + t.Fatal("expected rule count drift to be detected and repaired") + } + if got := len(env.postroutingRules(t)); got != env.expectedRules { + t.Fatalf("expected %d postrouting rules after repair, got %d", env.expectedRules, got) + } +} + +// TestEnsureHostRulesRepairsKubeletUIDChange verifies that a kubelet UID change across +// a kubelet restart (the rules embed the UID captured at programming time) is detected +// as drift and the rules are re-programmed with the new UID. +func TestEnsureHostRulesRepairsKubeletUIDChange(t *testing.T) { + env := setupHostReconcileTest(t, false) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + kubeletUIDProvider = func(string) (string, error) { + return "2000", nil + } + env.mock.resetCaptured() + + repaired, err := env.host.EnsureHostRules() + if err != nil { + t.Fatalf("EnsureHostRules: %v", err) + } + if !repaired { + t.Fatal("expected kubelet UID change to be detected and repaired") + } + txs := env.mock.GetCapturedRules() + if len(txs) == 0 || !strings.Contains(strings.Join(txs, "\n"), "skuid 2000") { + t.Fatalf("expected re-programmed rules to embed the new kubelet UID, got:\n%s", strings.Join(txs, "\n")) + } + + // With the new UID recorded, the next tick must converge without writes. + env.mock.resetCaptured() + repaired, err = env.host.EnsureHostRules() + if err != nil || repaired { + t.Fatalf("expected converged steady state with the new UID, got repaired=%v err=%v", repaired, err) + } + if txs := env.mock.GetCapturedRules(); len(txs) != 0 { + t.Fatalf("expected no transactions after UID repair converged, got %d", len(txs)) + } +} + +// failingListRules simulates a read-side failure that is not a "not found" condition +// (e.g. the nft binary failing), which must NOT be treated as drift. +type failingListRules struct { + *MockNftablesCapture + listErr error +} + +func (f *failingListRules) ListRules(context.Context, string) ([]*knftables.Rule, error) { + return nil, f.listErr +} + +// TestEnsureHostRulesSkipsRepairOnReadFailure verifies that when the convergence check +// itself fails (for reasons other than the table/chain being absent), the tick is +// skipped and reported as an error instead of blindly re-programming: the ticker +// provides the retry. +func TestEnsureHostRulesSkipsRepairOnReadFailure(t *testing.T) { + env := setupHostReconcileTest(t, false) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + listErr := errors.New("nft: resource temporarily unavailable") + env.api = &failingListRules{MockNftablesCapture: env.mock, listErr: listErr} + env.mock.resetCaptured() + + repaired, err := env.host.EnsureHostRules() + if err == nil { + t.Fatal("expected the read failure to be propagated") + } + if !errors.Is(err, listErr) { + t.Fatalf("expected the underlying list error, got: %v", err) + } + if repaired { + t.Fatal("expected no repair to be reported on a read failure") + } + if txs := env.mock.GetCapturedRules(); len(txs) != 0 { + t.Fatalf("expected no transactions on a read failure, got %d", len(txs)) + } +} + +// TestEnsureHostRulesSkipsRepairOnKubeletUIDError verifies that failing to determine +// the current kubelet UID (e.g. kubelet restarting at that instant) skips the tick +// without touching the healthy rules. +func TestEnsureHostRulesSkipsRepairOnKubeletUIDError(t *testing.T) { + env := setupHostReconcileTest(t, false) + + if err := env.host.CreateHostRulesForHealthChecks(); err != nil { + t.Fatalf("CreateHostRulesForHealthChecks: %v", err) + } + + uidErr := errors.New("kubelet process not found") + kubeletUIDProvider = func(string) (string, error) { + return "", uidErr + } + env.mock.resetCaptured() + + repaired, err := env.host.EnsureHostRules() + if err == nil { + t.Fatal("expected the kubelet UID error to be propagated") + } + if repaired { + t.Fatal("expected no repair to be reported on a kubelet UID read failure") + } + if txs := env.mock.GetCapturedRules(); len(txs) != 0 { + t.Fatalf("expected no transactions on a kubelet UID read failure, got %d", len(txs)) + } +} + +// TestEnsureHostRulesSelfSeedsWithoutCreate verifies that the reconcile is self-healing +// even if it somehow runs before the startup programming succeeded: the first tick +// programs the rules (reported as a repair), subsequent ticks converge. +func TestEnsureHostRulesSelfSeedsWithoutCreate(t *testing.T) { + env := setupHostReconcileTest(t, false) + + repaired, err := env.host.EnsureHostRules() + if err != nil { + t.Fatalf("EnsureHostRules: %v", err) + } + if !repaired { + t.Fatal("expected the first tick without prior programming to repair") + } + if got := len(env.postroutingRules(t)); got != env.expectedRules { + t.Fatalf("expected %d postrouting rules after self-seeding, got %d", env.expectedRules, got) + } + + env.mock.resetCaptured() + repaired, err = env.host.EnsureHostRules() + if err != nil || repaired { + t.Fatalf("expected converged steady state after self-seeding, got repaired=%v err=%v", repaired, err) + } +} diff --git a/cni/pkg/nftables/testdata/hostprobe.golden b/cni/pkg/nftables/testdata/hostprobe.golden index f40538c7b8..b4d48f2a72 100644 --- a/cni/pkg/nftables/testdata/hostprobe.golden +++ b/cni/pkg/nftables/testdata/hostprobe.golden @@ -1,4 +1,5 @@ add table inet istio-ambient-nat flush table inet istio-ambient-nat +add set inet istio-ambient-nat istio-inpod-probes-v4 { type ipv4_addr ; comment "UUID of the pods that are part of ambient mesh" ; } add chain inet istio-ambient-nat postrouting { type nat hook postrouting priority 100 ; } add rule inet istio-ambient-nat postrouting meta l4proto tcp skuid 1000 ip daddr @istio-inpod-probes-v4 counter snat to 169.254.7.127 diff --git a/cni/pkg/nftables/testdata/hostprobe_ipv6.golden b/cni/pkg/nftables/testdata/hostprobe_ipv6.golden index e10f9aa99c..e25351fa89 100644 --- a/cni/pkg/nftables/testdata/hostprobe_ipv6.golden +++ b/cni/pkg/nftables/testdata/hostprobe_ipv6.golden @@ -1,5 +1,7 @@ add table inet istio-ambient-nat flush table inet istio-ambient-nat +add set inet istio-ambient-nat istio-inpod-probes-v4 { type ipv4_addr ; comment "UUID of the pods that are part of ambient mesh" ; } +add set inet istio-ambient-nat istio-inpod-probes-v6 { type ipv6_addr ; comment "UUID of the pods that are part of ambient mesh" ; } add chain inet istio-ambient-nat postrouting { type nat hook postrouting priority 100 ; } add rule inet istio-ambient-nat postrouting meta l4proto tcp skuid 1000 ip daddr @istio-inpod-probes-v4 counter snat to 169.254.7.127 add rule inet istio-ambient-nat postrouting meta l4proto tcp skuid 1000 ip6 daddr @istio-inpod-probes-v6 counter snat to e9ac:1e77:90ca:399f:4d6d:ece2:2f9b:3164 diff --git a/cni/pkg/nodeagent/meshdataplane_linux.go b/cni/pkg/nodeagent/meshdataplane_linux.go index 2a3e0168ce..ba57235c07 100644 --- a/cni/pkg/nodeagent/meshdataplane_linux.go +++ b/cni/pkg/nodeagent/meshdataplane_linux.go @@ -18,6 +18,7 @@ import ( "errors" "net/netip" "sync" + "time" "golang.org/x/sys/unix" corev1 "k8s.io/api/core/v1" @@ -26,15 +27,37 @@ import ( set "istio.io/istio/cni/pkg/addressset" "istio.io/istio/cni/pkg/trafficmanager" "istio.io/istio/cni/pkg/util" + "istio.io/istio/pkg/monitoring" "istio.io/istio/pkg/util/sets" ) +var ( + reconcileResultTag = monitoring.CreateLabel("result") + + hostRulesReconciles = monitoring.NewSum( + "nodeagent_host_rules_reconciles_total", + "The total number of host rule reconcile operations that detected drift or failed (result: repaired/failed).", + ) +) + type meshDataplane struct { kubeClient kubernetes.Interface netServer MeshDataplane hostTrafficManager trafficmanager.TrafficRuleManager hostAddrSet set.AddressSetManager + // hostRulesReconcileInterval is the interval of the periodic host rules reconcile + // loop, <= 0 disables it. The loop detects and repairs host-level health check + // rules removed at runtime by external actors (firewalld reloads, an + // iptables-restore from an OS persistence unit, etc), see istio/istio#60607. + hostRulesReconcileInterval time.Duration + // stopHostRulesReconcile/hostRulesReconcileDone deterministically terminate the + // reconcile loop on Stop: the loop must have exited before the host rules are + // deleted, otherwise it could re-install the rules we just removed and leave + // residue behind after uninstall. + stopHostRulesReconcile context.CancelFunc + hostRulesReconcileDone chan struct{} + // branchENIRules tracks the branch ENI routing info we added rules for, // keyed by pod IP. We cache it at add time so teardown can delete the rules // even if aws-vpc-cni has already removed its iif rule (making re-detection fail). @@ -58,10 +81,57 @@ func (s *meshDataplane) ConstructInitialSnapshot(existingAmbientPods []*corev1.P // ConstructInitialSnapshot should always be invoked before this function. func (s *meshDataplane) Start(ctx context.Context) { s.netServer.Start(ctx) + + if s.hostRulesReconcileInterval > 0 { + loopCtx, cancel := context.WithCancel(ctx) + s.stopHostRulesReconcile = cancel + s.hostRulesReconcileDone = make(chan struct{}) + go s.reconcileHostRulesLoop(loopCtx) + } +} + +// reconcileHostRulesLoop periodically verifies the host-level health check rules and +// re-installs them when drift is detected. The design follows kube-proxy's periodic +// sync (30s by default) and kube-ovn's wait.Until idempotent re-assert: when there is +// no drift the verification is read-only (iptables-save + iptables -C on the iptables +// backend, an nft rule listing on the nftables backend) and no rule writes happen. +func (s *meshDataplane) reconcileHostRulesLoop(ctx context.Context) { + defer close(s.hostRulesReconcileDone) + log.Infof("starting host rules reconcile loop (interval: %v)", s.hostRulesReconcileInterval) + + ticker := time.NewTicker(s.hostRulesReconcileInterval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + log.Debug("host rules reconcile loop terminated") + return + case <-ticker.C: + repaired, err := s.hostTrafficManager.EnsureHostRules() + if err != nil { + // On error just log and retry on the next tick (the ticker itself + // provides throttling, no extra backoff needed) + log.Errorf("failed to reconcile host rules, will retry on next tick: %v", err) + hostRulesReconciles.With(reconcileResultTag.Value("failed")).Increment() + continue + } + if repaired { + log.Info("host rules drift detected and repaired") + hostRulesReconciles.With(reconcileResultTag.Value("repaired")).Increment() + } + } + } } // Stop terminates the netserver, flushes host ipsets, and removes host iptables healthprobe rules. func (s *meshDataplane) Stop(skipCleanup bool) { + // Deterministically stop the reconcile loop before cleaning up, so that it cannot + // race with DeleteHostRules and re-install the rules we just removed. + if s.stopHostRulesReconcile != nil { + s.stopHostRulesReconcile() + <-s.hostRulesReconcileDone + } + // Remove host rules (or not) that allow pod healthchecks to work. // These are not critical but if they are not in place pods that have // already been captured will eventually start to fail kubelet healthchecks. diff --git a/cni/pkg/nodeagent/meshdataplane_linux_test.go b/cni/pkg/nodeagent/meshdataplane_linux_test.go new file mode 100644 index 0000000000..544483090b --- /dev/null +++ b/cni/pkg/nodeagent/meshdataplane_linux_test.go @@ -0,0 +1,178 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package nodeagent + +import ( + "context" + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/mock" + + set "istio.io/istio/cni/pkg/addressset" + "istio.io/istio/cni/pkg/config" + "istio.io/istio/cni/pkg/ipset" + istiolog "istio.io/istio/pkg/log" + "istio.io/istio/pkg/test/util/assert" +) + +// fakeTrafficRuleManager records the sequence of EnsureHostRules/DeleteHostRules calls +// to verify the behavior of the host rules reconcile loop and the ordering guarantees +// on Stop. +type fakeTrafficRuleManager struct { + mu sync.Mutex + events []string + + ensureCh chan struct{} // signals every EnsureHostRules invocation + repaired bool + err error +} + +func (f *fakeTrafficRuleManager) CreateInpodRules(*istiolog.Scope, config.PodLevelOverrides) error { + return nil +} +func (f *fakeTrafficRuleManager) DeleteInpodRules(*istiolog.Scope) error { return nil } +func (f *fakeTrafficRuleManager) CreateHostRulesForHealthChecks() error { return nil } +func (f *fakeTrafficRuleManager) ReconcileModeEnabled() bool { return false } + +func (f *fakeTrafficRuleManager) DeleteHostRules() { + f.mu.Lock() + defer f.mu.Unlock() + f.events = append(f.events, "delete") +} + +func (f *fakeTrafficRuleManager) EnsureHostRules() (bool, error) { + f.mu.Lock() + f.events = append(f.events, "ensure") + f.mu.Unlock() + select { + case f.ensureCh <- struct{}{}: + default: + } + return f.repaired, f.err +} + +func (f *fakeTrafficRuleManager) snapshotEvents() []string { + f.mu.Lock() + defer f.mu.Unlock() + out := make([]string, len(f.events)) + copy(out, f.events) + return out +} + +func newReconcileTestDataplane(fakeTM *fakeTrafficRuleManager, interval time.Duration) *meshDataplane { + // Stop(false) goes through hostAddrSet Flush/DestroySet, back them with mocks + fakeIPSetDeps := ipset.FakeNLDeps() + fakeIPSetDeps.On("flush", mock.Anything).Return(nil).Maybe() + fakeIPSetDeps.On("destroySet", mock.Anything).Return(nil).Maybe() + ipsetInstance := ipset.IPSet{V4Name: "foo-v4", Prefix: "foo", Deps: fakeIPSetDeps} + + return &meshDataplane{ + netServer: &fakeServer{}, + hostTrafficManager: fakeTM, + hostAddrSet: set.NewIPSetWrapper(ipsetInstance), + hostRulesReconcileInterval: interval, + } +} + +func waitForEnsureCalls(t *testing.T, fakeTM *fakeTrafficRuleManager, n int) { + t.Helper() + for i := 0; i < n; i++ { + select { + case <-fakeTM.ensureCh: + case <-time.After(5 * time.Second): + t.Fatalf("reconcile loop did not tick in time (got %d/%d calls)", i, n) + } + } +} + +func TestHostRulesReconcileLoopTicksAndStops(t *testing.T) { + fakeTM := &fakeTrafficRuleManager{ensureCh: make(chan struct{}, 64), repaired: true} + dp := newReconcileTestDataplane(fakeTM, 5*time.Millisecond) + + dp.Start(context.Background()) + // The loop must invoke EnsureHostRules periodically at the configured interval + waitForEnsureCalls(t, fakeTM, 3) + + dp.Stop(false) + + // Once Stop returns the loop must have exited: + // 1. no ensure event may appear after the delete (cleanup) event, otherwise the + // cleaned-up rules would be re-installed + events := fakeTM.snapshotEvents() + deleteSeen := false + for _, e := range events { + if e == "delete" { + deleteSeen = true + } else if deleteSeen && e == "ensure" { + t.Fatalf("EnsureHostRules was invoked after DeleteHostRules, cleanup would be undone: %v", events) + } + } + assert.Equal(t, true, deleteSeen) + + // 2. no new ticks are produced + before := len(fakeTM.snapshotEvents()) + time.Sleep(30 * time.Millisecond) + assert.Equal(t, before, len(fakeTM.snapshotEvents())) +} + +func TestHostRulesReconcileLoopToleratesErrors(t *testing.T) { + // The loop must not exit when EnsureHostRules keeps failing; it must retry on the + // next tick + fakeTM := &fakeTrafficRuleManager{ensureCh: make(chan struct{}, 64), err: errors.New("boom")} + dp := newReconcileTestDataplane(fakeTM, 5*time.Millisecond) + + dp.Start(context.Background()) + waitForEnsureCalls(t, fakeTM, 3) + dp.Stop(true) +} + +func TestHostRulesReconcileDisabled(t *testing.T) { + fakeTM := &fakeTrafficRuleManager{ensureCh: make(chan struct{}, 1)} + dp := newReconcileTestDataplane(fakeTM, 0) + + dp.Start(context.Background()) + // The loop must not be started when interval <= 0 + assert.Equal(t, true, dp.stopHostRulesReconcile == nil) + + time.Sleep(20 * time.Millisecond) + assert.Equal(t, 0, len(fakeTM.snapshotEvents())) + + // Stop must not hang waiting for a loop that was never started + dp.Stop(true) +} + +func TestHostRulesReconcileStopIsIdempotentWithSkipCleanup(t *testing.T) { + // With skipCleanup=true (upgrade) the loop must still be stopped first, and + // DeleteHostRules must not be invoked + fakeTM := &fakeTrafficRuleManager{ensureCh: make(chan struct{}, 64)} + dp := newReconcileTestDataplane(fakeTM, 5*time.Millisecond) + + dp.Start(context.Background()) + waitForEnsureCalls(t, fakeTM, 1) + dp.Stop(true) + + events := fakeTM.snapshotEvents() + for _, e := range events { + if e == "delete" { + t.Fatalf("DeleteHostRules must not be invoked when skipCleanup=true: %v", events) + } + } + before := len(events) + time.Sleep(30 * time.Millisecond) + assert.Equal(t, before, len(fakeTM.snapshotEvents())) +} diff --git a/cni/pkg/nodeagent/options.go b/cni/pkg/nodeagent/options.go index a9b2bf02b1..a6d2492517 100644 --- a/cni/pkg/nodeagent/options.go +++ b/cni/pkg/nodeagent/options.go @@ -16,6 +16,7 @@ package nodeagent import ( "net/netip" + "time" "istio.io/istio/cni/pkg/util" "istio.io/istio/pkg/config/constants" @@ -57,6 +58,9 @@ type AmbientArgs struct { DNSCapture bool EnableIPv6 bool ReconcilePodRulesOnStartup bool + // ReconcileHostRulesInterval is the interval of the periodic runtime reconciliation + // of the host-level health check rules, <= 0 disables it (istio/istio#60607) + ReconcileHostRulesInterval time.Duration NativeNftables bool ForceIptablesBinary string } diff --git a/cni/pkg/nodeagent/server_linux.go b/cni/pkg/nodeagent/server_linux.go index e96ec36ef7..20c3929eb1 100644 --- a/cni/pkg/nodeagent/server_linux.go +++ b/cni/pkg/nodeagent/server_linux.go @@ -140,10 +140,11 @@ func initMeshDataplane(client kube.Client, args AmbientArgs) (*meshDataplane, er netServer := newNetServer(ztunnelServer, podNsMap, podTrafficManager, podNetns) return &meshDataplane{ - kubeClient: client.Kube(), - netServer: netServer, - hostTrafficManager: hostTrafficManager, - hostAddrSet: setManager, + kubeClient: client.Kube(), + netServer: netServer, + hostTrafficManager: hostTrafficManager, + hostAddrSet: setManager, + hostRulesReconcileInterval: args.ReconcileHostRulesInterval, }, nil } diff --git a/cni/pkg/trafficmanager/interface.go b/cni/pkg/trafficmanager/interface.go index f4192bb20b..e160b875e9 100644 --- a/cni/pkg/trafficmanager/interface.go +++ b/cni/pkg/trafficmanager/interface.go @@ -28,6 +28,13 @@ type TrafficRuleManager interface { DeleteInpodRules(log *istiolog.Scope) error CreateHostRulesForHealthChecks() error DeleteHostRules() + // EnsureHostRules verifies that the host-level rules still match the desired state, + // and re-installs them if they have drifted (e.g. removed by a firewalld reload or + // an external iptables-restore). It is idempotent and meant to be invoked from a + // periodic reconcile loop. The repaired return value reports whether drift was + // detected and a repair was performed. When the state cannot be verified (e.g. a + // transient read failure) an error is returned without any repair being attempted. + EnsureHostRules() (repaired bool, err error) ReconcileModeEnabled() bool } diff --git a/cni/pkg/trafficmanager/iptables_manager_linux.go b/cni/pkg/trafficmanager/iptables_manager_linux.go index 67ff2b9c41..af15dec6c1 100644 --- a/cni/pkg/trafficmanager/iptables_manager_linux.go +++ b/cni/pkg/trafficmanager/iptables_manager_linux.go @@ -104,6 +104,15 @@ func (m *IptablesTrafficManager) DeleteHostRules() { } } +// EnsureHostRules verifies the host-level iptables rules and re-installs them on drift +// (idempotent, meant for the periodic reconcile loop) +func (m *IptablesTrafficManager) EnsureHostRules() (bool, error) { + if m.hostIptables == nil { + return false, fmt.Errorf("host iptables configurator not available (this is likely a pod-only traffic manager)") + } + return m.hostIptables.EnsureHostRules() +} + // ReconcileModeEnabled returns true if reconciliation mode is enabled func (m *IptablesTrafficManager) ReconcileModeEnabled() bool { if m.podIptables == nil { diff --git a/cni/pkg/trafficmanager/nftables_manager_linux.go b/cni/pkg/trafficmanager/nftables_manager_linux.go index c19d9772e2..b078da6c34 100644 --- a/cni/pkg/trafficmanager/nftables_manager_linux.go +++ b/cni/pkg/trafficmanager/nftables_manager_linux.go @@ -87,6 +87,15 @@ func (m *NftablesTrafficManager) DeleteHostRules() { } } +// EnsureHostRules verifies the host-level nftables rules and re-installs them on drift +// (idempotent, meant for the periodic reconcile loop) +func (m *NftablesTrafficManager) EnsureHostRules() (bool, error) { + if m.hostNftables == nil { + return false, fmt.Errorf("host nftables configurator not available (this is likely a pod-only traffic manager)") + } + return m.hostNftables.EnsureHostRules() +} + // ReconcileModeEnabled returns true if reconciliation mode is enabled func (m *NftablesTrafficManager) ReconcileModeEnabled() bool { if m.podNftables == nil { diff --git a/manifests/charts/istio-cni/templates/configmap-cni.yaml b/manifests/charts/istio-cni/templates/configmap-cni.yaml index 54631513bb..28d4ef9c93 100644 --- a/manifests/charts/istio-cni/templates/configmap-cni.yaml +++ b/manifests/charts/istio-cni/templates/configmap-cni.yaml @@ -19,6 +19,7 @@ data: AMBIENT_DNS_CAPTURE: {{ .Values.ambient.dnsCapture | quote }} AMBIENT_IPV6: {{ .Values.ambient.ipv6 | quote }} AMBIENT_RECONCILE_POD_RULES_ON_STARTUP: {{ .Values.ambient.reconcileIptablesOnStartup | quote }} + AMBIENT_RECONCILE_HOST_RULES_INTERVAL: {{ .Values.ambient.reconcileHostRulesInterval | quote }} ENABLE_AMBIENT_DETECTION_RETRY: {{ .Values.ambient.enableAmbientDetectionRetry | quote }} {{- if .Values.cniConfFileName }} # K8S < 1.24 doesn't like empty values CNI_CONF_NAME: {{ .Values.cniConfFileName }} # Name of the CNI config file to create. Only override if you know the exact path your CNI requires.. diff --git a/manifests/charts/istio-cni/values.yaml b/manifests/charts/istio-cni/values.yaml index 8cfd6595c5..18c78fd335 100644 --- a/manifests/charts/istio-cni/values.yaml +++ b/manifests/charts/istio-cni/values.yaml @@ -73,6 +73,12 @@ _internal_defaults_do_not_set: # If enabled, and ambient is enabled, the CNI agent will reconcile incompatible iptables rules and chains at startup. # This is enabled by default reconcileIptablesOnStartup: true + # If ambient is enabled, the CNI agent will periodically verify the host-level health check + # SNAT rules (iptables nat POSTROUTING -> ISTIO_POSTRT, or the istio-ambient-nat table with + # native nftables) at this interval, and re-install them if they were removed by an external + # actor (e.g. a firewalld reload or an iptables-restore from an OS persistence unit). + # Set to "0" to disable the periodic reconciliation. + reconcileHostRulesInterval: "30s" # If enabled, and ambient is enabled, the CNI agent will always share the network namespace of the host node it is running on shareHostNetworkNamespace: false # If enabled, the CNI agent will retry checking if a pod is ambient enabled when there are errors diff --git a/operator/pkg/apis/validation/validation_test.go b/operator/pkg/apis/validation/validation_test.go index 43f57dad4a..5fa72c7bd3 100644 --- a/operator/pkg/apis/validation/validation_test.go +++ b/operator/pkg/apis/validation/validation_test.go @@ -286,6 +286,15 @@ cni: ambient: enabled: true reconcileIptablesOnStartup: true +`, + }, + { + desc: "CNIReconcileHostRulesInterval", + yamlStr: ` +cni: + ambient: + enabled: true + reconcileHostRulesInterval: "30s" `, }, { diff --git a/operator/pkg/apis/values_types.pb.go b/operator/pkg/apis/values_types.pb.go index ff3ea016d9..300cd12ebc 100644 --- a/operator/pkg/apis/values_types.pb.go +++ b/operator/pkg/apis/values_types.pb.go @@ -754,6 +754,10 @@ type CNIAmbientConfig struct { Ipv6 *wrapperspb.BoolValue `protobuf:"bytes,7,opt,name=ipv6,proto3" json:"ipv6,omitempty"` // If enabled, and ambient is enabled, iptables reconciliation will be enabled. ReconcileIptablesOnStartup *wrapperspb.BoolValue `protobuf:"bytes,9,opt,name=reconcileIptablesOnStartup,proto3" json:"reconcileIptablesOnStartup,omitempty"` + // The interval at which the CNI agent periodically verifies the ambient host-level + // health check rules and re-installs them if they were removed by an external actor, + // as a duration string (e.g. "30s"). "0" disables the periodic reconciliation. + ReconcileHostRulesInterval string `protobuf:"bytes,10,opt,name=reconcileHostRulesInterval,proto3" json:"reconcileHostRulesInterval,omitempty"` unknownFields protoimpl.UnknownFields sizeCache protoimpl.SizeCache } @@ -823,6 +827,13 @@ func (x *CNIAmbientConfig) GetReconcileIptablesOnStartup() *wrapperspb.BoolValue return nil } +func (x *CNIAmbientConfig) GetReconcileHostRulesInterval() string { + if x != nil { + return x.ReconcileHostRulesInterval + } + return "" +} + type CNIRepairConfig struct { state protoimpl.MessageState `protogen:"open.v1"` // Controls whether repair behavior is enabled. @@ -5640,7 +5651,7 @@ const file_pkg_apis_values_types_proto_rawDesc = "" + "\x0eCNIUsageConfig\x124\n" + "\aenabled\x18\x01 \x01(\v2\x1a.google.protobuf.BoolValueR\aenabled\x128\n" + "\achained\x18\x02 \x01(\v2\x1a.google.protobuf.BoolValueB\x02\x18\x01R\achained\x12\x1a\n" + - "\bprovider\x18\x03 \x01(\tR\bprovider\"\xae\x02\n" + + "\bprovider\x18\x03 \x01(\tR\bprovider\"\xee\x02\n" + "\x10CNIAmbientConfig\x124\n" + "\aenabled\x18\x01 \x01(\v2\x1a.google.protobuf.BoolValueR\aenabled\x12\x1c\n" + "\tconfigDir\x18\x03 \x01(\tR\tconfigDir\x12:\n" + @@ -5648,7 +5659,9 @@ const file_pkg_apis_values_types_proto_rawDesc = "" + "dnsCapture\x18\x05 \x01(\v2\x1a.google.protobuf.BoolValueR\n" + "dnsCapture\x12.\n" + "\x04ipv6\x18\a \x01(\v2\x1a.google.protobuf.BoolValueR\x04ipv6\x12Z\n" + - "\x1areconcileIptablesOnStartup\x18\t \x01(\v2\x1a.google.protobuf.BoolValueR\x1areconcileIptablesOnStartup\"\xad\x03\n" + + "\x1areconcileIptablesOnStartup\x18\t \x01(\v2\x1a.google.protobuf.BoolValueR\x1areconcileIptablesOnStartup\x12>\n" + + "\x1areconcileHostRulesInterval\x18\n" + + " \x01(\tR\x1areconcileHostRulesInterval\"\xad\x03\n" + "\x0fCNIRepairConfig\x124\n" + "\aenabled\x18\x01 \x01(\v2\x1a.google.protobuf.BoolValueR\aenabled\x12\x10\n" + "\x03hub\x18\x02 \x01(\tR\x03hub\x12(\n" + diff --git a/operator/pkg/apis/values_types.proto b/operator/pkg/apis/values_types.proto index fbb286f092..eadcd9fa30 100644 --- a/operator/pkg/apis/values_types.proto +++ b/operator/pkg/apis/values_types.proto @@ -183,6 +183,11 @@ message CNIAmbientConfig { // If enabled, and ambient is enabled, iptables reconciliation will be enabled. google.protobuf.BoolValue reconcileIptablesOnStartup = 9; + + // The interval at which the CNI agent periodically verifies the ambient host-level + // health check rules and re-installs them if they were removed by an external actor, + // as a duration string (e.g. "30s"). "0" disables the periodic reconciliation. + string reconcileHostRulesInterval = 10; } message CNIRepairConfig { diff --git a/releasenotes/notes/ambient-host-rules-reconcile.yaml b/releasenotes/notes/ambient-host-rules-reconcile.yaml new file mode 100644 index 0000000000..41c1ca35a8 --- /dev/null +++ b/releasenotes/notes/ambient-host-rules-reconcile.yaml @@ -0,0 +1,18 @@ +apiVersion: release-notes/v2 +kind: feature +area: traffic-management + +issue: + - 60607 + +releaseNotes: + - | + **Added** periodic runtime reconciliation of the ambient host-level health check SNAT rules + (iptables `nat` `POSTROUTING` -> `ISTIO_POSTRT`, or the `istio-ambient-nat` table with + native nftables). If an external actor (e.g. a `firewalld` + reload or an `iptables-restore` from an OS persistence unit) removes these rules while + `istio-cni-node` is running, they are now automatically detected and re-installed, instead + of causing permanent probe failures under `STRICT` mTLS until the DaemonSet is restarted. + The interval can be configured with `cni.ambient.reconcileHostRulesInterval` (default `30s`, + set to `"0"` to disable). A new `nodeagent_host_rules_reconciles_total` metric reports + detected drift and reconcile failures. \ No newline at end of file diff --git a/tools/istio-nftables/pkg/builder/nftables_api.go b/tools/istio-nftables/pkg/builder/nftables_api.go index bf74d4e167..20197a2d11 100644 --- a/tools/istio-nftables/pkg/builder/nftables_api.go +++ b/tools/istio-nftables/pkg/builder/nftables_api.go @@ -29,6 +29,11 @@ type NftablesAPI interface { Dump(tx *knftables.Transaction) string // ListElements returns a list of the elements in a set or map. (objectType should be "set" or "map".) ListElements(ctx context.Context, objectType, name string) ([]*knftables.Element, error) + // ListRules returns a list of the rules in a chain. As with knftables, the returned + // Rule objects have their Chain, Comment and Handle fields filled in, but not the + // actual Rule text, and the underlying interface must have an associated + // family/table. + ListRules(ctx context.Context, chain string) ([]*knftables.Rule, error) } // NftImpl is the real implementation of NftablesAPI using the actual knftables backend. @@ -66,6 +71,11 @@ func (r *NftImpl) ListElements(ctx context.Context, objectType, name string) ([] return r.nft.ListElements(ctx, objectType, name) } +// ListRules returns a list of the rules in a chain using the real knftables interface. +func (r *NftImpl) ListRules(ctx context.Context, chain string) ([]*knftables.Rule, error) { + return r.nft.ListRules(ctx, chain) +} + // MockNftables is a mock implementation of NftablesAPI for use in unit tests. // It uses knftables.Fake to simulate nftables behavior without making changes to the system. type MockNftables struct {