istio / api

API definitions for the Istio project
Apache License 2.0
460 stars 559 forks source link

RFC: Container API objects #651

Closed rshriram closed 6 months ago

rshriram commented 6 years ago

Decomposing APIs into smaller things (e.g., a separate resource for declaring subsets that decouples it from destination rules, or even the simple VirtualService and DestinationRule split) creates a lot of flexibility. At the same time, eventual consistency makes the implementation complex (atleast in networking) because if one or more pieces arrive late, the system (sidecars) could be in an inconsistent state (e.g., route points to versions, but the associated envoy clusters are not there because the destination rule has not arrived yet). We have been working around this issue by adding a slew of checks, behavioral assumptions/requirements on what the end user needs to do.

I propose that we create a new container API resource, one that can hold multiple types of APIs inside it. This provides us with the necessary collection object where all the related things are in one resource and can be applied without headaches.. At the same time, leaves the door open for flexibility, where people can use individual api objects to tweak stuff.

E.g.,

kind:NetworkingAPIContainer
spec:
 - type: virtualservice.networking.istio.io/v1alpha3
   spec:
      // virtual service spec
  - type: destinationrule.networking.istio.io/v1alpha3
     spec:
     // destination rule spec

The user can stuff any set of resources that need to be applied together. (we can tweak our docs as well). At the same time, users can also supply individual resources, as long as they understand the implications.

I know that networking can benefit from something like this. Not sure if the same is true in Auth/RBAC/Mixer ..

thoughts @geeknoid / @louiscryan ? or others (too many to name).

costinm commented 6 years ago

The replication delays are a problem - and some way to identify when 'all' configs have been seen is needed. We are currently depending too much on the debounce.

But I'm not sure how this will work - if we're not deprecating the old rules we'll keep having this problem so we still need to debounce and/or find other ways to deal with eventual consistency. Another problem is that separate resources may work better with RBAC - having the container will mean that we can't have different roles control specific types of configs.

Can we 'group' the configs but still keep them as separate resources ? It wouldn't be hard to add an change-ID to make sure different configs are applied together (as an extra attribute ) - the main problem is knowing how many resources are part of a change, so we wait for all. And the other problem is that we don't want user to manually deal with this. Maybe the CI/CD or UI could add 'change-ID' and 'change-count' as annotations - so we get equivalent grouping ?

Costin

On Tue, Oct 9, 2018 at 9:42 AM Shriram Rajagopalan notifications@github.com wrote:

Decomposing APIs into smaller things (e.g., a separate resource for declaring subsets that decouples it from destination rules, or even the simple VirtualService and DestinationRule split) creates a lot of flexibility. At the same time, eventual consistency makes the implementation complex (atleast in networking) because if one or more pieces arrive late, the system (sidecars) could be in an inconsistent state (e.g., route points to versions, but the associated envoy clusters are not there because the destination rule has not arrived yet). We have been working around this issue by adding a slew of checks, behavioral assumptions/requirements on what the end user needs to do.

I propose that we create a new container API resource, one that can hold multiple types of APIs inside it. This provides us with the necessary collection object where all the related things are in one resource and can be applied without headaches.. At the same time, leaves the door open for flexibility, where people can use individual api objects to tweak stuff.

E.g.,

kind:NetworkingAPIContainerspec:

  • type: virtualservice.networking.istio.io/v1alpha3 spec: // virtual service spec
    • type: destinationrule.networking.istio.io/v1alpha3 spec: // destination rule spec

The user can stuff any set of resources that need to be applied together. (we can tweak our docs as well). At the same time, users can also supply individual resources, as long as they understand the implications.

I know that networking can benefit from something like this. Not sure if the same is true in Auth/RBAC/Mixer ..

thoughts @geeknoid https://github.com/geeknoid / @louiscryan https://github.com/louiscryan ? or others (too many to name).

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/651, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFI6qVMJNJs1hjAUFEwHx5e42EDleakks5ujNH0gaJpZM4XTsN3 .

rshriram commented 6 years ago

Can we 'group' the configs but still keep them as separate resources ? It wouldn't be hard to add an change-ID to make sure different configs are applied together (as an extra attribute ) - the main problem is knowing how many resources are part of a change, so we wait for all. And the other problem is that we don't want user to manually deal with this. Maybe the CI/CD or UI could add 'change-ID' and 'change-count' as annotations - so we get equivalent grouping ?

