add a verify equivilant to prometheus visibility rules #100552

BenTheElder · 2021-03-25T06:13:49Z

What type of PR is this?

/kind bug
(we should have been enforcing this, the previous solution was lost in #99561)

What this PR does / why we need it:

Efficiently ensures that prometheus imports remain restricted per #89267 following the removal of bazel and visibility rules.

Which issue(s) this PR fixes:

Fixes #99876

Special notes for your reviewer:

We do have multiple other tools for validating imports, unfortunately none of them are suitable for this task, there's more on this in the script comments.

The short version: verify-imports is not capable of handling for this use case and is intended to restrict the imports within a package while verify-import-boss can technically express this but is far too expensive to run over the entire source tree and only checks specific directories.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/issues/2420

/sig testing instrumentation
/triage accepted

BenTheElder · 2021-03-25T06:19:58Z

cc @serathius xref: #99561 (comment) #99876 (comment)

hack/verify-prometheus-imports.sh

BenTheElder · 2021-03-25T06:30:24Z

hack/verify-prometheus-imports.sh

This is the current list of importers.
It also matches the previous visibility rule, though is more specific for component-base/metrics.

kubernetes/build/visible_to/BUILD

Lines 431 to 444 in a1f2787

package_group(

name = "vendor_githubcom_prometheus_CONSUMERS",

packages = [

"//cluster/images/etcd-version-monitor",

"//pkg/volume/util/operationexecutor",

"//staging/src/k8s.io/apiserver/pkg/admission/metrics",

"//staging/src/k8s.io/component-base/metrics/...",

"//test/e2e/apimachinery",

"//test/e2e_node",

"//test/integration/apiserver/flowcontrol",

"//test/integration/metrics",

"//vendor/...",

],

)

BenTheElder · 2021-03-25T06:56:48Z

/retest

BenTheElder · 2021-03-25T07:13:33Z

passed in 2s in CI https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/100552/pull-kubernetes-verify/1374967748962029568

BenTheElder · 2021-03-25T15:32:45Z

/retest
Integration flake

palnabarun · 2021-03-25T17:00:59Z

@BenTheElder -- do you want this PR to be in the milestone? asking since the issue is in milestone. :)

BenTheElder · 2021-03-25T17:10:56Z

Yes, let me fix that.

We want to prevent regressions here after dropping bazel, the test is very cheap, and the changes are contained to make verify / should not be flaky.

dims · 2021-03-25T17:25:33Z

hack/verify-prometheus-imports.sh

replace . with $KUBE_ROOT just to be sure?

Good catch, thanks. Will do 👍

actually, I cd'ed within this subshell instead. because then we don't need to do more path mangling to get package paths instead of absolute paths.

dims · 2021-03-25T17:27:28Z

hack/verify-prometheus-imports.sh

do all these options work well whether GNU grep or BSD grep is used?

yes. this doesn't use any GNU extensions, I tested this on mac first (2.5.1-FreeBSD) and then ~debian (GNU grep 3.6)

logicalhan · 2021-03-25T17:52:30Z

/assign

spiffxp

Non-blocking suggestions

spiffxp · 2021-03-25T18:42:56Z

hack/verify-prometheus-imports.sh

I think you could xargs dirname and uniq to avoid the array logic above but not a blocker

dirname only accepts a single argument on darwin, so it's not really going to help, we still need to do it serially.

I think we're going to switch to filepaths anyhow though, after discussing what is desired with @logicalhan elsewhere.

hack/verify-prometheus-imports.sh

k8s-ci-robot · 2021-03-25T18:51:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, spiffxp

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [BenTheElder,spiffxp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

logicalhan

/lgtm

BenTheElder · 2021-03-25T21:45:38Z

Changes:

this now has a list of specific files instead of packages
this now does not use go list, so we don't miss anything behind build tags
this now fails if files in the allow-list are not actually failing anymore and should be removed from the allow-list

BenTheElder mentioned this pull request Mar 25, 2021

bazel visibility rules need porting to make verify #99876

Closed

k8s-ci-robot requested review from fejta and xichengliudui March 25, 2021 06:14

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2021

BenTheElder requested review from dims and spiffxp and removed request for fejta March 25, 2021 06:19

BenTheElder assigned dims and spiffxp Mar 25, 2021

BenTheElder commented Mar 25, 2021

View reviewed changes

hack/verify-prometheus-imports.sh Outdated Show resolved Hide resolved

BenTheElder commented Mar 25, 2021

View reviewed changes

hack/verify-prometheus-imports.sh Outdated Show resolved Hide resolved

BenTheElder commented Mar 25, 2021

View reviewed changes

adtac mentioned this pull request Mar 25, 2021

[flake] k8s.io/kubernetes/test/integration/apiserver/admissionwebhook: TestMutatingWebhookResetsInvalidManagedFields #100556

Closed

BenTheElder added this to the v1.21 milestone Mar 25, 2021

dims reviewed Mar 25, 2021

View reviewed changes

BenTheElder force-pushed the prometheus-visibility branch 2 times, most recently from 83b769c to d4c4ed3 Compare March 25, 2021 17:51

k8s-ci-robot assigned logicalhan Mar 25, 2021

spiffxp approved these changes Mar 25, 2021

View reviewed changes

add a verify equivilant to prometheus visibility rules

7ede8a2

BenTheElder force-pushed the prometheus-visibility branch from d4c4ed3 to 7ede8a2 Compare March 25, 2021 21:42

logicalhan reviewed Mar 25, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 25, 2021

k8s-ci-robot merged commit 447e338 into kubernetes:master Mar 25, 2021

BenTheElder mentioned this pull request Mar 25, 2021

Reducing Kubernetes Build Maintenance kubernetes/enhancements#2420

Closed

12 tasks

BenTheElder deleted the prometheus-visibility branch March 25, 2021 23:20

	package_group(
	name = "vendor_githubcom_prometheus_CONSUMERS",
	packages = [
	"//cluster/images/etcd-version-monitor",
	"//pkg/volume/util/operationexecutor",
	"//staging/src/k8s.io/apiserver/pkg/admission/metrics",
	"//staging/src/k8s.io/component-base/metrics/...",
	"//test/e2e/apimachinery",
	"//test/e2e_node",
	"//test/integration/apiserver/flowcontrol",
	"//test/integration/metrics",
	"//vendor/...",
	],
	)

add a verify equivilant to prometheus visibility rules #100552

add a verify equivilant to prometheus visibility rules #100552

Uh oh!

Conversation

BenTheElder commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

palnabarun commented Mar 25, 2021

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

logicalhan commented Mar 25, 2021

Uh oh!

spiffxp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 25, 2021

Uh oh!

logicalhan left a comment

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented Mar 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

BenTheElder commented Mar 25, 2021 •

edited

Loading