Update Routes for VirtualService/VirtualRouter changes #8
Conversation
pkg/aws/appmesh.go
Outdated
| }, nil | ||
| } | ||
|
|
||
| func notFound(err error) bool { |
There was a problem hiding this comment.
Nit: I like the cleanup, but maybe a too general name? What about isErrCodeNotFound()?
| @@ -85,6 +85,8 @@ spec: | |||
| weight: 1 | |||
| - virtualNodeName: colorteller-blue-vn | |||
| weight: 1 | |||
| - virtualNodeName: colorteller-black-vn | |||
| weight: 1 | |||
There was a problem hiding this comment.
Is there a way to check that weight is between 1 and 100, and throw a useful error message if it is outside the boundary?
There was a problem hiding this comment.
I think the range validation (0-100) can be made in the CRD API spec with a regex.
pkg/aws/appmesh.go
Outdated
| } | ||
|
|
||
| return &Route{ | ||
| Data: *output.Route, |
There was a problem hiding this comment.
This will panic see 2fa2058
| } | ||
| extraTarget = []appmeshv1alpha1.WeightedTarget{ | ||
| {Weight: int64(1), VirtualNodeName: defaultNodeName}, | ||
| {Weight: int64(1), VirtualNodeName: "extra-node"}, |
There was a problem hiding this comment.
Weight should be different for each target, due to a the pointer ref in here https://github.com/aws/aws-app-mesh-controller-for-k8s/pull/7/files#diff-a9ebef8a191b968924fc4bb6a3d2f788L511 you'll end up with the same weight for all targets. Also there should be a test that checks the weights are created and changed accordingly.
There was a problem hiding this comment.
Good catch, thanks.
There was a problem hiding this comment.
I'll add this in another PR.
pkg/aws/appmesh.go
Outdated
| return nil, nil | ||
| } | ||
| if notFound(err) { | ||
| return nil, nil |
There was a problem hiding this comment.
Why does this returns nil instead of a not found error?
There was a problem hiding this comment.
I wanted an obvious way to distinguish from other errors, that allowed a simpler check than the aws error type switch statement in calling code, but perhaps I should either just return the aws error or wrap it and return it. I think probably just returning the aws error is fine and moving notFound() to the calling package.
| @@ -85,6 +85,8 @@ spec: | |||
| weight: 1 | |||
| - virtualNodeName: colorteller-blue-vn | |||
| weight: 1 | |||
| - virtualNodeName: colorteller-black-vn | |||
| weight: 1 | |||
There was a problem hiding this comment.
I think the range validation (0-100) can be made in the CRD API spec with a regex.
pkg/aws/appmesh.go
Outdated
| } | ||
|
|
||
| return &VirtualService{ | ||
| Data: *output.VirtualService, |
There was a problem hiding this comment.
This will panic see 2fa2058
This change creates, updates and deletes routes in AppMesh based on the VirtualService custom resource.
Similar to #7 except that it also cleans up routes for a virtual router that is referenced by a
VirtualServicecustom resource.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.