Skip to content

Conversation

su225
Copy link
Contributor

@su225 su225 commented Jun 6, 2022

Please provide a description of this PR:
Original PR - #26059
This PR resurrects it and only adds integration test to complete it.

Brief description: This PR adds support to injecting gRPC faults with given status codes. It builds on the original PR which seemed to be abandoned. The actual changes like validation and config-generation were already there in the original PR (full credits to the person who raised it) and was working fine. The only thing I added is the integration test as suggested by @howardjohn in #26059 (comment) .

Once this goes through we have to remove $hide_from_docs for the grpcStatus field and update the documentation to indicate that users have to specify upper-cased string like UNAVAILABLE as mentioned in https://github.com/grpc/grpc/blob/master/doc/statuscodes.md

Open question

  • Should we allow users to inject faults with OK? This is because it is technically not a fault. We can allow it to be consistent with HTTP, where specifying 200 OK is allowed.

su225 added 4 commits June 5, 2022 19:31
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
Signed-off-by: su225 <suchithjn22@gmail.com>
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 6, 2022
@su225 su225 changed the title Grpc status abort Support gRPC fault injection (allow specifying gRPC error codes for fault injection) Jun 6, 2022
su225 added 2 commits June 6, 2022 11:11
Signed-off-by: su225 <suchithjn22@gmail.com>
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

@@ -657,6 +658,36 @@ spec:
workloadAgnostic: true,
})

t.RunTraffic(TrafficTestCase{
name: "fault abort gRPC",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should try setting sourcematchers and targetMatchers to match.NotNaked, match.NotHeadless.

Currently we have proxyless grpc opted out; this seems like a REALLY useful feature for proxyless grpc.

I don't know if itworks though, so not certain that will pass

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it.

		sourceMatchers:   []match.Matcher{match.NotNaked, match.NotHeadless},
		targetMatchers:   []match.Matcher{match.NotNaked, match.NotHeadless},

And it is 🟢
image

Let's enable it?

@istio-testing istio-testing merged commit e8b889b into istio:master Jun 6, 2022
hemendrateli added a commit to hemendrateli/istio that referenced this pull request Sep 9, 2022
…evisions

We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
	a. gRPC fault
           injection(istio#39295)
	b. Ignore port number in domain
           matching(istio#40475)
	c. Tunneling outbound traffic :- new tunnel field got
           introduced(istio#37968)
	d. Fix consistent hash based on source IP for TCP
           proxy(istio#38438)
	e. Traffic policy load balancer API
           changes(istio#39742)
istio-testing pushed a commit that referenced this pull request Sep 12, 2022
…evisions (#40892)

We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
	a. gRPC fault
           injection(#39295)
	b. Ignore port number in domain
           matching(#40475)
	c. Tunneling outbound traffic :- new tunnel field got
           introduced(#37968)
	d. Fix consistent hash based on source IP for TCP
           proxy(#38438)
	e. Traffic policy load balancer API
           changes(#39742)
istio-testing pushed a commit to istio-testing/istio that referenced this pull request Sep 13, 2022
…evisions

We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
	a. gRPC fault
           injection(istio#39295)
	b. Ignore port number in domain
           matching(istio#40475)
	c. Tunneling outbound traffic :- new tunnel field got
           introduced(istio#37968)
	d. Fix consistent hash based on source IP for TCP
           proxy(istio#38438)
	e. Traffic policy load balancer API
           changes(istio#39742)
istio-testing added a commit that referenced this pull request Sep 13, 2022
…evisions (#40957)

We are adding min istio version for tests related to below PRs as this functionalities were not there in previous revisions :-
	a. gRPC fault
           injection(#39295)
	b. Ignore port number in domain
           matching(#40475)
	c. Tunneling outbound traffic :- new tunnel field got
           introduced(#37968)
	d. Fix consistent hash based on source IP for TCP
           proxy(#38438)
	e. Traffic policy load balancer API
           changes(#39742)

Co-authored-by: Hemendra Teli <hemendrat@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants