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

ambient: fix auto-allow waypoint #50710

Merged
merged 2 commits into from
Apr 29, 2024
Merged

ambient: fix auto-allow waypoint #50710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
815149b
ambient: fix auto-allow waypoint
howardjohn Apr 22, 2024
8c3042c
fix tests
stevenctl Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions pilot/pkg/serviceregistry/kube/controller/ambient/ambientindex_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1073,15 +1073,15 @@ func TestAmbientIndex_Policy(t *testing.T) {

func TestDefaultAllowWaypointPolicy(t *testing.T) {
// while the Waypoint is in testNS, the policies live in the Pods' namespaces
policyName := "istio_allow_waypoint_" + testNS + "_" + "waypoint-ns"
policyName := "ns1/istio_allow_waypoint_" + testNS + "_" + "waypoint-ns"
test.SetForTest(t, &features.DefaultAllowFromWaypoint, true)

s := newAmbientTestServer(t, testC, testNW)
setupPolicyTest(t, s)

t.Run("policy with service accounts", func(t *testing.T) {
assert.EventuallyEqual(t, func() []string {
waypointPolicy := s.authorizationPolicies.GetKey(krt.Key[model.WorkloadAuthorization]("ns1/" + policyName))
waypointPolicy := s.authorizationPolicies.GetKey(krt.Key[model.WorkloadAuthorization](policyName))
if waypointPolicy == nil {
return nil
}
Expand All @@ -1090,8 +1090,8 @@ func TestDefaultAllowWaypointPolicy(t *testing.T) {
return sm.GetExact()
})
}, []string{
"spiffe://cluster.local/ns/ns1/sa/namespace-wide",
"spiffe://cluster.local/ns/ns1/sa/waypoint-sa",
"cluster.local/ns/ns1/sa/namespace-wide",
"cluster.local/ns/ns1/sa/waypoint-sa",
})
})

Expand Down
4 changes: 3 additions & 1 deletion pilot/pkg/serviceregistry/kube/controller/ambient/policies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
package ambient

import (
"strings"

v1 "k8s.io/api/core/v1"

securityclient "istio.io/client-go/pkg/apis/security/v1beta1"
Expand Down Expand Up @@ -124,7 +126,7 @@ func implicitWaypointPolicy(ctx krt.HandlerContext, waypoint Waypoint) *model.Wo
{
Principals: slices.Map(waypoint.ServiceAccounts, func(sa string) *security.StringMatch {
return &security.StringMatch{MatchType: &security.StringMatch_Exact{
Exact: spiffe.MustGenSpiffeURI(waypoint.Namespace, sa),
Exact: strings.TrimPrefix(spiffe.MustGenSpiffeURI(waypoint.Namespace, sa), spiffe.URIPrefix),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do the SANs not have a spiffe:// prefix? Do SPIRE generated SANs have a spiffe prefix?

Copy link
Contributor

@bleggett bleggett Apr 29, 2024 •
edited

Choose a reason for hiding this comment

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

Also, can't we just use StringMatch_Prefix directly here, rather than trimming the prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        // Istio matches all assumes spiffe:// prefix. This includes prefix matches.
        // A prefix match for "*foo" means "spiffe://*foo".
        // So we strip it, and fail if it isn't present.
        let Some(check) = check.strip_prefix("spiffe://") else {
            return false;
        };

This is what zTunnel does

Copy link
Contributor

@bleggett bleggett Apr 29, 2024 •
edited

Choose a reason for hiding this comment

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

Istio matches all assumes spiffe:// prefix. This includes prefix matches.

why do we assume that, I wonder. It's not simpler than just matching on spiffe://ns/sa prefixes consistently everywhere, since, as @keithmattix pointed out, the spiffe:// prefix is non-optional.

Oh well.

Copy link
Member

Choose a reason for hiding this comment

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

Property of the AuthorizationPolicy API fwiw, not a recent decision in ztunnel

}}
}),
},
Expand Down
2 changes: 1 addition & 1 deletion pilot/pkg/serviceregistry/kube/controller/ambient/workloads.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,6 @@ func implicitWaypointPolicies(ctx krt.HandlerContext, Waypoints krt.Collection[W
if policy == "" {
return nil
}
return &policy
return ptr.Of(w.Namespace + "/" + policy)
})
}
12 changes: 10 additions & 2 deletions pkg/test/framework/components/echo/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,13 @@ func (c Config) IsRegularPod() bool {
!c.DualStack
}

// WaypointClient means the client supports HBONE and does zTunnel redirection.
// Currently, only zTunnel captured sources do this. Eventually this might be enabled
// for ingress and/or sidecars.
func (c Config) WaypointClient() bool {
return c.ZTunnelCaptured() && !c.IsUncaptured()
}

// ZTunnelCaptured returns true in ambient enabled namespaces where there is no sidecar
func (c Config) ZTunnelCaptured() bool {
haveSubsets := len(c.Subsets) > 0
Expand Down Expand Up @@ -597,8 +604,9 @@ func (c Config) WorkloadClass() WorkloadClass {
return StatefulSet
} else if c.IsSotw() {
return Sotw
} else if c.ZTunnelCaptured() &&
!(c.HasServiceAddressedWaypointProxy() || c.HasWorkloadAddressedWaypointProxy()) {
} else if c.HasAnyWaypointProxy() {
return Waypoint
} else if c.ZTunnelCaptured() && !c.HasAnyWaypointProxy() {
return Captured
}
if c.IsHeadless() {
Expand Down
11 changes: 10 additions & 1 deletion pkg/test/framework/components/echo/echotest/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,10 +191,19 @@ var ReachableDestinations CombinationFilter = func(from echo.Instance, to echo.I
reachableFromVM(from),
reachableFromProxylessGRPC(from),
reachableNakedDestinations(from),
reachableHeadlessDestinations(from)).
reachableHeadlessDestinations(from),
reachableWaypoints(from)).
GetMatches(to)
}

// reachableWaypoints removes waypointed targets when the client doesn't
func reachableWaypoints(from echo.Instance) match.Matcher {
if from.Config().WaypointClient() {
return match.Any
}
return match.NotWaypoint
}

// reachableHeadlessDestinations filters out headless services that aren't in the same cluster
// TODO(stevenctl): headless across-networks https://github.com/istio/istio/issues/38327
func reachableHeadlessDestinations(from echo.Instance) match.Matcher {
Expand Down
5 changes: 2 additions & 3 deletions pkg/test/framework/components/echo/match/matchers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,9 @@ func WaypointService() Matcher {
}
}

// add a new matcher for "captured service -> service"
func CapturedService() Matcher {
func AmbientCaptured() Matcher {
return func(i echo.Instance) bool {
return i.Config().ZTunnelCaptured()
return i.Config().ZTunnelCaptured() && !i.Config().IsUncaptured()
}
}

Expand Down
1 change: 1 addition & 0 deletions pkg/test/framework/components/echo/workloadclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ const (
StatefulSet WorkloadClass = "statefulset"
Headless WorkloadClass = "headless"
Captured WorkloadClass = "captured"
Waypoint WorkloadClass = "waypoint"
Standard WorkloadClass = "standard"
)
57 changes: 33 additions & 24 deletions tests/integration/ambient/baseline_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,7 @@ func supportsL7(opt echo.CallOptions, src, dst echo.Instance) bool {
// Assumption is ambient test suite sidecars will support HBONE
// If the assumption is incorrect hboneClient may return invalid result
func hboneClient(instance echo.Instance) bool {
return instance.Config().ZTunnelCaptured() ||
instance.Config().HasSidecar()
return instance.Config().ZTunnelCaptured()
}

func TestServices(t *testing.T) {
Expand All @@ -159,13 +158,6 @@ func TestServices(t *testing.T) {
opt.Check = tcpValidator
}

if !hboneClient(src) && dst.Config().HasAnyWaypointProxy() {
// For this case, it is broken if the src and dst are on the same node.
// Because client request is not captured to perform the hairpin
// TODO(https://github.com/istio/istio/issues/43238): fix this and remove this skip
t.Skip("https://github.com/istio/istio/issues/44530")
}

if !dst.Config().HasServiceAddressedWaypointProxy() &&
!src.Config().HasServiceAddressedWaypointProxy() &&
(src.Config().Service != dst.Config().Service) &&
Expand All @@ -176,12 +168,15 @@ func TestServices(t *testing.T) {
opt.Check = check.And(opt.Check, OriginalSourceCheck(t, src))
}

if src.Config().ZTunnelCaptured() && dst.Config().HasWorkloadAddressedWaypointProxy() {
// this is to svc traffic on a wl with only a workload addressed waypoint, it is going to bypass the waypoint by design
// we can't check http because we bypass the waypoint
// I don't think it makes sense to change the supportsL7 function for this case since it requires contect
// about how the traffic will be addressed
opt.Check = tcpValidator
// Non-HBONE clients will attempt to bypass the waypoint
if !src.Config().WaypointClient() && dst.Config().HasAnyWaypointProxy() {
opt.Check = check.NotOK()
}

// Any client will attempt to bypass a workload waypoint (not both service and workload waypoint)
// because this test always addresses by service.
if dst.Config().HasWorkloadAddressedWaypointProxy() && !dst.Config().HasServiceAddressedWaypointProxy() {
opt.Check = check.Error()
}

if src.Config().HasSidecar() && dst.Config().HasWorkloadAddressedWaypointProxy() {
Expand Down Expand Up @@ -217,22 +212,29 @@ func TestPodIP(t *testing.T) {
opt.Check = tcpValidator
}

if src.Config().IsUncaptured() && dst.Config().HasAnyWaypointProxy() {
// hairpinning isn't going to be implemented AND
// waypoint requirements are expressed via L4 policy which is not in place for this test:
// expected result is a plaintext passthrough by ztunnel
opt.Check = tcpValidator
opt.Address = dstWl.Address()
opt.Check = check.And(opt.Check, check.Hostname(dstWl.PodName()))

opt.Port = echo.Port{ServicePort: ports.All().MustForName(opt.Port.Name).WorkloadPort}
opt.ToWorkload = dst.WithWorkloads(dstWl)

// Uncaptured means we won't traverse the waypoint
// We cannot bypass the waypoint, so this fails.
if !src.Config().WaypointClient() && dst.Config().HasAnyWaypointProxy() {
opt.Check = check.NotOK()
}

// Only marked to use service waypoint. We'll deny since it's not traversed.
// Not traversed, since traffic is to-workload IP.
if dst.Config().HasServiceAddressedWaypointProxy() && !dst.Config().HasWorkloadAddressedWaypointProxy() {
opt.Check = check.NotOK()
}

if selfSend {
// Calls to ourself (by pod IP) are not captured
opt.Check = tcpValidator
}
opt.Address = dstWl.Address()
opt.Check = check.And(opt.Check, check.Hostname(dstWl.PodName()))

opt.Port = echo.Port{ServicePort: ports.All().MustForName(opt.Port.Name).WorkloadPort}
opt.ToWorkload = dst.WithWorkloads(dstWl)
t.NewSubTestf("%v", opt.Scheme).RunParallel(func(t framework.TestContext) {
src.WithWorkloads(srcWl).CallOrFail(t, opt)
})
Expand Down Expand Up @@ -2209,6 +2211,13 @@ func TestIngress(t *testing.T) {
if opt.Scheme != scheme.HTTP {
return
}

// Ingress currently never sends to Waypoints
// We cannot bypass the waypoint, so this fails.
if dst.Config().HasAnyWaypointProxy() {
opt.Check = check.Error()
}

t.ConfigIstio().Eval(apps.Namespace.Name(), map[string]string{
"Destination": dst.Config().Service,
}, `apiVersion: networking.istio.io/v1alpha3
Expand Down
10 changes: 4 additions & 6 deletions tests/integration/ambient/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,18 +208,16 @@ func SetupApps(t resource.Context, i istio.Instance, apps *EchoDeployments) erro
Replicas: 1,
Version: "v1",
Labels: map[string]string{
"app": ServiceAddressedWaypoint,
"version": "v1",
constants.AmbientUseWaypoint: "waypoint",
"app": ServiceAddressedWaypoint,
"version": "v1",
},
},
{
Replicas: 1,
Version: "v2",
Labels: map[string]string{
"app": ServiceAddressedWaypoint,
"version": "v2",
constants.AmbientUseWaypoint: "waypoint",
"app": ServiceAddressedWaypoint,
"version": "v2",
},
},
},
Expand Down
34 changes: 19 additions & 15 deletions tests/integration/pilot/common/routing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1666,14 +1666,15 @@ func gatewayCases(t TrafficContext) {
}

// SingleRegualrPod is already applied leaving one regular pod, to only regular pods should leave a single workload.
singleTarget := []match.Matcher{match.RegularPod}
// the following cases don't actually target workloads, we use the singleTarget filter to avoid duplicate cases
// Gateways don't support talking directly to waypoint, so we skip that here as well.
matchers := []match.Matcher{match.RegularPod, match.NotWaypoint}

gatewayListenPort := 80
gatewayListenPortName := "http"
// the following cases don't actually target workloads, we use the singleTarget filter to avoid duplicate cases
t.RunTraffic(TrafficTestCase{
name: "404",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: httpGateway("*", gatewayListenPort, gatewayListenPortName, "HTTP", t.Istio.Settings().IngressGatewayIstioLabel),
Expand All @@ -1691,7 +1692,7 @@ func gatewayCases(t TrafficContext) {
})
t.RunTraffic(TrafficTestCase{
name: "https redirect",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -1729,7 +1730,7 @@ spec:
t.RunTraffic(TrafficTestCase{
// See https://github.com/istio/istio/issues/27315
name: "https with x-forwarded-proto",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -1803,7 +1804,8 @@ spec:
},
})
t.RunTraffic(TrafficTestCase{
name: "cipher suite",
name: "cipher suite",
targetMatchers: []match.Matcher{match.NotWaypoint},
config: gatewayTmpl + httpVirtualServiceTmpl +
ingressutil.IngressKubeSecretYAML("cred", "{{.IngressNamespace}}", ingressutil.TLS, ingressutil.IngressCredentialA),
templateVars: func(src echo.Callers, dests echo.Instances) map[string]any {
Expand All @@ -1825,7 +1827,8 @@ spec:
workloadAgnostic: true,
})
t.RunTraffic(TrafficTestCase{
name: "optional mTLS",
name: "optional mTLS",
targetMatchers: []match.Matcher{match.NotWaypoint},
config: gatewayTmpl + httpVirtualServiceTmpl +
ingressutil.IngressKubeSecretYAML("cred", "{{.IngressNamespace}}", ingressutil.TLS, ingressutil.IngressCredentialA),
templateVars: func(src echo.Callers, dests echo.Instances) map[string]any {
Expand All @@ -1847,7 +1850,7 @@ spec:
t.RunTraffic(TrafficTestCase{
// See https://github.com/istio/istio/issues/34609
name: "http redirect when vs port specify https",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -1890,7 +1893,7 @@ spec:
// See https://github.com/istio/istio/issues/27315
// See https://github.com/istio/istio/issues/34609
name: "http return 400 with with x-forwarded-proto https when vs port specify https",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -1966,7 +1969,7 @@ spec:
t.RunTraffic(TrafficTestCase{
// https://github.com/istio/istio/issues/37196
name: "client protocol - http1",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -2009,7 +2012,7 @@ spec:
// https://github.com/istio/istio/issues/37196
name: "client protocol - http2",
skip: skipEnvoyPeerMeta,
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -2061,7 +2064,7 @@ spec:
})
t.RunTraffic(TrafficTestCase{
name: "wildcard hostname",
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -2150,7 +2153,7 @@ spec:
// https://github.com/istio/istio/issues/37196
name: fmt.Sprintf("client protocol - %v use client with %v", protoName, port),
skip: skipEnvoyPeerMeta,
targetMatchers: singleTarget,
targetMatchers: matchers,
workloadAgnostic: true,
viaIngress: true,
config: `apiVersion: networking.istio.io/v1alpha3
Expand Down Expand Up @@ -2241,7 +2244,7 @@ spec:
check.RequestHeader("Istio-Custom-Header", "user-defined-value")),
},
// to keep tests fast, we only run the basic protocol test per-workload and scheme match once (per cluster)
targetMatchers: singleTarget,
targetMatchers: matchers,
viaIngress: true,
workloadAgnostic: true,
})
Expand Down Expand Up @@ -4062,7 +4065,8 @@ spec:
claim: "wrong_claim"
---
`
matchers := []match.Matcher{match.Or(match.ServiceName(t.Apps.B.NamespacedName()), match.WaypointService(), match.CapturedService())}
// No waypoint here, these are all via ingress which doesn't forward to waypoint
matchers := []match.Matcher{match.Or(match.ServiceName(t.Apps.B.NamespacedName()), match.AmbientCaptured())}
headersWithToken := map[string][]string{
"Host": {"foo.bar"},
"Authorization": {"Bearer " + jwt.TokenIssuer1WithNestedClaims1},
Expand Down