Closed strekm closed 5 months ago
I think the cleanest way to do that is by having different implementations for resolving the Istio Operator manifest. We would have different Docker images and using a build tag we can configure what implementation is included.
We can use a build tag to conditionally add a watch for the config map with the custom configuration. The question is how do we continue from here? We can then have a logic in reconcile to read the Istio CR and reconcile it. I would not do that, since this mixes up a lot of existing code, seems to me like a workaround and is harder to understand.
I'd rather reduce the requeue time of the reconciliation of Istio CR(e.g. to 1 or 2 min) and reconcile the changes in the config map with the next reconciliation. This is easier to implement, and we don't need a separation of the code depending on the build tag.
There is a new Interface for the ConfigResolver:
type ConfigResolver interface {
// Resolves istio operator config as a string.
Resolve() string, error
}
Add a new resolver that will have the new functionality and that is used when the build tag allow-custom-istio-operator-config
is set.
// +build experimental
package istio
type experimentalConfigResolver struct {
}
func NewConfigResolver() ConfigResolver {
return &experimentalConfigResolver{}
}
func (d *experimentalConfigResolver) Resolve() (string, error) {
// Resolving from custom istio operator from CM and as a fallback from default manifest and merge with Istio CR ")
return "istio operator config", nil
}
Add the default resolver with the existing functionality that is used when the build tag allow-custom-istio-operator-config
is not set.
// +build !experimental
package istio
type defaultConfigResolver struct {
}
func NewConfigResolver() ConfigResolver {
return &defaultConfigResolver{}
}
func (d *defaultConfigResolver) Resolve() (string, error) {
// Resolving from default manifest and merge with Istio CR
return "istio operator config", nil
}
type ConfigResolver interface {
// Resolves istio operator config as a string.
Resolve() (string, error)
}
We want to call the ConfigResolver as part of the Istio installation Reconcile and replace the current creation of the Istio operator manifest here.
The following implementation would be replace with a call to the resolver that returns the Istio Operator manifest. We can also make the ConfigResolver a field on the Installation
struct and initialise it there, it's an implementation detail.
r := NewConfigResolver()
istioOperatorManifest, err := r.Resolve();
if err != nil {
...
}
err = i.IstioClient.Install(istioOperatorManifest)
how avoid custom configuration rollback during Istio reconciliation
The resolving will be part of the normal reconciliation, just by replacing the retrieval of the Istio Operator config. Therefore should be no issue with rollbacks, since the config from the config map would be used as long as it exists.
image build separation (different controller?)
The separation is handled by the different implementations of the interface and build tags.
validation of custom configuration
We should not do any additional validation, but rely on the validation done by Istio installation.
So we came up with a POC results. With some help from @triffer I've come up with a working solution that is separated with build tags, as was proposed. The implementation is generally based on proposal above.
When manager is built with -tags=experimental
the experimental implementation of a resolver is used:
istio-system/custom-istio-operator
with a data under key istio-operator.yaml
To build manager with experimental feature enabled, use command:
go build -a -tags=experimental -o manager-experimental main.go
To run the manager:
make install
.kubectl create configmap -n istio-system custom-istio-operator --from-file=path/to/custom-istio-operator.yaml
./manager-experimental
config/samples/operator_v1alpha2_istio.yaml
I already see some drawbacks of this Impl:
-experimental
.wait-for-status
step in our pipeline, I'm afraid that we will hit the rate limits much more frequently. @kolodziejczak can elaborate more on that topic. This doesn't scale well.go build
I personally don't like the idea. It seems to be too hacky. This feature should be part of IstioCR and contain a nice and convenient interface for the customer to enable/disable some alpha functions for a price of no SLA. This way we still have control over istio operator CR, and have control over which alpha features are enabled.
Questions
Q: If the templating is dropped, how will the image version be handled?
There is also an open question regarding the Istio proxy sidecar restart behaviour. Since we allow to configure the whole Istio operator, we also allow to use different proxy sidecar images. In this case, we need to think about how the proxy sidecar restart will behave. One solution could be to dynamically create a restart predicate for this image and version and use it in the proxy sidecar restart.
After the meeting we came to some conclusions:
Things to consider:
Description Scope of this task is to figure out best way to enable users wanting to provide configuration that is not available via Istio CR. This functionality should be separated from managed offering. There should not be possibility to switch between experimental and managed offering during runtime.
Additional configuration is provided via CM that is observed by Istio manager. Strategy of applying should be replace.
Proposed solution should be low maintenance and utilise usage of experimental offering. implementation should be common for both ofeering but functionality should separated by image building.
Open questions:
DoD:
- [ ] Provide unit and integration tests.- [ ] If you changed the resource limits, explain why it was needed.- [ ] If the default configuration of Istio Operator has been changed, you performed a manual upgrade test to verify that the change can be rolled out correctly.- [ ] Verify that your contributions don't decrease code coverage. If they do, explain why this is the case.- [ ] Add release notes.Attachments part of: #669