the solution to all this is the container object that provides a virtual changeID, indicates the resources that are part of a change. Works very well with CI/CD as well as it simplifies ci/cd logic - no need to sequence

costinm commented 6 years ago

Container is a solution - but it has its own problems. For example we lose RBAC ( different roles having permissions to different specific resources ). And automation tools that would just edit a specific resource ( for example to shift traffic ) will now need to edit the larger object.

Are we going to require that a virtual host is defined in a single container, and not defined/modified in any other container or standalone CRD ? If yes - we'll have a set of problems, if no - another.

It's also not solving the problem completely - we still need to watch ServiceAccounts from k8s endpoints (with their own eventual-consistency issues ), label changes on the pods, service annotations and changes.

Maybe instead use a "ChangeSet" object - optional and used by CI/CD, tools, UI, but not humans - like your container, but with references to the a set of resources that are changed. This gets the grouping property of container - but keeps the modular API and is backward compatible.

Costin

On Tue, Oct 9, 2018 at 6:28 PM Shriram Rajagopalan notifications@github.com wrote:

Can we 'group' the configs but still keep them as separate resources ? It wouldn't be hard to add an change-ID to make sure different configs are applied together (as an extra attribute ) - the main problem is knowing how many resources are part of a change, so we wait for all. And the other problem is that we don't want user to manually deal with this. Maybe the CI/CD or UI could add 'change-ID' and 'change-count' as annotations - so we get equivalent grouping ?

the solution to all this is the container object that provides a virtual changeID, indicates the resources that are part of a change. Works very well with CI/CD as well as it simplifies ci/cd logic - no need to sequence

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/651#issuecomment-428406840, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFI6hAzCUafcLEMgQ23Kb8qplPJoMtSks5ujU0hgaJpZM4XTsN3 .

ozevren commented 6 years ago

How will a container model work with MCP? The configs will need to be resliced as per-type before delivery to Pilot.

As observed, ChangeSet tagging approach suffers from total-set problem. Trying to fix this by trying to use something like "count" opens other issues (i.e. successive overwrites of resources with different changeset membership for example.).

rshriram commented 6 years ago

The container is a type by itself. As far as mcp is concerned, it is streaming types.. Shouldn't reslice within the container.. Its upto Pilot/Mixer or others to reslice based on the business logic.

rshriram commented 6 years ago

Its same as saying, Galley should not be merging multiple gateways or virtual services or destination rules. That logic and know how is in pilot.

mandarjog commented 6 years ago

If decomposing API into smaller objects is a big headache (I believe it is) then we should stop decomposing it. Object references are useful when there are many objects referring to the same object. I don't think we have such thing with VirtualService and DestinationRule, we can actually eliminate the references at the cost of a small amount of duplication. Duplication can be solved in CI/CD by templating, but istio api does not need to deal with these references.

I think references are ok when there are safe failure modes, which is not the case with subsets. This proposal is half way between removing references and introducing transactions.

I would vote to do those two things separately.

For example we can add the ability to optionally inline a subset definition in a VirtualService.

mandarjog commented 6 years ago

The other issue is that VritualService and subset is not a matter of normal referential integrity. The subsets are referred by name but there is no resource that the system can lookup to ensure of its existence. One needs to look inside the destination rule to guarantee referential integrity.

If we had direct resource references, we could add a proto annotation on fields so that an external systems (like galley) could enforce referential integrity. With some complexity, we can add proto annotations for references. All this can be dynamically read and enforced.

ozevren commented 6 years ago

Its same as saying, Galley should not be merging multiple gateways or virtual services or destination rules. That logic and know how is in pilot.

We still may want to do modifications or introspection. For one, it needs to be validated.

If we add an "array-of-things" type in MCP, that allows us to do transactions. However, it would be at the expense of fine-grained incremental updates.

For things like Mixer types, where there are heavy referential relationships, this "sub-model" needs to translate well. (i.e. can I reference an Instance in a group from a rule on the outside? etc.)

ozevren commented 6 years ago

If decomposing API into smaller objects is a big headache (I believe it is) then we should stop decomposing it. Object references are useful when there are many objects referring to the same object. I don't think we have such thing with VirtualService and DestinationRule, we can actually eliminate the references at the cost of a small amount of duplication. Duplication can be solved in CI/CD by templating, but istio api does not need to deal with these references.

