Support self provisioned ES in streaming strategy

[pavolloffay]

Signed-off-by: Pavol Loffay ploffay@redhat.com

[pavolloffay]

@kevinearls could you please create a jira to run this in the internal build?

[kevinearls]

@pavolloffay I've added a note to TRACING-806 to check once this is merged. I think it will cover that case.

[jpkrohling]

This doesn't seem to belong here... We can't expect to add a cassandra, badger, in-memory to the strategies. There has to be a better way to do this.

[pavolloffay]

The main motivation is to set path to cert generation scrip. Tests use a different path than binary.

[jpkrohling]

Can this be done via viper, or added to the context? Sounds a bit outside of the scope to change the strategy signature just to be able to get some information internal to the auto-provisioning of one of the storages...

[pavolloffay]

So when creating the self-provisioned ES we need two things: path to cert generation script and more importantly secretes from the environment.

The path could be done via viper (although there are downsides of using viper in the impl packages, it's cleaner to pass params directly if we are talking about a package down in the stack).

Anyways the production strategy is using exactly the same construction.

[jpkrohling]

It sounds reasonable to me to require access to a configuration value (via viper or not) if the path to the cert generation is required. In fact, we are using viper already in this source for other purposes:

jaeger-operator/pkg/strategy/streaming.go

Lines 90 to 98 in 5999ea6

	if viper.GetString("platform") == v1.FlagPlatformOpenShift {
	if q := route.NewQueryRoute(jaeger).Get(); nil != q {
	manifest.routes = append(manifest.routes, *q)
	}
	} else {
	if q := ingress.NewQueryIngress(jaeger).Get(); nil != q {
	manifest.ingresses = append(manifest.ingresses, *q)
	}
	}

[jpkrohling]

Can't they be mounted in the file system directly from the secrets, like it's done in the Kafka auto-provisioning?

jaeger-operator/pkg/strategy/streaming.go

Lines 145 to 173 in 5999ea6

	kuVolume := corev1.Volume{
	Name: fmt.Sprintf("kafkauser-%s", ku.Name),
	VolumeSource: corev1.VolumeSource{
	Secret: &corev1.SecretVolumeSource{
	SecretName: ku.Name,
	},
	},
	}
	// this is the volume containing the CA cluster cert
	kuCAVolume := corev1.Volume{
	Name: fmt.Sprintf("kafkauser-%s-cluster-ca", jaeger.Name), // the cluster name is the jaeger name
	VolumeSource: corev1.VolumeSource{
	Secret: &corev1.SecretVolumeSource{
	SecretName: fmt.Sprintf("%s-cluster-ca-cert", jaeger.Name),
	},
	},
	}
	commonSpec.Volumes = append(commonSpec.Volumes, kuVolume, kuCAVolume)
	// and finally, the mount paths to have the secrets in the container file system
	kuVolumeMount := corev1.VolumeMount{
	Name: fmt.Sprintf("kafkauser-%s", ku.Name),
	MountPath: clientCertPath,
	}
	kuCAVolumeMount := corev1.VolumeMount{
	Name: fmt.Sprintf("kafkauser-%s-cluster-ca", jaeger.Name), // the cluster name is the jaeger name
	MountPath: clusterCAPath,
	}
	commonSpec.VolumeMounts = append(commonSpec.VolumeMounts, kuVolumeMount, kuCAVolumeMount)

[pavolloffay]

They are mounted to Jaeger filesystem. But for the use case I am mentioned we need certs in the operator filesystem and not Jaeger instance (see the operator in my previous comment). The cert generation script then assures certs are valid and not expired...

[jpkrohling]

But for the use case I am mentioned we need certs in the operator filesystem and not Jaeger instance (see the operator in my previous comment). The cert generation script then assures certs are valid and not expired...

This is another signal that there's a mismatch between things that should be operator-scoped and operand-scoped. Renewal of the certs, as well as creating/managing the secrets are operations that should be made by the operator independent of the reconciliation logic. The reconciliation logic should only be about the operand.

[pavolloffay]

Renewal of the certs, as well as creating/managing the secrets are operations that should be made by the operator independent of the reconciliation logic.

Why handling objects/dependencie/whatever is required for the operand part of the reconciliation? My understanding of reconciliation logic is to prepare objects/dependencie/whatever for the operand.

[jpkrohling]

I don't know, you said it yourself, but I probably misunderstood what you meant...

IIRC the certs are copied to operator filesystem in case of operator restart (or rescheduling to another node?)

[jpkrohling]

Most of this new code seems to be duplicated from the production strategy. Can't we find a way to reuse it? Duplicating a couple of lines is fine, but this is a bit more than just a couple...

jaeger-operator/pkg/strategy/production.go

Lines 106 to 126 in 5999ea6

	if storage.ShouldDeployElasticsearch(jaeger.Spec.Storage) {
	err := es.CreateCerts()
	if err != nil {
	jaeger.Logger().WithError(err).Error("failed to create Elasticsearch certificates, Elasticsearch won't be deployed")
	} else {
	c.secrets = es.ExtractSecrets()
	c.elasticsearches = append(c.elasticsearches, *es.Elasticsearch())
	es.InjectStorageConfiguration(&queryDep.Spec.Template.Spec)
	es.InjectStorageConfiguration(&cDep.Spec.Template.Spec)
	if indexCleaner != nil {
	es.InjectSecretsConfiguration(&indexCleaner.Spec.JobTemplate.Spec.Template.Spec)
	}
	for i := range esRollover {
	es.InjectSecretsConfiguration(&esRollover[i].Spec.JobTemplate.Spec.Template.Spec)
	}
	for i := range c.dependencies {
	es.InjectSecretsConfiguration(&c.dependencies[i].Spec.Template.Spec)
	}
	}
	}

[pavolloffay]

I can extract this to a func, but anyways the whole streaming strategy is mostly copy of production strategy with added ingester.

[jpkrohling]

I can understand the streaming strategy code being very similar to the production when it comes to the assembled objects (deployments, services, ...), as the final set of objects is quite similar, but the logic around the self-provisioning of ES could be abstracted away. For instance, here's how the auto-provisioning of Kafka happens:

jaeger-operator/pkg/strategy/streaming.go

Lines 61 to 66 in 5999ea6

	// we provision a Kafka when no brokers have been set, or, when we are not in the first run,
	// when we know we've been the ones placing the broker information in the configuration
	if (!pfound && !cfound) || provisioned {
	jaeger.Logger().Info("Provisioning Kafka, this might take a while...")
	manifest = autoProvisionKafka(ctx, jaeger, manifest)
	}

The strategy itself needs to know only whether the provisioning should happen. The auto-provision procedure will accept a manifest, and return a modified manifest, which might or not contain the Kafka objects required to do the provisioning.

[pavolloffay]

I have moved it to a shared func.

[pavolloffay]

@jpkrohling I have moved the ES cert generation to jaeger controller as discussed on the call. Please have a look. If that is fine I will run e2e tests on OCP before merging.

[jpkrohling]

LGTM!

[jpkrohling]

s/secretes/secrets

[jpkrohling]

Was it placed here by make format?

[pavolloffay]

yes

It either places it after core imports or after 3rd party imports in a separate block. However when I put it into operator imports it leaves it there.