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

Add helm flags for custom tls secret and caBundle in blueprint webhook controller #1712

Merged
merged 12 commits into from
Mar 9, 2023

Conversation

akankshakumari393
Copy link
Contributor

@akankshakumari393 akankshakumari393 commented Oct 27, 2022

Change Overview

Add helm flags for accepting tls certs in k8s secret for Blueprint validation webhook controller. Also modified the chart accordingly

Pull request type

Please check the type of change your PR introduces:

  • 🚧 Work in Progress
  • 🌈 Refactoring (no functional changes, no api changes)
  • 🐹 Trivial/Minor
  • 🐛 Bugfix
  • 🌻 Feature
  • 🗺️ Documentation
  • 🤖 Test

Issues

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E
  1. deploy kanister with bpValidatingWebhook.tls.mode set to auto. This would create the secret and use self signed certificate
    helm install kanister ./helm/kanister-operator/ --namespace kanister --create-namespace --set bpValidatingWebhook.tls.mode=auto
$kubectl get pod -n kanister
NAME                                          READY   STATUS    RESTARTS   AGE
kanister-kanister-operator-85c6c84469-ctt7m   1/1     Running   0          4s

$kubectl get secret -n kanister kanister-webhook-certs
NAME                     TYPE                DATA   AGE
kanister-webhook-certs   kubernetes.io/tls   2      93s
  1. deploy kanister with bpValidatingWebhook.tls.mode set to custom. we would have to create a tls secret containing cert details.
# create self signed CA certificate 

openssl genrsa -out ca.key 2048

openssl req -new -key ca.key -subj "/CN=kanister-kanister-operator-ca" -out ca.csr

openssl x509 -req -in ca.csr -signkey ca.key -CAcreateserial  -out ca.crt -days 365

# create tls certificate

openssl genrsa -out tls.key 2048

# create configuration file for kanister operator
vi openssl.cnf

[req]
req_extensions = v3_req
distinguished_name = req_distinguished_name
[req_distinguished_name]
[ v3_req ]
basicConstraints = CA:FALSE
keyUsage = nonRepudiation, digitalSignature, keyEncipherment
subjectAltName = @alt_names
[alt_names]
DNS.1 = kanister-kanister-operator.kanister
DNS.2 = kanister-kanister-operator.kanister.svc

# sign it using CA cert

openssl req -new -key tls.key -subj "/CN=kanister-kanister-operator" -out tls.csr -config openssl.cnf

openssl x509 -req -in tls.csr -CA ca.crt -CAkey ca.key -CAcreateserial -out tls.crt -extensions v3_req -extfile openssl.cnf -days 365
  • create a TLS secret in kanister namespace
    ``
$kubectl create secret tls my-tls-secret --cert tls.crt --key tls.key -n kansiter

$kubectl get secret -n kanister tls-certs
NAME                     TYPE                DATA   AGE
my-tls-secret   kubernetes.io/tls   2      93s
  • get ca bundle field
cat ca.crt | base64 -w 0
LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMxekNDQWI4Q0ZCZnlMdkZNZ2xlVDJGMXpVTXNSc21rWTNPOXVNQTBHQ1NxR1NJYjNEUUVCQ3dVQU1DZ3gKSmpBa0JnTlZCQU1NSFd0aGJtbHpkR1Z5TFd0aGJtbHpkR1Z5TFc5d1pYSmhkRzl5TFdOaE1CNFhEVEl6TURJdwpNakV5TlRJMU9Gb1hEVEkwTURJd01qRXlOVEkxT0Zvd0tERW1NQ1FHQTFVRUF3d2RhMkZ1YVhOMFpYSXRhMkZ1CmFYTjBaWEl0YjNCbGNtRjBiM0l0WTJFd2dnRWlNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0SUJEd0F3Z2dFS0FvSUIKQVFEMEMrdktWUFpuTWZ1QU9oZU9tcndPMmpQeXlTUWZRRExsTzN6R2RKa01keWJ6a3Z3QlA0aVFlaTBzb09sdQpIbjBJRURDZzdRbUZiUWpNREpWM2pYV0NtUGg0VmpXRWRRNnhkSzd6ejZHd3ZPaFh0MFZxcURhSWJGRG1QS2xMCnlxVnVpWVR6Y1BQRnBwZFZKRnNFNGxBdmF5Q1BUT0xLZUpCVHVDcWVvQWxZZUFnRnFGV0daUGltNW9ZSmVnZk8KNy90REVsZGFBOERVNS8wR0xjSG5lS2pZRS9vbFpNVjVGRjZCZ2ttUjRHamxqQ01PRkFyQ3hxeHpVZXB5RFFhUQp1YWRRYkRHME5HTElLdURMeVZJQTFtMnpCYUE1M3JnaXdCTXZwWGJjSmR5OFZOdGtLcFdnMk8wanNNdkdhZiszCnhzRjhDd3BuWkpzTWprNW5Pc3ZGbHdjWkFnTUJBQUV3RFFZSktvWklodmNOQVFFTEJRQURnZ0VCQUZVL0YzZDUKNGx2MHkvelllMWNuZ3EzZHJPMnZjZS81UkV4RlBFellTYTE2WFZFczZWTmVUaXlod0R1MktzU0VNbHFxOVlFMQpZZkxoU3hVZHhrRGxPd0ViV2FVOFRVLytiUytmWnUzVU0wZTBwdnJtOW8zMjVFdjQyczN6UkNYUjcxRGY4NGhnCkhhbWY2MTd6TUhQMFlMb2RJNDN4QlBQMXhTbXdIQXA4di9qcDFrSEU2Zjg4enBOc2NVaEl4dUE2elIvQkRpaEoKdy9NSVF2TWFWNzhOUDdxM1hMa1ZtSlNZK1BkMG4xZ2pIMFNNNGlYM1NVZXZBK2NjNDdRTmpHNjJERmpRTHBTcwpEQkRkRGltZmFNUlBYSWl0c2ZJY1ZXMzhHVlRsUnJTTi9nUUl3UDZxVjMzSUN4RVdwNERRM2UxY1NPU2RBQUxmClA2b0U1TmtVOWw5M1dROD0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo=

  • Install kanister with tls mode set to custom
helm upgrade --install kanister ./helm/kanister-operator/ --namespace kanister --create-namespace --set bpValidatingWebhook.tls.mode=custom --set bpValidatingWebhook.tls.caBundle=LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUMxekNDQWI4Q0ZCZnlMdkZNZ2xlVDJGMXpVTXNSc21rWTNPOXVNQTBHQ1NxR1NJYjNEUUVCQ3dVQU1DZ3gKSmpBa0JnTlZCQU1NSFd0aGJtbHpkR1Z5TFd0aGJtbHpkR1Z5TFc5d1pYSmhkRzl5TFdOaE1CNFhEVEl6TURJdwpNakV5TlRJMU9Gb1hEVEkwTURJd01qRXlOVEkxT0Zvd0tERW1NQ1FHQTFVRUF3d2RhMkZ1YVhOMFpYSXRhMkZ1CmFYTjBaWEl0YjNCbGNtRjBiM0l0WTJFd2dnRWlNQTBHQ1NxR1NJYjNEUUVCQVFVQUE0SUJEd0F3Z2dFS0FvSUIKQVFEMEMrdktWUFpuTWZ1QU9oZU9tcndPMmpQeXlTUWZRRExsTzN6R2RKa01keWJ6a3Z3QlA0aVFlaTBzb09sdQpIbjBJRURDZzdRbUZiUWpNREpWM2pYV0NtUGg0VmpXRWRRNnhkSzd6ejZHd3ZPaFh0MFZxcURhSWJGRG1QS2xMCnlxVnVpWVR6Y1BQRnBwZFZKRnNFNGxBdmF5Q1BUT0xLZUpCVHVDcWVvQWxZZUFnRnFGV0daUGltNW9ZSmVnZk8KNy90REVsZGFBOERVNS8wR0xjSG5lS2pZRS9vbFpNVjVGRjZCZ2ttUjRHamxqQ01PRkFyQ3hxeHpVZXB5RFFhUQp1YWRRYkRHME5HTElLdURMeVZJQTFtMnpCYUE1M3JnaXdCTXZwWGJjSmR5OFZOdGtLcFdnMk8wanNNdkdhZiszCnhzRjhDd3BuWkpzTWprNW5Pc3ZGbHdjWkFnTUJBQUV3RFFZSktvWklodmNOQVFFTEJRQURnZ0VCQUZVL0YzZDUKNGx2MHkvelllMWNuZ3EzZHJPMnZjZS81UkV4RlBFellTYTE2WFZFczZWTmVUaXlod0R1MktzU0VNbHFxOVlFMQpZZkxoU3hVZHhrRGxPd0ViV2FVOFRVLytiUytmWnUzVU0wZTBwdnJtOW8zMjVFdjQyczN6UkNYUjcxRGY4NGhnCkhhbWY2MTd6TUhQMFlMb2RJNDN4QlBQMXhTbXdIQXA4di9qcDFrSEU2Zjg4enBOc2NVaEl4dUE2elIvQkRpaEoKdy9NSVF2TWFWNzhOUDdxM1hMa1ZtSlNZK1BkMG4xZ2pIMFNNNGlYM1NVZXZBK2NjNDdRTmpHNjJERmpRTHBTcwpEQkRkRGltZmFNUlBYSWl0c2ZJY1ZXMzhHVlRsUnJTTi9nUUl3UDZxVjMzSUN4RVdwNERRM2UxY1NPU2RBQUxmClA2b0U1TmtVOWw5M1dROD0KLS0tLS1FTkQgQ0VSVElGSUNBVEUtLS0tLQo= --set bpValidatingWebhook.tls.secretName=my-tls-secret

  • Verify
