Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kube-proxy: Optionally do privileged configs only #120864

Merged
merged 1 commit into from
Oct 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions cmd/kube-proxy/app/server.go
Expand Up @@ -108,6 +108,8 @@ type Options struct {
WriteConfigTo string
// CleanupAndExit, when true, makes the proxy server clean up iptables and ipvs rules, then exit.
CleanupAndExit bool
// InitAndExit, when true, makes the proxy server makes configurations that need privileged access, then exit.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: possibly typo here?

- InitAndExit, when true, makes the proxy server makes configurations that need privileged access, then exit.
+ InitAndExit, when true, makes the proxy server execute configurations that need privileged access, then exit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lars meant "it makes the proxy server make changes to the node configuration that need privileges", whereas I think you're interpreting it as "it makes the proxy server process the kube-proxy configuration options that need privileges".

Maybe just copy the text from the CLI flag below: "InitAndExit, when true, makes kube-proxy perform any initialization steps that must be done with full root privileges, and then exit"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it matters much. People that are interested in this function will get it anyway, so I let it be

InitAndExit bool
// WindowsService should be set to true if kube-proxy is running as a service on Windows.
// Its corresponding flag only gets registered in Windows builds
WindowsService bool
Expand Down Expand Up @@ -168,7 +170,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
"The purpose of this format is make sure you have the opportunity to notice if the next release hides additional metrics, "+
"rather than being surprised when they are permanently removed in the release after that. "+
"This parameter is ignored if a config file is specified by --config.")

fs.BoolVar(&o.InitAndExit, "init-only", o.InitAndExit, "If true, perform any initialization steps that must be done with full root privileges, and then exit. After doing this, you can run kube-proxy again with only the CAP_NET_ADMIN capability.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is mentioned in kubernetes/website#43680, but I wonder if it makes sense to mention here that the changes done by privileged initialization step will persist, and therefore running kube-proxy again without permissions will work? Something like:

- If true, perform any initialization steps that must be done with full root privileges,
+ If true, perform and persist any initialization steps that must be done with full root privileges,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as previous comment.

fs.Var(&o.config.Mode, "proxy-mode", "Which proxy mode to use: on Linux this can be 'iptables' (default) or 'ipvs'. On Windows the only supported value is 'kernelspace'."+
"This parameter is ignored if a config file is specified by --config.")

Expand Down Expand Up @@ -376,10 +378,13 @@ func (o *Options) Run() error {
return cleanupAndExit()
}

proxyServer, err := newProxyServer(o.config, o.master)
proxyServer, err := newProxyServer(o.config, o.master, o.InitAndExit)
if err != nil {
return err
}
if o.InitAndExit {
return nil
}

o.proxyServer = proxyServer
return o.runLoop()
Expand Down Expand Up @@ -589,7 +594,7 @@ type ProxyServer struct {
}

// newProxyServer creates a ProxyServer based on the given config
func newProxyServer(config *kubeproxyconfig.KubeProxyConfiguration, master string) (*ProxyServer, error) {
func newProxyServer(config *kubeproxyconfig.KubeProxyConfiguration, master string, initOnly bool) (*ProxyServer, error) {
s := &ProxyServer{Config: config}

cz, err := configz.New(kubeproxyconfig.GroupName)
Expand Down Expand Up @@ -653,7 +658,7 @@ func newProxyServer(config *kubeproxyconfig.KubeProxyConfiguration, master strin
klog.ErrorS(err, "Kube-proxy configuration may be incomplete or incorrect")
}

s.Proxier, err = s.createProxier(config, dualStackSupported)
s.Proxier, err = s.createProxier(config, dualStackSupported, initOnly)
if err != nil {
return nil, err
}
Expand Down
6 changes: 5 additions & 1 deletion cmd/kube-proxy/app/server_others.go
Expand Up @@ -125,7 +125,7 @@ func (s *ProxyServer) platformCheckSupported() (ipv4Supported, ipv6Supported, du
}

// createProxier creates the proxy.Provider
func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguration, dualStack bool) (proxy.Provider, error) {
func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguration, dualStack, initOnly bool) (proxy.Provider, error) {
var proxier proxy.Provider
var err error

Expand Down Expand Up @@ -175,6 +175,7 @@ func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguratio
s.Recorder,
s.HealthzServer,
config.NodePortAddresses,
initOnly,
)
} else {
// Create a single-stack proxier if and only if the node does not support dual-stack (i.e, no iptables support).
Expand All @@ -201,6 +202,7 @@ func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguratio
s.Recorder,
s.HealthzServer,
config.NodePortAddresses,
initOnly,
)
}

