diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index bc253239db..2d66292133 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,4 +1,4 @@ - + diff --git a/Gopkg.lock b/Gopkg.lock index c9ce2f8cab..622557b36c 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -891,14 +891,16 @@ revision = "e3762e86a74c878ffed47484592986685639c2cd" [[projects]] - digest = "1:16c26aad2883eddb0caa103f510401b468e71ae8854b93c340de63bd57015002" + digest = "1:9070222ca967d09b3966552a161dd4420d62315964bf5e1efd8cc4c7c30ebca8" name = "sigs.k8s.io/testing_frameworks" packages = [ "integration", + "integration/addr", "integration/internal", ] pruneopts = "UT" - revision = "f53464b8b84b4507805a0b033a8377b225163fea" + revision = "d348cb12705b516376e0c323bacca72b00a78425" + version = "v0.1.1" [solve-meta] analyzer-name = "dep" diff --git a/Gopkg.toml b/Gopkg.toml index b92ddd0d65..f75ea91105 100644 --- a/Gopkg.toml +++ b/Gopkg.toml @@ -44,6 +44,10 @@ required = ["sigs.k8s.io/testing_frameworks/integration", name = "k8s.io/client-go" version = "kubernetes-1.12.3" +[[constraint]] + name = "sigs.k8s.io/testing_frameworks" + version = "v0.1.1" + [[constraint]] name = "github.com/onsi/ginkgo" version = "v1.5.0" @@ -67,9 +71,6 @@ required = ["sigs.k8s.io/testing_frameworks/integration", # these are not listed explicitly until we get version tags, # since dep doesn't like bare revision dependencies -# [[constraint]] -# name = "sigs.k8s.io/testing_frameworks" -# # [[constraint]] # name = "github.com/go-logr/logr" # diff --git a/RELEASE.md b/RELEASE.md deleted file mode 100644 index c3ab6479f4..0000000000 --- a/RELEASE.md +++ /dev/null @@ -1,8 +0,0 @@ -# Release Process - -The Kubernetes controller-runtime Project is released on an as-needed basis. The process is as follows: - -1. An issue is proposing a new release with a changelog since the last release -1. 2 [OWNERS](OWNERS) must LGTM this release -1. An OWNER runs `git tag -s $VERSION` and inserts the changelog and pushes the tag with `git push $VERSION` -1. The release issue is closed diff --git a/VERSIONING.md b/VERSIONING.md index f345bbf3dd..2ba11588ca 100644 --- a/VERSIONING.md +++ b/VERSIONING.md @@ -21,9 +21,9 @@ changes will go into an semi-immediate patch or minor release - Please *try* to avoid breaking changes when you can. They make users - face difficult decisions (when do I go through the pain of upgrading), - and make life hard for maintainers and contributors (dealing with - differences on stable branches). + face difficult decisions ("when do I go through the pain of + upgrading?"), and make life hard for maintainers and contributors + (dealing with differences on stable branches). ### Mantainers @@ -65,7 +65,9 @@ greatest code, including breaking changes, happens on master. The *release-X* branches contain stable, backwards compatible code. Every major (X) release, a new such branch is created. It is from these -branches that minor and patch releases are tagged. +branches that minor and patch releases are tagged. If some cases, it may +be neccessary open PRs for bugfixes directly against stable branches, but +this should generally not be the case. The maintainers are responsible for updating the contents of this branch; generally, this is done just before a release using release tooling that @@ -73,12 +75,8 @@ filters and checks for changes tagged as breaking (see below). ### Tooling -* [gen-release-notes.sh](hack/gen-release-notes.sh): generate release - notes for a range of commits, and check for next version type - (***TODO***) - -* [cherrypick-minor-version.sh](hack/cherrypick-minor-version.sh): update - a stable branch with appropriate commits from the master (***TODO***). +* [release-notes.sh](hack/release-notes.sh): generate release notes + for a range of commits, and check for next version type (***TODO***) * [verify-commit-messages.sh](hack/verify-commit-messages.sh): check that your PR and/or commit messages have the right versioning icon @@ -95,14 +93,14 @@ a: - Docs: :book: (`:book:`) - Infra/Tests/Other: :running: (`:running:`) -Individual commits may be tagged separately, but will generally be assumed -to match the PR. For instance, if you have a bugfix in with a breaking -change, it's generally encouraged to submit the bugfix separately, but if -you must put them in one PR, mark the commit separately. +You can also use the equivalent emoji directly, since GitHub doesn't +render the `:xyz:` aliases in PR titles. -*Commits marked separately will be treated similiarly to a distinct PR by -the cherrypick scripts*. Therefore, only use them if you want the given -commit to be considered separately. +Individual commits should not be tagged separately, but will generally be +assumed to match the PR. For instance, if you have a bugfix in with +a breaking change, it's generally encouraged to submit the bugfix +separately, but if you must put them in one PR, mark the commit +separately. ### Commands and Workflow @@ -124,21 +122,44 @@ a command reference. Minor and patch releases are generally done immediately after a feature or bugfix is landed, or sometimes a series of features tied together. -Major releases are done once a sufficient amount of breaking changes are -accrued. Since we don't intend to have a ton of these, the maintainers -will evaluate when to do a major release as it comes up. +Minor releases will only be tagged on the *most recent* major release +branch, except in exceptional circumstances. Patches will be backported +to maintained stable versions, as needed. + +Major releases are done shortly after a breaking change is merged -- once +a breaking change is merged, the next release *must* be a major revison. +We don't intend to have a lot of these, so we may put off merging breaking +PRs until a later date. ### Exact Steps -1. (*if doing a minor or patch release*) Update the release-X branch with - the latest set of changes using the cherrypick tooling (***TODO***) +Follow the release-specific steps below, then follow the general steps +after that. + +#### Minor and patch releases + +1. Update the release-X branch with the latest set of changes by calling + `git rebase master` from the release branch. + +#### Major releases -2. Generate release notes using the release note tooling (***TODO***) +1. Create a new release branch named `release-X` (where `X` is the new + version) off of master. + +#### General + +2. Generate release notes using the release note tooling. 3. Add a release for controller-runtime on GitHub, using those release notes, with a title of `vX.Y.Z`. + +4. Do a similar process for + [controller-tools](https://github.com/kubernetes-sigs/controller-tools) -4. Announce the release in `#kubebuilder` on Slack with a pinned message. +5. Announce the release in `#kubebuilder` on Slack with a pinned message. + +6. Potentially update + [kubebuilder](https://github.com/kubernetes-sigs/kubebuilder) as well. ### Breaking Changes @@ -148,8 +169,11 @@ maintainers/contributors, who have to deal with differences between master and stable branches. That being said, we'll occaisonally want to make breaking changes. They'll -be merged onto master, but won't make it into a release immediately (see -[Release Proccess](#release-process)). +be merged onto master, and will then trigger a major release (see [Release +Proccess](#release-process)). Because breaking changes induce a major +revision, the maintainers may delay a particular breaking change until +a later date when they are ready to make a major revision with a few +breaking changes. If you're going to make a breaking change, please make sure to explain in detail why it's helpful. Is it necessary to cleanly resolve an issue? @@ -158,6 +182,11 @@ Does it improve API ergonomics? Maintainers should treat breaking changes with caution, and evaluate potential non-breaking solutions (see below). +Note that API breakage in public APIs due to dependencies will trigger +a major revision, so you may occaisonally need to have a major release +anyway, due to changes in libraries like `k8s.io/client-go` or +`k8s.io/apimachinery`. + *NB*: Pre-1.0 releases treat breaking changes a bit more lightly. We'll still consider carefully, but the pre-1.0 timeframe is useful for converging on a ergonomic API. diff --git a/example/main.go b/example/main.go index f68a094225..fa3b52b89a 100644 --- a/example/main.go +++ b/example/main.go @@ -108,8 +108,8 @@ func main() { entryLog.Info("setting up webhook server") as, err := webhook.NewServer("foo-admission-server", mgr, webhook.ServerOptions{ - Port: 9876, - CertDir: "/tmp/cert", + Port: 9876, + CertDir: "/tmp/cert", DisableWebhookConfigInstaller: &disableWebhookConfigInstaller, BootstrapOptions: &webhook.BootstrapOptions{ Secret: &apitypes.NamespacedName{ diff --git a/hack/release/common.sh b/hack/release/common.sh new file mode 100644 index 0000000000..4e9fbbf934 --- /dev/null +++ b/hack/release/common.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +shopt -s extglob + +cr_major_pattern=":warning:|$(printf "\xe2\x9a\xa0")" +cr_minor_pattern=":sparkles:|$(printf "\xe2\x9c\xa8")" +cr_patch_pattern=":bug:|$(printf "\xf0\x9f\x90\x9b")" +cr_docs_pattern=":book:|$(printf "\xf0\x9f\x93\x96")" +cr_other_pattern=":running:|$(printf "\xf0\x9f\x8f\x83")" + +# cr::symbol-type-raw turns :xyz: and the corresponding emoji +# into one of "major", "minor", "patch", "docs", "other", or +# "unknown", ignoring the '!' +cr::symbol-type-raw() { + case $1 in + @(${cr_major_pattern})?('!')) + echo "major" + ;; + @(${cr_minor_pattern})?('!')) + echo "minor" + ;; + @(${cr_patch_pattern})?('!')) + echo "patch" + ;; + @(${cr_docs_pattern})?('!')) + echo "docs" + ;; + @(${cr_other_pattern})?('!')) + echo "other" + ;; + *) + echo "unknown" + ;; + esac +} + +# cr::symbol-type turns :xyz: and the corresponding emoji +# into one of "major", "minor", "patch", "docs", "other", or +# "unknown". +cr::symbol-type() { + local type_raw=$(cr::symbol-type-raw $1) + if [[ ${type_raw} == "unknown" ]]; then + echo "unknown" + return + fi + + if [[ $1 == *'!' ]]; then + echo "major" + return + fi + + echo ${type_raw} +} + +# git::is-release-branch-name checks if its argument is a release branch name +# (release-0.Y or release-X). +git::is-release-branch-name() { + [[ ${1-must specify release branch name to check} =~ release-((0\.[[:digit:]])|[[:digit:]]+) ]] +} + +# git::ensure-release-branch checks that we're on a release branch +git::ensure-release-branch() { + local current_branch=$(git rev-parse --abbrev-ref HEAD) + if ! git::is-release-branch-name ${current_branch}; then + echo "branch ${current_branch} does not appear to be a release branch (release-X)" >&2 + exit 1 + fi +} + +# git::export-current-version outputs the current version +# as exported variables (${maj,min,patch}_ver, last_tag) after +# checking that we're on the right release branch. +git::export-current-version() { + # make sure we're on a release branch + git::ensure-release-branch + + # deal with the release-0.1 branch, or similar + local release_ver=${BASH_REMATCH[1]} + maj_ver=${release_ver} + local tag_pattern='v${maj_ver}.([[:digit:]]+).([[:digit]]+)' + if [[ ${maj_ver} =~ 0\.([[:digit:]]+) ]]; then + maj_ver=0 + min_ver=${BASH_REMATCH[1]} + local tag_pattern="v0.(${min_ver}).([[:digit:]]+)" + fi + + # make sure we've got a tag that matches our release branch + last_tag=$(git describe --tags --abbrev=0) # try to fetch just the "current" tag name + if [[ ! ${last_tag} =~ ${tag_pattern} ]]; then + echo "tag ${last_tag} does not appear to be a release for this release (${release_ver})-- it should be v${maj_ver}.Y.Z" >&2 + exit 1 + fi + + export min_ver=${BASH_REMATCH[1]} + export patch_ver=${BASH_REMATCH[2]} + export maj_ver=${maj_ver} + export last_tag=${last_tag} +} + +# git::next-version figures out the next version to tag +# (it also sets the current version variables to the current version) +git::next-version() { + git::export-current-version + + local feature_commits=$(git rev-list ${last_tag}..${end_range} --grep="${cr_minor_pattern}") + local breaking_commits=$(git rev-list ${last_tag}..${end_range} --grep="${cr_major_pattern}") + + if [[ -z ${breaking_commits} && ${maj_ver} > 0 ]]; then + local next_ver="v$(( maj_ver + 1 )).0.0" + elif [[ -z ${feature_commits} ]]; then + local next_ver="v${maj_ver}.$(( min_ver + 1 )).0" + else + local next_ver="v${maj_ver}.${min_ver}.$(( patch_ver + 1 ))" + fi + + echo "${next_ver}" +} diff --git a/hack/release/release-notes.sh b/hack/release/release-notes.sh new file mode 100755 index 0000000000..1a98343af4 --- /dev/null +++ b/hack/release/release-notes.sh @@ -0,0 +1,103 @@ +#!/usr/bin/env bash + +set -e +set -o pipefail + +# import our common stuff +source "$(dirname ${BASH_SOURCE})/common.sh" + +# TODO: work with both release branch and major release +git::ensure-release-branch +git::export-current-version +# check the next version +next_ver=$(git::next-version) + +features="" +bugfixes="" +breaking="" +unknown="" +MERGE_PR="Merge pull request #([[:digit:]]+) from ([[:alnum:]-]+)/.+" +NEWLINE=" +" +head_commit=$(git rev-parse HEAD) +while read commit_word commit; do + read title + read # skip the blank line + read prefix body + + if [[ ${prefix} == v*.*.* && ( ${commit} == ${head_commit} || $(git tag --points-at ${commit}) == v*.*.* ) ]]; then + # skip version merges + continue + fi + set +x + if [[ ! ${title} =~ ${MERGE_PR} ]]; then + echo "Unable to determine PR number for merge ${commit} with title '${title}', aborting." >&2 + exit 1 + fi + pr_number=${BASH_REMATCH[1]} + pr_type=$(cr::symbol-type ${prefix}) + pr_title=${body} + if [[ ${pr_type} == "unknown" ]]; then + pr_title="${prefix} ${pr_title}" + fi + case ${pr_type} in + major) + breaking="${breaking}- ${pr_title} (#${pr_number})${NEWLINE}" + ;; + minor) + features="${features}- ${pr_title} (#${pr_number})${NEWLINE}" + ;; + patch) + bugfixes="${bugfixes}- ${pr_title} (#${pr_number})${NEWLINE}" + ;; + docs|other) + # skip non-code-changes + ;; + unknown) + unknown="${unknown}- ${pr_title} (#${pr_number})${NEWLINE}" + ;; + *) + echo "unknown PR type '${pr_type}' on PR '${pr_title}'" >&2 + exit 1 + esac +done <<<$(git rev-list ${last_tag}..HEAD --merges --pretty=format:%B) + +# TODO: sort non merge commits with tags + +[[ -n "${breaking}" ]] && printf '\e[1;31mbreaking changes this version\e[0m' >&2 +[[ -n "${unknown}" ]] && printf '\e[1;35munknown changes in this release -- categorize manually\e[0m' >&2 + +echo "" >&2 +echo "" >&2 +echo "# ${next_ver}" + +if [[ -n ${breaking} ]]; then + echo "" + echo "## :warning: Breaking Changes" + echo "" + echo "${breaking}" +fi + +if [[ -n ${features} ]]; then + echo "" + echo "## :sparkles: New Features" + echo "" + echo "${features}" +fi + +if [[ -n ${bugfixes} ]]; then + echo "" + echo "## :bug: Bug Fixes" + echo "" + echo "${bugfixes}" +fi + +if [[ -n ${unknown} ]]; then + echo "" + echo "## :question: *categorize these manually*" + echo "" + echo "${unknown}" +fi + +echo "" +echo "*Thanks to all our contributors!*" diff --git a/pkg/envtest/server.go b/pkg/envtest/server.go index 6756dae545..30cc18a233 100644 --- a/pkg/envtest/server.go +++ b/pkg/envtest/server.go @@ -59,7 +59,7 @@ var DefaultKubeAPIServerFlags = []string{ "--cert-dir={{ .CertDir }}", "--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}", "--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}", - "--secure-port=0", + "--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}", "--admission-control=AlwaysAdmit", } diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go index 4d6f80f6c7..6a44ea9fc3 100644 --- a/pkg/internal/controller/controller.go +++ b/pkg/internal/controller/controller.go @@ -175,7 +175,9 @@ func (c *Controller) processNextWorkItem() bool { // Update metrics after processing each item reconcileStartTS := time.Now() - defer c.updateMetrics(time.Now().Sub(reconcileStartTS)) + defer func() { + c.updateMetrics(time.Now().Sub(reconcileStartTS)) + }() obj, shutdown := c.Queue.Get() if obj == nil { @@ -214,13 +216,15 @@ func (c *Controller) processNextWorkItem() bool { c.Queue.AddRateLimited(req) log.Error(err, "Reconciler error", "controller", c.Name, "request", req) ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc() - + ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "error").Inc() return false } else if result.RequeueAfter > 0 { c.Queue.AddAfter(req, result.RequeueAfter) + ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "requeue_after").Inc() return true } else if result.Requeue { c.Queue.AddRateLimited(req) + ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "requeue").Inc() return true } @@ -231,6 +235,7 @@ func (c *Controller) processNextWorkItem() bool { // TODO(directxman12): What does 1 mean? Do we want level constants? Do we want levels at all? log.V(1).Info("Successfully Reconciled", "controller", c.Name, "request", req) + ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, "success").Inc() // Return true, don't take a break return true } diff --git a/pkg/internal/controller/controller_test.go b/pkg/internal/controller/controller_test.go index 1269c07a29..1e66cc6fe2 100644 --- a/pkg/internal/controller/controller_test.go +++ b/pkg/internal/controller/controller_test.go @@ -408,6 +408,130 @@ var _ = Describe("controller", func() { // TODO(community): write this test }) + Context("prometheus metric reconcile_total", func() { + var reconcileTotal dto.Metric + + BeforeEach(func() { + ctrlmetrics.ReconcileTotal.Reset() + reconcileTotal.Reset() + }) + + It("should get updated on successful reconciliation", func(done Done) { + Expect(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "success").Write(&reconcileTotal) + if reconcileTotal.GetCounter().GetValue() != 0.0 { + return fmt.Errorf("metric reconcile total not reset") + } + return nil + }()).Should(Succeed()) + + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(stop)).NotTo(HaveOccurred()) + }() + By("Invoking Reconciler which will succeed") + ctrl.Queue.Add(request) + + Expect(<-reconciled).To(Equal(request)) + Eventually(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "success").Write(&reconcileTotal) + if actual := reconcileTotal.GetCounter().GetValue(); actual != 1.0 { + return fmt.Errorf("metric reconcile total expected: %v and got: %v", 1.0, actual) + } + return nil + }, 2.0).Should(Succeed()) + + close(done) + }, 2.0) + + It("should get updated on reconcile errors", func(done Done) { + Expect(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "error").Write(&reconcileTotal) + if reconcileTotal.GetCounter().GetValue() != 0.0 { + return fmt.Errorf("metric reconcile total not reset") + } + return nil + }()).Should(Succeed()) + + fakeReconcile.Err = fmt.Errorf("expected error: reconcile") + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(stop)).NotTo(HaveOccurred()) + }() + By("Invoking Reconciler which will give an error") + ctrl.Queue.Add(request) + + Expect(<-reconciled).To(Equal(request)) + Eventually(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "error").Write(&reconcileTotal) + if actual := reconcileTotal.GetCounter().GetValue(); actual != 1.0 { + return fmt.Errorf("metric reconcile total expected: %v and got: %v", 1.0, actual) + } + return nil + }, 2.0).Should(Succeed()) + + close(done) + }, 2.0) + + It("should get updated when reconcile returns with retry enabled", func(done Done) { + Expect(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "retry").Write(&reconcileTotal) + if reconcileTotal.GetCounter().GetValue() != 0.0 { + return fmt.Errorf("metric reconcile total not reset") + } + return nil + }()).Should(Succeed()) + + fakeReconcile.Result.Requeue = true + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(stop)).NotTo(HaveOccurred()) + }() + By("Invoking Reconciler which will return result with Requeue enabled") + ctrl.Queue.Add(request) + + Expect(<-reconciled).To(Equal(request)) + Eventually(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "requeue").Write(&reconcileTotal) + if actual := reconcileTotal.GetCounter().GetValue(); actual != 1.0 { + return fmt.Errorf("metric reconcile total expected: %v and got: %v", 1.0, actual) + } + return nil + }, 2.0).Should(Succeed()) + + close(done) + }, 2.0) + + It("should get updated when reconcile returns with retryAfter enabled", func(done Done) { + Expect(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "retry_after").Write(&reconcileTotal) + if reconcileTotal.GetCounter().GetValue() != 0.0 { + return fmt.Errorf("metric reconcile total not reset") + } + return nil + }()).Should(Succeed()) + + fakeReconcile.Result.RequeueAfter = 5 * time.Hour + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(stop)).NotTo(HaveOccurred()) + }() + By("Invoking Reconciler which will return result with requeueAfter enabled") + ctrl.Queue.Add(request) + + Expect(<-reconciled).To(Equal(request)) + Eventually(func() error { + ctrlmetrics.ReconcileTotal.WithLabelValues(ctrl.Name, "requeue_after").Write(&reconcileTotal) + if actual := reconcileTotal.GetCounter().GetValue(); actual != 1.0 { + return fmt.Errorf("metric reconcile total expected: %v and got: %v", 1.0, actual) + } + return nil + }, 2.0).Should(Succeed()) + + close(done) + }, 2.0) + }) + Context("should update prometheus metrics", func() { It("should requeue a Request if there is an error and continue processing items", func(done Done) { var queueLength, reconcileErrs dto.Metric @@ -415,7 +539,7 @@ var _ = Describe("controller", func() { Expect(func() error { ctrlmetrics.QueueLength.WithLabelValues(ctrl.Name).Write(&queueLength) if queueLength.GetGauge().GetValue() != 0.0 { - return fmt.Errorf("metrics not reset") + return fmt.Errorf("metric queue length not reset") } return nil }()).Should(Succeed()) @@ -424,7 +548,7 @@ var _ = Describe("controller", func() { Expect(func() error { ctrlmetrics.ReconcileErrors.WithLabelValues(ctrl.Name).Write(&reconcileErrs) if reconcileErrs.GetCounter().GetValue() != 0.0 { - return fmt.Errorf("metrics not reset") + return fmt.Errorf("metric reconcile errors not reset") } return nil }()).Should(Succeed()) @@ -444,7 +568,7 @@ var _ = Describe("controller", func() { Eventually(func() error { ctrlmetrics.QueueLength.WithLabelValues(ctrl.Name).Write(&queueLength) if queueLength.GetGauge().GetValue() != 1.0 { - return fmt.Errorf("metrics not updated") + return fmt.Errorf("metric queue length not updated") } return nil }, 2.0).Should(Succeed()) diff --git a/pkg/internal/controller/metrics/metrics.go b/pkg/internal/controller/metrics/metrics.go index 944ca8e035..1411a8d846 100644 --- a/pkg/internal/controller/metrics/metrics.go +++ b/pkg/internal/controller/metrics/metrics.go @@ -29,24 +29,34 @@ var ( Help: "Length of reconcile queue per controller", }, []string{"controller"}) + // ReconcileTotal is a prometheus counter metrics which holds the total + // number of reconciliations per controller. It has two labels. controller label refers + // to the controller name and result label refers to the reconcile result i.e + // success, error, requeue, requeue_after + ReconcileTotal = prometheus.NewCounterVec(prometheus.CounterOpts{ + Name: "controller_runtime_reconcile_total", + Help: "Total number of reconciliations per controller", + }, []string{"controller", "result"}) + // ReconcileErrors is a prometheus counter metrics which holds the total // number of errors from the Reconciler ReconcileErrors = prometheus.NewCounterVec(prometheus.CounterOpts{ Name: "controller_runtime_reconcile_errors_total", - Help: "Total number of reconcile errors per controller", + Help: "Total number of reconciliation errors per controller", }, []string{"controller"}) // ReconcileTime is a prometheus metric which keeps track of the duration - // of reconciles + // of reconciliations ReconcileTime = prometheus.NewHistogramVec(prometheus.HistogramOpts{ Name: "controller_runtime_reconcile_time_seconds", - Help: "Length of time per reconcile per controller", + Help: "Length of time per reconciliation per controller", }, []string{"controller"}) ) func init() { metrics.Registry.MustRegister( QueueLength, + ReconcileTotal, ReconcileErrors, ReconcileTime, // expose process metrics like CPU, Memory, file descriptor usage etc. diff --git a/pkg/webhook/admission/http.go b/pkg/webhook/admission/http.go index 824b6af063..5b91ad2b50 100644 --- a/pkg/webhook/admission/http.go +++ b/pkg/webhook/admission/http.go @@ -24,6 +24,7 @@ import ( "io" "io/ioutil" "net/http" + "time" "k8s.io/api/admission/v1beta1" admissionv1beta1 "k8s.io/api/admission/v1beta1" @@ -31,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/serializer" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/webhook/admission/types" + "sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics" ) var admissionv1beta1scheme = runtime.NewScheme() @@ -47,6 +49,9 @@ func addToScheme(scheme *runtime.Scheme) { var _ http.Handler = &Webhook{} func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { + startTS := time.Now() + defer metrics.RequestLatency.WithLabelValues(wh.Name).Observe(time.Now().Sub(startTS).Seconds()) + var body []byte var err error @@ -55,14 +60,14 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if body, err = ioutil.ReadAll(r.Body); err != nil { log.Error(err, "unable to read the body from the incoming request") reviewResponse = ErrorResponse(http.StatusBadRequest, err) - writeResponse(w, reviewResponse) + wh.writeResponse(w, reviewResponse) return } } else { err = errors.New("request body is empty") log.Error(err, "bad request") reviewResponse = ErrorResponse(http.StatusBadRequest, err) - writeResponse(w, reviewResponse) + wh.writeResponse(w, reviewResponse) return } @@ -72,7 +77,7 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { err = fmt.Errorf("contentType=%s, expect application/json", contentType) log.Error(err, "unable to process a request with an unknown content type", "content type", contentType) reviewResponse = ErrorResponse(http.StatusBadRequest, err) - writeResponse(w, reviewResponse) + wh.writeResponse(w, reviewResponse) return } @@ -80,16 +85,24 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) { if _, _, err := admissionv1beta1schemecodecs.UniversalDeserializer().Decode(body, nil, &ar); err != nil { log.Error(err, "unable to decode the request") reviewResponse = ErrorResponse(http.StatusBadRequest, err) - writeResponse(w, reviewResponse) + wh.writeResponse(w, reviewResponse) return } // TODO: add panic-recovery for Handle reviewResponse = wh.Handle(context.Background(), types.Request{AdmissionRequest: ar.Request}) - writeResponse(w, reviewResponse) + wh.writeResponse(w, reviewResponse) } -func writeResponse(w io.Writer, response types.Response) { +func (wh *Webhook) writeResponse(w io.Writer, response types.Response) { + if response.Response.Result.Code != 0 { + if response.Response.Result.Code == http.StatusOK { + metrics.TotalRequests.WithLabelValues(wh.Name, "true").Inc() + } else { + metrics.TotalRequests.WithLabelValues(wh.Name, "false").Inc() + } + } + encoder := json.NewEncoder(w) responseAdmissionReview := v1beta1.AdmissionReview{ Response: response.Response, @@ -97,6 +110,6 @@ func writeResponse(w io.Writer, response types.Response) { err := encoder.Encode(responseAdmissionReview) if err != nil { log.Error(err, "unable to encode the response") - writeResponse(w, ErrorResponse(http.StatusInternalServerError, err)) + wh.writeResponse(w, ErrorResponse(http.StatusInternalServerError, err)) } } diff --git a/pkg/webhook/admission/http_test.go b/pkg/webhook/admission/http_test.go index cdf6a90e77..8d3c0507b4 100644 --- a/pkg/webhook/admission/http_test.go +++ b/pkg/webhook/admission/http_test.go @@ -134,7 +134,7 @@ var _ = Describe("admission webhook http handler", func() { Type: types.WebhookTypeValidating, Handlers: []Handler{h}, } - expected := []byte(`{"response":{"uid":"","allowed":true}} + expected := []byte(`{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200}}} `) It("should return a response successfully", func() { wh.ServeHTTP(w, req) diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 0ae7a94177..ad908b4a54 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -132,6 +132,7 @@ func (w *Webhook) handleMutating(ctx context.Context, req atypes.Request) atypes for _, handler := range w.Handlers { resp := handler.Handle(ctx, req) if !resp.Response.Allowed { + setStatusOKInAdmissionResponse(resp.Response) return resp } if resp.Response.PatchType != nil && *resp.Response.PatchType != admissionv1beta1.PatchTypeJSONPatch { @@ -148,7 +149,10 @@ func (w *Webhook) handleMutating(ctx context.Context, req atypes.Request) atypes } return atypes.Response{ Response: &admissionv1beta1.AdmissionResponse{ - Allowed: true, + Allowed: true, + Result: &metav1.Status{ + Code: http.StatusOK, + }, Patch: marshaledPatch, PatchType: func() *admissionv1beta1.PatchType { pt := admissionv1beta1.PatchTypeJSONPatch; return &pt }(), }, @@ -159,16 +163,32 @@ func (w *Webhook) handleValidating(ctx context.Context, req atypes.Request) atyp for _, handler := range w.Handlers { resp := handler.Handle(ctx, req) if !resp.Response.Allowed { + setStatusOKInAdmissionResponse(resp.Response) return resp } } return atypes.Response{ Response: &admissionv1beta1.AdmissionResponse{ Allowed: true, + Result: &metav1.Status{ + Code: http.StatusOK, + }, }, } } +func setStatusOKInAdmissionResponse(resp *admissionv1beta1.AdmissionResponse) { + if resp == nil { + return + } + if resp.Result == nil { + resp.Result = &metav1.Status{} + } + if resp.Result.Code == 0 { + resp.Result.Code = http.StatusOK + } +} + // GetName returns the name of the webhook. func (w *Webhook) GetName() string { w.once.Do(w.setDefaults) diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index abdca8ae8e..5fe9b2aa6a 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -83,7 +83,7 @@ var _ = Describe("admission webhook", func() { }) It("should deny the request", func() { - expected := []byte(`{"response":{"uid":"","allowed":false}} + expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} `) wh.ServeHTTP(w, req) Expect(w.Body.Bytes()).To(Equal(expected)) @@ -102,7 +102,7 @@ var _ = Describe("admission webhook", func() { }) It("should deny the request", func() { - expected := []byte(`{"response":{"uid":"","allowed":false}} + expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} `) wh.ServeHTTP(w, req) Expect(w.Body.Bytes()).To(Equal(expected)) @@ -162,7 +162,8 @@ var _ = Describe("admission webhook", func() { Handlers: []Handler{patcher1, patcher2}, } expected := []byte( - `{"response":{"uid":"","allowed":true,"patch":"W3sib3AiOiJhZGQiLCJwYXRoIjoiL21ldGFkYXRhL2Fubm90YXRpb2` + + `{"response":{"uid":"","allowed":true,"status":{"metadata":{},"code":200},` + + `"patch":"W3sib3AiOiJhZGQiLCJwYXRoIjoiL21ldGFkYXRhL2Fubm90YXRpb2` + `4vbmV3LWtleSIsInZhbHVlIjoibmV3LXZhbHVlIn0seyJvcCI6InJlcGxhY2UiLCJwYXRoIjoiL3NwZWMvcmVwbGljYXMiLC` + `J2YWx1ZSI6IjIifSx7Im9wIjoiYWRkIiwicGF0aCI6Ii9tZXRhZGF0YS9hbm5vdGF0aW9uL2hlbGxvIiwidmFsdWUiOiJ3b3JsZCJ9XQ==",` + `"patchType":"JSONPatch"}} @@ -213,7 +214,7 @@ var _ = Describe("admission webhook", func() { Type: types.WebhookTypeMutating, Handlers: []Handler{errPatcher}, } - expected := []byte(`{"response":{"uid":"","allowed":false}} + expected := []byte(`{"response":{"uid":"","allowed":false,"status":{"metadata":{},"code":200}}} `) It("should deny the request", func() { wh.ServeHTTP(w, req) diff --git a/pkg/webhook/bootstrap.go b/pkg/webhook/bootstrap.go index f0ac9425e3..b789c05315 100644 --- a/pkg/webhook/bootstrap.go +++ b/pkg/webhook/bootstrap.go @@ -297,6 +297,17 @@ func (s *Server) validatingWHConfigs() (runtime.Object, error) { } func (s *Server) admissionWebhook(path string, wh *admission.Webhook) (*admissionregistration.Webhook, error) { + if wh.NamespaceSelector == nil && len(s.Service.Namespace) > 0 { + wh.NamespaceSelector = &metav1.LabelSelector{ + MatchExpressions: []metav1.LabelSelectorRequirement{ + { + Key: "control-plane", + Operator: metav1.LabelSelectorOpDoesNotExist, + }, + }, + } + } + webhook := &admissionregistration.Webhook{ Name: wh.GetName(), Rules: wh.Rules, diff --git a/pkg/webhook/internal/metrics/metrics.go b/pkg/webhook/internal/metrics/metrics.go new file mode 100644 index 0000000000..ff82c10d8d --- /dev/null +++ b/pkg/webhook/internal/metrics/metrics.go @@ -0,0 +1,51 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package metrics + +import ( + "github.com/prometheus/client_golang/prometheus" + + "sigs.k8s.io/controller-runtime/pkg/metrics" +) + +var ( + // TotalRequests is a prometheus metric which counts the total number of requests that + // the webhook server has received. + TotalRequests = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "controller_runtime_webhook_requests_total", + Help: "Total number of admission requests", + }, + []string{"webhook", "succeeded"}, + ) + + // RequestLatency is a prometheus metric which is a histogram of the latency + // of processing admission requests. + RequestLatency = prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Name: "controller_runtime_webhook_latency_seconds", + Help: "Histogram of the latency of processing admission requests", + }, + []string{"webhook"}, + ) +) + +func init() { + metrics.Registry.MustRegister( + TotalRequests, + RequestLatency) +} diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/addr/manager.go b/vendor/sigs.k8s.io/testing_frameworks/integration/addr/manager.go new file mode 100644 index 0000000000..7523ce860c --- /dev/null +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/addr/manager.go @@ -0,0 +1,24 @@ +package addr + +import ( + "net" +) + +// Suggest suggests a address a process can listen on. It returns +// a tuple consisting of a free port and the hostname resolved to its IP. +func Suggest() (port int, resolvedHost string, err error) { + addr, err := net.ResolveTCPAddr("tcp", "localhost:0") + if err != nil { + return + } + l, err := net.ListenTCP("tcp", addr) + if err != nil { + return + } + port = l.Addr().(*net.TCPAddr).Port + defer func() { + err = l.Close() + }() + resolvedHost = addr.IP.String() + return +} diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/apiserver.go b/vendor/sigs.k8s.io/testing_frameworks/integration/apiserver.go index 3bf2d81b6c..4f1e28267d 100644 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/apiserver.go +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/apiserver.go @@ -6,6 +6,7 @@ import ( "net/url" "time" + "sigs.k8s.io/testing_frameworks/integration/addr" "sigs.k8s.io/testing_frameworks/integration/internal" ) @@ -16,6 +17,9 @@ type APIServer struct { // If this is not specified, we default to a random free port on localhost. URL *url.URL + // SecurePort is the additional secure port that the APIServer should listen on. + SecurePort int + // Path is the path to the apiserver binary. // // If this is left as the empty string, we will attempt to locate a binary, @@ -87,6 +91,14 @@ func (s *APIServer) Start() error { return err } + // Defaulting the secure port + if s.SecurePort == 0 { + s.SecurePort, _, err = addr.Suggest() + if err != nil { + return err + } + } + s.processState.HealthCheckEndpoint = "/healthz" s.URL = &s.processState.URL @@ -110,3 +122,10 @@ func (s *APIServer) Start() error { func (s *APIServer) Stop() error { return s.processState.Stop() } + +// APIServerDefaultArgs exposes the default args for the APIServer so that you +// can use those to append your own additional arguments. +// +// The internal default arguments are explicitely copied here, we don't want to +// allow users to change the internal ones. +var APIServerDefaultArgs = append([]string{}, internal.APIServerDefaultArgs...) diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/doc.go b/vendor/sigs.k8s.io/testing_frameworks/integration/doc.go index 01bc5ce558..bbd88d94a9 100644 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/doc.go +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/doc.go @@ -78,7 +78,7 @@ location (`${FRAMEWORK_DIR}/assets/bin/`). Arguments for Etcd and APIServer -Those components will start without any configuration. However, if you want our +Those components will start without any configuration. However, if you want or need to, you can override certain configuration -- one of which are the arguments used when calling the binary. @@ -86,6 +86,13 @@ When you choose to specify your own set of arguments, those won't be appended to the default set of arguments, it is your responsibility to provide all the arguments needed for the binary to start successfully. +However, the default arguments for APIServer and Etcd are exported as +`APIServerDefaultArgs` and `EtcdDefaultArgs` from this package. Treat those +variables as read-only constants. Internally we have a set of default +arguments for defaulting, the `APIServerDefaultArgs` and `EtcdDefaultArgs` are +just copies of those. So when you override them you loose access to the actual +internal default arguments, but your override won't affect the defaulting. + All arguments are interpreted as go templates. Those templates have access to all exported fields of the `APIServer`/`Etcd` struct. It does not matter if those fields where explicitly set up or if they were defaulted by calling the @@ -93,19 +100,18 @@ those fields where explicitly set up or if they were defaulted by calling the executed and right after the defaulting of all the struct's fields has happened. - // All arguments needed for a successful start must be specified - etcdArgs := []string{ - "--listen-peer-urls=http://localhost:0", - "--advertise-client-urls={{ .URL.String }}", - "--listen-client-urls={{ .URL.String }}", - "--data-dir={{ .DataDir }}", - // add some custom arguments - "--this-is-my-very-important-custom-argument", - "--arguments-dont-have-to-be-templates=but they can", + // When you want to append additional arguments ... + etcd := &Etcd{ + // Additional custom arguments will appended to the set of default + // arguments + Args: append(EtcdDefaultArgs, "--additional=arg"), + DataDir: "/my/special/data/dir", } + // When you want to use a custom set of arguments ... etcd := &Etcd{ - Args: etcdArgs, + // Only custom arguments will be passed to the binary + Args: []string{"--one=1", "--two=2", "--three=3"}, DataDir: "/my/special/data/dir", } diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/etcd.go b/vendor/sigs.k8s.io/testing_frameworks/integration/etcd.go index 7bfa758e29..43cb314d7b 100644 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/etcd.go +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/etcd.go @@ -100,3 +100,10 @@ func (e *Etcd) Start() error { func (e *Etcd) Stop() error { return e.processState.Stop() } + +// EtcdDefaultArgs exposes the default args for Etcd so that you +// can use those to append your own additional arguments. +// +// The internal default arguments are explicitely copied here, we don't want to +// allow users to change the internal ones. +var EtcdDefaultArgs = append([]string{}, internal.EtcdDefaultArgs...) diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/address_manager.go b/vendor/sigs.k8s.io/testing_frameworks/integration/internal/address_manager.go deleted file mode 100644 index 7ad4ab7eaa..0000000000 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/address_manager.go +++ /dev/null @@ -1,53 +0,0 @@ -package internal - -import ( - "fmt" - "net" -) - -// AddressManager allocates a new address (interface & port) a process -// can bind and keeps track of that. -type AddressManager struct { - port int - host string -} - -// Initialize returns a address a process can listen on. It returns -// a tuple consisting of a free port and the hostname resolved to its IP. -func (d *AddressManager) Initialize() (port int, resolvedHost string, err error) { - if d.port != 0 { - return 0, "", fmt.Errorf("this AddressManager is already initialized") - } - addr, err := net.ResolveTCPAddr("tcp", "localhost:0") - if err != nil { - return - } - l, err := net.ListenTCP("tcp", addr) - if err != nil { - return - } - d.port = l.Addr().(*net.TCPAddr).Port - defer func() { - err = l.Close() - }() - d.host = addr.IP.String() - return d.port, d.host, nil -} - -// Port returns the port that this AddressManager is managing. Port returns an -// error if this AddressManager has not yet been initialized. -func (d *AddressManager) Port() (int, error) { - if d.port == 0 { - return 0, fmt.Errorf("this AdressManager is not initialized yet") - } - return d.port, nil -} - -// Host returns the host that this AddressManager is managing. Host returns an -// error if this AddressManager has not yet been initialized. -func (d *AddressManager) Host() (string, error) { - if d.host == "" { - return "", fmt.Errorf("this AdressManager is not initialized yet") - } - return d.host, nil -} diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/apiserver.go b/vendor/sigs.k8s.io/testing_frameworks/integration/internal/apiserver.go index a5256aee08..68a54dc751 100644 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/apiserver.go +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/internal/apiserver.go @@ -5,7 +5,7 @@ var APIServerDefaultArgs = []string{ "--cert-dir={{ .CertDir }}", "--insecure-port={{ if .URL }}{{ .URL.Port }}{{ end }}", "--insecure-bind-address={{ if .URL }}{{ .URL.Hostname }}{{ end }}", - "--secure-port=0", + "--secure-port={{ if .SecurePort }}{{ .SecurePort }}{{ end }}", } func DoAPIServerArgDefaulting(args []string) []string { diff --git a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/process.go b/vendor/sigs.k8s.io/testing_frameworks/integration/internal/process.go index 620c6a99c5..d7a0885668 100644 --- a/vendor/sigs.k8s.io/testing_frameworks/integration/internal/process.go +++ b/vendor/sigs.k8s.io/testing_frameworks/integration/internal/process.go @@ -13,6 +13,8 @@ import ( "github.com/onsi/gomega/gbytes" "github.com/onsi/gomega/gexec" + + "sigs.k8s.io/testing_frameworks/integration/addr" ) type ProcessState struct { @@ -63,8 +65,7 @@ func DoDefaulting( } if listenUrl == nil { - am := &AddressManager{} - port, host, err := am.Initialize() + port, host, err := addr.Suggest() if err != nil { return DefaultedProcessInput{}, err }