Skip to content

Switch to use upstream audit handler #11192

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

Merged
merged 4 commits into from
Oct 22, 2016
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pkg/cmd/server/api/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,8 @@ func GetMasterFileReferences(config *MasterConfig) []*string {
refs = append(refs, &config.ControllerConfig.ServiceServingCert.Signer.KeyFile)
}

refs = append(refs, &config.AuditConfig.AuditFilePath)

return refs
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/cmd/server/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,14 @@ type AuditConfig struct {
// If this flag is set, audit log will be printed in the logs.
// The logs contains, method, user and a requested URL.
Enabled bool
// All requests coming to the apiserver will be logged to this file.
AuditFilePath string
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see validation on these fields and I don't see this path listed in the list of paths for resolution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deads2k what kind of validation you're thinking about? that the path field is non-empty and ints are >= 0? I don't see any value in that.

// Maximum number of days to retain old log files based on the timestamp encoded in their filename.
MaximumFileRetentionDays int
// Maximum number of old log files to retain.
MaximumRetainedFiles int
// Maximum size in megabytes of the log file before it gets rotated. Defaults to 100MB.
MaximumFileSizeMegabytes int
}

// JenkinsPipelineConfig holds configuration for the Jenkins pipeline strategy
Expand Down
8 changes: 6 additions & 2 deletions pkg/cmd/server/api/v1/swagger_doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,12 @@ func (AssetExtensionsConfig) SwaggerDoc() map[string]string {
}

var map_AuditConfig = map[string]string{
"": "AuditConfig holds configuration for the audit capabilities",
"enabled": "If this flag is set, basic audit log will be printed in the logs. The logs contains, method, user and a requested URL.",
"": "AuditConfig holds configuration for the audit capabilities",
"enabled": "If this flag is set, audit log will be printed in the logs. The logs contains, method, user and a requested URL.",
"auditFilePath": "All requests coming to the apiserver will be logged to this file.",
"maximumFileRetentionDays": "Maximum number of days to retain old log files based on the timestamp encoded in their filename.",
"maximumRetainedFiles": "Maximum number of old log files to retain.",
"maximumFileSizeMegabytes": "Maximum size in megabytes of the log file before it gets rotated. Defaults to 100MB.",
}

