feat: Implement the admission server and a validation webhook for plugins #573

fgksgf · 2021-07-06T01:27:50Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bugfix
New feature provided
Improve performance
Backport patches
Related issues
Implement the admission server #244

New feature or improvement

Describe the details and related test reports.
Implement an admission server via kubewebhook to validate configurations like plugins.

TODO

implement an admission server
implement a validating webhook for plugins
add YAML of webhook
add e2e test

codecov-commenter · 2021-07-06T01:30:27Z

Codecov Report

Merging #573 (240ffae) into master (62b7590) will increase coverage by 0.40%.
The diff coverage is 50.64%.

@@            Coverage Diff             @@
##           master     #573      +/-   ##
==========================================
+ Coverage   34.31%   34.72%   +0.40%     
==========================================
  Files          57       60       +3     
  Lines        5746     5892     +146     
==========================================
+ Hits         1972     2046      +74     
- Misses       3527     3596      +69     
- Partials      247      250       +3

Impacted Files	Coverage Δ
pkg/api/router/router.go	`75.00% <ø> (ø)`
pkg/api/validation/apisix_route.go	`21.53% <21.53%> (ø)`
pkg/api/validation/utils.go	`46.15% <46.15%> (ø)`
pkg/api/server.go	`81.81% <79.31%> (+5.95%)`	⬆️
cmd/ingress/ingress.go	`77.27% <100.00%> (+0.26%)`	⬆️
pkg/api/router/webhook.go	`100.00% <100.00%> (ø)`
pkg/config/config.go	`86.44% <100.00%> (+0.72%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62b7590...240ffae. Read the comment docs.

pkg/api/validation/plugin_schema.go

gxthrj

BTW, Need to add e2e test case to cover the logic of validating with the schema from APISIX.

pkg/api/validation/apisix_route.go

pkg/api/router/webhook.go

fgksgf · 2021-08-15T06:56:25Z

BTW, Need to add e2e test case to cover the logic of validating with the schema from APISIX.

Ok, where should I put the code, test/e2e/features/ or test/e2e/ingress/?

Makefile

cmd/ingress/ingress.go

conf/config-default.yaml

tokers · 2021-08-23T01:04:26Z

pkg/api/server.go

+		httpServer.GET("/debug/pprof/*profile", gin.WrapF(srv.pprofMu.ServeHTTP))
+	}
+
+	cert, err := tls.LoadX509KeyPair(cfg.CertFilePath, cfg.KeyFilePath)


Should it be optional? Only enable TLS if the certificate and private key are specified simultaneously.

Because cfg.CertFilePath and cfg.KeyFilePath have default values.

Then you should prepare the default certificate and private key, or it's difficult to start the server in the local environment.

pkg/api/server.go

pkg/api/validation/apisix_route.go

tokers · 2021-08-23T01:11:22Z

pkg/api/validation/utils.go

+
+// GetSchemaClient returns a Schema client in the singleton way.
+// It can query the schema of objects from APISIX.
+func GetSchemaClient() (apisix.Schema, error) {


Why fetch schema in the singleton way, schema is synchronized periodically.

I didn't get your point, it seems that the singleton will not interfere with the synchronization process.

Just ignore my comment as long as the schema cache is refreshed periodically.

pkg/api/validation/utils.go

test/e2e/ingress/webhook.go

tokers · 2021-08-23T01:15:13Z

@fgksgf After this PR was approved, we need to change helm chart so that people can use it.

test/e2e/scaffold/ssl.go

test/e2e/scaffold/scaffold.go

utils/webhook-create-signed-cert.sh

fgksgf · 2021-08-25T13:40:50Z

@gxthrj @tokers @tao12345666333 PTAL

test/e2e/scaffold/ingress.go

samples/deploy/admission/webhook-registration.yaml

pkg/api/server.go

pkg/api/router/webhook.go

tokers · 2021-08-30T02:09:30Z

pkg/api/server.go

+		httpServer.GET("/debug/pprof/*profile", gin.WrapF(srv.pprofMu.ServeHTTP))
+	}
+
+	cert, err := tls.LoadX509KeyPair(cfg.CertFilePath, cfg.KeyFilePath)


Then you should prepare the default certificate and private key, or it's difficult to start the server in the local environment.

pkg/api/server.go

pkg/api/validation/apisix_route.go

test/e2e/scaffold/scaffold.go

fgksgf · 2021-08-30T07:10:52Z

reply: #573 (comment)

If we provide default TLS Certificates, they are not signed by CA, and the webhook still can't work. We can accomplish this after we have cert-manager.

I can help integrate cert-manager after webhooks are implemented.

tokers · 2021-08-30T13:42:48Z

reply: #573 (comment)

If we provide default TLS Certificates, they are not signed by CA, and the webhook still can't work. We can accomplish this after we have cert-manager.

I can help integrate cert-manager after webhooks are implemented.

We may use CertificateSigningRequest, and give documentation to guide people.

fgksgf · 2021-08-30T13:45:40Z

reply: #573 (comment)

If we provide default TLS Certificates, they are not signed by CA, and the webhook still can't work. We can accomplish this after we have cert-manager.
I can help integrate cert-manager after webhooks are implemented.

We may use CertificateSigningRequest, and give documentation to guide people.

Good idea. But how about file a new PR to do this, because the current PR is a bit huge.

tokers · 2021-08-30T13:46:47Z

reply: #573 (comment)

If we provide default TLS Certificates, they are not signed by CA, and the webhook still can't work. We can accomplish this after we have cert-manager.
I can help integrate cert-manager after webhooks are implemented.

We may use CertificateSigningRequest, and give documentation to guide people.

Good idea. But how about file a new PR to do this, because the current PR is a bit huge.

Sure.

fgksgf · 2021-08-30T13:58:21Z

Webhook E2E test:

gxthrj reviewed Jul 6, 2021

View reviewed changes

pkg/api/validation/plugin_schema.go Outdated Show resolved Hide resolved

fgksgf added 8 commits August 14, 2021 21:18

rebase master

6eaf6eb

rebase master

8592e69

implement the validating webhook for plugins

c3c5c60

add webhook manifests

428a3d2

add webhook manifests

0c2d854

fix lint

27c344b

rebase master

087e0ea

update go mod

a0a5438

fgksgf force-pushed the webhook branch from 3881465 to a0a5438 Compare August 14, 2021 13:23

fix ci

cacdc3b

fgksgf marked this pull request as ready for review August 14, 2021 13:34

fgksgf requested a review from gxthrj August 14, 2021 13:34

gxthrj reviewed Aug 14, 2021

View reviewed changes

pkg/api/validation/apisix_route.go Show resolved Hide resolved

pkg/api/router/webhook.go Outdated Show resolved Hide resolved

address review comments

586e5eb

add unit test for webhook

3ee39a0

tao12345666333 added this to the 1.3.0 milestone Aug 18, 2021

fgksgf added 4 commits August 22, 2021 22:19

debug webhook

e080c56

debug webhook

c564a26

e2e test

d3c52d4

webhook manifests

d5f0f91

fgksgf changed the title ~~[WIP] Implement the admission server~~ Implement the admission server Aug 22, 2021

tao12345666333 requested review from gxthrj, tokers and tao12345666333 August 22, 2021 15:59

delete focus in e2e test

bb99fa4

fgksgf changed the title ~~Implement the admission server~~ feat: Implement the admission server and a validation webhook for plugins Aug 23, 2021

tokers reviewed Aug 23, 2021

View reviewed changes

gxthrj added the area/admission label Aug 23, 2021

fgksgf added 3 commits August 25, 2021 21:30

address review comments

46d49cf

debug e2e test

25fda76

Merge branch 'master' into webhook

9fd71ab

fgksgf commented Aug 25, 2021

View reviewed changes

test/e2e/scaffold/ssl.go Outdated Show resolved Hide resolved

fgksgf commented Aug 25, 2021

View reviewed changes

test/e2e/scaffold/scaffold.go Outdated Show resolved Hide resolved

fgksgf commented Aug 25, 2021

View reviewed changes

utils/webhook-create-signed-cert.sh Outdated Show resolved Hide resolved

fgksgf requested a review from tokers August 25, 2021 13:40

e2e: generate webhook certs in beforeEach

f4c086e

gxthrj reviewed Aug 29, 2021

View reviewed changes

test/e2e/scaffold/ingress.go Outdated Show resolved Hide resolved

test/e2e/scaffold/ingress.go Outdated Show resolved Hide resolved

fgksgf added 2 commits August 29, 2021 22:07

address review comments

c975b50

update example manifests

372da26

gxthrj approved these changes Aug 29, 2021

View reviewed changes

gxthrj reviewed Aug 29, 2021

View reviewed changes

samples/deploy/admission/webhook-registration.yaml Show resolved Hide resolved

tokers reviewed Aug 30, 2021

View reviewed changes

address review comments

240ffae

tokers approved these changes Aug 31, 2021

View reviewed changes

gxthrj merged commit 75a2aaa into apache:master Sep 1, 2021

fgksgf deleted the webhook branch September 2, 2021 09:31

tao12345666333 mentioned this pull request Nov 18, 2021

Implement the admission server #244

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement the admission server and a validation webhook for plugins #573

feat: Implement the admission server and a validation webhook for plugins #573

fgksgf commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

gxthrj left a comment

fgksgf commented Aug 15, 2021

tokers Aug 23, 2021

fgksgf Aug 23, 2021

tokers Aug 30, 2021

tokers Aug 23, 2021

fgksgf Aug 25, 2021

tokers Aug 30, 2021

tokers commented Aug 23, 2021

fgksgf commented Aug 25, 2021

tokers Aug 30, 2021

fgksgf commented Aug 30, 2021

tokers commented Aug 30, 2021

fgksgf commented Aug 30, 2021

tokers commented Aug 30, 2021

fgksgf commented Aug 30, 2021 •

edited

Loading

feat: Implement the admission server and a validation webhook for plugins #573

feat: Implement the admission server and a validation webhook for plugins #573

Conversation

fgksgf commented Jul 6, 2021 • edited Loading

New feature or improvement

TODO

codecov-commenter commented Jul 6, 2021 • edited Loading

Codecov Report

gxthrj left a comment

Choose a reason for hiding this comment

fgksgf commented Aug 15, 2021

tokers Aug 23, 2021

Choose a reason for hiding this comment

fgksgf Aug 23, 2021

Choose a reason for hiding this comment

tokers Aug 30, 2021

Choose a reason for hiding this comment

tokers Aug 23, 2021

Choose a reason for hiding this comment

fgksgf Aug 25, 2021

Choose a reason for hiding this comment

tokers Aug 30, 2021

Choose a reason for hiding this comment

tokers commented Aug 23, 2021

fgksgf commented Aug 25, 2021

tokers Aug 30, 2021

Choose a reason for hiding this comment

fgksgf commented Aug 30, 2021

tokers commented Aug 30, 2021

fgksgf commented Aug 30, 2021

tokers commented Aug 30, 2021

fgksgf commented Aug 30, 2021 • edited Loading

fgksgf commented Jul 6, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

fgksgf commented Aug 30, 2021 •

edited

Loading