seldon-core icon indicating copy to clipboard operation
seldon-core copied to clipboard

Add Prefix for Kafka SSL Elements

Open stephen37 opened this issue 3 years ago • 4 comments

When someone is using Kafka over SSL for logging messages to Kafka or consuming messages from Kafka to run predictions, we need to add the different certificates needed. While the current implementation works well for that, it only works for a single cluster.

In some cases we want to consume messages from a topic in a specific cluster and then log the responses of the model in a different topic in another cluster, it is not possible for now as we except the certificates to be the same when consuming and logging.

By adding a prefix to the environment variables used to get the different certificates, we could make it possible to use multiple clusters.

I suggest we only change the function GetSslElements() https://github.com/SeldonIO/seldon-core/blob/master/executor/api/util/utils.go#L164-L178

Instead of having

func GetSslElements() *SslKakfa {
	sslElements := SslKakfa{
		ClientCert: GetEnv("KAFKA_SSL_CLIENT_CERT", ""),
		ClientKey:  GetEnv("KAFKA_SSL_CLIENT_KEY", ""),
		CACert:     GetEnv("KAFKA_SSL_CA_CERT", ""),
		// If we use path to files instead of string
		ClientCertFile: GetEnv("KAFKA_SSL_CLIENT_CERT_FILE", ""),
		ClientKeyFile:  GetEnv("KAFKA_SSL_CLIENT_KEY_FILE", ""),
		CACertFile:     GetEnv("KAFKA_SSL_CA_CERT_FILE", ""),
		// Optional password
		ClientKeyPass: GetEnv("KAFKA_SSL_CLIENT_KEY_PASS", ""),
	}
	return &sslElements

}

We could have

func CheckPrefixSSLLogger() bool {
    for _, envStr := range os.Environ() {
        if strings.HasPrefix(envStr, 'LOGGER') {
            return true
        }
    }
    return false
}

func GetSslElements() *SslKakfa {
    if CheckPrefixSSLLogger() { 
        sslElements := SslKakfa{
            ClientCert: GetEnv("LOGGER_KAFKA_SSL_CLIENT_CERT", ""),
	    ClientKey:  GetEnv("LOGGER_KAFKA_SSL_CLIENT_KEY", ""),
	    CACert:     GetEnv("LOGGER_KAFKA_SSL_CA_CERT", ""),
        }
        return &sslElements
    }
    if CheckPrefixSSLExecutor() {
        ...
        return &sslElements
    } 
    sslElements := SslKakfa{
        ClientCert: GetEnv("KAFKA_SSL_CLIENT_CERT", ""),
        ClientKey:  GetEnv("KAFKA_SSL_CLIENT_KEY", ""),
        CACert:     GetEnv("KAFKA_SSL_CA_CERT", ""),
        // If we use path to files instead of string
        ClientCertFile: GetEnv("KAFKA_SSL_CLIENT_CERT_FILE", ""),
        ClientKeyFile:  GetEnv("KAFKA_SSL_CLIENT_KEY_FILE", ""),
        CACertFile:     GetEnv("KAFKA_SSL_CA_CERT_FILE", ""),
        // Optional password
        ClientKeyPass: GetEnv("KAFKA_SSL_CLIENT_KEY_PASS", ""),
    }
    return &sslElements

}

N.B.: The code will be a bit different, it's only here to show what I have in mind.

I am available to write the code :D

stephen37 avatar Jun 22 '22 11:06 stephen37

This seems reasonable if we can make the current default easy so people don't need to specify both

ukclivecox avatar Jun 24 '22 07:06 ukclivecox

A PR would indeed be welcome!

ukclivecox avatar Jun 24 '22 07:06 ukclivecox

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

github-actions[bot] avatar Jul 28 '22 02:07 github-actions[bot]

Still planned, I've been struggling because of the M1 processor on my mac and the different dependencies but it's still on my backlog.

stephen37 avatar Jul 28 '22 07:07 stephen37

Any update if this is still a priority for you?

ukclivecox avatar Dec 19 '22 10:12 ukclivecox

Closing this. Please reopen if still needed.

ukclivecox avatar Mar 04 '23 10:03 ukclivecox