Integrate IstioControlPlaneSpec with MeshConfig

[ostromart]

First pass for integrating these two protos per the integration proposal doc.
Once there's basic agreement, there will be follow-on work items:

1. Translate between v1alpha1<->v1alpha2 MeshConfig. Required for backwards compatibility with existing components that expect v1alpha1, and in the other direction for user migration to v1alpha2.
2. Translate from v1alpha2->values.yaml.
3. Port of the remainder of values.yaml into v1alpha2.

[howardjohn]

How do we support multiple gateways? Should we make this a repeated field?

This may apply to other components as well in the future for canary, etc, but as far as I know people are only really running multiple gateways

[ostromart]

Gateways are likely to be user config so it's not clear if this needs to cover all the use cases. For now it's there to support demo/out of the box installs but for serious use, users would turn these off and create their own.
I'd argue for leaving it as is, otherwise this may get very complex.

[howardjohn]

will the operator not support users configuring their own gateways?

[ostromart]

They would do it outside of the operator, basically it would be treated like all other Istio user config. The reason is that it has been a struggle to create a sufficiently broad definition for gateway to accommodate all cases. But if the existing Ingress/Egress specs above are sufficient to cover most user cases, we can make these a repeated field and call it a day.

[ayj]

Repeating these as-is results in multiple gateway LBs with identical config (e.g. ports for for multi-cluster passthrough and VM expansion). We probably want two different things:

1. A default set of gateway(s) with appropriate Istio config to satisfy the MeshConfig contract. And some way to disable these defaults.

2. A method for users to create their own Istio gateway LBs.

I believe (2) is technically possible using helm by disabling all components except the gateway. I don't think that is appropriate here. We need to differentiate default from user gateways if they both exist in MeshConfig. Alternatively, we add another API for creating gateway LB instances. In either case, the operator could manage the gateway lifecycle.

[ostromart]

IMO the gateway question requires significantly more work on both the API and resulting operator code. Given the short amount of time for 1.4, I propose we defer this work till 1.5.
Since we still have the helm passthrough in this API, we could just remove the gateway component completely for this release and ask users to use the helm API for gateway components.

[rshriram]

90% of the stuff in Mesh config is for Pilot's consumption only. Even if its authN or mixer report server or what not. Given that, I find it confusing to have settings used by pilot be present under citadel, combined with helm chart stuff for citadel.

All in all, I am not convinced that this is the right approach to reconciling values.yaml with mesh config. We should move stuff from values into meshConfig if it doesn't exist. And then we can simply have a ComponentSpec that captures the settings that we currently have in pilot/citadel/gateway/telemetry, etc. IF we go down this path, we wont have to have a v1alpha2 of MeshConfig. We just need a k8s.proto and the componentSpec.proto

[ostromart]

@rshriram the fact that most of the parameters are consumed by pilot is an implementation detail that most users don't know or care about. The issue here is that we have a huge, totally flat structure, which is only going to get bigger as we migrate values.yaml parameters into it. This is one way to impose some structure on it that follows relatively well established feature groupings.
I agree there's a migration cost involved but if we want to take the pain this is the right moment to do it imo.

[ostromart]

@linsun what we have here is the first step, not the final product. The final product will be:

• values.yaml will be deprecated (it can continue to be supported outside of istio/api as long as we have helm users)
• any settings that we want from values.yaml that are not already in here will be migrated gradually into MeshConfig. Since we already cover all the k8s related settings in IstioControlSpec (which probably needs a different name now) and the really important runtime config is in MeshConfig, I'm not sure what that actually leaves.

For 1.4, there will indeed be multiple paths to access some parameters (through MeshConfig and values.yaml field embedded in it). At least I don't see how we can do away with values.yaml at this point without migrating a bunch of config knobs. Some of these are fairly easy but some (like user defined gateways) are quite hard.

[costinm]

Proxy settings are already expressed in a structure - no need to create a parallel one, you can add it there.

[ostromart]

Which structure?

[smawson]

istio.mesh.v1alpha1.ProxyConfig

Available as the field "default_config", unfortunate choice of name.

It does seem easy to just throw the k8s resource spec in there.

[costinm]

How about: for telemetry and policy just use the value of mixerAddress (already existing) in 1.4. This is how users are trained to control telemetry, no need to add an API just to remove it soon after.

Besides - telemetry will remain enabled even if mixer is not running. This is clearly not correct - users still get most of the telemetry from envoy.

[ostromart]

Done.

[smawson]

We maybe should add Mixer to these names to make it clear these are configs for MixerTelemetry, not Telemetry overall.

[costinm]

Same as with the other - I would run it by security WG, but I think the proper way would be to have a "security" section at top level, and maybe a provider: with citadel as an option, or the non-citadel CA provider URL or other related settings. There is a proposal to add trust aliases as well, sds may migrate there as well.

Meanwhile - if users want to disable citadel in 1.3 they can use the values.yaml override.

[ostromart]

Sounds good, but I feel like this is something we should tackle incrementally rather than this PR, since it's a real API change. The original changes I made were a purely mechanical reshuffling of parameters into groups, your proposal has some actual meat in it.

[smawson]

