Remove usage of global options from iptables cell #29088

pippolo84 · 2023-11-09T16:42:00Z

Remove usage of option.Config in iptables cell. These are just leftover after the cell has been modularized.

The option prepend-iptables-chains used to have an env var alias named CILIUM_PREPEND_IPTABLES_CHAIN. It was there to ensure backward compatibility. But the alias is exactly the same string as the usual name for the option (prefix is "CILIUM_" and option name is "PREPEND_IPTABLES_CHAIN"). In other words, there was actually no alias.
Thus it is safe to move the option in the cell.Config(), even if hive package does not support additional aliasies for the same config option.
CILIUM_PREPEND_IPTABLES_CHAIN alias has been deprecated with a warning and a note in the v1.15 Upgrade Notes. Read this comment for more info.

~~Depends on: #28746~~

pippolo84 · 2023-11-14T08:46:12Z

/test

thorn3r

changes look fairly straight-forward, but looks like there's quite a few tests failing

pippolo84 · 2023-11-14T15:44:26Z

changes look fairly straight-forward, but looks like there's quite a few tests failing

Missed the copy of the cell config in newIptablesManager, added a commit (cd09780) to fix it.

pippolo84 · 2023-11-14T15:48:07Z

/test

asauber

Re-ran failing clustermesh test

asauber

Hey @pippolo84, would you be able to link to some documentation (or code) that indicates that the env var alias would be preserved?

Alternatively, can you provide some interactive output when running the agent which shows that it is used with the new implementation?

pippolo84 · 2023-11-15T11:12:56Z

Hey @pippolo84, would you be able to link to some documentation (or code) that indicates that the env var alias would be preserved?

Alternatively, can you provide some interactive output when running the agent which shows that it is used with the new implementation?

Turned out that I was wrong. The alias is there for a reason.
The old code binds the env var CILIUM_PREPEND_IPTABLES_CHAIN (note the missing trailing S in the word CHAIN) using:

option.BindEnvWithLegacyEnvFallback(vp, option.PrependIptablesChainsName, "CILIUM_PREPEND_IPTABLES_CHAIN")

While, moving the option to be part of the iptables cell config, the hive package binds the env var here:

cilium/pkg/hive/hive.go

Line 136 in 32fbb7f

if err := h.viper.BindEnv(f.Name, h.getEnvName(f.Name)); err != nil {

and it builds the env var name to bind to from the option name, so prepend-iptables-chains (with the trailing S).

Anyway, I've discussed it offline with @aanm and setting this option through the fallback env var can be deprecated. I'll add a warning at the start of the cell and a line in the update notes.

pippolo84 · 2023-11-15T11:44:47Z

/test

learnitall

Small nit on the upgrade notes, but otherwise LGTM.

Documentation/operations/upgrade.rst

thorn3r

aside from the open conversation, LGTM!

learnitall

Looks good for docs, thanks!

asauber · 2023-11-16T22:14:56Z

Sounds good to me, and thanks for the link

pippolo84 · 2023-11-18T10:32:58Z

/test

pippolo84 · 2023-11-23T10:26:09Z

@ldelossa gentle ping 🙏

ldelossa

Sorry for delay, LGTM :)

Copy the cell specific config parameters passed through dependency injection into the manager struct. Fixes: a6a2b73 ("iptables: Add a cell for iptables config manager") Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

Since iptables manager has been modularized, it should not depend directly on the global daemon config. Instead, it should read the configuration from its cell. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

prepend-iptables-chains is used to enable prepending iptables chains instead of appending, and it is relevant only for the iptables manager. Since the iptables manager has been modularized, let's move this into the cell "private" config. The option used to have an env var alias named `CILIUM_PREPEND_IPTABLES_CHAIN`, for backward compatibility reason. The alias has been deprecated and a warning will be printed in case that env var will be found not empty at iptables cell startup. Besides, the env var deprecation has been described in the v1.15 upgrade notes. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

iptables manager has been modularized into a cell, so the tests should config it through the fields in the manager struct itself, not through the global config options. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 · 2023-11-30T16:52:00Z

/test

