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

Contribute guides for TLS #374

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

israel-hdez
Copy link

Proposed Changes

This adds documentation for guiding users on how to secure KServe workloads with TLS. The guides are broken down in several sections: from what is needed to enable mTLS in Istio, to how to allow requests from workloads outside the service mesh and still secure traffic with TLS.

This documentation is valid only after kserve/kserve#3737 is merged.

Copy link

netlify bot commented Jun 14, 2024

Deploy Preview for elastic-nobel-0aef7a ready!

Name Link
🔨 Latest commit c6bc29b
🔍 Latest deploy log https://app.netlify.com/sites/elastic-nobel-0aef7a/deploys/6675dec1bc972b00099c6269
😎 Deploy Preview https://deploy-preview-374--elastic-nobel-0aef7a.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@israel-hdez
Copy link
Author

In draft, as I still need to verify all is correct.

servers:
- hosts:
- '*'
Copy link
Contributor

Choose a reason for hiding this comment

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

The domain seems too broad. It would be better to narrow it down to *.svc.cluster.local."

Copy link
Author

Choose a reason for hiding this comment

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

I saw in Knative docs that the provided manifests also use * for their local gateway. So, I simply used the same.

I can change if you think it is really better.

Copy link
Member

Choose a reason for hiding this comment

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

This may need to match the cname in the tls credential

```

You will need to create a Secret named `kserve-cluster-local-tls` holding the TLS certificate. You may get this certificate from cert-manager.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to add here that the Secret should be created under the istio-system.

Copy link

@spolti spolti left a comment

Choose a reason for hiding this comment

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

just a few comments, not mandatory though.
Nice doc @israel-hdez

Copy link

oss-prow-bot bot commented Jun 18, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: israel-hdez, spolti
Once this PR has been reviewed and has the lgtm label, please assign theofpa for approval by writing /assign @theofpa in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@israel-hdez
Copy link
Author

@spolti @Jooho I hopefully fixed all your comments.

@spolti Please, re-check. Your suggestions were subtle and I may have missed some.

@spolti
Copy link

spolti commented Jun 20, 2024

Sounds good @edgar, thanks!

This adds documentation for guiding users on how to secure KServe
workloads with TLS. The guides are broken down in several sections: from
what is needed to enable mTLS in Istio, to how to allow requests from
workloads outside the service mesh and still secure traffic with TLS.

Signed-off-by: Edgar Hernández <[email protected]>
@israel-hdez
Copy link
Author

I pushed some small amendments and confirmed that the configurations work.

@israel-hdez israel-hdez marked this pull request as ready for review June 21, 2024 20:14
@oss-prow-bot oss-prow-bot bot requested a review from theofpa June 21, 2024 20:14
@terrytangyuan
Copy link
Member

/assign @yuzisun

namespace: istio-system
labels:
experimental.istio.io/disable-gateway-port-translation: "true"
Copy link
Member

Choose a reason for hiding this comment

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

What does this label do ? might be good to add an explanation

Copy link
Author

Choose a reason for hiding this comment

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

I simply took it from Knative's gateway manifests, to have similar configurations here in KServe.
It is related to this issue: kubeflow/manifests#2082. Istio added this label as a workaround.

AFAIK, Istio has already implemented the long-term solution, but Knative still hasn't moved to it. Istio project may simply drop this label, given it is experimental. I may propose the migration to Knative project, and then we could also move.

Comment on lines +406 to +410
"knativeLocalGatewayService": "knative-local-gateway.istio-system.svc.cluster.local",
"localGateway": "istio-system/kserve-local-gateway",
"localGatewayService": "kserve-local-gateway.istio-system.svc.cluster.local"
}
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to include a diagram to explain the data plane path how the traffic is secured with these configured gateways

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.

5 participants