It seems totally reasonable to have a top-level "citadel" field in the spec and configure the component. That should hold citadel-specific config, and a "security" section could have security-specific config (maybe you nest citadel under security if we feel the need to).

But generally I agree on holding off a major refactor of mesh config in this PR.

[costinm]

Same as in the other changes - please use the feature or behavior, not component/install name.

mcp_server ?

There is already a section on mcp addresses - I suspect we can just check if istio-galley.istio-system is on the list to
determine if galley should run, do maybe no need to add this redundant section ?

If you just need a place to attach k8s - I mentioned using a map<string,k8sresourceSpec> - keyed by component name. Or keeping them in a separate file, to not make the mesh config dependent on internal code names.
( see the discussion in Chiron in env wg)

[ostromart]

We're conflating two things here: "feature" settings and "component" settings. I had feature structs in the first attempt to integrate the protos but I've since taken them out, since there's nothing left there anymore. The feature settings should not have any internal names.
The component settings necessarily name internal components because they map resources 1:1 with those components. Again I think this boils down to listing the components vs. your map proposal, let's continue in that thread.

[smawson]

👍 to being explicit about the difference between configuration of a feature (such as whether config validation is on) and configuration of a component (such as whether we run galley, and if so how it is run). These are different concerns and should remain separate.

[costinm]

This is interesting because Istio supports multiple ingress gateways. Same for egress.

[ostromart]

Gateways anyway require values.yaml API to be used. This just allows the built-in gateways' resources to be controlled through this new API. User gateways can still be defined, but their resources must be set using values.yaml (for now).

[costinm]

Maybe this is also something to keep for 1.5 ? Little time, too much to test ?

[costinm]

I want to be clear on one thing: for 1.4 Operator should work just fine without this PR, using a separate values.yaml in the current format.

So we are not forced to merge everything in 1.4 or quickly - and remove it later, we can make sure each
setting makes sense and involve the WG responsible for the feature to get the right API.

[linsun]

@costinm thanks, I think I agree with that assessment for 1.4

@ostromart re your comment, This is exactly what I'm confused...."any settings that we want from values.yaml that are not already in here will be migrated gradually into MeshConfig. Since we already cover all the k8s related settings in IstioControlSpec (which probably needs a different name now) and the really important runtime config is in MeshConfig, I'm not sure what that actually leaves." Why are we moving everything from values.yaml to mesh config, vs just moving things from values.yaml but not covered in IstioControlPlaneSpec to MeshConfig?

[sdake]

I am a little confused as well. I thought the agreement was to have runtime configurable options go to MeshConfig, and have install time configurable options go to IstioControlPlane. It may be that IstiioControlPlane additionally contains runtime configuration options (via values.yaml) until they are migrated to MeshConfig - not IstioControlPlane.

This raises the question of how does the operator's controller manage runtime operations...

[rshriram]

@ostromart I like this model. I was thinking that we could [in future] also categorize the mesh config..

MeshConfig:
 NetworkingConfig
 TelemetryConfig
 SecurityConfig
 ComponentSpecs: .. whatever you have currently under install

where the existing settings can be moved under these subsections. And

[rshriram]

LGTM after all the comments from others are addressed.

[ostromart]

@costinm @linsun the operator will not work fine without this PR. This PR represents the minimal amount of integration we need for operator to use the converged MeshConfig that we agreed to.
What I don't want this PR to become is a complete overhaul of the values.yaml API. That's not what we agreed to and such a PR will never complete in the forseeable future.
I propose we agree to a minimum integration - which I think this PR is now very close to - and leave the bigger task of migrating values.yaml incrementally since that involves different WGs and is better handled through smaller, incremental PRs rather than a single giant one.

[ostromart]

@linsun sorry, that's exactly what I meant - we will move any fields in values.yaml that are not already in IstioControlPlaneSpec into MeshConfig, unless we decide to deprecate them instead.

[ostromart]

@sdake that's correct - values.yaml config that covers "install time" (k8s resources, HPA etc) is moving into IstioControlPlaneSpec in this PR. Going forward, only the fields in values.yaml not already in MeshConfig or IstioControlPlaneSpec will be considered for migration into MeshConfig or deprecated. Finally, we will only have MeshConfig (for runtime) and ICPS (for install resources).
Operator will use at least ICPS, although it's possible for it to act on changes in runtime config if there's a need to do so.

[linsun]

@ostromart thank you for the important clarification!

[istio-testing]

@ostromart: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
gencheck_api	d0a6e0d	link	/test gencheck_api
build_api	d0a6e0d	link	/test build_api

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.

[smawson]

Would be good to split this pr and just have this be changes to the install spec (which should show up as a move instead of a new file, or is it not in this repo?).

Then we can remove the profile and values stuff from it, and just have it be a component spec.

[smawson]

Rather than creating empty messages can we move a few fields into each of these (or rather copy them over to do the migration)?

Or if we don't want to do that yet maybe leave MeshConfig alone and have this PR just clean up control plane spec?

[smawson]

Should rename this proto to components.proto or controlplane.proto or something.

[smawson]

Different profile? We maybe don't want to talk about values since those will go away?

[ostromart]

not stale, just sleeping...

[ostromart]

Continued in #1177.