Fix service port naming convention

[lujiajing1126]

• Reproduce the issue with e2e test
• Fix unit tests
• Fix issue Bad naming convention for service name #1360

[codecov]

Codecov Report

Merging #1368 (6e7c287) into master (fb3e8eb) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1368   +/-   ##
=======================================
  Coverage   86.22%   86.22%           
=======================================
  Files          90       90           
  Lines        5146     5146           
=======================================
  Hits         4437     4437           
  Misses        539      539           
  Partials      170      170           

Impacted Files	Coverage Δ
pkg/service/collector.go	100.00% <100.00%> (ø)

---

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 fb3e8eb...6e7c287. Read the comment docs.

[lujiajing1126]

@jpkrohling I have added an e2e with istio integration which can reproduce the issue #1360

[jpkrohling]

That looks awesome! I'm not sure the CI will be able to handle that, but it would be great if this works.

[lujiajing1126]

That looks awesome! I'm not sure the CI will be able to handle that, but it would be great if this works.

Now everything is fine. Besides, I am worried about this issue istio/istio#16282.

I guess we also need an e2e case for gRPC over tls.

[jpkrohling]

@kevinearls would you like to review this one?

[kevinearls]

In general this looks pretty good. I've requested replacing some "if err" blocks with calls to stretchr require as that makes code more concise and gives better error information.

As the test method is quite long I've requested refactoring a couple of wait blocks into functions, one which can stay in this file but another which should go to utils.go as it is also used by other tests.

[lujiajing1126]

Any other comment?

[kevinearls]

@lujiajing1126 No not from me. Once you take out that error parameter I'll approve the PR.

[lujiajing1126]

@lujiajing1126 No not from me. Once you take out that error parameter I'll approve the PR.

Thanks for pointing that out, I even didn't notice that. Fixed.

[lujiajing1126]

@jpkrohling Could you pls have a final check? Or any other comment?

[jpkrohling]

If @kevinearls approved, that's good enough for me :-) Thanks for your PR!

[lujiajing1126]

If @kevinearls approved, that's good enough for me :-) Thanks for your PR!

Thanks! But it seems the merge process has to be triggered manually since github workflow files have been changed