do not apply rate limits for sds server #41213

ramaraochavali · 2022-09-29T09:42:16Z

When DiscoveryServer is instantiated as a SDS Service in agent, it applies default rate limit of 1 which prints log lines like this in agent

2022-09-29T03:33:23.234076Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234092Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234052Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234060Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234010Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234077Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234125Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234132Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234033Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234041Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234067Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234010Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234096Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234179Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234183Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline
2022-09-29T03:33:23.234042Z	warn	ads	ADS: "@" exceeded rate limit: rate: Wait(n=1) would exceed context deadline

Also the log lines like

2022-09-29T03:33:23.515201Z	info	ads	ADS: new connection for node:istio-ingressgateway-processing-us-west-2c-6c74f945bc-6cpvz.sfproxy-13

for every secret request is confusing.

This PR fixes both

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

istio-policy-bot · 2022-09-29T09:42:18Z

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

ramaraochavali · 2022-09-29T09:45:13Z

envoyproxy/envoy#21600 - This might improve things but still rate limiting is not needed for sds

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu · 2022-09-29T10:27:31Z

pilot/pkg/xds/discovery.go

@@ -154,6 +154,9 @@ type DiscoveryServer struct {
 	// ClusterAliases are aliase names for cluster. When a proxy connects with a cluster ID
 	// and if it has a different alias we should use that a cluster ID for proxy.
 	ClusterAliases map[cluster.ID]cluster.ID
+
+	// Indicates this is running as an Sds Server.
+	SdsServer bool


i donot think this is a good mark. I am thinking not add any filed here

Any reason why you think we should add any fields here?

Do you have any alternatives to identify otherwise? we can drive this by req.typeURL if you think that is better - I can change.

Sorry we can not do by typeURL at the place where we apply rate limit (new stream init)

+1, can we just make the rate limit configurable and make it MaxInt for SDS?

configurable you mean in DiscoveryServer? I can make that change but some log lines are also confusing that for each SDS stream - connection id keep incrementing. Should we leave them as is?

new connection for node:istio-ingressgateway-processing-us-west-2c-6c74f945bc-6cpvz.sfproxy**-13**

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-09-30T09:49:14Z

@howardjohn made changes. PTAL

howardjohn

rate limit part lgtm, not sure about the logs

howardjohn · 2022-09-30T13:44:18Z

pilot/pkg/xds/ads.go

@@ -147,11 +147,19 @@ func (s *DiscoveryServer) receive(con *Connection, identities []string) {
 		req, err := con.stream.Recv()
 		if err != nil {
 			if istiogrpc.IsExpectedGRPCError(err) {
-				log.Infof("ADS: %q %s terminated", con.peerAddr, con.conID)
+				if req != nil && req.TypeUrl == v3.SecretType {
+					log.Infof("ADS: connection terminated for secrets:%v", strings.Join(req.ResourceNames, ","))


Secret is used even for Istiod, and may have 10000s of secrets, not sure we should log this

howardjohn · 2022-09-30T13:45:02Z

security/pkg/nodeagent/sds/sdsservice.go

@@ -58,6 +60,8 @@ var _ model.XdsResourceGenerator = &sdsservice{}

 func NewXdsServer(stop chan struct{}, gen model.XdsResourceGenerator) *xds.DiscoveryServer {
 	s := xds.NewXDS(stop)
+	// No ratelimit for SDS calls in agent.
+	s.DiscoveryServer.RequestRateLimit = rate.NewLimiter(rate.Limit(math.MaxInt), 1)


nit: rate.Inf exists, but its basically the same

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-09-30T14:16:04Z

@howardjohn fixed logs

ramaraochavali · 2022-10-09T06:45:12Z

@howardjohn can you PTAL when you get chance?

hzxuzhonghu · 2022-10-09T07:26:30Z

security/pkg/nodeagent/sds/sdsservice.go

@@ -58,6 +59,8 @@ var _ model.XdsResourceGenerator = &sdsservice{}

 func NewXdsServer(stop chan struct{}, gen model.XdsResourceGenerator) *xds.DiscoveryServer {
 	s := xds.NewXDS(stop)
+	// No ratelimit for SDS calls in agent.
+	s.DiscoveryServer.RequestRateLimit = rate.NewLimiter(rate.Inf, 1)


how about not setting limit at all

That would be good. I see we already handle zero limit

istio/pilot/pkg/xds/discovery.go

Line 662 in 247e396

if s.requestRateLimit.Limit() == 0 {

hzxuzhonghu · 2022-10-09T07:27:24Z

pilot/pkg/xds/ads.go

@@ -147,11 +147,15 @@ func (s *DiscoveryServer) receive(con *Connection, identities []string) {
 		req, err := con.stream.Recv()
 		if err != nil {
 			if istiogrpc.IsExpectedGRPCError(err) {
-				log.Infof("ADS: %q %s terminated", con.peerAddr, con.conID)
+				if req != nil && req.TypeUrl != v3.SecretType {


Why do you check not secret type?

Because for secret types we do not want to log this messages as it can be 1000s as it logs for every secret.

np, without ratelimit, will error happen

This is actually important for new connection with out this check

2022-09-29T03:33:23.515201Z info ads ADS: new connection for node:istio-ingressgateway-processing-us-west-2c-6c74f945bc-6cpvz.sfproxy-13

lots of these mis leading logs with come. So I protected at both places

This can happen only at L180, how about changing to debug

I think that log at info level is useful

Seems like we are hiding a useful error message. This seems to be because we are mixing "SecretType" to mean "istio-agent SDS server", when its not the case...

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn

I don't feel comfortable hiding all the secret errors. IMO if we want to modify logging (which is a big if, it seems over complex), it should be done as DiscoveryServer params, not based on Type

howardjohn · 2022-10-10T16:09:51Z

pilot/pkg/xds/ads.go

@@ -147,11 +147,15 @@ func (s *DiscoveryServer) receive(con *Connection, identities []string) {
 		req, err := con.stream.Recv()
 		if err != nil {
 			if istiogrpc.IsExpectedGRPCError(err) {
-				log.Infof("ADS: %q %s terminated", con.peerAddr, con.conID)
+				if req != nil && req.TypeUrl != v3.SecretType {


Seems like we are hiding a useful error message. This seems to be because we are mixing "SecretType" to mean "istio-agent SDS server", when its not the case...

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali · 2022-10-11T03:55:52Z

Since there is no consensus on logging changes, I have reverted them for now. I certainly think

2022-09-29T03:33:23.515201Z info ads ADS: new connection for node:istio-ingressgateway-processing-us-west-2c-6c74f945bc-6cpvz.sfproxy-13

is pretty confusing log line in agent but I will think about it a bit more to get proper solution.

do not apply rate limits for sds server

3440efa

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

ramaraochavali requested review from a team as code owners September 29, 2022 09:42

istio-policy-bot added the release-notes-none Indicates a PR that does not require release notes. label Sep 29, 2022

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 29, 2022

fix ut

d0877ae

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

hzxuzhonghu reviewed Sep 29, 2022

View reviewed changes

configure no rate limit for sds

60a1856

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Sep 30, 2022

View reviewed changes

fix logs

d90b287

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 30, 2022

hzxuzhonghu reviewed Oct 9, 2022

View reviewed changes

zero rate limit

ac2865a

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn reviewed Oct 10, 2022

View reviewed changes

revert log changes

cdfdd24

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

howardjohn approved these changes Oct 11, 2022

View reviewed changes

istio-testing merged commit ac7f820 into istio:master Oct 11, 2022

ramaraochavali deleted the fix/sds_service branch October 11, 2022 14:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

do not apply rate limits for sds server #41213

do not apply rate limits for sds server #41213

ramaraochavali commented Sep 29, 2022 •

edited

Loading

istio-policy-bot commented Sep 29, 2022

ramaraochavali commented Sep 29, 2022

hzxuzhonghu Sep 29, 2022

ramaraochavali Sep 29, 2022

ramaraochavali Sep 29, 2022

howardjohn Sep 29, 2022

ramaraochavali Sep 29, 2022

ramaraochavali commented Sep 30, 2022

howardjohn left a comment

howardjohn Sep 30, 2022

howardjohn Sep 30, 2022

ramaraochavali commented Sep 30, 2022

ramaraochavali commented Oct 9, 2022

hzxuzhonghu Oct 9, 2022

ramaraochavali Oct 9, 2022

hzxuzhonghu Oct 9, 2022

ramaraochavali Oct 9, 2022

hzxuzhonghu Oct 9, 2022

ramaraochavali Oct 9, 2022

hzxuzhonghu Oct 9, 2022

ramaraochavali Oct 9, 2022

howardjohn Oct 10, 2022

howardjohn left a comment

howardjohn Oct 10, 2022

ramaraochavali commented Oct 11, 2022

do not apply rate limits for sds server #41213

do not apply rate limits for sds server #41213

Conversation

ramaraochavali commented Sep 29, 2022 • edited Loading

istio-policy-bot commented Sep 29, 2022

ramaraochavali commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Sep 30, 2022

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Sep 30, 2022

ramaraochavali commented Oct 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Oct 11, 2022

ramaraochavali commented Sep 29, 2022 •

edited

Loading