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.

Sign up for GitHub

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

Jump to bottom

Audit Policy File #3692

Merged
merged 1 commit into from Oct 29, 2017
Merged

Audit Policy File #3692

merged 1 commit into from Oct 29, 2017

Conversation

gambol99
Copy link
Contributor

@gambol99 gambol99 commented Oct 23, 2017 •
edited

The current implementation doesn't allow users to set the advanced audit policy location. Note, the file contents can be pushed by a FileAsset a sample given below .... Or do we want an explicit secret for this? ..

fileAssets:
- name: audit-policy.conf
  path: /srv/kubernetes/audit-policy.conf
  roles: [Master]
  content: |
    some_content

related to #3672

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 23, 2017
@chrislovecnm
Copy link
Contributor

Are the contents secret? Feel like we should build the manifest properly

@chrislovecnm
Copy link
Contributor

Rather we should behold the config file properly for the user. Stuff like this is important and frankly should be easy for the user

@gambol99
Copy link
Contributor Author

gambol99 commented Oct 23, 2017 •
edited

hi @chrislovecnm ... i'm assuming you mean allowing the user to write the rules directly into the cluster spec and get nodeup to write the manifest out? ... I can't find anywhere else we are doing though; the closest 'recent' PR involving a manifest would encryption at rest .. but here were using a secret and simply creating via the kops create command. We could reuse this method but add some validation as the very least? ... I'm not sure if we have a consistent way of doing this ... file assets has the +1 of changes being detectable to rollouts ...

@gambol99
Copy link
Contributor Author

/assign @gambol99

KashifSaadat
KashifSaadat reviewed Oct 24, 2017
View reviewed changes
pkg/apis/kops/v1alpha2/componentconfig.go Outdated
@@ -248,6 +248,8 @@ type KubeAPIServerConfig struct {
AuditLogMaxBackups *int32 `json:"auditLogMaxBackups,omitempty" flag:"audit-log-maxbackup"`
// The maximum size in megabytes of the audit log file before it gets rotated. Defaults to 100MB.
AuditLogMaxSize *int32 `json:"auditLogMaxSize,omitempty" flag:"audit-log-maxsize"`
// AuditPolicyFile is path to a audit policy configuraion file on the masters
Copy link
Contributor

Choose a reason for hiding this comment

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

.. is the path to an audit policy configuration ..

@chrislovecnm
Copy link
Contributor

So we should start with what they have upstream https://github.com/kubernetes/kubernetes/blob/v1.8.0-beta.0/cluster/gce/gci/configure-helper.sh#L532 and build from there.

Let me think this through a bit

chrislovecnm
chrislovecnm reviewed Oct 24, 2017
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

Question

pkg/apis/kops/componentconfig.go Outdated
@@ -248,6 +248,8 @@ type KubeAPIServerConfig struct {
AuditLogMaxBackups *int32 `json:"auditLogMaxBackups,omitempty" flag:"audit-log-maxbackup"`
// The maximum size in megabytes of the audit log file before it gets rotated. Defaults to 100MB.
AuditLogMaxSize *int32 `json:"auditLogMaxSize,omitempty" flag:"audit-log-maxsize"`
// AuditPolicyFile is path to a audit policy configuraion file on the masters
AuditPolicyFile string `json:"auditPolicyFile,omitempty" flag:"audit-policy-file"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Fully qualified path and file name? It is not just the path.

/mnt/my-audit-file.comf

@chrislovecnm
Copy link
Contributor

Let get the comments reading better on the API values and get this in. Examples are awesome.

We can figure out how to add the policy file better later. Almost like an addon...

@gambol99
Audit Policy File
e995c7b 
Extending the KubeAPI component config to allow setting the audit-policy-file
@gambol99
Copy link
Contributor Author

i've updated the field comments @chrislovecnm ... we can use this weeks 'out of hours' for a implementation of content

@chrislovecnm
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 26, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2017
@chrislovecnm
Copy link
Contributor

/retest

Why has this not merged?

@chrislovecnm chrislovecnm reopened this Oct 28, 2017
@chrislovecnm
Copy link
Contributor

/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit cc4d4c4 into kubernetes:master Oct 29, 2017
@azman0101 azman0101 mentioned this pull request Dec 13, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KashifSaadat KashifSaadat KashifSaadat left review comments

@chrislovecnm chrislovecnm chrislovecnm approved these changes

Assignees

@chrislovecnm chrislovecnm

@gambol99 gambol99

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@gambol99 @chrislovecnm @k8s-github-robot @KashifSaadat @k8s-ci-robot