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

destination: send Opaque protocol hint for opaque ports #10301

Merged
merged 6 commits into from
Apr 14, 2023
Merged

destination: send Opaque protocol hint for opaque ports #10301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8415f25
update proxy-api to v0.8.0
hawkw Feb 9, 2023
e10d05a
also update policy-controller proxy-api dep to v0.8.0
hawkw Feb 9, 2023
37f6ed9
destination: send `Opaque` protocol hint for opaque ports
hawkw Feb 9, 2023
b4a1ea7
fix conflicting proxy-api rust deps
hawkw Feb 9, 2023
c8b451e
make h2 upgrade and opaque mutually exclusive
hawkw Feb 9, 2023
d8c2dd6
Merge branch 'main' into eliza/9888
hawkw Apr 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions controller/api/destination/endpoint_translator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,14 +385,10 @@ func createWeightedAddr(address watcher.Address, opaquePorts map[uint32]struct{}
// translation)
weightedAddr.ProtocolHint = &pb.ProtocolHint{}
if controllerNSLabel != "" && !isSkippedInboundPort {
if enableH2Upgrade {
weightedAddr.ProtocolHint.Protocol = &pb.ProtocolHint_H2_{
H2: &pb.ProtocolHint_H2{},
}
}
// If address is set as opaque by a Server, or its port is set as
// opaque by annotation or default value, then hint its proxy's
// inbound port.
// inbound port, and set the hinted protocol to Opaque, so that the
// client proxy does not send a SessionProtocol.
_, opaquePort := opaquePorts[address.Port]
if address.OpaqueProtocol || opaquePort {
port, err := getInboundPort(&address.Pod.Spec)
Expand All @@ -402,6 +398,13 @@ func createWeightedAddr(address watcher.Address, opaquePorts map[uint32]struct{}
weightedAddr.ProtocolHint.OpaqueTransport = &pb.ProtocolHint_OpaqueTransport{
InboundPort: port,
}
weightedAddr.ProtocolHint.Protocol = &pb.ProtocolHint_Opaque_{
Opaque: &pb.ProtocolHint_Opaque{},
}
}
} else if enableH2Upgrade {
weightedAddr.ProtocolHint.Protocol = &pb.ProtocolHint_H2_{
H2: &pb.ProtocolHint_H2{},
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this might read more clearly if we put the enableH2Upgrade block as an else if here. This helps communicate that these are mutually exclusive and that opaque takes precedence, rather than relying on ordering and overwriting. However, if we do that, we'll need to be a bit careful to make sure we properly handle the case when OpaqueProtocol is set but getInboundPort fails (what even IS the correct behavior in that case?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, AFAICT if getInboundPort fails, that means that we couldn't parse a port from the pod spec's proxy container's env vars, or the pod doesn't have a proxy container. if there's no proxy at all, we shouldn't be enabling the opaque transport flag... but if the env var is malformed, what should we do? just skip the update entirely?

Copy link
Member

Choose a reason for hiding this comment

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

Previously, if getInboundPort failed, we would log and leave the protocol hint set to H2 (if enabled). Not sure if that was the right behavior to begin with, but after this change the behavior would be to leave the protocol completely unset.

Would it make sense to still set ProtocolHint_Opaque, even if the getInboundPort fails? Is that a valid configuration? Can opaque traffic be sent without the tagged transport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously, if getInboundPort failed, we would log and leave the protocol hint set to H2 (if enabled). Not sure if that was the right behavior to begin with, but after this change the behavior would be to leave the protocol completely unset.

I think the new behavior is more correct. We should not be attempting an H2 upgrade if targeting a port that's marked as opaque, since we know that the proxy on the receiving end will not attempt a downgrade back to HTTP/1.

Would it make sense to still set ProtocolHint_Opaque, even if the getInboundPort fails? Is that a valid configuration? Can opaque traffic be sent without the tagged transport?

The Opaque protocol hint is meaningless without tagged transport, as it only configures what SessionProtocol is sent when using tagged transport. perhaps we should still send it in this case, but we should not be populating the ProtocolHint.OpaqueTransport field, as we don't know the port that connections should be directed to, and therefore cannot use opaque transport.

}
Expand Down
57 changes: 57 additions & 0 deletions controller/api/destination/endpoint_translator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,38 @@ var (
},
}

podOpaque = watcher.Address{
IP: "1.1.1.4",
Port: 4,
Pod: &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Name: "pod4",
Namespace: "ns",
Labels: map[string]string{
k8s.ControllerNSLabel: "linkerd",
k8s.ProxyDeploymentLabel: "deployment-name",
},
Annotations: map[string]string{
k8s.ProxyOpaquePortsAnnotation: "4",
},
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Name: k8s.ProxyContainerName,
Env: []corev1.EnvVar{
{
Name: envInboundListenAddr,
Value: "0.0.0.0:4143",
},
},
},
},
},
},
OpaqueProtocol: true,
}

remoteGateway1 = watcher.Address{
IP: "1.1.1.1",
Port: 1,
Expand Down Expand Up @@ -340,6 +372,31 @@ func TestEndpointTranslatorForPods(t *testing.T) {
t.Fatalf("Expected TlsIdentity to be [%v] but was [%v]", expectedTLSIdentity, actualTLSIdentity)
}
})

t.Run("Sends Opaque ProtocolHint for opaque ports", func(t *testing.T) {
expectedProtocolHint := &pb.ProtocolHint{
Protocol: &pb.ProtocolHint_Opaque_{
Opaque: &pb.ProtocolHint_Opaque{},
},
OpaqueTransport: &pb.ProtocolHint_OpaqueTransport{
InboundPort: 4143,
},
}

mockGetServer, translator := makeEndpointTranslator(t)

translator.Add(mkAddressSetForServices(podOpaque))

addrs := mockGetServer.updatesReceived[0].GetAdd().GetAddrs()
if len(addrs) != 1 {
t.Fatalf("Expected [1] address returned, got %v", addrs)
}

actualProtocolHint := addrs[0].GetProtocolHint()
if diff := deep.Equal(actualProtocolHint, expectedProtocolHint); diff != nil {
t.Fatalf("ProtocolHint: %v", diff)
}
})
}

func TestEndpointTranslatorForZonedAddresses(t *testing.T) {
Expand Down