Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

Add kubefed validation webhook #862

Merged
merged 6 commits into from
May 16, 2019
Merged

Conversation

font
Copy link
Contributor

@font font commented May 8, 2019

NOTE: This PR is not reviewable until #857 #867 is merged.

This adds the scaffolding to enable kubefed webhook validation for core API types we plan to move to beta. Particularly the 2 APIs added for validation in this PR:

  1. FederatedTypeConfig: currently only verifies target kind is set.
  2. FederatedCluster: no validation at the moment.

This PR can be merged (pending #857 #867 merging) as follow-on work/discussion is needed to implement the actual validation pieces.

@pmorie did all the work here. I've just mainly added scaffolding for FederatedCluster while cleaning up the history, fixing few chart and deployment issues, etc.

TODO in follow-up PRs:

  • Implement validation pieces for core API types moving to beta
  • Add support for using k8s CA instead of self-generated one
  • Tests

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2019
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 8, 2019
@marun
Copy link
Contributor

marun commented May 9, 2019

Before WIP is removed, I'd like to see the reviewable changes squashed down to as few orthogonal commits as possible and highlighted for potential reviewers. The amount of unreviewable content in this PR makes reviewing via 'Files Changed' all but impossible, and having to review changes commit by commit only works if the commits are few in number and easily identifiable as orthogonal.

@font font force-pushed the webhook branch 3 times, most recently from d7e6341 to 330fadc Compare May 9, 2019 17:14
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 10, 2019
@font
Copy link
Contributor Author

font commented May 10, 2019

@marun Is this more manageable now that deps have been merged separately?

@font font changed the title WIP: Add kubefed validation webhook Add kubefed validation webhook May 10, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2019
@font
Copy link
Contributor Author

font commented May 10, 2019

I've rebased and this is now ready for review.

@shashidharatd
Copy link
Contributor

@font, is there any particular reason this PR not using the scaffolding provided by kubebuilder for webhooks? https://book.kubebuilder.io/beyond_basics/webhook_installer_generator.html

@xunpan
Copy link
Contributor

xunpan commented May 13, 2019

+1 with @shashidharatd. With kubebuilder, we do not need to maintain an independent webhook binary. It seems simpler.

@pmorie
Copy link
Contributor

pmorie commented May 13, 2019

@font, is there any particular reason this PR not using the scaffolding provided by kubebuilder for webhooks? https://book.kubebuilder.io/beyond_basics/webhook_installer_generator.html

From what I was able to tell, the kubebuilder support for webhooks is very minimal and depends on you using a particular controller framework which we are not using. We can refactor to it in the future if we want to but ironically using the kubebuilder facility would require significantly more intrusive changes than the approach this PR takes.

we do not need to maintain an independent webhook binary. It seems simpler

We're not adding a new binary; the webhook server is integrated with hyperfed so there will not be a new binary present in the image, just another symlink to hyperfed.

@irfanurrehman
Copy link
Contributor

@font @pmorie I am not sure how is this tested. Is there a plan to introduce testing for these validations at some point?

@pmorie
Copy link
Contributor

pmorie commented May 13, 2019

@font @pmorie I am not sure how is this tested. Is there a plan to introduce testing for these validations at some point?

Generally validations are tested in unit tests for the validation methods; from an e2e standpoint they're exercised every time an object is created so they are naturally exercised by e2e scenarios. I think it would be fine to add a validation test for the initial validation in this PR in a follow, since the travis job has passed.

WDYT @irfanurrehman

Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

'webhook' is a very generic term. Is there a reason not to use a more specific name (e.g. admission-webhook), or is more than admission behavior expected to be implemented?

charts/federation-v2/charts/webhook/Chart.yaml Outdated Show resolved Hide resolved
charts/federation-v2/README.md Outdated Show resolved Hide resolved
cmd/hyperfed/main.go Outdated Show resolved Hide resolved
cmd/webhook/main.go Outdated Show resolved Hide resolved
cmd/webhook/main.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: font

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

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2019
@font font force-pushed the webhook branch 3 times, most recently from 14bf01f to abbfea5 Compare May 15, 2019 23:50
@font
Copy link
Contributor Author

font commented May 16, 2019

@marun pending CI passing this should be ready to re-review.

Copy link
Contributor

@marun marun left a comment

Choose a reason for hiding this comment

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

Functionally this LGTM. Some reversion of accidental rename changes required (the joy of rebasing after a rename).

Makefile Show resolved Hide resolved
cmd/hyperfed/main.go Outdated Show resolved Hide resolved
scripts/deploy-federation.sh Outdated Show resolved Hide resolved
scripts/deploy-federation.sh Outdated Show resolved Hide resolved
scripts/deploy-federation.sh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@font font left a comment

Choose a reason for hiding this comment

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

@marun Not sure how some of those got in there as they should never have been conflicts. I've updated. PTAL.

cmd/hyperfed/main.go Outdated Show resolved Hide resolved
scripts/deploy-federation.sh Outdated Show resolved Hide resolved
scripts/deploy-federation.sh Outdated Show resolved Hide resolved
@marun
Copy link
Contributor

marun commented May 16, 2019

LGTM

@irfanurrehman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 16, 2019
@k8s-ci-robot k8s-ci-robot merged commit ec2b1a7 into kubernetes-retired:master May 16, 2019
@font font deleted the webhook branch May 22, 2019 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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 Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants