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
Fix default auditing options. #60739
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 |
---|---|---|
|
@@ -116,15 +116,15 @@ func NewAuditOptions() *AuditOptions { | |
WebhookOptions: AuditWebhookOptions{ | ||
BatchOptions: AuditBatchOptions{ | ||
Mode: ModeBatch, | ||
BatchConfig: defaultLogBatchConfig, | ||
BatchConfig: pluginbuffered.NewDefaultBatchConfig(), | ||
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. @crassirostris please confirm that these settings are what we want. 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. Yes, though it's preferrable to merge this PR after #60926, to avoid showing regressions in the scale tests |
||
}, | ||
InitialBackoff: pluginwebhook.DefaultInitialBackoff, | ||
}, | ||
LogOptions: AuditLogOptions{ | ||
Format: pluginlog.FormatJson, | ||
BatchOptions: AuditBatchOptions{ | ||
Mode: ModeBatch, | ||
BatchConfig: pluginbuffered.NewDefaultBatchConfig(), | ||
Mode: ModeBlocking, | ||
BatchConfig: defaultLogBatchConfig, | ||
}, | ||
}, | ||
} | ||
|
@@ -145,46 +145,10 @@ func (o *AuditOptions) Validate() []error { | |
if len(o.WebhookOptions.ConfigFile) > 0 { | ||
allErrors = append(allErrors, fmt.Errorf("feature '%s' must be enabled to set option --audit-webhook-config-file", features.AdvancedAuditing)) | ||
} | ||
} else { | ||
// check webhook configuration | ||
if err := validateBackendMode(pluginwebhook.PluginName, o.WebhookOptions.BatchOptions.Mode); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
if err := validateBackendBatchConfig(pluginwebhook.PluginName, o.LogOptions.BatchOptions.BatchConfig); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
|
||
// check log configuration | ||
if err := validateBackendMode(pluginlog.PluginName, o.LogOptions.BatchOptions.Mode); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
if err := validateBackendBatchConfig(pluginlog.PluginName, o.LogOptions.BatchOptions.BatchConfig); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
|
||
// Check log format | ||
validFormat := false | ||
for _, f := range pluginlog.AllowedFormats { | ||
if f == o.LogOptions.Format { | ||
validFormat = true | ||
break | ||
} | ||
} | ||
if !validFormat { | ||
allErrors = append(allErrors, fmt.Errorf("invalid audit log format %s, allowed formats are %q", o.LogOptions.Format, strings.Join(pluginlog.AllowedFormats, ","))) | ||
} | ||
} | ||
|
||
// Check validities of MaxAge, MaxBackups and MaxSize of log options | ||
if o.LogOptions.MaxAge < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxage %v can't be a negative number", o.LogOptions.MaxAge)) | ||
} | ||
if o.LogOptions.MaxBackups < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxbackup %v can't be a negative number", o.LogOptions.MaxBackups)) | ||
} | ||
if o.LogOptions.MaxSize < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxsize %v can't be a negative number", o.LogOptions.MaxSize)) | ||
} | ||
allErrors = append(allErrors, o.LogOptions.Validate()...) | ||
allErrors = append(allErrors, o.WebhookOptions.Validate()...) | ||
|
||
return allErrors | ||
} | ||
|
@@ -198,7 +162,15 @@ func validateBackendMode(pluginName string, mode string) error { | |
return fmt.Errorf("invalid audit %s mode %s, allowed modes are %q", pluginName, mode, strings.Join(AllowedModes, ",")) | ||
} | ||
|
||
func validateBackendBatchConfig(pluginName string, config pluginbuffered.BatchConfig) error { | ||
func validateBackendBatchOptions(pluginName string, options AuditBatchOptions) error { | ||
if err := validateBackendMode(pluginName, options.Mode); err != nil { | ||
return err | ||
} | ||
if options.Mode != ModeBatch { | ||
// Don't validate the unused options. | ||
return nil | ||
} | ||
config := options.BatchConfig | ||
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. What will happen if --audit-log-batch-max-wait is set to zero? 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. It will hotloop. Not great, but not exactly invalid. I'd prefer to leave it as-is here. |
||
if config.BufferSize <= 0 { | ||
return fmt.Errorf("invalid audit batch %s buffer size %v, must be a positive number", pluginName, config.BufferSize) | ||
} | ||
|
@@ -317,8 +289,52 @@ func (o *AuditLogOptions) AddFlags(fs *pflag.FlagSet) { | |
" gate. Known formats are "+strings.Join(pluginlog.AllowedFormats, ",")+".") | ||
} | ||
|
||
func (o *AuditLogOptions) Validate() []error { | ||
// Check whether the log backend is enabled based on the options. | ||
if !o.enabled() { | ||
return nil | ||
} | ||
|
||
var allErrors []error | ||
if advancedAuditingEnabled() { | ||
if err := validateBackendBatchOptions(pluginlog.PluginName, o.BatchOptions); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
|
||
// Check log format | ||
validFormat := false | ||
for _, f := range pluginlog.AllowedFormats { | ||
if f == o.Format { | ||
validFormat = true | ||
break | ||
} | ||
} | ||
if !validFormat { | ||
allErrors = append(allErrors, fmt.Errorf("invalid audit log format %s, allowed formats are %q", o.Format, strings.Join(pluginlog.AllowedFormats, ","))) | ||
} | ||
} | ||
|
||
// Check validities of MaxAge, MaxBackups and MaxSize of log options, if file log backend is enabled. | ||
if o.MaxAge < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxage %v can't be a negative number", o.MaxAge)) | ||
} | ||
if o.MaxBackups < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxbackup %v can't be a negative number", o.MaxBackups)) | ||
} | ||
if o.MaxSize < 0 { | ||
allErrors = append(allErrors, fmt.Errorf("--audit-log-maxsize %v can't be a negative number", o.MaxSize)) | ||
} | ||
|
||
return allErrors | ||
} | ||
|
||
// Check whether the log backend is enabled based on the options. | ||
func (o *AuditLogOptions) enabled() bool { | ||
return o != nil && o.Path != "" | ||
} | ||
|
||
func (o *AuditLogOptions) getWriter() io.Writer { | ||
if o.Path == "" { | ||
if !o.enabled() { | ||
return nil | ||
} | ||
|
||
|
@@ -359,8 +375,26 @@ func (o *AuditWebhookOptions) AddFlags(fs *pflag.FlagSet) { | |
"Deprecated, use --audit-webhook-initial-backoff instead.") | ||
} | ||
|
||
func (o *AuditWebhookOptions) Validate() []error { | ||
if !o.enabled() { | ||
return nil | ||
} | ||
|
||
var allErrors []error | ||
if advancedAuditingEnabled() { | ||
if err := validateBackendBatchOptions(pluginwebhook.PluginName, o.BatchOptions); err != nil { | ||
allErrors = append(allErrors, err) | ||
} | ||
} | ||
return allErrors | ||
} | ||
|
||
func (o *AuditWebhookOptions) enabled() bool { | ||
return o != nil && o.ConfigFile != "" | ||
} | ||
|
||
func (o *AuditWebhookOptions) applyTo(c *server.Config) error { | ||
if o.ConfigFile == "" { | ||
if !o.enabled() { | ||
return nil | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,172 @@ | ||
/* | ||
Copyright 2018 The Kubernetes 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 options | ||
|
||
import ( | ||
stdjson "encoding/json" | ||
"fmt" | ||
"io/ioutil" | ||
"os" | ||
"testing" | ||
|
||
"k8s.io/apiserver/pkg/server" | ||
"k8s.io/client-go/tools/clientcmd/api/v1" | ||
|
||
"github.com/spf13/pflag" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestAuditValidOptions(t *testing.T) { | ||
webhookConfig := makeTmpWebhookConfig(t) | ||
defer os.Remove(webhookConfig) | ||
|
||
testCases := []struct { | ||
name string | ||
options func() *AuditOptions | ||
expected string | ||
}{{ | ||
name: "default", | ||
options: NewAuditOptions, | ||
}, { | ||
name: "default log", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.Path = "/audit" | ||
return o | ||
}, | ||
expected: "log", | ||
}, { | ||
name: "default webhook", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.WebhookOptions.ConfigFile = webhookConfig | ||
return o | ||
}, | ||
expected: "buffered<webhook>", | ||
}, { | ||
name: "default union", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.Path = "/audit" | ||
o.WebhookOptions.ConfigFile = webhookConfig | ||
return o | ||
}, | ||
expected: "union[log,buffered<webhook>]", | ||
}, { | ||
name: "custom", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.BatchOptions.Mode = ModeBatch | ||
o.LogOptions.Path = "/audit" | ||
o.WebhookOptions.BatchOptions.Mode = ModeBlocking | ||
o.WebhookOptions.ConfigFile = webhookConfig | ||
return o | ||
}, | ||
expected: "union[buffered<log>,webhook]", | ||
}} | ||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
options := tc.options() | ||
require.NotNil(t, options) | ||
|
||
// Verify flags don't change defaults. | ||
fs := pflag.NewFlagSet("Test", pflag.PanicOnError) | ||
options.AddFlags(fs) | ||
require.NoError(t, fs.Parse(nil)) | ||
assert.Equal(t, tc.options(), options, "Flag defaults should match default options.") | ||
|
||
assert.Empty(t, options.Validate(), "Options should be valid.") | ||
config := &server.Config{} | ||
require.NoError(t, options.ApplyTo(config)) | ||
if tc.expected == "" { | ||
assert.Nil(t, config.AuditBackend) | ||
} else { | ||
assert.Equal(t, tc.expected, fmt.Sprintf("%s", config.AuditBackend)) | ||
} | ||
}) | ||
} | ||
} | ||
|
||
func TestAuditInvalidOptions(t *testing.T) { | ||
testCases := []struct { | ||
name string | ||
options func() *AuditOptions | ||
}{{ | ||
name: "invalid log format", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.Path = "/audit" | ||
o.LogOptions.Format = "foo" | ||
return o | ||
}, | ||
}, { | ||
name: "invalid log mode", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.Path = "/audit" | ||
o.LogOptions.BatchOptions.Mode = "foo" | ||
return o | ||
}, | ||
}, { | ||
name: "invalid log buffer size", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.LogOptions.Path = "/audit" | ||
o.LogOptions.BatchOptions.Mode = "batch" | ||
o.LogOptions.BatchOptions.BatchConfig.BufferSize = -3 | ||
return o | ||
}, | ||
}, { | ||
name: "invalid webhook mode", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.WebhookOptions.ConfigFile = "/audit" | ||
o.WebhookOptions.BatchOptions.Mode = "foo" | ||
return o | ||
}, | ||
}, { | ||
name: "invalid webhook buffer throttle qps", | ||
options: func() *AuditOptions { | ||
o := NewAuditOptions() | ||
o.WebhookOptions.ConfigFile = "/audit" | ||
o.WebhookOptions.BatchOptions.Mode = "batch" | ||
o.WebhookOptions.BatchOptions.BatchConfig.ThrottleQPS = -1 | ||
return o | ||
}, | ||
}} | ||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
options := tc.options() | ||
require.NotNil(t, options) | ||
assert.NotEmpty(t, options.Validate(), "Options should be invalid.") | ||
}) | ||
} | ||
} | ||
|
||
func makeTmpWebhookConfig(t *testing.T) string { | ||
config := v1.Config{ | ||
Clusters: []v1.NamedCluster{ | ||
{Cluster: v1.Cluster{Server: "localhost", InsecureSkipTLSVerify: true}}, | ||
}, | ||
} | ||
f, err := ioutil.TempFile("", "k8s_audit_webhook_test_") | ||
require.NoError(t, err, "creating temp file") | ||
require.NoError(t, stdjson.NewEncoder(f).Encode(config), "writing webhook kubeconfig") | ||
require.NoError(t, f.Close()) | ||
return f.Name() | ||
} |
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.
nit: why not backend.String() instead of
printf("%s", ...)
?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.
The backend interface doesn't have string. I could add String() (or just Name()) to the interface, or try to cast to
fmt.Stringer
here, but I thought theSprintf
was simpler. I don't have any real preference though...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's fine as it is then.
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.
I'd actually prefer to have a dedicated method in the interface. Otherwise you have to have implicit knowledge that stringifying a backend should give a short name of the backend, not e.g. full struct