Repository: kubernetes
Issue: generic vs platform/backend-specific options in kube-proxy
Labels: sig/network,area/kube-proxy,area/ipvs,triage/accepted

There was some discussion in #117297 about generic vs platform-specific options, especially around `--detect-local-mode`, which can be specified on Windows even though it doesn't actually make sense there. (Or at least, it's not _implemented_ there... I'm not sure if winkernel would benefit from it or not?)

Current version of kube-proxy config:

```
kind: KubeProxyConfiguration
apiVersion: kubeproxy.config.k8s.io/v1alpha1
bindAddress: 0.0.0.0
bindAddressHardFail: false
clientConnection:
  acceptContentTypes: ""
  burst: 10
  contentType: application/vnd.kubernetes.protobuf
  kubeconfig: ""
  qps: 5
clusterCIDR: ""
configSyncPeriod: 15m0s
conntrack:
  maxPerCore: 32768
  min: 131072
  tcpCloseWaitTimeout: 1h0m0s
  tcpEstablishedTimeout: 24h0m0s
detectLocal:
  bridgeInterface: ""
  interfaceNamePrefix: ""
detectLocalMode: ""
enableProfiling: false
healthzBindAddress: 0.0.0.0:10256
hostnameOverride: ""
iptables:
  localhostNodePorts: true
  masqueradeAll: false
  masqueradeBit: 14
  minSyncPeriod: 1s
  syncPeriod: 30s
ipvs:
  excludeCIDRs: null
  minSyncPeriod: 0s
  scheduler: ""
  strictARP: false
  syncPeriod: 30s
  tcpFinTimeout: 0s
  tcpTimeout: 0s
  udpTimeout: 0s
metricsBindAddress: 127.0.0.1:10249
mode: ""
nodePortAddresses: null
oomScoreAdj: -999
portRange: ""
showHiddenMetricsForVersion: ""
winkernel:
  enableDSR: false
  forwardHealthCheckVip: false
  networkName: ""
  rootHnsEndpointName: ""
  sourceVip: ""
```

So, things that are maybe mis-organized:

- `conntrack`: Platform-specific to Linux. (Ignored on Windows.) On Linux, it potentially has backend-specific _relevance_ (eg, it might not be important to a Cilium-like backend) but the actual implementation is in `cmd/kube-proxy` and is not backend-specific.
- `detectLocalMode`/`detectLocal`: Theoretically platform-agnostic but currently Linux-specific (and even iptables-specific) in implementation. (Ignored on Windows.) If we added Windows support we might need a different set of modes. For the future `nftables` backend, we will probably want the same modes as we have for `iptables` and `ipvs`, though we will need an nftables-based implementation of each one.
  - `clusterCIDR`: only used with `detectLocalMode: ClusterCIDR`, but for historical reasons is not part of the `detectLocal` block.
- `syncPeriod`/`minSyncPeriod`: Currently backend-specific (with separate `iptables` and `ipvs` versions), but they are used by all three existing backends in the same way (primarily for constructing a BoundedFrequencyRunner). (There is no `winkernel`-specific version of those options; the winkernel code actually uses the _iptables_ `syncPeriod`/`minSyncPeriod`...) In https://github.com/kubernetes/kubernetes/pull/117436/commits/eacbf32344ec11e3da31c44541453b4ce972b5b9 in my refactoring branch, I make those options be backend-agnostic (with the BFR being constructed in backend-agnostic code), so I was thinking that we should deprecate the backend-specific versions and replace them with top-level options (along with the already-backend-agnostic `configSyncPeriod`).
  - [EDIT] Also, we currently use `iptables.syncPeriod` to configure the healthz server regardless of backend.
- `masqueradeAll`: Currently marked as an `iptables` option, but is used by both `iptables` and `ipvs`. AIUI, this is needed with some CNI plugins where replies to pod-to-service-IP-to-pod traffic would otherwise not get routed back through the host network namespace to get un-DNATted. Assuming such network plugins still exist, `masqueradeAll` would still be needed with `nftables` as well. Although at a high-level there is nothing Linux platform-specific about this, it may be addressing a problem that doesn't exist with Windows Kubernetes networking?
- `masqueradeBit`: Platform-specific to Linux. Currently marked as an `iptables` option, but is used by both `iptables` and `ipvs`. The current plan for `nftables` should _not_ need this.
- `oomScoreAdj`: Platform-specific to Linux. (Will cause a non-fatal error to be logged if set on Windows.)
- `portRange`: Backend-specific to `userspace` but not labeled as such and so we forgot to remove it. Oops.

----

So... first question: does it matter? Do we want to try to make this better?

If yes, should we create a `v1alpha2` or `v1beta1` version of the config file?

Should we have a `linux:` subsection for `conntrack` and `oomScoreAdj`?

Should we make `detectLocalMode` be Linux-specific, or alternatively, should we make it into separate/redundant options under `iptables:` and `ipvs:` (and `nftables:`)?

Should we make `syncPeriod`/`minSyncPeriod` be platform-agnostic? (If not, then presumably we should add `winkernel.syncPeriod` and `winkernel.minSyncPeriod` rather than having it use the iptables ones.)

Should we make `iptables.masqueradeAll` be either platform-agnostic or Linux-specific rather than iptables-specific? (It seems like the problem it fixes doesn't exist on Windows...)

Should we add `ipvs.masqueradeBit` so the ipvs backend doesn't have to use the iptables option?

Should we drop `portRange` since it was only used by `userspace`? (Can we do that in `v1alpha1`? We dropped some other userspace-only options before, we just missed this one...)

/sig network
/area kube-proxy
/area ipvs
cc @thockin @aojea 
cc @MikeZappa87 for Windows thoughts