pippolo84 added release-note/misc This PR makes changes that have no direct user impact. dont-merge/blocked Another PR must be merged before this one. area/modularization Relates to code modularization and maintenance. labels Nov 9, 2023

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from a8ae5e9 to aed37fb Compare November 10, 2023 16:58

pippolo84 removed the dont-merge/blocked Another PR must be merged before this one. label Nov 10, 2023

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from aed37fb to 52e7575 Compare November 10, 2023 17:00

pippolo84 marked this pull request as ready for review November 10, 2023 17:51

pippolo84 requested review from a team as code owners November 10, 2023 17:51

pippolo84 requested review from thorn3r, asauber and ldelossa November 10, 2023 17:51

pippolo84 mentioned this pull request Nov 10, 2023

iptables: Read devices from Table[*Device] #29112

Closed

thorn3r reviewed Nov 14, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from 52e7575 to 442d43f Compare November 14, 2023 15:42

asauber reviewed Nov 14, 2023

View reviewed changes

asauber self-requested a review November 14, 2023 18:09

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from 442d43f to e312678 Compare November 15, 2023 11:29

pippolo84 requested a review from a team as a code owner November 15, 2023 11:29

pippolo84 requested a review from learnitall November 15, 2023 11:29

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch 2 times, most recently from fc27e64 to b89d746 Compare November 15, 2023 11:40

pippolo84 requested a review from thorn3r November 15, 2023 11:44

learnitall suggested changes Nov 15, 2023

View reviewed changes

Documentation/operations/upgrade.rst Outdated Show resolved Hide resolved

thorn3r approved these changes Nov 15, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from b89d746 to 3150137 Compare November 16, 2023 09:26

pippolo84 requested a review from learnitall November 16, 2023 09:27

learnitall approved these changes Nov 16, 2023

View reviewed changes

asauber approved these changes Nov 16, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from 3150137 to 3e25d62 Compare November 18, 2023 10:32

ldelossa approved these changes Nov 30, 2023

View reviewed changes

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from 3e25d62 to 55e9665 Compare November 30, 2023 16:50

pippolo84 added 4 commits November 30, 2023 17:51

iptables: Copy config at cell startup

a53d1d4

Copy the cell specific config parameters passed through dependency injection into the manager struct. Fixes: a6a2b73 ("iptables: Add a cell for iptables config manager") Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

iptables: Remove usage of global option EnableIPv6

6b9581c

Since iptables manager has been modularized, it should not depend directly on the global daemon config. Instead, it should read the configuration from its cell. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

iptables: Use cell config in iptables tests

6610b5c

iptables manager has been modularized into a cell, so the tests should config it through the fields in the manager struct itself, not through the global config options. Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

pippolo84 force-pushed the pr/pippolo84/iptables-global-cfg-leftover branch from 55e9665 to 6610b5c Compare November 30, 2023 16:51

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 30, 2023

aanm added this pull request to the merge queue Dec 1, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Dec 1, 2023

aanm added this pull request to the merge queue Dec 1, 2023

Merged via the queue into cilium:main with commit f908909 Dec 1, 2023

Remove usage of global options from iptables cell #29088

Remove usage of global options from iptables cell #29088

Uh oh!

Conversation

pippolo84 commented Nov 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pippolo84 commented Nov 14, 2023

Uh oh!

thorn3r left a comment

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Nov 14, 2023

Uh oh!

pippolo84 commented Nov 14, 2023

Uh oh!

asauber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asauber left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Nov 15, 2023

Uh oh!

pippolo84 commented Nov 15, 2023

Uh oh!

learnitall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thorn3r left a comment

Choose a reason for hiding this comment

Uh oh!

learnitall left a comment

Choose a reason for hiding this comment

Uh oh!

asauber commented Nov 16, 2023

Uh oh!

pippolo84 commented Nov 18, 2023

Uh oh!

pippolo84 commented Nov 23, 2023

Uh oh!

ldelossa left a comment

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Nov 30, 2023

Uh oh!

Uh oh!

Uh oh!

pippolo84 commented Nov 9, 2023 •

edited

Loading

asauber left a comment •

edited

Loading

asauber left a comment •

edited

Loading