Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

traffic-split: update root service selector & targetPort usage #4902

Merged
merged 2 commits into from
Jul 15, 2022

Conversation

shashankram
Copy link
Member

@shashankram shashankram commented Jul 14, 2022

Description:
This change does the following:

  1. Fixes the incorrect legacy behavior where traffic
    directed to a root service specified in a TrafficSplit
    resource can direct traffic to pods that do not match
    the root service's selector. Not only was this behavior
    confusing, it also significantly complicated code paths
    that required special handling of this scenario that is
    unintuitive. Going forward, the root service selector
    must match pods for traffic splitting to those pods to
    function. Existing e2e tests relying on this unsupported
    behavior have been updated to correctly configure selectors
    and labels on services and pods backing them. A redundant
    test explicitly testing the only supported scenario after
    this change has been removed. The automated demo has
    also been updated to correctly configure the selector and
    labels.

  2. Fixes Specifying root service port to a port other than containerPort/TargetPort of the leaf service doesn't not work #4894, where the TargetPort on the root service was
    expected to match the ContainerPort on the pod backing
    the service. Per SMI's TrafficSplit API, the TargetPort
    on the root does not need to match the ContainerPort
    on the pod, thus allowing newer application versions
    to change the ports they listen on without needing
    to update the root service definition.

Testing done:
Unit tests, e2e tests

Affected area:

Functional Area
SMI Policy [X]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project? no

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change? yes

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)? traffic-split: update root service usage osm-docs#413

@shashankram shashankram added the wip Work-in-Progress label Jul 14, 2022
@shashankram shashankram force-pushed the fix-4894 branch 2 times, most recently from 799967d to 3353f54 Compare July 14, 2022 23:33
This change does the following:

1. Fixes the incorrect legacy behavior where traffic
   directed to a root service specified in a TrafficSplit
   resource can direct traffic to pods that do not match
   the root service's selector. Not only was this behavior
   confusing, it also significantly complicated code paths
   that required special handling of this scenario that is
   unintuitive. Going forward, the root service selector
   must match pods for traffic splitting to those pods to
   function. Existing e2e tests relying on this unsupported
   behavior have been updated to correctly configure selectors
   and labels on services and pods backing them. A redundant
   test explicitly testing the only supported scenario after
   this change has been removed. The automated demo has
   also been updated to correctly configure the selector and
   labels.

2. Fixes openservicemesh#4894, where the TargetPort on the root service was
   expected to match the ContainerPort on the pod backing
   the service. Per SMI's TrafficSplit API, the TargetPort
   on the root does not need to match the ContainerPort
   on the pod, thus allowing newer application versions
   to change the ports they listen on without needing
   to update the root service definition.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2022

Codecov Report

Merging #4902 (0dd11db) into main (d5d3a25) will decrease coverage by 0.08%.
The diff coverage is 71.69%.

@@            Coverage Diff             @@
##             main    #4902      +/-   ##
==========================================
- Coverage   68.73%   68.64%   -0.09%     
==========================================
  Files         220      220              
  Lines       15934    15938       +4     
==========================================
- Hits        10952    10941      -11     
- Misses       4930     4945      +15     
  Partials       52       52              
Flag Coverage Δ
unittests 68.64% <71.69%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/k8s/mock_controller_generated.go 12.96% <0.00%> (-1.18%) ⬇️
pkg/k8s/types.go 100.00% <ø> (ø)
pkg/catalog/outbound_traffic_policies.go 92.74% <83.33%> (-2.92%) ⬇️
pkg/k8s/client.go 94.98% <87.50%> (-0.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5d3a25...0dd11db. Read the comment docs.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
@shashankram shashankram removed the wip Work-in-Progress label Jul 15, 2022
@shashankram shashankram marked this pull request as ready for review July 15, 2022 18:00
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 15, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 15, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 15, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
Copy link
Contributor

@keithmattix keithmattix left a comment

Choose a reason for hiding this comment

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

LGTM, love to see code removed

@shashankram shashankram merged commit 5f54056 into openservicemesh:main Jul 15, 2022
@shashankram shashankram deleted the fix-4894 branch July 15, 2022 22:23
shashankram added a commit to shashankram/osm that referenced this pull request Jul 15, 2022
…ervicemesh#4902)

This change does the following:

1. Fixes the incorrect legacy behavior where traffic
   directed to a root service specified in a TrafficSplit
   resource can direct traffic to pods that do not match
   the root service's selector. Not only was this behavior
   confusing, it also significantly complicated code paths
   that required special handling of this scenario that is
   unintuitive. Going forward, the root service selector
   must match pods for traffic splitting to those pods to
   function. Existing e2e tests relying on this unsupported
   behavior have been updated to correctly configure selectors
   and labels on services and pods backing them. A redundant
   test explicitly testing the only supported scenario after
   this change has been removed. The automated demo has
   also been updated to correctly configure the selector and
   labels.

2. Fixes openservicemesh#4894, where the TargetPort on the root service was
   expected to match the ContainerPort on the pod backing
   the service. Per SMI's TrafficSplit API, the TargetPort
   on the root does not need to match the ContainerPort
   on the pod, thus allowing newer application versions
   to change the ports they listen on without needing
   to update the root service definition.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
trstringer pushed a commit that referenced this pull request Jul 18, 2022
…sage (#4902) (#4905)

* traffic-split: update root service selector & targetPort usage (#4902)

This change does the following:

1. Fixes the incorrect legacy behavior where traffic
   directed to a root service specified in a TrafficSplit
   resource can direct traffic to pods that do not match
   the root service's selector. Not only was this behavior
   confusing, it also significantly complicated code paths
   that required special handling of this scenario that is
   unintuitive. Going forward, the root service selector
   must match pods for traffic splitting to those pods to
   function. Existing e2e tests relying on this unsupported
   behavior have been updated to correctly configure selectors
   and labels on services and pods backing them. A redundant
   test explicitly testing the only supported scenario after
   this change has been removed. The automated demo has
   also been updated to correctly configure the selector and
   labels.

2. Fixes #4894, where the TargetPort on the root service was
   expected to match the ContainerPort on the pod backing
   the service. Per SMI's TrafficSplit API, the TargetPort
   on the root does not need to match the ContainerPort
   on the pod, thus allowing newer application versions
   to change the ports they listen on without needing
   to update the root service definition.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>

* release-notes: add note on traffic split behavior

Adds a note regarding the change to drop support
to split traffic from a root service to pods that
do not match the selector on the root service.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to openservicemesh/osm-docs that referenced this pull request Jul 18, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 18, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 18, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
shashankram added a commit to shashankram/osm-docs that referenced this pull request Jul 18, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
addozhang pushed a commit to flomesh-io/osm-docs that referenced this pull request Aug 3, 2022
This change updates the documentation to
reflect the change made to the traffic split
behavior in openservicemesh/osm#4902.

Signed-off-by: Shashank Ram <shashr2204@gmail.com>
Signed-off-by: Addo.Zhang <duwasai@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants