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

Default TLS for BentoDeployment model servers #116

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rauerhans
Copy link
Contributor

@rauerhans rauerhans commented May 16, 2023

Closes #87

This feature allows the yatai-deployment controller to add TLS config to the ingress of a Bento deployment through added parameters to the network ConfigMap.

Previously this was only possible by adding the ingress TLS config to the BentoDeployment CRD, which limited the usability of the UI.
(Cert-manager dedicated ingress annotations could be added through the network ConfigMap, but the ingress TLS config with host and secretName had to be set manually in the CRD.)

Changes

  • Add ingress-tls-mode field to network ConfigMap
  • Add ingress-static-tls-secret field to network ConfigMap
  • Implement controller logic to add TLS to ingress depending which on ingress-tls-mode field

Setup can be validated using this image rauerhans/yatai-deployment:1.1.12
in the yatai-deployment deployment and playing by adding the new fields to the network ConfigMap.

Functionality

Depending on the new ingress-tls-mode field in the network ConfigMap, the controller will add the TLS config (hosts is reused, secretName is inferred, based on setting) to the ingress of the Bento deployment.

  • If the mode is set to none, the controller will not add any TLS config to the ingress, as previously.
  • If the mode is set to static, the controller will reference to a wildcard certificate to the ingress of the Bento deployment.
  • If the mode is set to auto, reuse the name of the BentoDeployment CRD for the name of the TLS secret. This is useful in a scenario where the user wants to use cert-manager to generate a certificate for the ingress of the Bento deployment, so in addition you would have the appropriate annotations in the network ConfigMap, see below.

Mode: None

As previously, only way to add TLS is through the field in the deployment CRD.

Mode: Static

The controller will create a wildcard certificate for each deployment.

  • Network Configmap for mode "static"
apiVersion: v1
kind: ConfigMap
metadata:
  name: network
  namespace: yatai
data:
  domain-suffix: yatai.dev.plural.sh
  ingress-annotations: ""
  ingress-class: nginx
  ingress-path: /
  ingress-path-type: ImplementationSpecific
  ingress-static-tls-secret: yatai-deployment-static-tls
  ingress-tls-mode: static
  • One wildcard certificate. Needs to be created manually.
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: yatai-deployment-static-tls
  namespace: yatai
spec:
  dnsNames:
    - '*.yatai.dev.plural.sh'
  issuerRef:
    group: cert-manager.io
    kind: ClusterIssuer
    name: letsencrypt-prod
  secretName: yatai-deployment-static-tls
  usages:
    - digital signature
    - key encipherment
  • results in one secret to be used by all deployments
apiVersion: v1
kind: Secret
metadata:
  name: yatai-deployment-static-tls
  namespace: yatai
  labels:
    controller.cert-manager.io/fao: 'true'
type: kubernetes.io/tls
data:
  tls.crt: xxx
  tls.key: xxx
  • ingress of deployment A created by yatai-deployment controller
kind: Ingress
metadata:
  name: dep-a
  namespace: yatai
  labels:
    yatai.ai/bento-deployment: dep-a
    yatai.ai/bento-deployment-component-type: api-server
    yatai.ai/bento-deployment-target-type: production
    yatai.ai/bento-repository: iris_classifier_pydantic_test
    yatai.ai/bento-version: zavubixtgkiqotwt
    yatai.ai/creator: yatai-deployment
  annotations:
    cert-manager.io/cluster-issuer: letsencrypt-prod
    kubernetes.io/tls-acme: 'true'
    yatai.ai/bento-repository: iris_classifier_pydantic_test
    yatai.ai/bento-version: zavubixtgkiqotwt
spec:
  ingressClassName: nginx
  tls:
    - hosts:
        - dep-a-yatai.yatai.dev.plural.sh
      secretName: yatai-deployment-static-tls
  rules:
    - host: dep-a-yatai.yatai.dev.plural.sh
      http:
        paths:
          - path: /
            pathType: ImplementationSpecific
            backend:
              service:
                name: dep-a
                port:
                  name: http
  • ingress deployment B
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: dep-b
  namespace: yatai
  labels:
    yatai.ai/bento-deployment: dep-b
    yatai.ai/bento-deployment-component-type: api-server
    yatai.ai/bento-deployment-target-type: production
    yatai.ai/bento-repository: iris_classifier_pydantic_test
    yatai.ai/bento-version: f26pbxhtfczdgtwt
    yatai.ai/creator: yatai-deployment
  annotations:
    cert-manager.io/cluster-issuer: letsencrypt-prod
    kubernetes.io/tls-acme: 'true'
    nginx.ingress.kubernetes.io/ssl-redirect: 'false'
    yatai.ai/bento-repository: iris_classifier_pydantic_test
    yatai.ai/bento-version: f26pbxhtfczdgtwt
spec:
  ingressClassName: nginx
  tls:
    - hosts:
        - dep-b-yatai.yatai.dev.plural.sh
      secretName: yatai-deployment-static-tls
  rules:
    - host: dep-b-yatai.yatai.dev.plural.sh
      http:
        paths:
          - path: /
            pathType: ImplementationSpecific
            backend:
              service:
                name: iris-classifier-pydantic-test
                port:
                  name: http

Mode: Auto

Use default annotations to add TLS to ingress through cert-manager.
Results in a concrete TLS secret for each Bento deployment.

  • Network Configmap for mode "auto"
apiVersion: v1
kind: ConfigMap
metadata:
  name: network
  namespace: yatai
data:
  domain-suffix: yatai.dev.plural.sh
  ingress-annotations: >-
    {"cert-manager.io/cluster-issuer":"letsencrypt-prod","kubernetes.io/tls-acme":"true"}
  ingress-class: nginx
  ingress-path: /
  ingress-path-type: ImplementationSpecific
  ingress-static-tls-secret: ""
  ingress-tls-mode: auto

@yetone
Copy link
Member

yetone commented May 17, 2023

Thank you! You have done a great job!

@rauerhans
Copy link
Contributor Author

cool, lmk if you want to change anything, this is an important feature for us

@yetone
Copy link
Member

yetone commented May 17, 2023

How about adding a relevant e2e test?

@rauerhans
Copy link
Contributor Author

okay I will see what I can do when I find the time, where do I put this, somewhere here I suppose?
https://github.com/bentoml/yatai-deployment/tree/main/tests/e2e

@rauerhans
Copy link
Contributor Author

Hey just finished with the tests, I tested everything locally. Because of your triage job I have no way of testing it your GH action setup, but I can vouch that "it works on my machine" 😄 with the image I built locally. Hope you find it to your liking.

  • .github/workflows/e2e.yaml: This file includes changes to the workflow for running end-to-end tests. It adds new steps to test the BentoML deployment with different ingress TLS modes: 'none', 'auto', and 'static'. The appropriate environment variables are set for each test.
  • controllers/bentodeployment_controller.go: Updated ingress configuration handling for different TLS modes.
  • helm/yatai-deployment/templates/configmap-network.yaml: Added ingress TLS mode and static TLS secret name configuration options.
  • helm/yatai-deployment/values.yaml: Added ingress TLS mode and static TLS secret name configuration options. The layers.network section includes ingressTlsMode and ingressStaticTlsSecretName values to configure the ingress TLS mode and static TLS secret name.
  • scripts/quick-install-yatai-deployment.sh: Updated the script to include the INGRESS_TLS_MODE and INGRESS_STATIC_TLS_SECRET_NAME environment variables. These variables are used to set the ingress TLS mode and static TLS secret name during the installation of the BentoML deployment.
  • tests/e2e/e2e_test.go: Added tests for ingress creation and TLS mode configuration. It validates the creation of the Ingress resource and checks the correctness of the TLS configuration based on the configured TLS mode.
  • tests/e2e/example_with_ingress.yaml: New example configuration file for BentoML deployment with ingress enabled.
  • tests/e2e/installation_test.sh: Pass INGRESS_TLS_MODE and INGRESS_STATIC_TLS_SECRET_NAME
  • tests/e2e/installation_test_ingress.sh: Now also installs the Ingress controller and then installs the everything with the appropriate ingress TLS mode configuration.
  • tests/gh-actions/kind-cluster-1-23.yaml: Updated KinD cluster configuration with extra port mappings and node label to make it compatible with nginx ingress.
  • tests/gh-actions/kind-cluster-1-26.yaml: same as above

TIA for your time reviewing this ❤️

@yetone
Copy link
Member

yetone commented May 25, 2023

You are amazing, this PR is too professional!

@yetone
Copy link
Member

yetone commented May 25, 2023

/run-e2e

Update: You can check the progress here

@rauerhans
Copy link
Contributor Author

hmmm, it's still running on the old test setup though, you need to set it up to use the new github actions

@yetone
Copy link
Member

yetone commented May 25, 2023

Can you work on a separate PR for .github/workflows? The current mechanism is that only .github/workflows under the main branch can be used to execute github actions

@davidspek
Copy link
Contributor

@yetone I've just created #120 to update the github workflow. I tried to do it in a way that hopefully doesn't break tests before the PR is merged but I'd still aim to merge this PR quickly after #120.

@yetone
Copy link
Member

yetone commented Jun 21, 2023

CI related PR has been merged.

@davidspek davidspek force-pushed the default-tls branch 2 times, most recently from 3051336 to bd452ba Compare June 22, 2023 08:16
add ingress tls options to configmap

refactor, allow auto vs static modes

adapt helm chart to changes

renamt to secret name

add validations

fix helm validation

parametrize tls mode in quick installation script

test installation with ingress

validate ingress/tls behaviour

add ingress test action

default installtion tls mode should be none

use my image

make kind ingress compatible

need an example where ingress is enabled

polish test code

rename example with ingress

fix template condition

commit before we delete it

delete env vars for local testing

make sure configmap is refreshed

add gh action for all three modes

for 1-26 we need the kind cluster config too

fix lynt issues

fix lint

Signed-off-by: David van der Spek <[email protected]>
@davidspek
Copy link
Contributor

@yetone This PR has been rebased on main so it should be possible to run the e2e test now.

@FogDong
Copy link
Member

FogDong commented Jun 25, 2023

/run-e2e
Update: You can check the progress here

Signed-off-by: David van der Spek <[email protected]>
@davidspek
Copy link
Contributor

davidspek commented Jun 26, 2023

@FogDong @yetone It seems that I needed to do a chmod +x so that the script is executable. Can you rerun the e2e tests?

@yetone
Copy link
Member

yetone commented Jun 26, 2023

/run-e2e
Update: You can check the progress here

@davidspek
Copy link
Contributor

@yetone It looks like the e2e is erroring on waiting for the yatai-deployment-default-domain to be complete saying it can't be found, but the pods output from the previous command shows the job has already run and was successful.

@davidspek
Copy link
Contributor

Could it be that the current e2e test is a bit flaky?

@yetone
Copy link
Member

yetone commented Jun 27, 2023

@davidspek You may need to change the configuration of the kind cluster so that it installs loadbalancer.

https://kind.sigs.k8s.io/docs/user/loadbalancer/

@yetone
Copy link
Member

yetone commented Aug 3, 2023

/run-e2e
Update: You can check the progress here

@yetone
Copy link
Member

yetone commented Aug 4, 2023

/run-e2e
Update: You can check the progress here

@davidspek
Copy link
Contributor

Sorry @yetone I was away on vacation the past few weeks. Had you already tried updating the kind config to add the load balancer before rerunning the E2E tests? Or is there something we should do on our end?

@a-pichard
Copy link

Similar to #123
I believe we should close this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to configure tis secret for network layer
6 participants