func (AuditConfig) SwaggerDoc() map[string]string {
Expand Down
10 changes: 9 additions & 1 deletion pkg/cmd/server/api/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,17 @@ type MasterConfig struct {

// AuditConfig holds configuration for the audit capabilities
type AuditConfig struct {
// If this flag is set, basic audit log will be printed in the logs.
// If this flag is set, audit log will be printed in the logs.
// The logs contains, method, user and a requested URL.
Enabled bool `json:"enabled"`
// All requests coming to the apiserver will be logged to this file.
AuditFilePath string `json:"auditFilePath"`
// Maximum number of days to retain old log files based on the timestamp encoded in their filename.
MaximumFileRetentionDays int `json:"maximumFileRetentionDays"`
// Maximum number of old log files to retain.
MaximumRetainedFiles int `json:"maximumRetainedFiles"`
// Maximum size in megabytes of the log file before it gets rotated. Defaults to 100MB.
MaximumFileSizeMegabytes int `json:"maximumFileSizeMegabytes"`
}

// JenkinsPipelineConfig holds configuration for the Jenkins pipeline strategy
Expand Down
4 changes: 4 additions & 0 deletions pkg/cmd/server/api/v1/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,11 @@ assetConfig:
namedCertificates: null
requestTimeoutSeconds: 0
auditConfig:
auditFilePath: ""
enabled: false
maximumFileRetentionDays: 0
maximumFileSizeMegabytes: 0
maximumRetainedFiles: 0
controllerConfig:
serviceServingCert:
signer: null
Expand Down
22 changes: 22 additions & 0 deletions pkg/cmd/server/api/validation/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,28 @@ func ValidateMasterConfig(config *api.MasterConfig, fldPath *field.Path) Validat

validationResults.Append(ValidateControllerConfig(config.ControllerConfig, fldPath.Child("controllerConfig")))

validationResults.Append(ValidateAuditConfig(config.AuditConfig, fldPath.Child("auditConfig")))

return validationResults
}

func ValidateAuditConfig(config api.AuditConfig, fldPath *field.Path) ValidationResults {
validationResults := ValidationResults{}

if len(config.AuditFilePath) == 0 {
// for backwards compatibility reasons we can't error this out
validationResults.AddWarnings(field.Required(fldPath.Child("auditFilePath"), "audit can now be logged to a separate file"))
}
if config.MaximumFileRetentionDays < 0 {
validationResults.AddErrors(field.Invalid(fldPath.Child("maximumFileRetentionDays"), config.MaximumFileRetentionDays, "must be greater than or equal to 0"))
}
if config.MaximumRetainedFiles < 0 {
validationResults.AddErrors(field.Invalid(fldPath.Child("maximumRetainedFiles"), config.MaximumRetainedFiles, "must be greater than or equal to 0"))
}
if config.MaximumFileSizeMegabytes < 0 {
validationResults.AddErrors(field.Invalid(fldPath.Child("maximumFileSizeMegabytes"), config.MaximumFileSizeMegabytes, "must be greater than or equal to 0"))
}

return validationResults
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/cmd/server/api/validation/master_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ func TestValidateAdmissionPluginConfigConflicts(t *testing.T) {
// these fields have warnings in the empty case
defaultWarningFields := sets.NewString(
"serviceAccountConfig.managedNames", "serviceAccountConfig.publicKeyFiles", "serviceAccountConfig.privateKeyFile", "serviceAccountConfig.masterCA",
"projectConfig.securityAllocator", "kubernetesMasterConfig.proxyClientInfo")
"projectConfig.securityAllocator", "kubernetesMasterConfig.proxyClientInfo", "auditConfig.auditFilePath")

for _, tc := range testCases {
results := ValidateMasterConfig(&tc.options, nil)
Expand Down
113 changes: 0 additions & 113 deletions pkg/cmd/server/origin/audit.go

This file was deleted.

43 changes: 0 additions & 43 deletions pkg/cmd/server/origin/audit_test.go

This file was deleted.

37 changes: 36 additions & 1 deletion pkg/cmd/server/origin/master.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"crypto/tls"
"encoding/json"
"fmt"
"io"
"net/http"
"os"
"regexp"
Expand All @@ -16,6 +17,7 @@ import (
"github.com/go-openapi/spec"
"github.com/golang/glog"
"github.com/prometheus/client_golang/prometheus"
"gopkg.in/natefinch/lumberjack.v2"

kapi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/meta"
Expand All @@ -25,6 +27,7 @@ import (
"k8s.io/kubernetes/pkg/apimachinery/registered"
v1beta1extensions "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
"k8s.io/kubernetes/pkg/apiserver"
"k8s.io/kubernetes/pkg/apiserver/audit"
"k8s.io/kubernetes/pkg/client/restclient"
kclient "k8s.io/kubernetes/pkg/client/unversioned"
clientadapter "k8s.io/kubernetes/pkg/client/unversioned/adapters/internalclientset"
Expand Down Expand Up @@ -180,7 +183,22 @@ func (c *MasterConfig) Run(protected []APIInstaller, unprotected []APIInstaller)
handler = c.authorizationFilter(handler)
handler = c.impersonationFilter(handler)
// audit handler must comes before the impersonationFilter to read the original user
handler = c.auditHandler(handler)
if c.Options.AuditConfig.Enabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of ugly to have this code block here. Hopefully, we are able to get rid of this in a future rebase.

Copy link
Contributor Author

@soltysh soltysh Oct 18, 2016

Choose a reason for hiding this comment

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

We'll be able to move it where necessary, but the option will stay forever with us ;)

attributeGetter := apiserver.NewRequestAttributeGetter(c.getRequestContextMapper(), c.getRequestInfoResolver())
var writer io.Writer
if len(c.Options.AuditConfig.AuditFilePath) > 0 {
writer = &lumberjack.Logger{
Filename: c.Options.AuditConfig.AuditFilePath,
MaxAge: c.Options.AuditConfig.MaximumFileRetentionDays,
MaxBackups: c.Options.AuditConfig.MaximumRetainedFiles,
MaxSize: c.Options.AuditConfig.MaximumFileSizeMegabytes,
}
} else {
// backwards compatible writer to regular log
writer = cmdutil.NewGLogWriterV(0)
}
handler = audit.WithAudit(handler, attributeGetter, writer)
}
handler = authenticationHandlerFilter(handler, c.Authenticator, c.getRequestContextMapper())
handler = namespacingFilter(handler, c.getRequestContextMapper())
handler = cacheControlFilter(handler, "no-store") // protected endpoints should not be cached
Expand Down Expand Up @@ -876,6 +894,23 @@ func (c *MasterConfig) getRequestContextMapper() kapi.RequestContextMapper {
return c.RequestContextMapper
}

// getRequestInfoResolver returns a request resolver.
func (c *MasterConfig) getRequestInfoResolver() *apiserver.RequestInfoResolver {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will bite us in the next rebase. Upstream genericapiserver only knows about one APIPrefix and one APIGroupPrefix. Looks like we have to turn those into lists in order to support origin's setup. /cc @deads2k

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 do hope the next rebase (I'm the lucky looser/winner) will straight this out.

Copy link
Contributor

Choose a reason for hiding this comment

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

WIP PR for multiple api group prefixes in genericapiserver: kubernetes/kubernetes#35021

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know when you start the master.go rebase. We have moved around so much stuff, hopefully to the better.

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 remember, we'll be in touch, definitely.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will bite us in the next rebase. Upstream genericapiserver only knows about one APIPrefix and one APIGroupPrefix. Looks like we have to turn those into lists in order to support origin's setup. /cc @deads2k

Those are all legacy prefixes which I just made have a list upstream: https://github.com/kubernetes/kubernetes/blob/master/pkg/genericapiserver/config.go#L175

if c.RequestInfoResolver == nil {
c.RequestInfoResolver = &apiserver.RequestInfoResolver{
APIPrefixes: sets.NewString(strings.Trim(LegacyOpenShiftAPIPrefix, "/"),
strings.Trim(OpenShiftAPIPrefix, "/"),
strings.Trim(KubernetesAPIPrefix, "/"),
strings.Trim(KubernetesAPIGroupPrefix, "/")), // all possible API prefixes
GrouplessAPIPrefixes: sets.NewString(strings.Trim(LegacyOpenShiftAPIPrefix, "/"),
strings.Trim(OpenShiftAPIPrefix, "/"),
strings.Trim(KubernetesAPIPrefix, "/"),
), // APIPrefixes that won't have groups (legacy)
}
}
return c.RequestInfoResolver
}

// RouteAllocator returns a route allocation controller.
func (c *MasterConfig) RouteAllocator() *routeallocationcontroller.RouteAllocationController {
osclient, kclient := c.RouteAllocatorClients()
Expand Down
2 changes: 2 additions & 0 deletions pkg/cmd/server/origin/master_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ type MasterConfig struct {

// RequestContextMapper maps requests to contexts
RequestContextMapper kapi.RequestContextMapper
// RequestInfoResolver is responsible for reading request attributes
RequestInfoResolver *apiserver.RequestInfoResolver

AdmissionControl admission.Interface

Expand Down
17 changes: 14 additions & 3 deletions vendor/k8s.io/kubernetes/pkg/apiserver/audit/audit.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion vendor/k8s.io/kubernetes/pkg/apiserver/audit/audit_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.