Tls Passthrough support #643
Conversation
| @@ -28,33 +30,29 @@ func (t *latticeServiceModelBuildTask) extractListenerInfo( | |||
| if err != nil { | |||
There was a problem hiding this comment.
Just to make the extractListenerInfo() code logic more easy to follow (by reversing the parentRef.SectionName != nil condition and removing the found := false flag and return the function earlier ).
and override protocol string to "TLS_PASSTHROUGH" when create the lattice listener for k8s Gateway with section.TLS.Mode==Passthrough
zijun726911
force-pushed
the
tls-passthrough-support
branch
11 times, most recently
from
94336dd to
eefb72a
Compare
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
eefb72a to
595fff8
Compare
.github/workflows/publish-doc.yaml
Outdated
| @@ -26,7 +26,7 @@ jobs: | |||
| pip install mkdocs-material mike | |||
| - name: Build | |||
| run: | | |||
| mike deploy 1.0.5 latest --update-aliases --push | |||
| mike deploy v1.0.6-rc1 latest --update-aliases --push | |||
There was a problem hiding this comment.
These changes seems not related to TLS passthrough feature. Can you separate them into different CR? thanks
There was a problem hiding this comment.
This one is a little bit special, need to bump up the doc version here. otherwise the the doc version 1.0.5 will include the tls-passthrough.md, but the the controller v1.0.5 actually don't support the TLSRoute.
https://www.gateway-api-controller.eks.aws.dev/1.0.5/
What I am thinking is we can improve the whole Github Action (CI pipeline) publish-doc.yaml (in the future), for example, main branch code change trigger the mike deploy dev latest, and branch release-xxxxx trigger the mike deploy v1.x.x --push
zijun726911
changed the title
There was a problem hiding this comment.
tlsroute is still experimental crd and not in the standard gateway api, copy from:
https://github.com/kubernetes-sigs/gateway-api/blob/main/config/crd/experimental/gateway.networking.k8s.io_tlsroutes.yaml
.github/workflows/publish-doc.yaml
Outdated
| @@ -26,7 +26,7 @@ jobs: | |||
| pip install mkdocs-material mike | |||
| - name: Build | |||
| run: | | |||
| mike deploy 1.0.5 latest --update-aliases --push | |||
| mike deploy v1.0.6-rc1 latest --update-aliases --push | |||
There was a problem hiding this comment.
This one is a little bit special, need to bump up the doc version here. otherwise the the doc version 1.0.5 will include the tls-passthrough.md, but the the controller v1.0.5 actually don't support the TLSRoute.
https://www.gateway-api-controller.eks.aws.dev/1.0.5/
What I am thinking is we can improve the whole Github Action (CI pipeline) publish-doc.yaml (in the future), for example, main branch code change trigger the mike deploy dev latest, and branch release-xxxxx trigger the mike deploy v1.x.x --push
There was a problem hiding this comment.
Thanks for taking on this massive change @zijun726911 and thanks to @liwenwu-amazon for the initial logic. I've added quite a few comments - please reach out if anything needs clarification.
I understand these changes all go together, but it would be really helpful to have this PR broken up. I'd love to see something like:
- Refactoring (no logic changes)
- New code/logic changes
- Documentation (can even break this up if needed)
I'm OK reviewing 1+2 together, but if we could decouple documentation that would be really helpful. In theory, the docs for this should all be additive functionality anyway.
pkg/controllers/route_controller.go
Outdated
| //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=grpcroutes;httproutes,verbs=get;list;watch;create;update;patch;delete | ||
| //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=grpcroutes/status;httproutes/status,verbs=get;update;patch | ||
| //+kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=grpcroutes/finalizers;httproutes/finalizers,verbs=update | ||
|
|
||
| // +kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=tlsroutes;grpcroutes;httproutes,verbs=get;list;watch;create;update;patch;delete | ||
| // +kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=tlsroutes/status;grpcroutes/status;httproutes/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=tlsroutes/finalizers;grpcroutes/finalizers;httproutes/finalizers,verbs=update |
There was a problem hiding this comment.
Do we actually use these kubebuilder directives?
There was a problem hiding this comment.
The +kubebuilder:rbac annotation instructs the controller-gen rbac command to generate the ClusterRole, however our Makefile don't run the controller-gen rbac, I think they are useless, will remove them
There was a problem hiding this comment.
Yeah that's what I thought. Just wanted to double check. Thanks for checking.
| defaultAction := vpclattice.RuleAction{ | ||
| FixedResponse: &vpclattice.FixedResponseAction{ | ||
| StatusCode: aws.Int64(404), | ||
| }, | ||
| } | ||
|
|
||
| if modelListener.Spec.Protocol == vpclattice.ListenerProtocolTlsPassthrough { | ||
| // Fill the defaultAction tgs for TLS_PASSTHROUGH lattice listener | ||
| var latticeTGs []*vpclattice.WeightedTargetGroup | ||
| for _, modelTG := range defaultActionTgs { | ||
| latticeTG := vpclattice.WeightedTargetGroup{ | ||
| TargetGroupIdentifier: aws.String(modelTG.LatticeTgId), | ||
| Weight: aws.Int64(modelTG.Weight), | ||
| } | ||
| latticeTGs = append(latticeTGs, &latticeTG) | ||
| } | ||
| d.log.Debugf("For TLS_PASSTHROUGH listener, forward to default target groups %v", latticeTGs) | ||
| defaultAction = vpclattice.RuleAction{ | ||
| Forward: &vpclattice.ForwardAction{ | ||
| TargetGroups: latticeTGs, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you refactor this to a method? Something like getDefaultAction or similar. Alternatively, maybe just the logic in the if block. It actually looks pretty similar to the logic in rule_manager.go, maybe see if it's possible to combine?
test/pkg/test/tlsroute.go
Outdated
| SectionName: lo.ToPtr(gwv1.SectionName("tls")), | ||
| }}, | ||
| }, | ||
| Hostnames: []gwv1.Hostname{"tls.test.com"}, |
There was a problem hiding this comment.
Let's use something that isn't a real domain here.
There was a problem hiding this comment.
Changed in the next revision.
| Expect(*tgSummary.VpcIdentifier).To(Equal(os.Getenv("CLUSTER_VPC_ID"))) | ||
| Expect(*tgSummary.Protocol).To(Equal("TCP")) | ||
| if tg.Config.HealthCheck != nil { | ||
| Expect(*tg.Config.HealthCheck.Enabled).To(BeFalse()) |
There was a problem hiding this comment.
Since we already need a TG policy here, would it be a good idea to include an enabled health check to make sure that's being set up properly?
| } | ||
|
|
||
| targetsV1 := testFramework.GetTargets(ctx, tgSummary, deployment1) | ||
| Expect(*tgSummary.Port).To(BeEquivalentTo(80)) |
There was a problem hiding this comment.
This 80 is come from here:
The Target Group port is kind of useless, only the lattice targets' port are useful and guide vpc lattice to route traffic to that port. i.e, :
apiVersion: gateway.networking.k8s.io/v1alpha2
kind: TLSRoute
metadata:
name: rate-tls-passthrough
spec:
hostnames:
- tls-rate.my-test.com
parentRefs:
- name: my-hotel-tls-passthrough
sectionName: tls
rules:
- backendRefs:
- name: tls-rate1
kind: Service
port: 443 <-------- This port + k8s service ports intersection define to the lattice target port
There was a problem hiding this comment.
Should we just drop this Expect() then since we don't really care?
| pod := pods[0] | ||
|
|
||
| Eventually(func(g Gomega) { | ||
| cmd := fmt.Sprintf("curl -k https://tls.test.com:444 --resolve tls.test.com:444:%s", dnsIP[0]) |
There was a problem hiding this comment.
Should we have an explicit check for port 444 on the listener?
| Expect(*tg.Config.HealthCheck.Enabled).To(BeFalse()) | ||
| } | ||
| targetsV1 := testFramework.GetTargets(ctx, tgSummary, deployment1) | ||
| Expect(*tgSummary.Port).To(BeEquivalentTo(80)) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Same here, should we drop the Expect()?
There was a problem hiding this comment.
protocolVersion:
description: The protocol version used when performing health
checks on targets. Defaults to HTTP/1.
Hc config protocol version needs to be changed too, it doesn't have a default for tcp tg, customers need to specify both health check protocol and the protocol version
There was a problem hiding this comment.
will remove the sentence "Defaults to HTTP/1." in the next revision
- Address PR comments
|
One other question around TLSRouteRule/BackendRef from the spec: https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1alpha2/tlsroute_types.go#L103 Just curious what our current approach does with invalid BackendRefs. I think this is probably a less vital part of the spec to honour, especially given it's still experiemental, but would be good to describe the expected behaviour here. |
I will document this part. actually the current behavior is, for example for this TLSRoute: If the tls-rate2 targetGroup is not ready, it will block the traffic for both tls-rate1 and tls-rate2 ( base on the ResolveRuleTgIds() logic ) |
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
febd9cf to
5bc2283
Compare
zijun726911
force-pushed
the
tls-passthrough-support
branch
2 times, most recently
from
ff999a2 to
9b3f323
Compare
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
9b3f323 to
6f3d2aa
Compare
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
e63fa8b to
2b19a8a
Compare
There was a problem hiding this comment.
Thanks again @zijun726911, for all your work on this. I just have a couple remaining requests to work through then I think we'll be all set.
| @@ -80,6 +89,45 @@ func (l *listenerSynthesizer) Synthesize(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (l *listenerSynthesizer) getLatticeListenerDefaultAction(ctx context.Context, modelListenerProtocol string) ( | |||
There was a problem hiding this comment.
Looking at this again it seems like this logic is out of place in the synthesizer.
The default action should be part of the model listener spec. Then, we would set the default action inside model_build_listener.go.
In Synthesize() above, we would resolve target group Ids for the default action. Finally, we would read the default action off the modelListener and convert to Lattice format inside the listener_manager.go#Upsert method. Upsert would then not need an additional argument for the default action.
Can you please have a look at this approach and see if it's feasible?
There was a problem hiding this comment.
Thanks for your suggestion! I did some changes in the new revision and that did make the code cleaner:
-
Added the field
DefaultActionin themodelListener.ListenerSpecaws-application-networking-k8s/pkg/model/lattice/listener.go
Lines 7 to 25 in 036e2a3
-
Completely skip to create stackRule for TLS_PASSTHROUGH gwListener. i.e, completely don't call
latticeServiceModelBuildTask.buildRules()for TLS_PASSTHROUGH gwListener, therefore removing the "long comments" in thebuildRules()and therefore removing the seemly confusing codel.tgManager.ResolveRuleTgIds(ctx, stackRules[0], l.stack)(changed tol.tgManager.ResolveRuleTgIds(ctx, stackListener.Spec.DefaultAction.Forward, l.stack)instead). And since we don't create any stackRule for TLS_PASSTHROUGH gwListener, I also remove the TlsPassthrough skipping logic in the(r *ruleSynthesizer) createOrUpdateRules() -
Added the
(t *latticeServiceModelBuildTask) getListenerDefaultActionin themodel_build_listener(Added unit test to cover build TLS_PASSTHROUGH listener)aws-application-networking-k8s/pkg/gateway/model_build_listener.go
Lines 128 to 151 in cd6072c
-
Changed the logic of
(l *listenerSynthesizer) getLatticeListenerDefaultAction(, removed the seemly confusing partaws-application-networking-k8s/pkg/deploy/lattice/listener_synthesizer.go
Lines 92 to 127 in cd6072c
l.tgManager.ResolveRuleTgIds(ctx, stackRules[0], l.stack), change tol.tgManager.ResolveRuleTgIds(ctx, stackListener.Spec.DefaultAction.Forward, l.stack);()instead. -
Still keep the
defaultActionargument for thelistenerManager.Upsert(), since in the listenerSynthesizer, it needs to convert themodel.ListenerDefaultActioninto thevpcLattice.ListenerDefaultAction, and pass thevpcLattice.ListenerDefaultActiontolistenerManager.Upsert()to "determine" (needToUpdateDefaultAction()) whether the lattice.UpdateListener() need to be called.
Ran the full test suite for the new revision(skip the RAM test):
• [257.448 seconds]
------------------------------
[SynchronizedAfterSuite]
/Volumes/workspace/aws-application-networking-k8s/test/suites/integration/suite_test.go:72
{“level”:“info”,“ts”:“2024-06-05T23:08:51.716-0700",“caller”:“test/framework.go:264",“msg”:“Deleting objects: *v1.Gateway/test-gateway, *v1.Pod/grpc-runner”}
{“level”:“info”,“ts”:“2024-06-05T23:08:51.758-0700",“caller”:“test/framework.go:282",“msg”:“Waiting for NotFound, objects: *v1.Gateway/test-gateway, *v1.Pod/grpc-runner”}
[SynchronizedAfterSuite] PASSED [31.193 seconds]
------------------------------
Ran 66 of 69 Specs in 2412.270 seconds
SUCCESS! -- 66 Passed | 0 Failed | 0 Pending | 3 Skipped
--- PASS: TestIntegration (2412.27s)
PASS
pkg/controllers/route_controller.go
Outdated
| ) | ||
|
|
||
| var routeTypeToFinalizer = map[core.RouteType]string{ | ||
| core.HttpRouteType: "httproute.k8s.aws/resources", | ||
| core.GrpcRouteType: "grpcroute.k8s.aws/resources", | ||
| core.TlsRouteType: "tlsroute.k8s.aws/resource", |
There was a problem hiding this comment.
The others are "resources" but this one is "resource"?
There was a problem hiding this comment.
Changed in the new revisions
pkg/gateway/model_build_rule.go
Outdated
|
|
||
| // For TLSRoute, although in here we create a lattice listener rule with PathMatchValue = "/", but it will not be used in the rule_synthesizer, | ||
| // Since vpc lattice doesn't support any rule matching for TLS_PASSTHROUGH listener, | ||
| // the rule_synthesizer will skip updating the rule while the listener_synthesizer will fill the listener with the default action route to the referenced TCP target groups | ||
| // https://docs.aws.amazon.com/vpc-lattice/latest/ug/tls-listeners.html |
There was a problem hiding this comment.
The reason I'd like to change is that right now we have a few comment lines explaining why the code should drop through to the else for TLS passthrough. A multi-line explanation in comments can be a warning sign we don't have the code structured correctly. I'd like us to rather take an approach that is explicit, intuitive to follow, and needs no justifying or explanatory comments. I think what I proposed (or something like it) would do the trick.
Ideally we would redefine the interfaces so there were separate L4 and L7 interfaces, and there would be no Matches() at all on TCPRoute. This would be a larger change and is probably OK to skip for now, but we should not be relying on the empty list behavior here.
| } | ||
|
|
||
| targetsV1 := testFramework.GetTargets(ctx, tgSummary, deployment1) | ||
| Expect(*tgSummary.Port).To(BeEquivalentTo(80)) |
There was a problem hiding this comment.
Should we just drop this Expect() then since we don't really care?
| Expect(*tg.Config.HealthCheck.Enabled).To(BeFalse()) | ||
| } | ||
| targetsV1 := testFramework.GetTargets(ctx, tgSummary, deployment1) | ||
| Expect(*tgSummary.Port).To(BeEquivalentTo(80)) |
There was a problem hiding this comment.
Same here, should we drop the Expect()?
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
a6916b0 to
cd6072c
Compare
Changed in the new revision. please check: #643 (comment)
Removed assertion for the tg port in the new revision. |
…he model_build_listener
zijun726911
force-pushed
the
tls-passthrough-support
branch
from
cd6072c to
036e2a3
Compare
| @@ -80,6 +89,45 @@ func (l *listenerSynthesizer) Synthesize(ctx context.Context) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (l *listenerSynthesizer) getLatticeListenerDefaultAction(ctx context.Context, stackListener *model.Listener) ( | |||
There was a problem hiding this comment.
The conversion logic from model -> Lattice type typically lives in each component's manager. Here, it should only resolve the target group Ids.
Please move getDefaultAction logic inside listener_manager. The logic should assume the target group Ids have already been resolved. However, it needs to handling the case where the target group ID is set to model.InvalidBackendRefTgId. This can happen when the backendRef points to something that doesn't exist. Note we should have a test specifically for InvalidBackendRefTgId handling, which I don't think we do yet since I don't see it referenced here.
There was a problem hiding this comment.
Working on in. Will do the exactly you describe here in the new revision. Will mimic the r *defaultRuleManager) buildLatticeRule() to get the listener defaultAction
aws-application-networking-k8s/pkg/deploy/lattice/rule_manager.go
Lines 92 to 143 in 3e65856
There was a problem hiding this comment.
Sent the new revision can you take a look? Did the changes exactly as you describe above. Thank you! b4dfd4d
Note we should have a test specifically for InvalidBackendRefTgId handling
The test cases: Test_ListenerManager_getLatticeListenerDefaultAction_TLS_PASSTHROUGH_Listener covered that.
Also ran the full e2e test suite against new revision
[SynchronizedAfterSuite]
/Volumes/workspace/aws-application-networking-k8s/test/suites/integration/suite_test.go:72
{“level”:“info”,“ts”:“2024-06-07T01:26:52.316-0700",“caller”:“test/framework.go:264",“msg”:“Deleting objects: *v1.Gateway/test-gateway, *v1.Pod/grpc-runner”}
{“level”:“info”,“ts”:“2024-06-07T01:26:52.357-0700",“caller”:“test/framework.go:282",“msg”:“Waiting for NotFound, objects: *v1.Gateway/test-gateway, *v1.Pod/grpc-runner”}
[SynchronizedAfterSuite] PASSED [31.041 seconds]
------------------------------
Ran 66 of 69 Specs in 2407.814 seconds
SUCCESS! -- 66 Passed | 0 Failed | 0 Pending | 3 Skipped
--- PASS: TestIntegration (2407.82s)
PASS
ok [github.com/aws/aws-application-networking-k8s/test/suites/integration](http://github.com/aws/aws-application-networking-k8s/test/suites/integration) 2408.753s
| if err := l.tgManager.ResolveRuleTgIds(ctx, stackListener.Spec.DefaultAction.Forward, l.stack); err != nil { | ||
| return nil, fmt.Errorf("failed to resolve rule tg ids, err = %v", err) | ||
| } |
There was a problem hiding this comment.
Bring this block into the Synthesize() method.
There was a problem hiding this comment.
Will change in the new revision
| if stackListener.Spec.DefaultAction == nil || | ||
| (stackListener.Spec.DefaultAction.FixedResponseStatusCode == nil && stackListener.Spec.DefaultAction.Forward == nil) { | ||
| return nil, fmt.Errorf("invalid listener default action, must be either fixed response or forward") | ||
| } |
There was a problem hiding this comment.
We should rather validate this when we add the listener to the stack in listener.go#NewListener().
There was a problem hiding this comment.
Will mimic the func (t *TargetGroupSpec) Validate(), and put that validation in the new function func (spec *ListenerSpec) Validate()
| @@ -41,3 +60,24 @@ func NewListener(stack core.Stack, spec ListenerSpec) (*Listener, error) { | |||
|
|
|||
| return listener, nil | |||
| } | |||
|
|
|||
| func (spec *ListenerSpec) Validate() error { | |||
| if stackListener.Spec.Protocol == vpclattice.ListenerProtocolTlsPassthrough { | ||
| return nil, fmt.Errorf("TLSRoute %s/%s must have at least one valid backendRef target group", stackListener.Spec.K8SRouteNamespace, stackListener.Spec.K8SRouteName) | ||
| } else { | ||
| return nil, fmt.Errorf("unreachable code, since the defaultAction for non-TLS_PASSTHROUGH listener is always the FixedResponse 404") |
Initial version cherry picked from: https://github.com/liwenwu-amazon/aws-application-networking-k8-publics/tree/tls-route-support-mar20
User experiences about TLSRoute, TargetGroupPolicy of this PR are same with the
liwenwu-amazon:tls-route-support-mar20 branch.main branchtoliwenwu-amazon:tls-route-support-mar20 branchdiff: zijun726911#3TLS Passthrough Behaviors Summary
Kind:TLSRoutek8s resource, the controller will under the hood create VPC Lattice Service with TLS_PASSTHROUGH protocol listener and TCP type TargetGroup.Kind:ServiceExportk8s resource and create theKind:TargetGroupPolicyresource to specify the TargetGroup protocol to TCP. The controller will under the hood create VPC Lattice TCP TargetGroupKind:TargetGroupPolicyto override the healthcheck config for either Service (backendref by TLSRoute) or ServiceExportapplication-networking.k8s.aws/ProtocolVersion:""Changes
The controller logic changes:
protocol:TCPTargetGroupPolicy.rule_synthesizer.go, skips to create rules for TLSRoute since Lattice TLS_PASSTHROUGH listener can only have one defaultAction and without any addition rules. In thelistener_synthesizer.go, fill the lattice listener's defaultAction to point to the referred TCP TargetGroups.rule_synthesizerandlistener_synthesizerneed theResolveRuleTgIds()function (originally resided in the rule_synthesizer.go), so I move theResolveRuleTgIds()andfindSvcExportTG()to thetarget_group_manager.goto avoid copy paste same code, and therule_synthesizerandlistener_synthesizercan both use the sames.tgManager.ResolveRuleTgIds(ctx, rule, r.stack)Test_ResolveRuleTgIds()to the target_group_manager_test.goCreate()andUpdate()methods). If the user specify the healthcheck setting by TargetGroupPolicy, the default healthcheck setting will be overridden. (Disable healthcheck by default for TCP TargetGroup is also the default VPC Lattice API default behavior) https://docs.aws.amazon.com/vpc-lattice/latest/ug/target-group-health-checks.htmlapplication-networking.k8s.aws/ProtocolVersion:"". In the(t *TargetGroupSpec) Validate()remove theProtocolVersionfromrequiredFields.targetRef ServiceExport, now, the targetgrouppolicy_controllerWatch()the ServiceExport resource changee2e test changes
protocol:TCPTargetGroupPolicyCRDs && Image build && helm chart related changes
Testing
Changes for the new revision: #643 (comment)
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.