-
Notifications
You must be signed in to change notification settings - Fork 366
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
Add OFSwitch connection check to Agent's liveness probes #4126
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
"k8s.io/apimachinery/pkg/fields" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
"k8s.io/apimachinery/pkg/util/wait" | ||
"k8s.io/apiserver/pkg/server/options" | ||
"k8s.io/client-go/informers" | ||
coreinformers "k8s.io/client-go/informers/core/v1" | ||
"k8s.io/client-go/tools/cache" | ||
|
@@ -71,7 +72,6 @@ import ( | |
"antrea.io/antrea/pkg/ovs/ovsctl" | ||
"antrea.io/antrea/pkg/signals" | ||
"antrea.io/antrea/pkg/util/channel" | ||
"antrea.io/antrea/pkg/util/cipher" | ||
"antrea.io/antrea/pkg/util/env" | ||
"antrea.io/antrea/pkg/util/k8s" | ||
"antrea.io/antrea/pkg/version" | ||
|
@@ -748,25 +748,27 @@ func run(o *Options) error { | |
|
||
go agentMonitor.Run(stopCh) | ||
|
||
cipherSuites, err := cipher.GenerateCipherSuitesList(o.config.TLSCipherSuites) | ||
if err != nil { | ||
return fmt.Errorf("error generating Cipher Suite list: %v", err) | ||
} | ||
bindAddress := net.IPv4zero | ||
if o.nodeType == config.ExternalNode { | ||
bindAddress = ipv4Localhost | ||
} | ||
secureServing := options.NewSecureServingOptions().WithLoopback() | ||
secureServing.BindAddress = bindAddress | ||
secureServing.BindPort = o.config.APIPort | ||
secureServing.CipherSuites = o.tlsCipherSuites | ||
secureServing.MinTLSVersion = o.config.TLSMinVersion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In original code, secureServing.MinTLSVersion should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are different. The original code sets the Basically the previous code passed empty options when creating APIServer, translated the options itself with duplicate code in Antrea, and mutated APIServer directly. |
||
authentication := options.NewDelegatingAuthenticationOptions() | ||
authorization := options.NewDelegatingAuthorizationOptions().WithAlwaysAllowPaths("/healthz", "/livez", "/readyz") | ||
apiServer, err := apiserver.New( | ||
agentQuerier, | ||
networkPolicyController, | ||
mcastController, | ||
externalIPController, | ||
bindAddress, | ||
o.config.APIPort, | ||
secureServing, | ||
authentication, | ||
authorization, | ||
*o.config.EnablePrometheusMetrics, | ||
o.config.ClientConnection.Kubeconfig, | ||
cipherSuites, | ||
cipher.TLSVersionMap[o.config.TLSMinVersion], | ||
v4Enabled, | ||
v6Enabled) | ||
if err != nil { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
"github.com/spf13/pflag" | ||
"gopkg.in/yaml.v2" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
cliflag "k8s.io/component-base/cli/flag" | ||
"k8s.io/component-base/featuregate" | ||
"k8s.io/klog/v2" | ||
|
||
|
@@ -61,6 +62,8 @@ type Options struct { | |
configFile string | ||
// The configuration object | ||
config *agentconfig.AgentConfig | ||
// tlsCipherSuites is a slice of TLSCipherSuites mapped to input provided by user. | ||
tlsCipherSuites []string | ||
// IPFIX flow collector address | ||
flowCollectorAddr string | ||
// IPFIX flow collector protocol | ||
|
@@ -124,6 +127,10 @@ func (o *Options) validate(args []string) error { | |
klog.Info("OVS 'netdev' datapath is not fully supported at the moment") | ||
} | ||
|
||
if err := o.validateTLSOptions(); err != nil { | ||
return err | ||
} | ||
|
||
if config.ExternalNode.String() == o.config.NodeType && !features.DefaultFeatureGate.Enabled(features.ExternalNode) { | ||
return fmt.Errorf("nodeType %s requires feature gate ExternalNode to be enabled", o.config.NodeType) | ||
} | ||
|
@@ -171,6 +178,23 @@ func (o *Options) setDefaults() { | |
} | ||
} | ||
|
||
func (o *Options) validateTLSOptions() error { | ||
_, err := cliflag.TLSVersion(o.config.TLSMinVersion) | ||
if err != nil { | ||
return fmt.Errorf("invalid TLSMinVersion: %v", err) | ||
} | ||
trimmedTLSCipherSuites := strings.ReplaceAll(o.config.TLSCipherSuites, " ", "") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 186-192 is similar to cipher.GenerateCipherSuitesList(), the difference is new code has a check with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As explained above, the code in
|
||
if trimmedTLSCipherSuites != "" { | ||
tlsCipherSuites := strings.Split(trimmedTLSCipherSuites, ",") | ||
_, err = cliflag.TLSCipherSuites(tlsCipherSuites) | ||
if err != nil { | ||
return fmt.Errorf("invalid TLSCipherSuites: %v", err) | ||
} | ||
o.tlsCipherSuites = tlsCipherSuites | ||
} | ||
return nil | ||
} | ||
|
||
func (o *Options) validateAntreaProxyConfig() error { | ||
if !features.DefaultFeatureGate.Enabled(features.AntreaProxy) { | ||
// Validate service CIDR configuration if AntreaProxy is not enabled. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
// Copyright 2022 Antrea 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 main | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
|
||
agentconfig "antrea.io/antrea/pkg/config/agent" | ||
) | ||
|
||
func TestOptionsValidateTLSOptions(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
config *agentconfig.AgentConfig | ||
expectedErr string | ||
}{ | ||
{ | ||
name: "empty input", | ||
config: &agentconfig.AgentConfig{ | ||
TLSCipherSuites: "", | ||
TLSMinVersion: "", | ||
}, | ||
expectedErr: "", | ||
}, | ||
{ | ||
name: "invalid TLSMinVersion", | ||
config: &agentconfig.AgentConfig{ | ||
TLSCipherSuites: "", | ||
TLSMinVersion: "foo", | ||
}, | ||
expectedErr: "invalid TLSMinVersion", | ||
}, | ||
{ | ||
name: "invalid TLSCipherSuites", | ||
config: &agentconfig.AgentConfig{ | ||
TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, foo", | ||
TLSMinVersion: "VersionTLS10", | ||
}, | ||
expectedErr: "invalid TLSCipherSuites", | ||
}, | ||
{ | ||
name: "valid input", | ||
config: &agentconfig.AgentConfig{ | ||
TLSCipherSuites: "TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305, TLS_RSA_WITH_AES_128_GCM_SHA256", | ||
TLSMinVersion: "VersionTLS12", | ||
}, | ||
expectedErr: "", | ||
}, | ||
} | ||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
o := &Options{config: tt.config} | ||
err := o.validateTLSOptions() | ||
if tt.expectedErr == "" { | ||
assert.NoError(t, err) | ||
} else { | ||
assert.ErrorContains(t, err, tt.expectedErr) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question - why we increase it to 10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It typically takes more than 2 seconds to connect to OVS from agent logs in my testbed. I think 5 seconds may cause a liveness failure at startup in production clusters when nodes are under resource pressure. The liveness is a backup solution just in case reconnection doesn't work as expected so it doesn't need to be too aggresive.
Besides, all Kubernetes components use 10 seconds initialDelaySeconds.