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.

Sign up for GitHub

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

Jump to bottom

[wip] dont apply client TLS filtering on gateways #39829

Closed
wants to merge 6 commits into from
Closed

[wip] dont apply client TLS filtering on gateways #39829

wants to merge 6 commits into from

Conversation

stevenctl
Copy link
Contributor

No description provided.

@stevenctl stevenctl requested review from a team as code owners July 7, 2022 18:00
@istio-testing istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2022
hzxuzhonghu
hzxuzhonghu reviewed Jul 8, 2022
View reviewed changes
pilot/pkg/xds/endpoint_builder.go Outdated
@@ -384,9 +386,12 @@ type mtlsChecker struct {
subsetPolicyMode map[string]*networkingapi.ClientTLSSettings_TLSmode
// the tlsMode of the root traffic policy if it's set
rootPolicyMode *networkingapi.ClientTLSSettings_TLSmode

// Indicates the cluster we're checking settings for doesn't care about mTLS settings on the client (DestinationRule).
passthroughMode bool
Copy link
Member

Choose a reason for hiding this comment

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

This is confusing, i think you mean for east west gateway, the tls mode of from DR is ignorred

Copy link
Contributor Author

@stevenctl stevenctl Jul 8, 2022 •
edited

Choose a reason for hiding this comment

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

cluster as in xDS cluster. The srv passthrough clusters don't actually apply client TLS settings since they don't terminate/originate any TLS.

@stevenctl
Copy link
Contributor Author

stevenctl commented Jul 8, 2022 •
edited

So I changed this a bit to allow the "workaround destinationrule" to work, even though it shouldn't be needed.
Without ClientTLSSettings forcing ISTIO_MUTUAL tls mode, we allow either PeerAuthentication or the tlsMode label to cause the endpoint filtering.

In the egress case, it's the tlsMode causing the issue. We only have that label on sidecar pods. We could either:

  • Add the security.istio.io/tlsMode: istio label to the egress (and probably other gateways?)
  • Inside of pilot code (probably here) implicitly make the label istio: egressgateway (and probably others) ~= tlsMode: istio.

The latter will fix things without users adding the label though..

@stevenctl stevenctl mentioned this pull request Jul 8, 2022
Open
howardjohn
howardjohn reviewed Jul 11, 2022
View reviewed changes
manifests/charts/gateways/istio-egress/values.yaml Outdated
@@ -16,6 +16,7 @@ gateways:
labels:
app: istio-egressgateway
istio: egressgateway
security.istio.io/tlsMode: istio
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem safe? Won't we accidentally start doing autoMTLS to the gateway?

@stevenctl stevenctl requested a review from a team as a code owner July 11, 2022 16:55
stevenctl
stevenctl commented Jul 11, 2022
View reviewed changes
pilot/pkg/model/service.go
@@ -831,6 +832,12 @@ func (ep *IstioEndpoint) DeepCopy() *IstioEndpoint {
return copyInternal(ep).(*IstioEndpoint)
}

var gatewayNames = sets.New("ingressgateway", "egressgateway")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have these as constants anywhere? Is this even truly a "standard"?

Copy link
Member

Choose a reason for hiding this comment

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

Seems not a standard naming

@stevenctl
Copy link
Contributor Author

/retest

hzxuzhonghu
hzxuzhonghu reviewed Jul 13, 2022
View reviewed changes
pilot/pkg/xds/endpoint_builder.go
@@ -419,16 +425,21 @@ func (c *mtlsChecker) computeForEndpoint(ep *model.IstioEndpoint) {
if drMode := c.mtlsModeForDestinationRule(ep); drMode != nil {
switch *drMode {
case networkingapi.ClientTLSSettings_DISABLE:
c.mtlsDisabledHosts[lbEpKey(ep.EnvoyEndpoint)] = struct{}{}
return
if !c.sniDNAT {
Copy link
Member

Choose a reason for hiding this comment

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

I am confused here, shouldn't check sniDNAT first and set it to c.mtlsDisabledHosts[lbEpKey(ep.EnvoyEndpoint)] in any case?

pilot/pkg/xds/endpoint_builder.go
// if endpoint has no sidecar or explicitly tls disabled by "security.istio.io/tlsMode" label.
if ep.TLSMode != model.IstioMutualTLSModeLabel {
// the endpoint must either be a part of istio or have the securty.istio.io/tlsMode: istio label
if !ep.IsIstioGateway() && ep.TLSMode != model.IstioMutualTLSModeLabel {
Copy link
Member

Choose a reason for hiding this comment

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

Why checking IsGateway here? IIRC， tls mode is always set on gateway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's true from my testing. I don' think checking "is gateway" is a good idea, more of an experiment so far.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jul 30, 2022
@istio-testing istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 4, 2022
fix from othe PR
7349536 
Change-Id: Ie1448ef9d9c24bcd0869008357004e2995cf3856
@istio-testing
Copy link
Collaborator

@stevenctl: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
lint_istio 7349536 link true /test lint_istio
gencheck_istio 7349536 link true /test gencheck_istio

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@stevenctl
Copy link
Contributor Author

Included #40243 + the "fix" for eastwest gateway being considered TLS-enabled and things look stable.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 6, 2022
@istio-testing
Copy link
Collaborator

@stevenctl: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 4, 2022
@istio-policy-bot
Copy link

🚧 This issue or pull request has been closed due to not having had activity from an Istio team member since 2022-08-05. If you feel this issue or pull request deserves attention, please reopen the issue. Please see this wiki page for more information. Thank you for your contributions.

Created by the issue and PR lifecycle manager.

@istio-policy-bot istio-policy-bot added the lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. label Sep 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@costinm costinm Awaiting requested review from costinm

@jacob-delgado jacob-delgado Awaiting requested review from jacob-delgado

@Monkeyanator Monkeyanator Awaiting requested review from Monkeyanator

Assignees
No one assigned
Labels
do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. lifecycle/automatically-closed Indicates a PR or issue that has been closed automatically. lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stevenctl @istio-testing @istio-policy-bot @howardjohn @hzxuzhonghu