Add native openapi, grpc, and rest apis support to Istio #368
Conversation
…nd generic rest apis
googlebot
added
the
cla: yes
apis/v1alpha1/api.proto
Outdated
| OpenAPIv2 openapiv2 = 2; | ||
| OpenAPIv3 openapiv3 = 3; | ||
| Grpc grpc = 4; | ||
| } |
There was a problem hiding this comment.
-
http_generic is low level and internal. opanapi and grpc are user facing, high level, they should not be mixed.
-
Here openapi only for generating api attributes, or it may generate other config, such as auth or security, or api-key requirement
There was a problem hiding this comment.
Is HTTP generic more general than OpenAPI or less general?
There was a problem hiding this comment.
Alternate name suggestions welcome :)
I'd like to retain the option for a user to define a basic REST API without needing to restore to OpenAPI. cc @mpnally
There was a problem hiding this comment.
In general, I think we want security definitions to be ignored in this context. AFAIK, the current standard schema is not sufficient anyway to configure Istio authentication, e.g. JWT validation. api_key may be a special case since it's defined as a security definition but not covered by authentication policy. As an exception, we could add ApiKey submessage back to the generic HttpApi message and extract that from OpenAPI during normalization.
apis/v1alpha1/api.proto
Outdated
| message OpenApiv2 { | ||
| oneof data { | ||
| // utf-8 encoding of OpenAPI v2 document written in YAML or JSON. | ||
| string yaml = 1; |
There was a problem hiding this comment.
Use google.protobuf.Struct for YAML or JSON fields?
It will keep the data structured without lose any information.
There was a problem hiding this comment.
Some downstream tools for conversion, normalization, validation, and analysis expect YAML/JSON input, e.g. https://github.com/googleapis/gnostic, https://godoc.org/github.com/go-openapi/validate. We could convert back to JSON/YAML from google.protobuf.Struct internally but we risk losing providence.
apis/v1alpha1/api.proto
Outdated
| message Grpc { | ||
| oneof data { | ||
| // Binary content of the proto descriptor set for the gRPC service. | ||
| bytes binary = 1; |
There was a problem hiding this comment.
Note the name after oneof doesn't appear in field name, so you would better to name this as descriptor_set_binary. ditto for above.
There was a problem hiding this comment.
What's the purpose of oneof constraint here?
There was a problem hiding this comment.
Why do we need so much structure? Is this actually any better than
message ApiSpec {
string type; // or enum
bytes data;
}
There is no type safety with all these messages anyway.
There was a problem hiding this comment.
I'd rather have strongly discriminated types, since in addition to the raw data for a given format, we can also include additional metadata by adding new fields to the message.
There was a problem hiding this comment.
Do we really need to wrap an external schema as a CRD? I have seen an API spec that is over 20MB, which can not fit into CRD. Another issue is many API spec is auto generated from other build rules, not written as CRD yaml file. It would be much better to refer to the API spec as a reference than embed it as bytes.
There was a problem hiding this comment.
Inlined and externally referenced schemas are both viable models. Something like envoy API's DataSource may work here to allow for multiple schema sources? Validation would be required for inline case to avoid size issues. The external reference could be an http path or even service name to enable server reflection.
There was a problem hiding this comment.
For example,
message SchemaSource {
oneof specifier {
// Remote reference to schema.
string external_reference = 1;
// Bytes inlined in the configuration.
bytes inline_bytes = 2;
// String inlined in the configuration.
string inline_string = 3;
}
}GRPC would likely use inline_bytes for small-ish services or an external reference. OpenAPI could use inline_string (via multi-line yaml), inline_bytes (base64 encoded?), or external reference, e.g.
spec:
openapiv2:
externalReference: https://storage.googleapis.com/ayj0/petstore.jsonNon-HTTP external references are another option if stronger ACLs are preferred, e.g.
spec:
openapiv2:
externalReference: gs://ayj0/petstore.json
apis/v1alpha1/api.proto
Outdated
| // Binary content of the proto descriptor set for the gRPC service. | ||
| bytes binary = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
It is better to add a field repeated string services to specify service names explicitly. It is common that a descriptor set have multiple services, due to dependencies, but only a subset of them are served by a backend.
apis/v1alpha1/api.proto
Outdated
|
|
||
| package istio.apis.v1alpha1; | ||
|
|
||
| option go_package="istio.io/api/apis/v1alpha1"; |
There was a problem hiding this comment.
nit: api/apis feels like stutter. Maybe use IDL - standing for interface description, instead, e.g. api/idl.
There was a problem hiding this comment.
IDL isn't appropriate for this package IMO.
To avoid the direct studder, perhaps istio.apimanagement.v1alpha1 would be better?
There was a problem hiding this comment.
apimanagement might be too broad when we consider versioning. The IDL may reach beta fairly soon given we're referencing existing schema. But other features not may be developed for some time. If they're in the same group we either need to "hold back" the apimanagement group to v1alpha or let end up needing to add wip features to a beta/stable group.
Alternatively, we add more granular groups and use idl here and add a separate group(s) later for the other features.
apis/v1alpha1/api.proto
Outdated
| message OpenApiv2 { | ||
| oneof data { | ||
| // utf-8 encoding of OpenAPI v2 document written in YAML or JSON. | ||
| string yaml = 1; |
apis/v1alpha1/api.proto
Outdated
| message Grpc { | ||
| oneof data { | ||
| // Binary content of the proto descriptor set for the gRPC service. | ||
| bytes binary = 1; |
There was a problem hiding this comment.
What's the purpose of oneof constraint here?
apis/v1alpha1/api.proto
Outdated
| // | ||
| // istioctl create httpapi petstore --type=openapiv2 --from-file=petstore-simple.yaml | ||
| // | ||
| message HTTPAPI { |
There was a problem hiding this comment.
HttpApi, or perhaps something better :)
There was a problem hiding this comment.
Http probably ought to be in the name since we might, in theory, support other protocols in the future. HttpIdl or HttpInterfaceDefinition?
apis/v1alpha1/api.proto
Outdated
| OpenAPIv2 openapiv2 = 2; | ||
| OpenAPIv3 openapiv3 = 3; | ||
| Grpc grpc = 4; | ||
| } |
There was a problem hiding this comment.
Is HTTP generic more general than OpenAPI or less general?
apis/v1alpha1/api.proto
Outdated
| @@ -0,0 +1,197 @@ | |||
| // Copyright 2017 Istio Authors | |||
apis/v1alpha1/api.proto
Outdated
| message Grpc { | ||
| oneof data { | ||
| // Binary content of the proto descriptor set for the gRPC service. | ||
| bytes binary = 1; |
There was a problem hiding this comment.
Why do we need so much structure? Is this actually any better than
message ApiSpec {
string type; // or enum
bytes data;
}
There is no type safety with all these messages anyway.
apis/v1alpha1/api.proto
Outdated
| // Optional namespace of the API. Defaults to the encompassing | ||
| // HTTPAPIBinding's metadata namespace field. | ||
| string namespace = 2; | ||
| } |
There was a problem hiding this comment.
I don't think we want to define a reference message for each type.
There was a problem hiding this comment.
Definitely not. I'd much rather define the destination binding inline in the top-level type. That does mean some potential duplicate if an API is shared across services, but that is partly mitigated with the proposed external_reference.
apis/v1alpha1/api.proto
Outdated
| // REQUIRED. One or more API references that should be mapped to | ||
| // the specified service(s). The aggregate collection of match | ||
| // conditions defined in the APIs should not overlap. | ||
| repeated HTTPAPIReference apis = 2; |
There was a problem hiding this comment.
How is this different from generic Istio routing?
There was a problem hiding this comment.
Not sure I understand the question. Are you asking why APIs aren't defined/bound by existing routing APIs?
| string http_method = 2; | ||
|
|
||
| oneof pattern { | ||
| // URI template to match against as defined by |
There was a problem hiding this comment.
Do you really promise of the full spec?
There was a problem hiding this comment.
It looks like we ought to restrict support to a much smaller subset based on OAI/OpenAPI-Specification#1459. But it's not clear to me what wording or reference I should include here to specify the semantics. Is "path templating matching as defined by OAS 3.0.1" with a reference to the spec sufficient, or is additional clarification needed?
There was a problem hiding this comment.
apis/v1alpha1/api.proto
Outdated
| // name: petstore | ||
| // spec: | ||
| // openapiv2: | | ||
| // swagger: "2.0" |
There was a problem hiding this comment.
How does k8s handle these type of foreign configs? Can we just store the Swagger file somewhere directly? How do you plan to store the proto descriptor here?
There was a problem hiding this comment.
ConfigMaps store data inline, e.g. https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#create-configmaps-from-directories.
Secrets also store data inline. Binary data is typically basd64 encoded.
base64 encoding may be okay for smallish grpc services and demos where external file store is not desirable. We could add support for external_reference for all other cases. See the SchemeSource example below.
apis/v1alpha1/api.proto
Outdated
|
|
||
| package istio.apis.v1alpha1; | ||
|
|
||
| option go_package="istio.io/api/apis/v1alpha1"; |
There was a problem hiding this comment.
IDL isn't appropriate for this package IMO.
To avoid the direct studder, perhaps istio.apimanagement.v1alpha1 would be better?
apis/v1alpha1/api.proto
Outdated
| option (gogoproto.gostring_all) = false; | ||
|
|
||
| // OpenAPI v2 specification | ||
| // https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md |
There was a problem hiding this comment.
Can make this a real markdown link.
There was a problem hiding this comment.
Yup. This all needs better documentation with examples before its merged.
apis/v1alpha1/api.proto
Outdated
| message OpenApiv3 { | ||
| oneof data { | ||
| // utf-8 encoding of OpenAPI v3 document written in YAML or JSON. | ||
| string yaml = 1; |
There was a problem hiding this comment.
For all of these fields, I think we want a name representing the semantic of the data rather than its format.
apis/v1alpha1/api.proto
Outdated
| message Grpc { | ||
| oneof data { | ||
| // Binary content of the proto descriptor set for the gRPC service. | ||
| bytes binary = 1; |
There was a problem hiding this comment.
I'd rather have strongly discriminated types, since in addition to the raw data for a given format, we can also include additional metadata by adding new fields to the message.
|
We should ask at least someone from Kubernetes community about how to represent foreign specifications. In order to represent Google APIs, such as Compute Engine API, the API spec can be more than 10MB. If we are building an infrastructure, we must be able to handle at least 100MB. While the number sounds silly, but that is what the reality is. People like to pull in unnecessary dependencies and create bloated APIs. |
|
Kubernetes volumes may be another option for managing large (> 1mb) API specs. Our API spec CRD would need a reference to the backing volume and sub-path within that volume that holds the API spec document(s). @erictune, @lavalamp - any additional recommendations on how Istio might be able to manage API spec that exceed CRD size limitations? |
|
What size are we talking about? |
|
At least 100Mb per @wora's comments. Many Google API's are at least 10Mb (e.g. Compute Engine API). |
|
The entire Kubernetes API spec is 3.5 MB. What is going in the spec that is making it so big? Kubernetes Volumes are not going to work as an overflow system (apiserver has no way to retrieve data from a volume). I do not see the 1MB limit being relaxed in the near future. Compression? Omit documentation? |
|
I think the issue is the API _representation_. For example, protobuf
descriptors are pulling all transitive dependencies, duplicating most of
the commonly used types in every single API descriptor. I wonder if
something can be done about a more efficient IDL description language
representation, as that would solve the problem at the root.
…On Mon, Feb 12, 2018 at 10:33 AM Daniel Smith ***@***.***> wrote:
The *entire* Kubernetes API spec is 3.5 MB. What is going in the spec
that is making it so big?
Kubernetes Volumes are not going to work as an overflow system (apiserver
has no way to retrieve data from a volume). I do not see the 1MB limit
being relaxed in the near future.
Compression? Omit documentation?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#368 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AJGIxm9T0cgYMgX0tdQqP6GWgM7YZLhVks5tUIQGgaJpZM4R_Rgt>
.
|
|
Is it still that large if you prune all types not actually used in the API?
Keep in mind you only have to represent a single CRUD-supporting object at
a time.
Also (possibly relevant) keep in mind that you can't really represent
non-CRUD actions at all at the moment. Subresources for CRDs are planned
but not here yet and may not have infinite flexibility-- design is somewhat
TBD last I heard.
…On Mon, Feb 12, 2018 at 10:47 AM, Kuat ***@***.***> wrote:
I think the issue is the API _representation_. For example, protobuf
descriptors are pulling all transitive dependencies, duplicating most of
the commonly used types in every single API descriptor. I wonder if
something can be done about a more efficient IDL description language
representation, as that would solve the problem at the root.
On Mon, Feb 12, 2018 at 10:33 AM Daniel Smith ***@***.***>
wrote:
> The *entire* Kubernetes API spec is 3.5 MB. What is going in the spec
> that is making it so big?
>
> Kubernetes Volumes are not going to work as an overflow system (apiserver
> has no way to retrieve data from a volume). I do not see the 1MB limit
> being relaxed in the near future.
>
> Compression? Omit documentation?
>
> —
> You are receiving this because you commented.
>
>
> Reply to this email directly, view it on GitHub
> <#368 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/
AJGIxm9T0cgYMgX0tdQqP6GWgM7YZLhVks5tUIQGgaJpZM4R_Rgt>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#368 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAnglgCqxVUcyIp2fXTd8eP3Yk1hSvtYks5tUIdegaJpZM4R_Rgt>
.
|
|
@lavalamp, I think you might be referring to CRD validation with OpenAPI v3 schema? I don't expect the schema of Istio CRD themselves to exceed the 1mb limitation. This issue is referring to existing HTTP-based API services (e.g. gRPC service) we'd like to add to Istio mesh with API-level policy/telemetry support. Asking a user to refactor their API so it can be supported by Istio is likely not an option in the long term. Pruning types/documentation is suitable for simple mapping of method/path to API operation (see proposed UX features requires documentation, e.g. developer portals. We could store the API documentation elsewhere, but we'd need to retain a reference inline for discoverability. For the purposes of transcoding and request-level validation, pruning unused types/services from proto descriptor sets might help, but I'm not sure if it we get us under the 1mb limit for extremely large services, e.g. some Google APIs. |
apis/v1alpha1/api.proto
Outdated
| // could be hidden from the user. Alternatively, it might be useful | ||
| // to reflect this back into the user-facing configuration resource | ||
| // to aid in debugability. | ||
| HTTPGeneric normalized_http = 6; |
There was a problem hiding this comment.
Is this the canonical api definition that includes all the information that Istio needs for its attribute generation? If so, why do we ever need to in-line the other often-bloated representations? Wouldn't it suffice to just import the other formats (with a reference back to the source), and only storing the generic form in the CRD, e.g., import swagger using istioctl create httpapi ...?
There was a problem hiding this comment.
HTTPGeneric is sufficient for API attribute generation. Transcoding, request-level validation, enhanced tracing, and UX discovery require additional information. "reference back to the source" is one valid option and is what @wora suggested here. We'd want to standardize on what a valid "source" is so that tooling can consistently reason about it.
There was a problem hiding this comment.
I think I'm still missing something. Doesn't the additional information required by transcoding, request-level validation, etc., need to be converted into a single normalized format used at runtime? Otherwise we need multiple implementations of these functions, don't we? In other words, I was thinking that HTTPGeneric could include ALL the required information, and therefore all we would need to store in the CRD.
There was a problem hiding this comment.
Not necessarily. Envoy's gRPC-JSON transcoder needs the proto descriptor set. Some developer portals expect the full OpenAPI/gRPC spec. HTTPGeneric as it is today isn't sufficient. We'd need to extend it and may end up with something similar to https://github.com/googleapis/googleapis/blob/master/google/api/service.proto (minus all of the backend, policy, billing, etc. fields).
|
@ayj Thanks, sorry-- yes, I thought this was talking about the schema to be sent with the CRD. I agree that Istio probably ought to handle anything out there and I don't have a strong opinion about where such things should be stored, other than that in a CR or elsewhere in the control plane isn't really an option, at least not in the short term :( |
|
Are the content-type-specific API payload schemas even useful at this level, or just potentially used by policy engine plugins? I'm not only concerned about the size, but also about how many different formats and versions are supported, how clear it is what subset of each is supported, scope creep into the API management space, and so on. What can be done to keep this layer minimalistic and extensible? |
|
Creating a superset spec to represent other specs would be a huge effort. We tried once, e.g. We need to handle very large APIs. While we don't like them, but that is that the reality is. If Istio cannot handle Compute or YouTube, it would be difficult to be a real platform. |
|
This API spec only describes the API surface (e.g. methods, paths, parameters, docs). Policy (auth, quota, etc) are defined by other configuration, though they may reference the attributes defined by the API spec (e.g. I think we only care about OpenAPI v2/v3, gRPC, and the generic HTTP form. I haven't heard anyone ask for anything else yet. We definitely ought to be conservative about introducing additional schema/versions without a strong justification. |
* separate check and report clusters * fix * nits * nit * backward compat
* Update STYLE-GUIDE.md Mention the CRD kind must match proto message. * Update STYLE-GUIDE.md Mention `apiVersion` must match proto package. * Update STYLE-GUIDE.md Explain apiVersion = group/version.
Update guideline not to use acronyms in API definition. Fixed istio#364.
Add a rule for repeated fields.
* Address comment * Address comment * Fix typo
* Doc clarifications * regenerate go files
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
googlebot
added
cla: no
ayj
changed the title
| // Grpc, and HttpGeneric in the `apis.istio.io/v1alpha1` apiversion. | ||
| google.protobuf.Struct schema = 2; | ||
|
|
||
| // List of destination services that this API is bound |
There was a problem hiding this comment.
@rshriram, @ZackButcher - the definition and usage of gateways and destinations here probably aren't 100% correct, but the intent was to align with the concepts defined by routing APIs. What I'd like to do is describe an HTTP API and bind that API to a service, subset of services (e.g. new version of a service has a new API), and/or standalone edge proxies (e.g. ingress). This would translate into Pilot delivering the API spec as part of an envoy filter / route. I'm hoping the conceptual alignment will make it easier to integrate with the ongoing Pilot feature enhancements.
There was a problem hiding this comment.
Are you trying to pin the API spec to a proxy here, wherein this applies in the inbound path?
There was a problem hiding this comment.
Yes, that's one use case, e.g. mixerclient needs IDL to produce API operation attribute based on HTTP verb/path.
|
Hey is there a doc or issue describing the proposal in a little more detail? I have a hard time visualizing how the pieces are intended to fit together in the context of a single proto like this. I'd much prefer commenting in a doc sent to the api management working group. I'd like to see this built in a pluggable way, so additional api descriptor languages can be plugged in and translated to a smaller set of representations that go to the proxy. I don't think we can expect an additional schema to result in new code in pilot and proxy in addition to the crd as a reasonable model here. On the other hand the set of schemas may be small enough we just hard code a few and call it done, but I generally dislike those kinds of answers. I also wouldn't bother with trying to support externalized schemas at all right now, and instead focus on a simple CRD model + a translation to a smaller internal representation that we pass down to the proxy which is offered as a library and as a hook. Then anyone with a super large schema can compile it themselves using the library. Otherwise you're asking for a lot of trouble trying to deal with external schemas, and Istio isn't authoritative, and it is going to hurt. |
|
I'll close this issue for now and start a doc as @smawson suggested. We can discuss/review in the next WG meeting on 2/28. |
Background
Istio API spec and attribute generation proposed how to configure an API in the Istio proxyto generate API related attributes. The result was the mixerclient api_spec.proto which is necessary but not sufficient to satisfy API mgmt support in Istio. At a minimum, users should author intent-based feature-level config without regard for which component(s) implement it (ref #344, https://goo.gl/QQPLdL).
The new types included in this PR are just a strawman to help with a discussion. It's mostly copy/pasted from the mixerclient spec with API keys removes and openapi/grpc added.
Motivations
Support user-facing configuration for describing API surface of services(s). Required to generate API attributes against which policy can be written (ref https://goo.gl/aw9M1w)
Native support for OpenAPI may provide better UX - (ref https://goo.gl/TkDYDx)
Request-level validation in mixer adapter or proxy (ref openapi service spec integration istio#2352)
gRPC Transcoding in proxy (ref https://www.envoyproxy.io/docs/envoy/latest/configuration/http_filters/grpc_json_transcoder_filter)
Enhanced envoy tracing with information from openapi (ref tracing: Use openapi definition for service to enhance tracing information envoyproxy/envoy#1897)
Unified location for discovery of API services within the mesh? Could possible augment the aggregate service registry information available in Pilot.
Proposal
Create a new API group for describing APIs and how they map to service(s) in Istio, e.g.
apis.istio.io/<version>. New type(s) are created in this group to describe APIs using OpenAPI (v2/v3), gRPC, and generic http verb/path pattern matching schemes.Some components may only want to consume the normalized generic HTTP form as its defined today for mixerclient. A mutating webhook can be transform high-level schemes such as OpenAPI/GRPC to the normalized form and store it along side the original spec before the resource is submitted to storage and advertised to components.
Once defined, Pilot would start consuming this new API(s) to create the mixerclient filter configuration. Additional features could be added later, e.g. transcoding, request valdiation, tracing.
open questions (non-exhaustive):
Should Istio openapi be natively support openapi? Should it reside exclusively outside of Istio, requiring client-side tooling to translate into normalized http scheme?
If "yes" to Add structure for istioapi in a new repo #1, should we support a single union type, unique type per scheme/protocol, or some combination in between, e.g.
How should API(s) be bound to services? Inline via repeated
destinationsfield or separate binding resource?