Add analyzer for empty protocol and addresses in Service Entry by zufardhiyaulhaq · Pull Request #28664 · istio/istio

zufardhiyaulhaq · 2020-11-06T19:03:31Z

Issue related: #27990

Add warning analyzer for empty Protocol and Addresses in ServiceEntry object. Since if both empty, probably will cause a mesh wide error.

result:

istio $ ./out/linux_amd64/istioctl analyze --use-kube=false galley/pkg/config/analysis/analyzers/testdata/serviceentry-empty-protocol-addresses.yaml -n default    master
Warning [IST0133] (ServiceEntry service-entry-test-03.default galley/pkg/config/analysis/analyzers/testdata/serviceentry-empty-protocol-addresses.yaml:49) one or more Protocol empty in ServiceEntry default/service-entry-test-03 with Addresses Empty.
Warning [IST0133] (ServiceEntry service-entry-test-04.default galley/pkg/config/analysis/analyzers/testdata/serviceentry-empty-protocol-addresses.yaml:65) one or more Protocol empty in ServiceEntry default/service-entry-test-04 with Addresses Empty.

[x] Configuration Infrastructure
[ ] Docs
[ ] Installation
[x] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

Pull Request Attributes
Please check any characteristics that apply to this pull request.
[x] Does not have any changes that may affect Istio users.

istio-testing · 2020-11-06T19:03:47Z

Hi @zufardhiyaulhaq. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

shamsher31 · 2020-11-09T07:20:01Z

/ok-to-test

shamsher31 · 2020-11-09T07:40:17Z

galley/pkg/config/analysis/analyzers/serviceentry/serviceentry.go

What if Addresses is set but the Ports[*].Protocol is empty?

if Addresses is set with Ports[*].Protocol is empty. It will not cause a cluster wide problem #27990 since we already have Addresses that match the traffic. CMIIW

shamsher31 · 2020-11-09T07:42:53Z

galley/pkg/config/analysis/msg/messages.yaml

Suggested change

description: "Addresses and Protocol is Empty in ServiceEntry."

description: "Addresses or Protocol is empty in ServiceEntry."

I prefer with Addresses and Protocol is empty in ServiceEntry. since this analyzer match if Addresses and Protocol are empty.

I think Addresses is optional but the user must set Protocol?

miss documentation probably, please check @howardjohn comment #27990 (comment)

this is fixed

zufardhiyaulhaq · 2020-11-09T07:46:07Z

I get this lint error, should I change the struct to Analyzer rather than ServiceEntryAnalyzer?

galley/pkg/config/analysis/analyzers/serviceentry/serviceentry.go:29:6: type name will be used as serviceentry.ServiceEntryAnalyzer by other packages, and that stutters; consider calling this Analyzer (golint)

galley/pkg/config/analysis/msg/messages.yaml

howardjohn · 2020-11-10T18:26:32Z

This should also be for protocol: TCP now that i think about it, not just unset

zufardhiyaulhaq · 2020-11-11T13:39:20Z

This should also be for protocol: TCP now that i think about it, not just unset

can you explain the behaviour when we using TCP port in serviceentry?

howardjohn · 2020-11-11T15:29:25Z

can you explain the behaviour when we using TCP port in serviceentry?

Yeah, basically the empty protocol means we set up listeners for TCP AND HTTP. The "tcp" part is what causes this issue of binding to all traffic on the port. So just TCP also has the same problem

zufardhiyaulhaq · 2020-11-11T16:29:35Z

can you explain the behaviour when we using TCP port in serviceentry?

Yeah, basically the empty protocol means we set up listeners for TCP AND HTTP. The "tcp" part is what causes this issue of binding to all traffic on the port. So just TCP also has the same problem

this only applies to ServiceEntry with empty addresses right (Protocol TCP)? since if we define the addresses, the listener only match the specific addresses. So we don't need to make analyzer for that cases.

zufardhiyaulhaq · 2020-11-12T08:57:06Z

@howardjohn should we create another code like IST0134 ServiceEntryProtocolTCPAndAddressesEmpty for tcp port? wdyt?

howardjohn · 2020-11-12T15:46:39Z

I would use a single code to apply to anything that can cause this symptom. IMO, code should be based on the user impact (capture all traffic on port),not the cause (missing protocol vs tcp)

…sses

zufardhiyaulhaq · 2020-11-16T07:58:54Z

@howardjohn adding TCP check also. I am dividing into other code since I am following the current code that based on the cause

Warning [IST0135] (ServiceEntry service-entry-test-03.default galley/pkg/config/analysis/analyzers/testdata/serviceentry-missing-addresses-protocol-tcp.yaml:49) missing Addresses with Protocol TCP, which can lead to undefined behavior.

Warning [IST0134] (ServiceEntry service-entry-test-03.default galley/pkg/config/analysis/analyzers/testdata/serviceentry-missing-addresses-protocol.yaml:49) missing Addresses and Protocol, which can lead to undefined behavior.
Warning [IST0134] (ServiceEntry service-entry-test-04.default galley/pkg/config/analysis/analyzers/testdata/serviceentry-missing-addresses-protocol.yaml:65) missing Addresses and Protocol, which can lead to undefined behavior.

zufardhiyaulhaq · 2020-11-20T16:40:00Z

Any update related to this folks?

howardjohn

LGTM

howardjohn · 2020-11-20T18:05:00Z

galley/pkg/config/analysis/msg/messages.yaml

      - name: err
        type: error

+  - name: "ServiceEntryMissingAddressesAndProtocol"


nit: Not sure we need two different messages for these - they are essentially identical

shamsher31 · 2020-11-23T04:39:24Z

galley/pkg/config/analysis/analyzers/serviceentry/protocoladresses.go

+
+	if se.Addresses == nil {
+		for index, port := range se.Ports {
+			if port.Protocol == "TCP" {


I think you can combine this condition with the above function. So there will be 2 checks.
one with port.Protocol == "" and other with port.Protocol == "TCP" in one function?

actually yes we can do that. do you have suggestions for the message in galley/pkg/config/analysis/msg/messages.yaml? actually, I am confused about how to create the message

cc @howardjohn

@zufardhiyaulhaq To create a new message, edit messages.yaml. Then run go generate in the messages directory after setting $REPO_ROOT. I do this:

export REPO_ROOT=~/go/src/istio.io/istio cd ~/go/src/istio.io/istio/galley/pkg/config/analysis/msg go generate

@esnible pardon me, not creating a new message. I mean the message format. currently, I am using two analyzer code (IST0134 and IST0135) to separate

empty Addresses and empty Protocol

empty Addresses with Protocol TCP

@shamsher31 this is done

shamsher31

Overall LGTM. I will let @esnible provide feedback on multiple messages.

zufardhiyaulhaq · 2020-11-30T08:36:12Z

can you review this PR @esnible? thanks

esnible · 2020-11-30T13:22:19Z

Would it make sense to have a single warning?

- name: "AddressesRequired"
    code: IST0134
    level: Warning
    description: “Virtual IP addresses are required for ports serving TCP (or unset) protocol”.
    template: "ServiceEntry addresses are required for this protocol.”

zufardhiyaulhaq · 2020-11-30T15:24:15Z

@esnible yes but we can still apply it directly. The behaviour with missing protocol is explained by @howardjohn in this thread #27990 (comment)

I can refactor to match only protocol TCP without addresses. but I think we must agree first probably to move to another issue for empty protocol.

esnible · 2020-11-30T17:29:13Z

@esnible yes but we can still apply it directly. The behaviour with missing protocol is explained by @howardjohn in this thread #27990 (comment)

I can refactor to match only protocol TCP without addresses. but I think we must agree first probably to move to another issue for empty protocol.

It is fine to keep this PR, with the check for empty Protocol, because it works. Combine the messages.

zufardhiyaulhaq · 2020-12-02T08:33:09Z

/retest

zufardhiyaulhaq · 2020-12-02T09:00:32Z

/retest

zufardhiyaulhaq · 2020-12-02T09:49:38Z

@esnible yes but we can still apply it directly. The behaviour with missing protocol is explained by @howardjohn in this thread #27990 (comment)
I can refactor to match only protocol TCP without addresses. but I think we must agree first probably to move to another issue for empty protocol.

It is fine to keep this PR, with the check for empty Protocol, because it works. Combine the messages.

this is done @esnible

thehackercat · 2022-06-16T09:56:28Z

should we add this rule to validationWebhook?

zufardhiyaulhaq requested a review from a team as a code owner November 6, 2020 19:03

istio-policy-bot added area/networking release-notes-none Indicates a PR that does not require release notes. labels Nov 6, 2020

google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Nov 6, 2020

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Nov 6, 2020

zufardhiyaulhaq mentioned this pull request Nov 6, 2020

ServiceEntry without protocol for https port removes PassthroughCluster routing #27990

Closed

istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 9, 2020

shamsher31 reviewed Nov 9, 2020

View reviewed changes

shamsher31 requested a review from a team November 9, 2020 07:56

esnible suggested changes Nov 9, 2020

View reviewed changes

galley/pkg/config/analysis/msg/messages.yaml Outdated Show resolved Hide resolved

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 15, 2020

zufardhiyaulhaq added 3 commits November 16, 2020 08:44

Init service entry analyzer, add analyze for empty protocol and addre…

9069b70

…sses

Refactor Service Entry Analyzer to pass linting

b7ab6d7

Refactor Service Entry Analyzer, add TCP protocol analyzer

4b1f269

zufardhiyaulhaq force-pushed the master branch from 4e09478 to 4b1f269 Compare November 16, 2020 07:57

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 16, 2020

fix messages.gen.go

b38a005

zufardhiyaulhaq requested review from esnible, howardjohn and shamsher31 and removed request for a team November 16, 2020 10:55

howardjohn reviewed Nov 20, 2020

View reviewed changes

shamsher31 reviewed Nov 23, 2020

View reviewed changes

refacor analyzeProtocolAddresses function

4e979e1

shamsher31 reviewed Nov 25, 2020

View reviewed changes

refactor to single message

8e20d2f

esnible approved these changes Dec 2, 2020

View reviewed changes

istio-testing merged commit 99aeb60 into istio:master Dec 2, 2020

zufardhiyaulhaq mentioned this pull request Dec 3, 2020

apply for member: zufardhiyaulhaq istio/community#503

Merged

5 tasks

shamsher31 mentioned this pull request Feb 17, 2021

IST0134 ServiceEntryAddressesRequired missing documentation #30872

Closed

zufardhiyaulhaq mentioned this pull request Feb 20, 2021

Add documentation for Analysis messsage IST0134 ServiceEntryAddresses… istio/istio.io#9020

Merged

incfly mentioned this pull request Oct 13, 2021

Bump up istio/istio to f527312d926e2a36b94fd1bb4bf72ef27ffda781. tetratelabs/istio#506

Merged

thehackercat mentioned this pull request Jun 20, 2022

Add protocol check for empty address in Service Entry #39495

Merged

	description: "Addresses and Protocol is Empty in ServiceEntry."
	description: "Addresses or Protocol is empty in ServiceEntry."

Conversation

zufardhiyaulhaq commented Nov 6, 2020

Uh oh!

istio-testing commented Nov 6, 2020

Uh oh!

shamsher31 commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zufardhiyaulhaq commented Nov 9, 2020

Uh oh!

Uh oh!

howardjohn commented Nov 10, 2020

Uh oh!

zufardhiyaulhaq commented Nov 11, 2020

Uh oh!

howardjohn commented Nov 11, 2020

Uh oh!

zufardhiyaulhaq commented Nov 11, 2020

Uh oh!

zufardhiyaulhaq commented Nov 12, 2020

Uh oh!

howardjohn commented Nov 12, 2020

Uh oh!

zufardhiyaulhaq commented Nov 16, 2020

Uh oh!

zufardhiyaulhaq commented Nov 20, 2020

Uh oh!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamsher31 Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zufardhiyaulhaq Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamsher31 left a comment

Choose a reason for hiding this comment

Uh oh!

zufardhiyaulhaq commented Nov 30, 2020

Uh oh!

esnible commented Nov 30, 2020

Uh oh!

zufardhiyaulhaq commented Nov 30, 2020

Uh oh!

esnible commented Nov 30, 2020

Uh oh!

zufardhiyaulhaq commented Dec 2, 2020

Uh oh!

zufardhiyaulhaq commented Dec 2, 2020

Uh oh!

zufardhiyaulhaq commented Dec 2, 2020

Uh oh!

thehackercat commented Jun 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

shamsher31 Nov 23, 2020 •

edited

Loading

zufardhiyaulhaq Nov 23, 2020 •

edited

Loading