feat(*): ignore invalid traffic targets #4177
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4177 +/- ##
==========================================
- Coverage 69.91% 69.91% -0.01%
==========================================
Files 212 212
Lines 11615 11631 +16
==========================================
+ Hits 8121 8132 +11
- Misses 3441 3447 +6
+ Partials 53 52 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
michelleN
force-pushed
the
traffictarget
branch
from
260e220 to
e6b6321
Compare
michelleN
force-pushed
the
traffictarget
branch
3 times, most recently
from
264be65 to
b6cb7a0
Compare
michelleN
force-pushed
the
traffictarget
branch
from
b6cb7a0 to
9658161
Compare
| @@ -870,156 +870,3 @@ func TestListInboundTrafficTargetsWithRoutes(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Is it true that this test was defacto moved from pkg/catalog/traffictarget_test.go to pkg/smi/client_test.go ?
| @@ -280,6 +291,10 @@ func (c *client) ListTrafficTargets(options ...TrafficTargetListOption) []*smiAc | |||
| continue | |||
| } | |||
|
|
|||
| if !isValidTrafficTarget(trafficTarget) { | |||
| continue | |||
There was a problem hiding this comment.
Would it make sense to log.Trace().Msgf() the fact that we found an invalid traffic target?
There was a problem hiding this comment.
Should we have a prometheus counter for invalid TrafficTargets? That might be a really good counter to add.
I took the liberty to create the following issue for this #4187
pkg/smi/client.go
Outdated
| if rules == nil { | ||
| return false | ||
| } | ||
| if len(rules) == 0 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I was curious if this could be collapsed. (Not proposing we do - just curious.)
It looks like len(rules) could cover both cases: https://play.golang.org/p/LzR_HKKywFj
pkg/smi/client_test.go
Outdated
| _, err = fakeClientSet.smiTrafficTargetClientSet.AccessV1alpha3().TrafficTargets("random-namespace").Create(context.TODO(), trafficTargetWithNamespaceMismatch, metav1.CreateOptions{}) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| _, err = fakeClientSet.smiTrafficTargetClientSet.AccessV1alpha3().TrafficTargets(testNamespaceName).Create(context.TODO(), trafficTargetWithInvalidRuleKind, metav1.CreateOptions{}) |
There was a problem hiding this comment.
Am I correct that we expect this line to not create a new TrafficTarget because it is invalid. So then a few lines below when we list the traffic targets - the count will still be 1?
I wonder if a comment here would help disambiguate this.
There was a problem hiding this comment.
I updated these tests to make things clearer
pkg/smi/client_test.go
Outdated
| @@ -261,6 +346,15 @@ var _ = Describe("When listing TrafficTargets", func() { | |||
|
|
|||
| _, err := fakeClientSet.smiTrafficTargetClientSet.AccessV1alpha3().TrafficTargets(testNamespaceName).Create(context.TODO(), trafficTarget, metav1.CreateOptions{}) | |||
| Expect(err).ToNot(HaveOccurred()) | |||
|
|
|||
There was a problem hiding this comment.
@michelleN would it be possible to add new tests for these new changes instead of updating the existing ones. I plan on refactoring the existing tests to address issues in the pub-sub usage (already have locally). If it's a separate function, would be much easier for me to incorporate those changes without accidentally leaving them out as a part of the cleanup.
There was a problem hiding this comment.
sure thing. done.
+ ignores invalid traffic targets when building envoy config + provides extra guarantee that even if invalid traffic targets exist in cluster before OSM is installed, OSM will ignore them resovles openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]>
michelleN
force-pushed
the
traffictarget
branch
from
9658161 to
9982f20
Compare
| @@ -135,7 +135,7 @@ func (mc *MeshCatalog) buildHTTPRouteConfigs(egressPolicy *policyV1alpha1.Egress | |||
| var httpRouteMatches []trafficpolicy.HTTPRouteMatch | |||
| httpMatchSpecified := false | |||
| for _, match := range egressPolicy.Spec.Matches { | |||
| if match.APIGroup != nil && *match.APIGroup == smiSpecs.SchemeGroupVersion.String() && match.Kind == httpRouteGroupKind { | |||
| if match.APIGroup != nil && *match.APIGroup == smiSpecs.SchemeGroupVersion.String() && match.Kind == smi.HTTPRouteGroupKind { | |||
| @@ -91,7 +77,7 @@ func (mc *MeshCatalog) getAllowedDirectionalServiceAccounts(svcIdentity identity | |||
| for _, trafficTarget := range allTrafficTargets { | |||
| spec := trafficTarget.Spec | |||
|
|
|||
| if spec.Destination.Kind != serviceAccountKind { | |||
| if spec.Destination.Kind != smi.ServiceAccountKind { | |||
+ ignores invalid traffic targets when building envoy config + provides extra guarantee that even if invalid traffic targets exist in cluster before OSM is installed, OSM will ignore them resovles openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]> Signed-off-by: Sneha Chhabria <[email protected]>
+ ignores invalid traffic targets when building envoy config + provides extra guarantee that even if invalid traffic targets exist in cluster before OSM is installed, OSM will ignore them resovles openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]>
exist in cluster before OSM is installed, OSM will ignore them
resovles #4116
Signed-off-by: Michelle Noorali [email protected]
Description:
Testing done:
Affected area:
Please answer the following questions with yes/no.
Does this change contain code from or inspired by another project?
Is this a breaking change?