Skip to content
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

Issue 7, 156: Admission webhook #159

Merged
merged 24 commits into from
Apr 15, 2019
Merged

Issue 7, 156: Admission webhook #159

merged 24 commits into from
Apr 15, 2019

Conversation

Tristan1900
Copy link
Member

@Tristan1900 Tristan1900 commented Apr 5, 2019

Change log description

  • Add admission webhook that supports version validation
  • Add unit test
  • Add e2e test

What the code does

There are two concepts here, the admission webhook server and the admission webhook. Admission webhook server is like an http server that serves the incoming requests. Admission webhook implements the application logic and register itself to the webhook server, there are two kinds of admission webhook, mutating admission webhook and validating admission Webhook, right now we only use mutating admission Webhook. The admission webhook server starts in the Operator pod with a fronting k8s service. When requests come into k8s, apiserver will route the request to that service and the backend admission webhook server will get that request and call the admission webhook for processing. The port that admission webhook server listens to is port 443. To bind port 443, we need to grant permission to Operator process using setcap CAP_NET_BIND_SERVICE=+eip.

There is a supported version list in our webhook. Any requests with an invalid Pravega version will be rejected. For example 0.6.0 is an invalid Pravega version because there is no such released version yet. There is also a supported upgrade path in our webhook, it will reject the request if the upgrade is not supported. For example, rolling upgrade from 0.3.0 to 0.4.0 is not supported. However, any upgrade to patch version is supported, such as 0.4.0 to 0.4.1.

Purpose of the change

How to test the change

Tests should pass


Signed-off-by: wenqi [email protected]

@adrianmo adrianmo changed the title (WIP) Admission webhook (WIP) Issue 7, 156: Admission webhook Apr 5, 2019
@adrianmo adrianmo added kind/enhancement Enhancement of an existing feature status/work-in-progress PR work is in progress; do not merge labels Apr 5, 2019
@adrianmo adrianmo changed the title (WIP) Issue 7, 156: Admission webhook Issue 7, 156: Admission webhook Apr 9, 2019
Copy link
Contributor

@adrianmo adrianmo left a comment

Choose a reason for hiding this comment

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

@Tristan1900 you did a great work with the admission webhook! I've added a few comments through the code :)

Dockerfile Show resolved Hide resolved
deploy/operator.yaml Outdated Show resolved Hide resolved
pkg/util/pravegacluster.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
wenqi added 5 commits April 9, 2019 15:44
Signed-off-by: wenqi <[email protected]>
Signed-off-by: wenqi <[email protected]>
Signed-off-by: wenqi <[email protected]>
Signed-off-by: wenqi <[email protected]>
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Outdated Show resolved Hide resolved
pkg/webhook/webhook.go Show resolved Hide resolved
@Tristan1900
Copy link
Member Author

Tristan1900 commented Apr 12, 2019

Thanks @adrianmo for the comments and changes! Could you please review it again? Thanks!

wenqi and others added 2 commits April 11, 2019 22:26
Signed-off-by: wenqi <[email protected]>
Signed-off-by: Adrián Moreno <[email protected]>
@adrianmo adrianmo force-pushed the admission-webhook branch from 40d4ddf to 8dd56fa Compare April 12, 2019 10:37
Copy link
Contributor

@adrianmo adrianmo left a comment

Choose a reason for hiding this comment

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

@Tristan1900 thanks for updating the PR and addressing my comment. It looks good, but I'm worried about one thing. We have lost the ability to run the operator locally, which has been very useful for testing out changes during the development. Have you investigated if it's there a way to re-enable it again? If we can't find a way and the webhook server is causing issues, it'd be OK from my side to explicitly disable the webhook when running the operator locally. What do you think?

@adrianmo adrianmo added status/ready The issue is ready to be worked on; or the PR is ready to review and removed status/work-in-progress PR work is in progress; do not merge labels Apr 15, 2019
@adrianmo adrianmo merged commit 3218b69 into master Apr 15, 2019
@adrianmo adrianmo deleted the admission-webhook branch April 15, 2019 15:21
@Tristan1900
Copy link
Member Author

@adrianmo Thanks for pointing that out! It makes sense to disable webhook when using up local. I made the change last Friday to address this but forgot to reply to your comments. Sorry about that. I see you've merged this PR, thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Enhancement of an existing feature status/ready The issue is ready to be worked on; or the PR is ready to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate upgrade requests Admission Controller for PravegaCluster resource
2 participants