Expand Down Expand Up @@ -247,6 +249,7 @@ func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguratio
config.IPVS.Scheduler,
config.NodePortAddresses,
kernelHandler,
initOnly,
)
} else {
var localDetector proxyutiliptables.LocalTrafficDetector
Expand Down Expand Up @@ -279,6 +282,7 @@ func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguratio
config.IPVS.Scheduler,
config.NodePortAddresses,
kernelHandler,
initOnly,
)
}
if err != nil {
Expand Down
5 changes: 4 additions & 1 deletion cmd/kube-proxy/app/server_windows.go
Expand Up @@ -79,7 +79,10 @@ func (s *ProxyServer) platformCheckSupported() (ipv4Supported, ipv6Supported, du
}

// createProxier creates the proxy.Provider
func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguration, dualStackMode bool) (proxy.Provider, error) {
func (s *ProxyServer) createProxier(config *proxyconfigapi.KubeProxyConfiguration, dualStackMode, initOnly bool) (proxy.Provider, error) {
if initOnly {
return nil, fmt.Errorf("--init-only is not implemented on Windows")
}
var healthzPort int
if len(config.HealthzBindAddress) > 0 {
_, port, _ := net.SplitHostPort(config.HealthzBindAddress)
Expand Down
14 changes: 12 additions & 2 deletions pkg/proxy/iptables/proxier.go
Expand Up @@ -233,6 +233,7 @@ func NewProxier(ipFamily v1.IPFamily,
recorder events.EventRecorder,
healthzServer *healthcheck.ProxierHealthServer,
nodePortAddressStrings []string,
initOnly bool,
) (*Proxier, error) {
nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings)

Expand All @@ -257,6 +258,11 @@ func NewProxier(ipFamily v1.IPFamily,
klog.InfoS("nf_conntrack_tcp_be_liberal set, not installing DROP rules for INVALID packets")
}

if initOnly {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, we only have to plumb the initOnly just for nf_conntrack_tcp_be_liberal and sysctlRouteLocalnet in the iptables mode :/

Copy link
Contributor Author

@uablrek uablrek Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is not, and shouldn't be, any difference between start with --init-only up until Run() is called. The rationale is here #120864 (comment).

klog.InfoS("System initialized and --init-only specified")
return nil, nil
}

// Generate the masquerade mark to use for SNAT rules.
masqueradeValue := 1 << uint(masqueradeBit)
masqueradeMark := fmt.Sprintf("%#08x", masqueradeValue)
Expand Down Expand Up @@ -330,21 +336,25 @@ func NewDualStackProxier(
recorder events.EventRecorder,
healthzServer *healthcheck.ProxierHealthServer,
nodePortAddresses []string,
initOnly bool,
) (proxy.Provider, error) {
// Create an ipv4 instance of the single-stack proxier
ipv4Proxier, err := NewProxier(v1.IPv4Protocol, ipt[0], sysctl,
exec, syncPeriod, minSyncPeriod, masqueradeAll, localhostNodePorts, masqueradeBit, localDetectors[0], hostname,
nodeIPs[v1.IPv4Protocol], recorder, healthzServer, nodePortAddresses)
nodeIPs[v1.IPv4Protocol], recorder, healthzServer, nodePortAddresses, initOnly)
if err != nil {
return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err)
}

ipv6Proxier, err := NewProxier(v1.IPv6Protocol, ipt[1], sysctl,
exec, syncPeriod, minSyncPeriod, masqueradeAll, false, masqueradeBit, localDetectors[1], hostname,
nodeIPs[v1.IPv6Protocol], recorder, healthzServer, nodePortAddresses)
nodeIPs[v1.IPv6Protocol], recorder, healthzServer, nodePortAddresses, initOnly)
if err != nil {
return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err)
}
if initOnly {
return nil, nil
}
return metaproxier.NewMetaProxier(ipv4Proxier, ipv6Proxier), nil
}

Expand Down
14 changes: 12 additions & 2 deletions pkg/proxy/ipvs/proxier.go
Expand Up @@ -340,6 +340,7 @@ func NewProxier(ipFamily v1.IPFamily,
scheduler string,
nodePortAddressStrings []string,
kernelHandler KernelHandler,
initOnly bool,
) (*Proxier, error) {
// Set the conntrack sysctl we need for
if err := proxyutil.EnsureSysctl(sysctl, sysctlVSConnTrack, 1); err != nil {
Expand Down Expand Up @@ -402,6 +403,11 @@ func NewProxier(ipFamily v1.IPFamily,
}
}

if initOnly {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have a comment here (and in iptables/proxies.go) that anything privileged should come before this point?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it might, but I leave it for now just to get it done 😃

klog.InfoS("System initialized and --init-only specified")
return nil, nil
}

// Generate the masquerade mark to use for SNAT rules.
masqueradeValue := 1 << uint(masqueradeBit)
masqueradeMark := fmt.Sprintf("%#08x", masqueradeValue)
Expand Down Expand Up @@ -490,6 +496,7 @@ func NewDualStackProxier(
scheduler string,
nodePortAddresses []string,
kernelHandler KernelHandler,
initOnly bool,
) (proxy.Provider, error) {

safeIpset := newSafeIpset(ipset)
Expand All @@ -499,7 +506,7 @@ func NewDualStackProxier(
exec, syncPeriod, minSyncPeriod, filterCIDRs(false, excludeCIDRs), strictARP,
tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit,
localDetectors[0], hostname, nodeIPs[v1.IPv4Protocol],
recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler)
recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler, initOnly)
if err != nil {
return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err)
}
Expand All @@ -508,10 +515,13 @@ func NewDualStackProxier(
exec, syncPeriod, minSyncPeriod, filterCIDRs(true, excludeCIDRs), strictARP,
tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit,
localDetectors[1], hostname, nodeIPs[v1.IPv6Protocol],
recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler)
recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler, initOnly)
if err != nil {
return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err)
}
if initOnly {
return nil, nil
}

// Return a meta-proxier that dispatch calls between the two
// single-stack proxier instances
Expand Down
1 change: 1 addition & 0 deletions pkg/proxy/kubemark/hollow_proxy.go
Expand Up @@ -109,6 +109,7 @@ func NewHollowProxyOrDie(
recorder,
nil,
[]string{},
false,
)
if err != nil {
return nil, fmt.Errorf("unable to create proxier: %v", err)
Expand Down