kubernetes-sigs / ip-masq-agent

Manage IP masquerade on nodes
Apache License 2.0
217 stars 70 forks source link

Proposal: Read a second ConfigMap for cloud provider usage #69

Closed mattstam closed 2 years ago

mattstam commented 3 years ago

Problem:

AKS needs to continuously reconcile the NonMasqueradeCIDRs to stay in sync with all of the CIDRs in the VNET used by a cluster. Otherwise, internal traffic ends up getting improperly masqueraded.

However, AKS cannot reconcile the existing ConfigMap because it is also used for scenarios were custom ranges have been manually written, such as in the case of a peered VNET.

GKE also faces the same problem, forcing manual updating of this ConfigMap instead of being able to keep it continuously updated:

If you use ip-masq-agent configured with the nonMasqueradeCIDRs parameter, you must update the nonMasqueradeCIDRs to include all Pod CIDR ranges.

Proposal:

Addition bool flag --enable-cloud-config that, when true, will extend the behavior of the syncConfig function to read in a second ConfigMap with the same data keys.

When this second ConfigMap is read, the nonMasqueradeCIDRs list is merged with the nonMasqueradeCIDRs from the first. Additional options such as masqLinkLocal, such as could be ignored or OR'd with the first.

The second ConfigMap's purpose will be for cloud providers to reconcile and keep in-sync with virtual network CIDRs, while the first ConfigMap will remain as a way for custom ranges to be added.

bowei commented 3 years ago

Maybe it makes more sense to have a configurable list of configmaps to watch. Are there other semantics that we need to maintain for the merge?

mattstam commented 3 years ago

@bowei That could definitely work. Though I'm not sure there is currently a use-case were n ConfigMaps need to be merged compared to a custom one + reconcilable one.

For the exact semantics everything should be backward compatible with the current behavior, so to achieve this I suggest all new behavior is locked behind a new flag:

enableClougConfig = flag.Bool("enable-cloud-config", false, "Whether to read and merge an additional configmap")

and updating the const:

configPath = "/etc/config/ip-masq-agent

to:

configCustomPath = "/etc/config/ip-masq-agent"
configCloudPath = "/etc/config/ip-masq-agent-cloud"

When this is flag is enabled, the behavior of syncConfig is:

Open to a list of ConfigMaps to watch but I do think for simplicity we might be better off with the above. Ready to implement either if that sounds good to you.

mattstam commented 2 years ago

Any other thoughts on this @MrHohn or @thockin

mattstam commented 2 years ago

or @mtaufen

mattstam commented 2 years ago

AKS is going to do custom workaround to get around this limitation, though if other cloud providers face this issue feel free to re-open this.