config: require non-empty strings

[PiotrSikora]

source/common/json/config_schemas.cc should enforce minLength: 1 on majority of the fields.

cc @mattklein123

[mattklein123]

cc @danielhochman something to take a look at when we are looking at better error messages.

[PiotrSikora]

Actually, how is this going to work with the proto-based APIs? Presumably, those are going to bypass JSON config schema, right?

[mattklein123]

Yes. I haven't thought about this much yet. Fundamentally, the core code will work on only JSON or proto. We will need to either convert proto to JSON and send it through the schema, or do it the other way around and convert JSON to proto. Originally, I had thought we would convert JSON to proto, but you make a valid point that JSON schema is more expressive in some ways than proto so we might want to do it the other way around. We will need to put some thought into this.

[PiotrSikora]

Maybe let's put this on hold until we figured it out, then?

For the record, I'd rather avoid conversion (one way or the other) and retain checks in the code.

[mattklein123]

Yes, we can figure this out in the context of figuring out how we do proto handling. At some level, we will have to convert to something, since we don't want to write parallel implementations of code that just convert data to objects. But yes, agreed, the gRPC/proto/JSON handling as it relates to schema and other stuff needs a lot of thinking.

[mattklein123]

From offline back and forth, it seems like we are converging on:

• gRPC APIs become the canonical v2 APIs
• We will use coming Envoy gRPC/JSON transcoder support to call JSON versions of the APIs. This will allow a single API definition for both gRPC and JSON.
• The existing v1 APIs will remain as legacy APIs. Internally, we will likely need to convert the responses to proto before processing.
• There are still questions about how to reconcile static configuration, error checking that the current JSON schema provides, and probably a bunch of other gotchas.

[PiotrSikora]

@mattklein123 do we have any parameters where it makes sense to use empty values? If not, then maybe just require all strings to be non-empty if set?

cc @louiscryan

[mattklein123]

I don't know. Probably not but I'm not sure? We will need to go through stuff and look carefully.

[mattklein123]

Going to close this out in favor of the v2 design work that is going on, including constraints. I think we have a good path forward on all of the issues raised here. cc @htuch