feat: Added the optional appProtocol field to Canary.Service

[philnichol]

Closes #1177

Hi, and thanks in advance for your review, and apologies for the questions!

This does work locally, it propagates the appProtocol field to the services if set, and works as expected without.

Raised as a draft since I had a couple of questions:

• Should we support the appProtocol field on multiple ports, or just the first one? Are the subsequent ports irrelevant for flagger? Subsequent ports are just a map[string]int32, so would need some refactoring to support this additional field.
• Wasn't sure how best to test this and was hoping for a bit of guidance, am very new to this project. Just add it to the kubernetes_default_test file? Anywhere else? And should this have it's own test case? Or would we just be happy to ensure it works if set by adding it to the existing Finalize block?

[aryan9600]

Hi @philnichol, thanks for this PR! To answer your questions:

• No, we can't set appProtocol on multiple ports. Those ports are extracted from the Deployment spec, when port discovery is enabled, so it's not possible to figure out the appProtocol for those ports.
• Yep, adding it to kubernetes_default_test should be fine. You'd need to modify the fixtures to return a Canary with the appProtocol field specified and then check in the tests, whether the created services have the field specified.

[philnichol]

Hi @aryan9600 , thanks for the feedback! I've updated the fixtures and tests, please let me know if I've missed anything.

[codecov-commenter]

Codecov Report

Merging #1185 (d798988) into main (a1e519b) will decrease coverage by 1.81%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
- Coverage   56.78%   54.96%   -1.82%     
==========================================
  Files          79       79              
  Lines        6622     6642      +20     
==========================================
- Hits         3760     3651     -109     
- Misses       2304     2418     +114     
- Partials      558      573      +15     

Impacted Files	Coverage Δ
pkg/router/kubernetes_default.go	63.97% <100.00%> (+0.53%)	⬆️
pkg/router/smi_v1alpha2.go	57.30% <0.00%> (-7.87%)	⬇️
pkg/router/smi_v1alpha3.go	57.30% <0.00%> (-7.87%)	⬇️
pkg/router/traefik.go	68.86% <0.00%> (-6.61%)	⬇️
pkg/router/gloo.go	69.72% <0.00%> (-6.08%)	⬇️
pkg/router/ingress.go	67.76% <0.00%> (-5.79%)	⬇️
pkg/router/appmesh_v1beta2.go	82.70% <0.00%> (-5.68%)	⬇️
pkg/router/smi.go	41.60% <0.00%> (-5.61%)	⬇️
pkg/router/skipper.go	54.03% <0.00%> (-5.32%)	⬇️
pkg/router/istio.go	76.07% <0.00%> (-5.00%)	⬇️
... and 8 more

---

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 a1e519b...d798988. Read the comment docs.

[aryan9600]

This LGTM, we just need the docs here to reflect this change.

[philnichol]

@aryan9600 have added that too, apologies for oversight.

[aryan9600]

Thanks for the patience while addressing all the comments, this LGTM now! 🚀

[philnichol]

Thank you for the patience in reviewing it!