feat(*): ensure traffic target in dest namespace #4151

michelleN · 2021-09-21T19:22:41Z

Description:
feat(*): add traffic target validation

Adds a validator for traffic target specifically
requiring that the namespace of a traffic target matches the
destination namespace.
Updates chart template for validating webhook configuration to optionally enable
validation of SMI Traffic Targets
Adds value for toggling validating SMI traffic target in values.yaml
Updates chart README
Updates e2es to create traffic targets in destination namespace
Adds e2e for SMI traffic target validation

Signed-off-by: Michelle Noorali [email protected]

Testing done:

Unit testing
Manual testing
Update e2es

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ ]
Certificate Management	[ ]
Control Plane	[ ]
Demo	[ ]
Documentation	[ ]
Egress	[ ]
Ingress	[ ]
Install	[ ]
Networking	[ ]
Observability	[ ]
Performance	[ ]
SMI Policy	[ X]
Security	[ ]
Sidecar Injection	[ ]
Tests	[ ]
Upgrade	[ ]
Other	[ ]

Please answer the following questions with yes/no.

Does this change contain code from or inspired by another project? No
- Did you notify the maintainers and provide attribution? N/A
Is this a breaking change? Yes

draychev · 2021-09-21T19:27:32Z

pkg/catalog/traffictarget.go

@@ -201,7 +201,7 @@ func (mc *MeshCatalog) getTCPRouteMatchesFromTrafficTarget(trafficTarget smiAcce

 // isValidTrafficTarget checks if the given SMI TrafficTarget object is valid
 func isValidTrafficTarget(t *smiAccess.TrafficTarget) bool {
-	return t != nil && t.Spec.Rules != nil && len(t.Spec.Rules) > 0 && hasValidRulesKind(t.Spec.Rules)
+	return t != nil && t.Spec.Rules != nil && len(t.Spec.Rules) > 0 && hasValidRulesKind(t.Spec.Rules) && t.Namespace == t.Spec.Destination.Namespace


I wonder if it'd be prudent to move the t != nil && t.Spec.Rules != nil && len(t.Spec.Rules) > 0 checks inside the hasValidRulesKind() function to lower the complexity of this line.

Another idea - it might be good to put a log statement here so when everything is good but the namespace is not the destination namespace - then we could Trace log a statement. This may help us debug a problem.

draychev · 2021-09-21T19:30:36Z

@michelleN Would it make sense to also to a check in our ValidationWebhook (and add it to a more general SMI validation) -- to make sure that the TrafficTarget namespace and destination namespaces match?

One other idea to explore -- when the TrafficTarget namespace and destination namespaces do not match -- we could update the Status field of the TrafficTarget with an error message: #3218

codecov-commenter · 2021-09-21T19:35:14Z

Codecov Report

Merging #4151 (7e642fa) into main (590f52d) will increase coverage by 0.48%.
The diff coverage is 88.88%.

@@            Coverage Diff             @@
##             main    #4151      +/-   ##
==========================================
+ Coverage   69.34%   69.83%   +0.48%     
==========================================
  Files         212      211       -1     
  Lines       11429    11472      +43     
==========================================
+ Hits         7925     8011      +86     
+ Misses       3451     3408      -43     
  Partials       53       53

Flag	Coverage Δ
unittests	`69.83% <88.88%> (+0.48%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/validator/validators.go	`91.80% <87.50%> (-0.65%)`	⬇️
pkg/validator/server.go	`76.59% <100.00%> (+19.60%)`	⬆️
pkg/certificate/rotor/rotor.go	`84.37% <0.00%> (-3.13%)`	⬇️
pkg/k8s/events/event_pubsub.go	`100.00% <0.00%> (ø)`
pkg/reconciler/mutating_webhook.go
pkg/injector/webhook.go	`93.00% <0.00%> (+0.18%)`	⬆️
pkg/providers/kube/client.go	`71.68% <0.00%> (+0.88%)`	⬆️

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 590f52d...7e642fa. Read the comment docs.

draychev

LGTM

This is going to save us a lot of headaches!

draychev · 2021-09-24T19:08:57Z

tests/e2e/e2e_smi_traffic_target_test.go

@@ -202,7 +202,7 @@ var _ = OSMDescribe("Test HTTP traffic with SMI TrafficTarget",

 // createPolicyForRoutePath creates an HTTPRouteGroup and TrafficTarget policy for the given source, destination and HTTP path regex
 func createPolicyForRoutePath(source, sourceNamespace, destination, destinationNamespace, pathRegex string) (smiSpecs.HTTPRouteGroup, smiAccess.TrafficTarget) {
-	routeGroupName := "test-route"
+	routeGroupName := source + "-" + destination


Should we use fmt.Sprintf() to follow Go best practices?

draychev · 2021-09-24T19:12:20Z

pkg/validator/validators.go

+		return nil, errors.Errorf("The traffic target namespace (%s) must match spec.Destination.Namespace (%s)",
+			trafficTarget.Namespace, trafficTarget.Spec.Destination.Namespace)


I'm not opposed returning errors.Errorf, but that would make it hard in the future should we want to differentiate between different types of errors returned by this validator.

I don't know that we have settled on a standard for this repo.
We tend to create an error and place it in the errors.go file in the package.

var ErrTrafficTargetNamespaceMismatch = errors.New("traffic target namespace mismatch")

and then here return nil, ErrTrafficTargetNamespaceMismatch
but also before the return log the full statement

log.Error().Err(ErrTrafficTargetNamespaceMismatch).Msgf("The traffic target namespace (%s) must match spec.Destination.Namespace (%s)", trafficTarget.Namespace, trafficTarget.Spec.Destination.Namespace)`

@draychev the recommended way to log errors currently is as follows:

For dynamic error messages without error codes, use errors.Errorf(), errors.New().

For static errors messages previously defined in errors.go, define it in pkg/errcode/errcode.go instead so it can be logged similar to other error codes with the ability to publish metrics per error code.

I am not opposed to defining errors in errors.go if they don't need additional data to be encoded in them, though the errcode model should be adopted to get the benefits of tooling we build to inspect error codes instead of error messages.

snehachhabria

I just merged #4160 and this is conflicting with the changes I recently checked in

charts/osm/templates/osm-validator-webhook.yaml

charts/osm/README.md

re-reviewed

pkg/validator/patch_test.go

+ Adds a validator for traffic target specifically requiring that the namespace of a traffic target matches the destination namespace. + Updates chart template for validating webhook configuration to optionally enable validation of SMI Traffic Targets + Adds value for toggling validating SMI traffic target in values.yaml + Updates chart README + Updates e2es to create traffic targets in destination namespace + Adds e2e for SMI traffic target validation + Adds validating traffic target rule for scenario when osm-controller is creating/updating validating webhook config partially resolves openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]>

Addressing PR comment openservicemesh#4151 (comment) Signed-off-by: Michelle Noorali <[email protected]>

+ adds var ErrTrafficTargetNamespaceMismatch + updates e2e line to use fmt.Sprintf Addressing PR comment openservicemesh#4151 (comment) and openservicemesh#4151 (comment) Signed-off-by: Michelle Noorali <[email protected]>

+ Adds a validator for traffic target specifically requiring that the namespace of a traffic target matches the destination namespace. + Updates chart template for validating webhook configuration to optionally enable validation of SMI Traffic Targets + Adds value for toggling validating SMI traffic target in values.yaml + Updates chart README + Updates e2es to create traffic targets in destination namespace + Adds e2e for SMI traffic target validation + Adds validating traffic target rule for scenario when osm-controller is creating/updating validating webhook config partially resolves openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]> Signed-off-by: Sneha Chhabria <[email protected]>

+ Adds a validator for traffic target specifically requiring that the namespace of a traffic target matches the destination namespace. + Updates chart template for validating webhook configuration to optionally enable validation of SMI Traffic Targets + Adds value for toggling validating SMI traffic target in values.yaml + Updates chart README + Updates e2es to create traffic targets in destination namespace + Adds e2e for SMI traffic target validation + Adds validating traffic target rule for scenario when osm-controller is creating/updating validating webhook config partially resolves openservicemesh#4116 Signed-off-by: Michelle Noorali <[email protected]>

draychev reviewed Sep 21, 2021

View reviewed changes

michelleN force-pushed the traffictarget branch from 6e4855d to c960417 Compare September 21, 2021 21:28