$ kubectl get pod -n kanister                                      
NAME                                          READY   STATUS        RESTARTS   AGE
kanister-kanister-operator-68457b58f9-tdj8z   1/1     Running       0          9s
kanister-kanister-operator-85c6c84469-5857z   0/1     Terminating   2          5d21h

$ kubectl get secret -n kanister kanister-webhook-certs
Error from server (NotFound): secrets "kanister-webhook-certs" not found
  • create blueprint
$kubectl create -f https://raw.githubusercontent.com/kanisterio/kanister/master/examples/mysql/mysql-blueprint.yaml -n kanister
blueprint.cr.kanister.io/mysql-blueprint created

@github-actions
Copy link
Contributor

Thanks for submitting this pull request 🎉. The team will review it soon and get back to you.

If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document.

@infraq infraq added this to In Progress in Kanister Oct 27, 2022
@akankshakumari393 akankshakumari393 changed the title accept TLS cert in K8s Secret for Validation webhook controller Add helm flags for using custom tls secret and caBundle used for Validation webhook controller Oct 27, 2022
@akankshakumari393 akankshakumari393 changed the title Add helm flags for using custom tls secret and caBundle used for Validation webhook controller Add helm flags for custom tls secret and caBundle in blueprint webhook controller Oct 27, 2022
@akankshakumari393 akankshakumari393 marked this pull request as ready for review October 27, 2022 14:56
@viveksinghggits
Copy link
Contributor

Hi @akankshakumari393 ,
I am not able to follow the test steps that you have documented here. In the second point your are saying create tls cert but the command that you have used is, to get the secret not create.
Also the helm install command that you have in the second point should specify where can I get the cabundle from.

@akankshakumari393
Copy link
Contributor Author

@viveksinghggits I did those steps manually, I used a self signed certificate, got out the required field values, created TLS secret from private key and certs. and populated the values of CA bundle during helm install.

I was trying to put that in the description but the fields values were too big to be described here.

@viveksinghggits
Copy link
Contributor

Can you write the test steps in a way that you can someone can follow them if they want to? If the certs are too big you can just replace them with ... you don't have to put the entire cert here.

@github-actions
Copy link
Contributor

This PR is marked as stale due to inactivity. Add a new comment to reactivate it.

@github-actions github-actions bot added the stale label Dec 31, 2022
@pavannd1
Copy link
Contributor

pavannd1 commented Jan 9, 2023

Relevant

@pavannd1 pavannd1 removed the stale label Jan 9, 2023
@akankshakumari393
Copy link
Contributor Author

I have updated the test plan to use a self signed certificate. PTAL.

@viveksinghggits
Copy link
Contributor

@akankshakumari393 can we not have caBundle also, in the secret so that we have to just pass the secret name in the install command? That would make the install command significantly easier for users.

helm upgrade --install kanister ./helm/kanister-operator/ --namespace kanister --create-namespace --set bpValidatingWebhook.tls.mode=custom --set bpValidatingWebhook.tls.secretName=my-tls-secret

helm/kanister-operator/values.yaml Outdated Show resolved Hide resolved
helm/kanister-operator/values.yaml Outdated Show resolved Hide resolved
helm/kanister-operator/values.yaml Outdated Show resolved Hide resolved
@viveksinghggits
Copy link
Contributor

Let's document this in the kanister docs as well.

@akankshakumari393
Copy link
Contributor Author

@akankshakumari393 can we not have caBundle also, in the secret so that we have to just pass the secret name in the install command? That would make the install command significantly easier for users.

@viveksinghggits we can do it, but that's not a ideal way. The certs information are in a tls type secret. Apart from it, If we even consider cert-manager tool, there also we do not combine the cabundle with secret.