+1. Having "thicker" objects with less inter-resource referential integrity headaches is a good win IMO.

kyessenov commented 6 years ago

+1 for less referential integrity issues with better layering of concerns into objects and thicker objects. I think the issue is that decomposition of objects Rule+Destination does not draw the line correctly in how to decompose concerns in networking. It is apparent since most people have to provide the two objects in tandem for many use-cases. A more natural decomposition would call for objects mapped to features based on intent (80% use a single object, <20% add one more object for some extra bit flipped).

costinm commented 6 years ago

It is clear that having a 'transactional' changes is a benefit. Container will allow this - I guess at host level. But why not take a step further and solve the root problem - that we need to apply a change set.

How about having a container that only has a ChangeID and a list of references to object ( name ). Pilot and/or Galley will keep any object that has a ChangeID metadata in a 'pending' list, until a Container with the same ChangeID is received AND all object references by the Container are in the pending list.

The Container could hold objects of any type - including multiple hosts. We can even use the ChangeID as 'version' when sending to envoy.

A CI/CD system could easily create such a container automatically - and may use the PR or commit SHA as ChangeID - with the extra benefit that any failure can be traced back to the PR. Assuming that the CI/CD system is also running tests - we'll know that when Pilot applies a config, it is a config that has been tested, not a subset or variant.

And we don't need to lose decomposition - plus don't restrict ourselves to 'host'.

Costin

On Wed, Oct 10, 2018 at 4:53 AM Ozben Evren notifications@github.com wrote:

How will a container model work with MCP? The configs will need to be resliced as per-type before delivery to Pilot.

As observed, ChangeSet tagging approach suffers from total-set problem. Trying to fix this by trying to use something like "count" opens other issues (i.e. successive overwrites of resources with different changeset membership for example.).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/651#issuecomment-428543007, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFI6qkkhFF_3hBhx854CJn_uf3onMD9ks5ujd-_gaJpZM4XTsN3 .

costinm commented 6 years ago

I would add: I don't mind a Container object as Shriram proposed, if it is just used between MCP config store and Pilot, so Pilot doesn't need to debounce ( which is how we currently deal with sets of changes). For any backend that support 'transactions' - Galley or the equivalent MCP adapter will group objects in Containers, and pilot will only deal with Containers.

I even like the use of Container for authoring - with tools like "istioctl apply container.yaml" decomposing it before saving, and adding a ChangeId to each object and a separate ChangeSet CR.

My only concern is on what is stored and RBACed in K8S: the set of changes should be decomposed before storing.

Git and PRs operate the same way - as a list of 'change ids', but resulting in the actual resources ( files ) getting updated.

Costin

On Thu, Oct 11, 2018 at 7:01 AM Costin Manolache costin@gmail.com wrote:

It is clear that having a 'transactional' changes is a benefit. Container will allow this - I guess at host level. But why not take a step further and solve the root problem - that we need to apply a change set.

How about having a container that only has a ChangeID and a list of references to object ( name ). Pilot and/or Galley will keep any object that has a ChangeID metadata in a 'pending' list, until a Container with the same ChangeID is received AND all object references by the Container are in the pending list.

The Container could hold objects of any type - including multiple hosts. We can even use the ChangeID as 'version' when sending to envoy.

A CI/CD system could easily create such a container automatically - and may use the PR or commit SHA as ChangeID - with the extra benefit that any failure can be traced back to the PR. Assuming that the CI/CD system is also running tests - we'll know that when Pilot applies a config, it is a config that has been tested, not a subset or variant.

And we don't need to lose decomposition - plus don't restrict ourselves to 'host'.

Costin

On Wed, Oct 10, 2018 at 4:53 AM Ozben Evren notifications@github.com wrote:

How will a container model work with MCP? The configs will need to be resliced as per-type before delivery to Pilot.

As observed, ChangeSet tagging approach suffers from total-set problem. Trying to fix this by trying to use something like "count" opens other issues (i.e. successive overwrites of resources with different changeset membership for example.).

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/istio/api/issues/651#issuecomment-428543007, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFI6qkkhFF_3hBhx854CJn_uf3onMD9ks5ujd-_gaJpZM4XTsN3 .

ayj commented 5 years ago

Is this issue still relevant? It's been open for a year with no activity.