michelleN changed the title ~~feat(*): ensure traffic target in dest namespace~~ [WIP] feat(*): ensure traffic target in dest namespace Sep 21, 2021

michelleN added wip Work-in-Progress do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 21, 2021

michelleN force-pushed the traffictarget branch 2 times, most recently from 647b1a9 to d92a61f Compare September 22, 2021 19:28

michelleN removed wip Work-in-Progress do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Sep 22, 2021

michelleN changed the title ~~[WIP] feat(*): ensure traffic target in dest namespace~~ feat(*): ensure traffic target in dest namespace Sep 22, 2021

michelleN marked this pull request as ready for review September 22, 2021 19:31

michelleN requested a review from a team as a code owner September 22, 2021 19:31

michelleN marked this pull request as draft September 22, 2021 19:35

shashankram approved these changes Sep 22, 2021

View reviewed changes

michelleN force-pushed the traffictarget branch 2 times, most recently from 193bbd4 to 7e642fa Compare September 23, 2021 21:19

michelleN marked this pull request as ready for review September 23, 2021 21:19

shashankram approved these changes Sep 24, 2021

View reviewed changes

draychev approved these changes Sep 24, 2021

View reviewed changes

snehachhabria previously requested changes Sep 24, 2021

View reviewed changes

charts/osm/templates/osm-validator-webhook.yaml Show resolved Hide resolved

michelleN force-pushed the traffictarget branch from 7e642fa to 9b9fb53 Compare September 27, 2021 18:59

snehachhabria reviewed Sep 27, 2021

View reviewed changes

charts/osm/README.md Show resolved Hide resolved

snehachhabria self-requested a review September 27, 2021 19:41

snehachhabria suggested changes Sep 27, 2021

View reviewed changes

pkg/validator/patch_test.go Outdated Show resolved Hide resolved

michelleN force-pushed the traffictarget branch from 9b9fb53 to 45fa02f Compare September 27, 2021 20:59

snehachhabria approved these changes Sep 27, 2021

View reviewed changes

michelleN force-pushed the traffictarget branch from 45fa02f to be9808b Compare September 27, 2021 21:01

michelleN merged commit f262ef5 into openservicemesh:main Sep 27, 2021

michelleN deleted the traffictarget branch September 27, 2021 21:24

michelleN pushed a commit to michelleN/osm that referenced this pull request Sep 28, 2021

ref(e2e): update lint to use fmt.Sprintf

aac9385

Addressing PR comment openservicemesh#4151 (comment) Signed-off-by: Michelle Noorali <[email protected]>

michelleN mentioned this pull request Sep 28, 2021

ref(e2e): return err for TrafficTarget ns mismatch #4179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(*): ensure traffic target in dest namespace #4151

feat(*): ensure traffic target in dest namespace #4151

michelleN commented Sep 21, 2021 •

edited

Loading

draychev Sep 21, 2021

draychev Sep 21, 2021

draychev commented Sep 21, 2021

codecov-commenter commented Sep 21, 2021 •

edited

Loading

draychev left a comment

draychev Sep 24, 2021

draychev Sep 24, 2021

shashankram Sep 24, 2021

snehachhabria left a comment •

edited

Loading

		return nil, errors.Errorf("The traffic target namespace (%s) must match spec.Destination.Namespace (%s)",
		trafficTarget.Namespace, trafficTarget.Spec.Destination.Namespace)

feat(*): ensure traffic target in dest namespace #4151

feat(*): ensure traffic target in dest namespace #4151

Conversation

michelleN commented Sep 21, 2021 • edited Loading

draychev Sep 21, 2021

Choose a reason for hiding this comment

draychev Sep 21, 2021

Choose a reason for hiding this comment

draychev commented Sep 21, 2021

codecov-commenter commented Sep 21, 2021 • edited Loading

Codecov Report

draychev left a comment

Choose a reason for hiding this comment

draychev Sep 24, 2021

Choose a reason for hiding this comment

draychev Sep 24, 2021

Choose a reason for hiding this comment

shashankram Sep 24, 2021

Choose a reason for hiding this comment

snehachhabria left a comment • edited Loading

Choose a reason for hiding this comment

michelleN commented Sep 21, 2021 •

edited

Loading

codecov-commenter commented Sep 21, 2021 •

edited

Loading

snehachhabria left a comment •

edited

Loading