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

kubeadm: Remove AuditPolicyConfiguration #70807

Merged
merged 1 commit into from Nov 12, 2018

Conversation

@Klaven
Copy link
Contributor

Klaven commented Nov 8, 2018

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Per office hours we decided to remove this. see kubernetes/kubeadm#1221
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#1221

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

kubeadm: remove the AuditPolicyConfiguration feature gate
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 8, 2018

Hi @Klaven. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@Klaven

This comment has been minimized.

Copy link
Contributor

Klaven commented Nov 8, 2018

@chuckha This is the PR we where talking about in office hours.

@@ -354,16 +348,12 @@ func TestGetAPIServerCommand(t *testing.T) {
},
},
{
name: "auditing is enabled with a custom log max age of 0",
name: "HA is enabled",

This comment has been minimized.

@Klaven

Klaven Nov 8, 2018

Contributor

This is interesting and I wonder if the correct answer is to remove this test? Originally yesterday it was HA and auditing are enabled Someone else removed the HA part and then I removed the auditing. If I don't remove the test, what should the tests name be? "Pretty standard bland test"? :)

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

+1 for removal.

This comment has been minimized.

@Klaven

Klaven Nov 8, 2018

Contributor

removing.

@Klaven Klaven changed the title fixes kubeadm 1221 to remove AuditPolicyConfiguration [WIP] fixes kubeadm 1221 to remove AuditPolicyConfiguration Nov 8, 2018

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 8, 2018

@Klaven as mentioned earlier, we do not include the issue that a PR fixes in the PR title.

@neolit123
Copy link
Member

neolit123 left a comment

the WIP is LGTM on a quick look.

@@ -354,16 +348,12 @@ func TestGetAPIServerCommand(t *testing.T) {
},
},
{
name: "auditing is enabled with a custom log max age of 0",
name: "HA is enabled",

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

+1 for removal.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 8, 2018

/priority important-soon
/ok-to-test

@Klaven Klaven changed the title [WIP] fixes kubeadm 1221 to remove AuditPolicyConfiguration [WIP] Remove AuditPolicyConfiguration Nov 8, 2018

@Klaven

This comment has been minimized.

Copy link
Contributor

Klaven commented Nov 8, 2018

I missed updating a test somehow. will also update that with the new tests that are being added.

@Klaven

This comment has been minimized.

Copy link
Contributor

Klaven commented Nov 8, 2018

@Klaven as mentioned earlier, we do not include the issue that a PR fixes in the PR title.

sorry. old habits die hard. my bad.

@@ -17,7 +17,6 @@ go_library(
"//pkg/kubelet/apis:go_default_library",
"//pkg/master/ports:go_default_library",
"//pkg/scheduler/apis/config:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/apis/config/v1alpha1:go_default_library",

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

i don't think this PR should be touching pkg/scheduler.
is this the result of ./hack/update-bazel.sh?

This comment has been minimized.

@Klaven

Klaven Nov 8, 2018

Contributor

yeah. I think so. I have just updated this. I don't know how it got touched. I caught this too. 🤔

This comment has been minimized.

@Klaven

Klaven Nov 8, 2018

Contributor

Sadly I squashed because I did another rebase on master and was not thinking about leaving the new commits there. my bad.

@Klaven Klaven force-pushed the Klaven:kubeadm_1221 branch from f4aceb2 to 5153c98 Nov 8, 2018

@luxas
Copy link
Member

luxas left a comment

Some comments, then LGTM

@@ -320,14 +317,3 @@ type HostPathMount struct {
// PathType is the type of the HostPath.
PathType v1.HostPathType `json:"pathType,omitempty"`
}

// AuditPolicyConfiguration holds the options for configuring the api server audit policy.
type AuditPolicyConfiguration struct {

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

From @fabriziopandini: We shouldn't remove from the existing v1alpha3 API, instead error if the field was set in v1alpha3 when converting to v1beta1

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

Also add a conversion_test.go unit test to error if it was set in v1alpha3

PathType: ""
ReadOnly: false
TimeoutForControlPlane: 4m0s
AuditPolicyConfiguration:

This comment has been minimized.

@luxas

luxas Nov 8, 2018

Member

This is ok, but it should be written as the v1alpha3 serialized version

@timothysc
Copy link
Member

timothysc left a comment

/approve
/test pull-kubernetes-e2e-gce-100-performance

Once @luxas comments are addressed LGTM

@Klaven Klaven force-pushed the Klaven:kubeadm_1221 branch from 7df65b7 to d4b0702 Nov 9, 2018

@fabriziopandini
Copy link
Contributor

fabriziopandini left a comment

@Klaven well done!

@lucas comment are addressed, so
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 9, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, Klaven, timothysc

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 9, 2018

/retest

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

4 similar comments
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 10, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 12, 2018

@Klaven needs a rebase.

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 12, 2018

@Klaven Klaven force-pushed the Klaven:kubeadm_1221 branch from d4b0702 to 75ab2a8 Nov 12, 2018

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 12, 2018

/retest

fixes kubeadm 1221 to remove AuditPolicyConfiguration
Added conversion test and failure.

@Klaven Klaven force-pushed the Klaven:kubeadm_1221 branch from 79bf139 to 064f74b Nov 12, 2018

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 12, 2018

/lgtm
/hold

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 12, 2018

/hold cancel
all comments seem to be addressed
@Klaven, thanks.

if there is anything else we can revisit in a new PR.

@k8s-ci-robot k8s-ci-robot merged commit 3c5c602 into kubernetes:master Nov 12, 2018

18 checks passed

cla/linuxfoundation Klaven authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment