Support new annotated format for upstreams #1237
Conversation
ndhanushkodi
requested review from
thisisnotashwin,
a team and
ishustava
and removed request for
a team
| case "dc": | ||
| datacenter = strings.TrimSpace(pieces[4]) | ||
| default: | ||
| return api.Upstream{}, errors.New(fmt.Sprintf("upstream structured incorrectly: %s", rawUpstream)) |
There was a problem hiding this comment.
I wonder if this should be a part of the Validation logic that we do in the connect webhook? that way, if the label is incorrect we can ensure pod creation fails with an accurate error message.
There was a problem hiding this comment.
Yeah, I think in order to do that we'd need to have the entire logic for this function live in the webhook since you can't catch these cases without parsing the entire annotation. I think it'd work, and it'd probably be nice to have the pod creation fail, so do you think we should have the logic in both places? The prepared query case and the "non-annotated" upstreams are just less likely to have parsing issues compared to this one due to the number of pieces to parse.
There was a problem hiding this comment.
If the two are in the same package, we should be able to extract this behavior in a way that can be reused potentially. I also think it is something we can do down the line as a nice to have. Let's get this merged in and create an asana task for the validation if that works for you as well!
There was a problem hiding this comment.
Yup totally makes sense, added an Asana
|
For posterity, the decisions are being recorded and reviewed in this doc which will turn into an RFC. |
ndhanushkodi
force-pushed
the
peering-dialer-controller
branch
from
428e78c to
3007ae6
Compare
ndhanushkodi
force-pushed
the
upstream-peer-support
branch
from
2248101 to
bdc7a04
Compare
There was a problem hiding this comment.
Great work on this @ndhanushkodi !! I have left a few comments. Feel free to merge this after you get a chance to look into those and the linting issues. It LGTM after that!! Love how this looks btw.
|
|
||
| } | ||
|
|
||
| func (r *EndpointsController) processAnnotatedUpstream(pod corev1.Pod, rawUpstream string) (api.Upstream, error) { |
There was a problem hiding this comment.
Can we copy the comment where you have described the formatting of the annotated upstream and add that above each of these method to describe the "shape" of the upstream that they will each process
| @@ -202,6 +202,164 @@ func TestProcessUpstreams(t *testing.T) { | |||
| consulNamespacesEnabled bool | |||
| consulPartitionsEnabled bool | |||
| }{ | |||
| { | |||
There was a problem hiding this comment.
Do we test the error cases here? Should we add tests to validate the behavior on a few error cases?
There was a problem hiding this comment.
Added tests for error cases, great callout!
ndhanushkodi
force-pushed
the
peering-dialer-controller
branch
from
6489f22 to
3705765
Compare
ndhanushkodi
force-pushed
the
upstream-peer-support
branch
from
bdc7a04 to
0b49984
Compare
ndhanushkodi
force-pushed
the
peering-dialer-controller
branch
from
9100d46 to
889fea7
Compare
ndhanushkodi
force-pushed
the
upstream-peer-support
branch
from
0dbc5dc to
70f5a76
Compare
ndhanushkodi
force-pushed
the
upstream-peer-support
branch
from
b2ed55d to
a8e6e19
Compare
Changes proposed in this PR:
The proxy configuration upstreams set during service registration needs to support peer names. This is for explicit upstreams configured via the connect-service-upstreams annotation, not Tproxy.
Currently, upstreams are specified in the following format:
See processUpstreams function for detail on how it’s parsed.
The cases for upstream services are:
We want to allow the currently supported upstream annotation format, for potential migration cases and to not make a breaking change. When you specify a peer for an upstream, you don’t also want to include the partition or datacenter because it’s not needed and could make the upstream ambiguous. So in addition to the currently supported formats, we can have a format where these would be valid:
[service-name].svc:[port]
[service-name].svc.[service-namespace].ns.[service-peer].peer:[port]
[service-name].svc.[service-namespace].ns.[service-partition].ap:[port]
[service-name].svc.[service-namespace].ns.[service-datacenter].dc:[port]
When namespaces/partitions are NOT enabled, these would be valid:
[service-name].svc:[port]
[service-name].svc.[service-peer].peer:[port]
[service-name].svc.[service-dc].dc:[port]
First, we split on “,”. Each upstream will be checked for if it fits the old structure, prepared query structure, or new structure. For the new structure, called the "annotated format" in this PR, once we split the upstream on “.”, if the second part is the string “svc”, we’ll know that it’s the new structure. Only one of peer, ap, or dc should be set, we can return an error otherwise.
How I've tested this PR:
unit tests
How I expect reviewers to test this PR:
👀
pipeline is red because the base branch is peering-dialer-controller whose tests are red. However this was unit tested on its own and those tests are green.
Checklist: