-
Notifications
You must be signed in to change notification settings - Fork 52
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
Enable GRPCRoute Traffic + Remaining GRPCRoute Event Handling #350
Conversation
# Conflicts: # test/pkg/test/framework.go
…g struct value instead of reference in httproute_controller, Using type switch with default error instead of if statement without error, Joined two logs into one
# Conflicts: # pkg/deploy/lattice/rule_manager.go # pkg/gateway/model_build_rule.go # pkg/gateway/model_build_rule_test.go # pkg/model/lattice/rule.go
Pull Request Test Coverage Report for Build 5957675227
💛 - Coveralls |
Can you add a grpc_xxx.md in the doc directory? You can put similar information you had in the CR description for now. So other folks can just follow that grpc_xxx.md to manually test e2e grpc functionalities. thanks |
Thank you for adding new logger and cleaning up logs. |
47 commits in diff seems not accurate, looks like your fork main branch out of sync |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, please address @mikhail-aws's comments
if domain, exists := route.Annotations[LatticeAssignedDomainName]; exists { | ||
for _, route := range routes { | ||
if route.DeletionTimestamp().IsZero() && len(route.K8sObject().GetAnnotations()) > 0 { | ||
if domain, exists := route.K8sObject().GetAnnotations()[LatticeAssignedDomainName]; exists { | ||
gw.Status.Addresses = append(gw.Status.Addresses, gateway_api.GatewayAddress{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we don't need this condition ? && len(route.K8sObject().GetAnnotations()) > 0
?
below condition should include that if domain, exists := route.K8sObject().GetAnnotations()[LatticeAssignedDomainName]; exists {
?
# Conflicts: # cmd/aws-application-networking-k8s/main.go # controllers/route_controller.go # controllers/serviceexport_controller.go # pkg/gateway/model_build_targetgroup.go # pkg/model/core/route.go
Will do this in a separate PR. Current focus is purely code change to unblock others. |
Most likely, though I will squash for the merge anyways. Will rebase after this to re-sync my local. |
Not sure why make presubmit fails here. It runs without issues on my end, and I have no file changes that aren't committed
|
try removing changes of |
What type of PR is this?
feature, cleanup
Which issue does this PR fix:
#25
What does this PR do / Why do we need it:
grpc-route.yaml
had an incorrectly configured match for the Greeter service and incorrect sectionName to allow trafficroute.go
,httproute.go
, andgrpcroute.go
Note
ServiceExport support is not included here, as it will require a massive refactor to how we store all target groups in the cache. @solmonk is working on something like this already, so he said he will own that refactor.
If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
kubectl apply -f examples/my-hotel-gateway-multi-listeners.yaml
kubectl apply -f examples/greeter-grpc-server.yaml
kubectl apply -f examples/greeter-grpc-route.yaml
kubectl get pods -A
kubectl exec -it <name-of-grpc-server-pod> -- bash
go run test.go <SERVICE DNS> <PORT>
Greeting: Hello world
Testing done on this change:
/helloworld.Greeter/
)Automation added to e2e:
This will be done separately
Will this PR introduce any new dependencies?:
No
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
No
Does this PR introduce any user-facing change?:
Yes
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.