Override audit policy for conformance informing jobs #19502
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
area/config
| env: | ||
| - name: ENABLE_APISERVER_ADVANCED_AUDIT | ||
| value: "true" | ||
| - name: ADVANCED_AUDIT_POLICY |
There was a problem hiding this comment.
I don't think we should change the default audit policy for the default e2e job
There was a problem hiding this comment.
Currently we need the results of a full test run (for all tests, not just the conformance runs, so we can see tests that we could promote, or emulate to hit with conformance tests.
Is there another job we could modify that has similar perodicity that's not the default job?
There was a problem hiding this comment.
I'm fine leaving it alone for now. It means we might be lacking non-conformance test coverage, but it's not as important as conformance coverage itself.
| env: | ||
| - name: ENABLE_APISERVER_ADVANCED_AUDIT | ||
| value: "true" | ||
| - name: ADVANCED_AUDIT_POLICY |
There was a problem hiding this comment.
Can you comment why this is here? In an ideal world, this job would be rewritten to patch the existing default policy .yaml, and explain why the patch. Pretend you're a future maintainer trying to understand what this is, whether this is safe to remove, or when/why it needs to be changed.
There was a problem hiding this comment.
It seems to be the correct place to over-ride based on the underlying scripts.
Should we instead update the cmd/args of the job to run a series of commands to first run 'sed/awk' to patch / remove this, before we run the existing job:
There was a problem hiding this comment.
The patch, if we found a way to cleanly apply it within the job:
diff default-policy.json new-policy.json 2>&1
71,75d70
< # Don't log events requests.
< - level: None
< resources:
< - group: "" # core
< resources: ["events"]
There was a problem hiding this comment.
I updated the PR to only update the conformance job, and added a comment as to why we override the audit policy, including that it would be better to patch rather than override if possible.
hh
force-pushed
the
conformance-audit-policy
branch
from
6fe3323 to
7bb4a9f
Compare
config/jobs/kubernetes/sig-cloud-provider/gcp/gce-conformance.yaml
Outdated
Show resolved
Hide resolved
hh
force-pushed
the
conformance-audit-policy
branch
2 times, most recently
from
3f41442 to
3fa5485
Compare
| # and then removed the block that says "don't log events" | ||
| # | ||
| # NOTE: if the default audit policy changes, this will need to be updated accordingly | ||
| # TODO(link-to-a-github-issue): ideally we could do this via a patch instead |
There was a problem hiding this comment.
I meant to imply you should make a github issue and link to it here
There was a problem hiding this comment.
Sorry I missed that: #19613 created and added to the comment.
Co-authored-by: Aaron Crickenberger <spiffxp@google.com>
hh
force-pushed
the
conformance-audit-policy
branch
from
3fa5485 to
57d7311
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hh, spiffxp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
@hh: Updated the
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
This caused the job to start failing (ref: #19613 (comment)) so I opened a revert #19618 |
An alternative to kubernetes/kubernetes#95388