I somehow agree that it would not be a user friendly process to add ca-bundle, but we would have to consider it, if we want to enforce bring your own certificate service.

@viveksinghggits
Copy link
Contributor

viveksinghggits commented Feb 6, 2023

@viveksinghggits we can do it, but that's not a ideal way. The certs information are in a tls type secret.

Can we not have another key in the secret? even if it's of type tls?

Apart from it, If we even consider cert-manager tool, there also we do not combine the cabundle with secret

are they doing the same thing that we are doing and not specifying caBundle is secret? If not, in that case we not say this is the reason because of which we should have caBundle separately, right?

@akankshakumari393
Copy link
Contributor Author

akankshakumari393 commented Feb 7, 2023

Can we not have another key in the secret? even if it's of type tls?

I don't think kubectl create secret tls supports any flag apart from --cert and --key. But we can add the data using declarative way in the secret.

@akankshakumari393
Copy link
Contributor Author

akankshakumari393 commented Feb 7, 2023

I strongly believe we should not bind ca-cert into the same secret. let's say later on we choose to keep validating webhook server separately, then the tls secrets should only be restricted to that component using rbac but having cacert in it would not allow us to do that.

Either we can ask users to create a new opaque secret with ca bundle information or we can keep the flag as it is.

@akankshakumari393
Copy link
Contributor Author

@pavannd1 need your opinion on how we are using cabundle to populate vwc. We can have it as flag or we can ask users to create another generic secret with cacert in it, and pass as helm flag. cc @viveksinghggits

Copy link
Contributor

@pavannd1 pavannd1 left a comment

Choose a reason for hiding this comment

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

@akankshakumari393 @viveksinghggits Can we recommend using a config map or secret for the CA certificate? We can do the decoding in the helm template right?

docs/install.rst Outdated Show resolved Hide resolved
@viveksinghggits
Copy link
Contributor

viveksinghggits commented Feb 22, 2023

We can use configmap as well, to store the caBundle. It's just, we will have to see how we can use that in ValidatingWebhookConfiguration.

@pavannd1
Copy link
Contributor

@viveksinghggits You are right! There isn't an easy solution for this. The solutions I found were using ca-injector which seems to be yet another controller we need to run to dynamically inject the ca bundle into the configuration 😓
We should probably go ahead with this approach for now

@viveksinghggits
Copy link
Contributor

@viveksinghggits You are right! There isn't an easy solution for this. The solutions I found were using ca-injector which seems to be yet another controller we need to run to dynamically inject the ca bundle into the configuration sweat We should probably go ahead with this approach for now

ok

@akankshakumari393
Copy link
Contributor Author

@viveksinghggits @pavannd1 It wasn't clear from that conversation. Did we decide on using Secret or configMap?

@viveksinghggits
Copy link
Contributor

I think Pavan is recommending that we should move ahead with the PR as it is. We can wait for him to confirm.

@akankshakumari393
Copy link
Contributor Author

@viveksinghggits You are right! There isn't an easy solution for this. The solutions I found were using ca-injector which seems to be yet another controller we need to run to dynamically inject the ca bundle into the configuration sweat
We should probably go ahead with this approach for now

@pavannd1 do we need modification in this PR?

Co-authored-by: Pavan Navarathna <6504783+pavannd1@users.noreply.github.com>
docs/install.rst Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
docs/install.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@viveksinghggits viveksinghggits left a comment

Choose a reason for hiding this comment

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

LGTM, can approve after comments are addressed.

helm/kanister-operator/values.yaml Outdated Show resolved Hide resolved
Kanister automation moved this from In Progress to Reviewer approved Mar 7, 2023
@pavannd1
Copy link
Contributor

pavannd1 commented Mar 7, 2023

@viveksinghggits You are right! There isn't an easy solution for this. The solutions I found were using ca-injector which seems to be yet another controller we need to run to dynamically inject the ca bundle into the configuration sweat
We should probably go ahead with this approach for now

@pavannd1 do we need modification in this PR?

No. We can move forward with this approach.

Signed-off-by: Akanksha Kumari <akankshakumari393@gmail.com>
@mergify mergify bot merged commit cd61459 into master Mar 9, 2023
Kanister automation moved this from Reviewer approved to Done Mar 9, 2023
@mergify mergify bot deleted the 1515 branch March 9, 2023 14:19
@muffl0n muffl0n mentioned this pull request Mar 24, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

Validating Webhook should accept TLS cert in K8s Secret
3 participants