Allow overriding webhook secret data keys by skonto · Pull Request #2662 · knative/pkg

skonto · 2022-12-16T12:11:24Z

Changes

Downstream we are using knative operator's webhook that is passed in shared main for creation but does not utilize certificates from pkg. OLM manages the cert lifecycle and is generated due to webhook configuration at the csv. The generated secret has the following data:

data:
  olmCAKey: ...
  tls.crt: ..
  tls.key: ...

We would like to be able to override the default keys so in general any external webhook that extends
webhook.AdmissionController or webhook.ConversionController can be used.

The caCert is not used when constructing a new Webhook so no need to override. Webhooks usually fetch it in order inject it at the webhook configuration via the webhook's reconciler logic.
Making generic the setup of existing keys eg in MakeSecret is out of scope.

/kind enhancement

codecov · 2022-12-16T12:19:07Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02% 🎉

Comparison is base (5ef4812) 81.75% compared to head (12544e2) 81.77%.
Report is 14 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2662      +/-   ##
==========================================
+ Coverage   81.75%   81.77%   +0.02%     
==========================================
  Files         167      167              
  Lines       10201    10214      +13     
==========================================
+ Hits         8340     8353      +13     
  Misses       1614     1614              
  Partials      247      247

Files Changed	Coverage Δ
webhook/helper.go	`100.00% <100.00%> (ø)`
webhook/webhook.go	`78.94% <100.00%> (+0.28%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

skonto · 2022-12-20T09:55:00Z

@dprotaso gentle ping

dprotaso · 2023-01-05T19:50:48Z

webhook/webhook.go

+				if val, ok := os.LookupEnv(certresources.ServerKeyEnv); ok {
+					sKey = val
+				}
+				if val, ok := os.LookupEnv(certresources.ServerCertEnv); ok {
+					sCert = val
+				}


test coverage?

Sure will add

dprotaso · 2023-01-05T20:15:12Z

webhook/certificates/resources/secret.go

 	// ServerKey is the name of the key associated with the secret's private key.
-	ServerKey = "server-key.pem"
+	ServerKey    = "server-key.pem"
+	ServerKeyEnv = "KNATIVE_SECRET_WEBHOOK_SERVER_KEY"


add godoc for these new vars.

Suggested change

ServerKeyEnv = "KNATIVE_SECRET_WEBHOOK_SERVER_KEY"

ServerKeyEnvOverride = "KNATIVE_WEBHOOK_SERVER_KEY"

How about WEBHOOK_CERTS_SECRET_SERVER_KEY/CERT? Cause I use WEBHOOK_CERTS_SECRET_NAME in #2685

I like the KNATIVE prefix tbh to separate from other env vars that could co-exist. Other than that its up to @dprotaso no strong preference.

We don't have KNATIVE_ prefixes for other environment variables ie. SYSTEM_NAMESPACE so I suggest dropping the prefix so that we remain consistent

webhook/certificates/resources/secret.go

skonto · 2023-02-20T20:56:43Z

/retest

dprotaso · 2023-02-24T15:28:00Z

webhook/certificates/resources/secret.go

+	// ServerKeyEnv is the env var name for the webhook secret's key eg. `tls.key`.
+	ServerKeyEnv = "KNATIVE_WEBHOOK_SERVER_KEY"
 	// ServerCert is the name of the key associated with the secret's public key.
 	ServerCert = "server-cert.pem"
+	// ServerCertEnv is the env var name for the webhook secret's ca data key eg. `tls.crt`.
+	ServerCertEnv = "KNATIVE_WEBHOOK_SERVER_CERT"


I realize now the env vars don't influence the certificates & keys being created. Thus I don't think these overrides should be in this package.

This package is for creating and updating certificates

Ok will do the webhook options approach thanks.

dprotaso · 2023-02-24T15:33:45Z

webhook/webhook.go

 				}
-
-				serverKey, ok := secret.Data[certresources.ServerKey]
+				sKey, sCert := certresources.GetSecretDataKeyNamesOrDefault()


Are you creating custom webhooks binaries where you drop the certificate controller?

If so it might be better to add these overrides to the webhook.Options and when they're empty default them to the keys in the certificates/resources package.

Makes sense the env var thingy is a bit too intrusive.

github-actions · 2023-05-26T01:30:51Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

skonto · 2023-07-11T11:21:42Z

@dprotaso gentle ping this is ready for review.

dprotaso · 2023-07-11T13:23:30Z

webhook/webhook.go

+	// ServerKeyEnv is the name for the webhook secret's data key eg. `tls.key`.
+	// Default value is `server-key.pem` if no value is passed.
+	ServerKey string


Comment doesn't match the var name.

Maybe SecretPrivateKeyName ?

I will change but to clarify the reason I kept that name is that I wanted to make explicit that I am overriding the same values here:

pkg/webhook/certificates/resources/secret.go

Line 29 in 7051d30

ServerKey = "server-key.pem"

dprotaso · 2023-07-11T13:24:20Z

webhook/webhook.go

+	// ServerCertEnv is the name for the webhook secret's ca data key eg. `tls.crt`.
+	// Default value is `server-cert.pem` if no value is passed.
+	ServerCert string


Comment doesn't match var name

Maybe SecretCertifcateName ?

dprotaso · 2023-07-11T13:26:12Z

webhook/certificates/resources/secret.go

+func GetSecretDataKeyNamesOrDefault(sKey string, sCert string) (serverKey string, serverCert string) {
+	serverKey = ServerKey
+	serverCert = ServerCert
+
+	if sKey != "" {
+		serverKey = sKey
+	}
+	if sCert != "" {
+		serverCert = sCert
+	}
+	return serverKey, serverCert
+}


seems like this could be a private helper in the webhook package

dprotaso · 2023-07-11T13:32:01Z

webhook/webhook_integration_test.go


+func TestMissingContentTypeCustomSecret(t *testing.T) {
+	defaultOptions := newCustomOptions()
+	certresources.MakeSecret = customSecretWithOverrides


This might cause flakiness when running tests in parallel.

We should just have this test perform it's own setup (ie. create it's own cert etc)

Also the test name and assertions is a bit misleading TestMissingContentTypeCustomSecret - we really want to assert that the right certificate is presented. Thus we should skip stuff that's not relevant

@dprotaso I copied the logic in certificates_test.go so probably that is unsafe too or is it that it always returns the default one and it worked so far? In any case I will take a look to separate it.

Anyway I updated the test to be more compact and have its own setup.

skonto · 2023-07-17T14:09:49Z

@dprotaso gentle ping.

skonto · 2023-08-02T07:57:26Z

@dprotaso gentle ping.

dprotaso · 2023-08-02T19:11:41Z

/lgtm
/approve

knative-prow · 2023-08-02T19:11:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

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:

~~webhook/OWNERS~~ [dprotaso,skonto]

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

knative-prow bot added kind/enhancement size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 16, 2022

knative-prow bot requested review from dprotaso, tcnghia and whaught December 16, 2022 12:11

dprotaso reviewed Jan 5, 2023

View reviewed changes

skonto force-pushed the inject_secret_data_keys branch from 6066fa5 to 5c5c9b8 Compare February 20, 2023 15:28

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 20, 2023

dprotaso mentioned this pull request Feb 21, 2023

Add certs secret name read from env #2685

Merged

dprotaso reviewed Feb 24, 2023

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 26, 2023

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2023

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 27, 2023

skonto changed the title ~~Allow overriding webhook secret data keys~~ [WIP] Allow overriding webhook secret data keys Jul 7, 2023

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2023

skonto force-pushed the inject_secret_data_keys branch from d0b449a to 9f240b9 Compare July 7, 2023 13:37

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2023

skonto mentioned this pull request Jul 11, 2023

[TEST-ONLY] Remove conversion patch openshift-knative/serverless-operator#2148

Closed

skonto changed the title ~~[WIP] Allow overriding webhook secret data keys~~ Allow overriding webhook secret data keys Jul 11, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2023

dprotaso reviewed Jul 11, 2023

View reviewed changes

skonto added 2 commits July 17, 2023 17:07

override secret data keys when creating new webhooks

16c942b

fixes

6b1fff0

skonto added 6 commits July 17, 2023 17:07

cleanup

d692f21

use webhook options

003016c

remove unused env var keys

6905bd3

fix docs

086ae99

update opt names and refactor integration test

e740d20

fixes

12544e2

skonto force-pushed the inject_secret_data_keys branch from b0270dc to 12544e2 Compare July 17, 2023 14:08

knative-prow bot assigned dprotaso Aug 2, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 2, 2023

knative-prow bot merged commit 8d3f951 into knative:main Aug 2, 2023

	ServerKeyEnv = "KNATIVE_SECRET_WEBHOOK_SERVER_KEY"
	ServerKeyEnvOverride = "KNATIVE_WEBHOOK_SERVER_KEY"

Conversation

skonto commented Dec 16, 2022

Changes

Uh oh!

codecov bot commented Dec 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skonto commented Dec 20, 2022

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!

dprotaso Feb 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

skonto commented Feb 20, 2023

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!

github-actions bot commented May 26, 2023

Uh oh!

skonto commented Jul 11, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

skonto Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto commented Jul 17, 2023

Uh oh!

skonto commented Aug 2, 2023

Uh oh!

dprotaso commented Aug 2, 2023

Uh oh!

knative-prow bot commented Aug 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 16, 2022 •

edited

Loading

dprotaso Feb 24, 2023 •

edited

Loading

skonto Jul 17, 2023 •

edited

Loading

skonto Jul 12, 2023 •

edited

Loading