add gRPC status in fault abort #26059

ckcd · 2020-08-03T06:30:26Z

Implement of grpc status in fault abort.

e.g.

apiVersion: networking.istio.io/v1beta1
kind: VirtualService
...
spec:
  hosts:
  - ratings
  http:
  - fault:
      abort:
        grpcStatus: DEADLINE_EXCEEDED
        percentage:
          value: 100

The related api is in istio/api:virtual_service.proto#L1816 and for now it's hide_from_docs.

And we discuss about whether the type of GrpcStatus should be string or uint32 in #24698. cc @howardjohn

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

istio-testing · 2020-08-03T06:30:41Z

Hi @ckcd. 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.

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.

hzxuzhonghu

lgtm

ramaraochavali · 2020-08-03T09:53:16Z

/ok-to-test

ramaraochavali · 2020-08-03T10:44:01Z

pkg/config/validation/validation.go

@@ -95,6 +97,27 @@ var (
 		http.MethodTrace:   true,
 	}

+	// golang supported gRPC status: https://github.com/grpc/grpc-go/blob/master/codes/codes.go
+	supportedGRPCStatus = map[string]codes.Code{


Can we remove this duplication and have these code mapping defined at a common place?

any suggestion? cause I cannot find a common place or any types dir. And the route.go(pilot/pkg/...) and validate.go(pkg/...) are in very different path.

somewhere in 'pkg/util/grpc.go` ?

Cool, I will do this change tomorrow, thanks~

Done, please take a look.

howardjohn

LGTM but we need to decide if we want string or int. Once we implement it we cannot change

ckcd · 2020-08-04T02:29:16Z

/label release-notes-none

ckcd · 2020-08-04T02:34:32Z

we need to decide if we want string or int. Once we implement it we cannot change

yes it's hard to choose. But I think string is more intuitive and easy-to-use than int though we need do some format conversion.

ckcd · 2020-08-04T02:35:27Z

BTW seems we need a release-notes-none label.

@howardjohn @ramaraochavali PTAL.

ckcd · 2020-08-24T07:14:52Z

@howardjohn @ramaraochavali

PTAL, thanks~

ckcd · 2020-08-24T07:19:58Z

/test integ-pilot-k8s-tests_istio

ramaraochavali

Thank you

ramaraochavali · 2020-08-24T07:43:52Z

/test release-notes_istio

shamsher31 · 2020-09-24T05:55:33Z

pilot/pkg/networking/core/v1alpha3/route/route.go

@@ -921,8 +922,13 @@ func translateFault(in *networking.HTTPFaultInjection) *xdshttpfault.HTTPFault {
 			out.Abort.ErrorType = &xdshttpfault.FaultAbort_HttpStatus{
 				HttpStatus: uint32(a.HttpStatus),
 			}
+		case *networking.HTTPFaultInjection_Abort_GrpcStatus:
+			code := grpcutil.SupportedGRPCStatus[a.GrpcStatus]


@ckcd What if an invalid gRPC status code is passed? Is the status code validated somewhere before the lookup?

Yes it's in pkg/config/validation/validation.go in this PR.

ckcd · 2020-09-25T09:53:55Z

/test integ-pilot-multicluster-tests_istio

ckcd · 2020-09-25T10:04:27Z

@howardjohn @rshriram @linsun @costinm
please take a look, thanks.

howardjohn · 2020-09-28T20:05:58Z

pkg/config/validation/validation.go

@@ -1994,6 +1994,13 @@ func validateHTTPStatus(status int32) error {
 	return nil
 }

+func validateGPRCStatus(status string) error {
+	if _, ok := grpcutil.SupportedGRPCStatus[status]; !ok {


I suspect a user may get confused by this. Can we expand this to say something like:

gRPC status "foo" is not supported. See https://github.com/grpc/grpc/blob/master/doc/statuscodes.md for a list of supported codes, for example "NOT_FOUND"`

This makes it clear that not_found or 5 are not valid.

Can you add this?

istio-testing · 2020-12-05T20:22:18Z

@ckcd: PR needs rebase.

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.

110y · 2021-01-12T02:30:45Z

@ckcd @howardjohn

I'd like to use the fault injection for gRPC, so what's the current status of this PR?

istio-testing · 2022-01-20T22:28:25Z

@ckcd: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
integ-cni-k8s-tests_istio	`06bf31e`	link	`/test integ-cni-k8s-tests_istio`
integ-istiodremote-k8s-tests_istio	`06bf31e`	link	`/test integ-istiodremote-k8s-tests_istio`
integ-pilot-istiodremote-tests_istio	`06bf31e`	link	`/test integ-pilot-istiodremote-tests_istio`
integ-telemetry-istiodremote-tests_istio	`06bf31e`	link	true	`/test integ-telemetry-istiodremote-tests_istio`
integ-pilot-istiodremote-multicluster-tests_istio	`06bf31e`	link	true	`/test integ-pilot-istiodremote-multicluster-tests_istio`
integ-security-istiodremote-tests_istio	`06bf31e`	link	true	`/test integ-security-istiodremote-tests_istio`
integ-cni_istio	`06bf31e`	link	true	`/test integ-cni_istio`
integ-telemetry_istio	`06bf31e`	link	true	`/test integ-telemetry_istio`
integ-distroless_istio	`06bf31e`	link	true	`/test integ-distroless_istio`
integ-operator-controller_istio	`06bf31e`	link	true	`/test integ-operator-controller_istio`
integ-helm_istio	`06bf31e`	link	true	`/test integ-helm_istio`
integ-pilot_istio	`06bf31e`	link	true	`/test integ-pilot_istio`
integ-security_istio	`06bf31e`	link	true	`/test integ-security_istio`

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. I understand the commands that are listed here.

howardjohn · 2022-05-31T22:16:35Z

Would be great to revive this (and add integ test at

istio/tests/integration/pilot/common/routing.go

Line 646 in 34d8837

abort:

)

Signed-off-by: su225 <suchithjn22@gmail.com>

…ault injection) (#39295) * resurrect PR - #26059 Signed-off-by: su225 <suchithjn22@gmail.com> * add gRPC status checker Signed-off-by: su225 <suchithjn22@gmail.com> * add gRPC abort integration test Signed-off-by: su225 <suchithjn22@gmail.com> * add release notes Signed-off-by: su225 <suchithjn22@gmail.com> * make linter happy * fix release notes Signed-off-by: su225 <suchithjn22@gmail.com>

howardjohn · 2022-07-01T23:22:58Z

Merged in #39295. Thanks!

ckcd requested review from a team as code owners August 3, 2020 06:30

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 3, 2020

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 3, 2020

istio-testing added the needs-ok-to-test label Aug 3, 2020

hzxuzhonghu reviewed Aug 3, 2020

View reviewed changes

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 Aug 3, 2020

ramaraochavali reviewed Aug 3, 2020

View reviewed changes

ckcd force-pushed the grpc_status branch from e7fb10c to 006df70 Compare August 3, 2020 12:16

howardjohn reviewed Aug 3, 2020

View reviewed changes

ckcd force-pushed the grpc_status branch 2 times, most recently from 61a116a to 23c1cec Compare August 4, 2020 01:57

ckcd requested review from costinm, linsun and rshriram as code owners August 4, 2020 01:57

ckcd force-pushed the grpc_status branch 3 times, most recently from 4af88d6 to 297ef8e Compare August 4, 2020 02:12

ckcd force-pushed the grpc_status branch from 297ef8e to ab98683 Compare August 4, 2020 03:33

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

add gRPC status in fault abort

06bf31e

ckcd force-pushed the grpc_status branch from ab98683 to 06bf31e Compare August 24, 2020 07:06

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

ramaraochavali added the release-notes-none Indicates a PR that does not require release notes. label Aug 24, 2020

ramaraochavali approved these changes Aug 24, 2020

View reviewed changes

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 23, 2020

shamsher31 added lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed and removed lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while labels Sep 23, 2020

shamsher31 reviewed Sep 24, 2020

View reviewed changes

shamsher31 mentioned this pull request Sep 24, 2020

Support gRPC status in fault abort istio/api#1653

Closed

hzxuzhonghu approved these changes Sep 28, 2020

View reviewed changes

howardjohn reviewed Sep 28, 2020

View reviewed changes

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

su225 added a commit to su225/istio that referenced this pull request Jun 6, 2022

resurrect PR - istio#26059

242759a

Signed-off-by: su225 <suchithjn22@gmail.com>

su225 mentioned this pull request Jun 6, 2022

Support gRPC fault injection (allow specifying gRPC error codes for fault injection) #39295

Merged

1 task

howardjohn closed this Jul 1, 2022

add gRPC status in fault abort #26059

add gRPC status in fault abort #26059

Uh oh!

Conversation

ckcd commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Aug 3, 2020

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Aug 3, 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!

howardjohn left a comment

Choose a reason for hiding this comment

Uh oh!

ckcd commented Aug 4, 2020

Uh oh!

ckcd commented Aug 4, 2020

Uh oh!

ckcd commented Aug 4, 2020

Uh oh!

ckcd commented Aug 24, 2020

Uh oh!

ckcd commented Aug 24, 2020

Uh oh!

ramaraochavali left a comment

Choose a reason for hiding this comment

Uh oh!

ramaraochavali commented Aug 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckcd commented Sep 25, 2020

Uh oh!

ckcd commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

istio-testing commented Dec 5, 2020

Uh oh!

110y commented Jan 12, 2021

Uh oh!

istio-testing commented Jan 20, 2022

Uh oh!

howardjohn commented May 31, 2022

Uh oh!

howardjohn commented Jul 1, 2022

Uh oh!

Uh oh!

ckcd commented Aug 3, 2020 •

edited

Loading