You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "fvaleri (via GitHub)" <gi...@apache.org> on 2024/11/06 16:38:21 UTC
[PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
fvaleri opened a new pull request, #17705:
URL: https://github.com/apache/kafka/pull/17705
This patch fixes an issue with the Docker image that fails when setting any env var.
```sh
$ docker run --rm -it -p 9092:9092 \
-e KAFKA_NUM_NETWORK_THREADS=4 \
apache/kafka:3.8.1
...
Exception in thread "main" org.apache.kafka.common.config.ConfigException: Missing required configuration `zookeeper.connect` which has no default value. at kafka.server.KafkaConfig.validateValues(KafkaConfig.scala:1232) at kafka.server.KafkaConfig.<init>(KafkaConfig.scala:1223) at kafka.server.KafkaConfig.<init>(KafkaConfig.scala:545) at kafka.tools.StorageTool$.$anonfun$execute$1(StorageTool.scala:72) at scala.Option.flatMap(Option.scala:283) at kafka.tools.StorageTool$.execute(StorageTool.scala:72) at kafka.tools.StorageTool$.main(StorageTool.scala:53) at kafka.docker.KafkaDockerWrapper$.main(KafkaDockerWrapper.scala:48) at kafka.docker.KafkaDockerWrapper.main(KafkaDockerWrapper.scala)
```
Basically, the KafkaDockerWrapper does not copy the default KRaft config before appending the user env vars as an override. The result is that you end up with a server.properties that contains only the env vars.
Additionally, if a user sets a custom empty config by mounting the wrong directory, the image silently runs the default configuration. This patch makes the image fail, so that the user can spot the error early on and correct the mount path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testPrepareServerConfigs` and `testPrepareServerConfigsWithoutMountedFile` are testing env var mechanism. Let me see if I can add a new one or improve them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testPrepareServerConfigs` and `testPrepareServerConfigsWithoutMountedFile` are testing env var mechanism. Let me see if I can add a new one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "VedarthConfluent (via GitHub)" <gi...@apache.org>.
VedarthConfluent commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2488517083
Hey @fvaleri
First of all thanks a lot for working on this. File Input is supposed to be optional. We can consider adding a message explicitly that file input is not being used when it is not detected.
> The env vars is appended to the default or user provided configuration file, so it can be a new configuration or an override of an existing one
- This approach is ideal for a scenario where the configs do not interfere or impact each other.
- Unfortunately in kafka there seems to be some configs that cannot be used together.
- Putting a default config file and allowing users to append on top of it may result in a scenario where user may not be aware of what the exact default configs are so may get surprised when a config related error is thrown.
- The ideal way to use env variable powered properties is via docker compose, which allows users to see all the configs that they actually need.
- In case the users want to have a common set of configs that they want to apply, they can use file input.
For example:-
If we use your changes as it is, then we will not be able to run the kafka server in Controller mode. The reason is that currently we use the server.properties file for default config, which assumes that the kafka server is supposed to be run in combined mode.
If we want to be able to append to the default properties, then we will need to make the container aware about what mode is it supposed to run on and use the appropriate default config. This will require significant changes to the behaviour of docker image, as currently the docker container is not aware of the properties and just passes them on to the server.
There always is a possibility that some user provided config might not work with the default config (cmiiw), so we will always be at risk of running into config errors if we append user configs over default configs.
We can consider an enhancement to remove the duplicated properties, but that is a separate issue. Even if we de-duplicate the configs, if default configs don't work with user provided configs, the error scenario will still occur.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testPrepareServerConfigs` and `testPrepareServerConfigsWithoutMountedFile` are testing env var mechanism.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "kagarwal06 (via GitHub)" <gi...@apache.org>.
kagarwal06 commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2485858962
>IMO these two phrases contradict each other
@fvaleri yes, the documentation is confusing. Instead it should have been
- **No properties provided:** Default configurations will be used.
- **Properties defined via file input only:** Default configurations will be ignored, and only the properties from the file will be used.
- **Properties defined via environment variables only:** Default configurations will be ignored, and only the properties from environment variables will be used.
- **Properties defined via both environment variables and file input:** Default configurations will be ignored. Properties from the file will be combined with those from environment variables, with environment variables taking precedence.
> but I don't think this is what users expect.
I agree that this may not provide the best user experience, but it helps eliminate confusion that could arise from default configurations when users provide their own configurations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2460266976
@mimaison fyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testGetServerConfigsFromEnv, testPrepareServerConfigs, testPrepareServerConfigsWithoutMountedFile, testPrepareServerConfigsWithoutEnvVariables` are also testing the env var config mechanism. Let me see if I can add a new one or improve them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
Sure. I'm going to add one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
Adding it back with the new behavior and a comment.
> I expect we should have a test for envVars, could you help add one?
I think the following tests are also testing the env var config mechanism. Let me see if I can add a new one or maybe just improve them.
- testGetServerConfigsFromEnv
- testPrepareServerConfigs
- testPrepareServerConfigsWithoutMountedFile
- testPrepareServerConfigsWithoutEnvVariables
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "showuon (via GitHub)" <gi...@apache.org>.
showuon commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1843368798
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
1. Why do we remove this test?
2. I expect we should have a test for envVars, could you help add one?
Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testGetServerConfigsFromEnv, testPrepareServerConfigs, are also testing the env var config mechanism. Let me see if I can add a new one or improve them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2488067362
@VedarthConfluent fyi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2490454037
@VedarthConfluent thanks so much for the explanation. Makes sense.
I'll close this PR and open a new one to improve the doc (readme). Hopefully we can avoid the confusion that led to this work, that was still useful.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2482649855
> we kept the current behaviour intentionally: [documentation link](https://github.com/apache/kafka/blob/trunk/docker/examples/README.md#running-on-default-configs)
@kagarwal06 in that document we first say:
> If any user provided config is provided, default configs will not be used.
and then we say:
> If properties are provided via environment variables only, default configs will be replaced by user provided properties.
IMO these two phrases contradict each other, because these env vars are user provided config. Current behavior seems in line with the first phrase, but I don't think this is what users expect. If I set any env var, now I'm also required to mount a config file with the rest ofthe configuration.
Can you clarify that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think the following tests are also testing the env var config mechanism. Let me see if I can add a new one or maybe just improve them.
- testGetServerConfigsFromEnv
- testPrepareServerConfigs
- testPrepareServerConfigsWithoutMountedFile
- testPrepareServerConfigsWithoutEnvVariables
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2486077040
> I can raise a PR to fix the documentation
That would be great. Thanks.
> I agree that this may not provide the best user experience, but it helps eliminate confusion that could arise from default configurations when users provide their own configurations.
Can you elaborate more on this? The env vars is appended to the default or user provided configuration file, so it can be a new configuration or an override of an existing one. What could be the problem with this approach? A better approach would be to implement replace logic, that would produce a clean configuration file (without duplicated properties).
Testcontainers seems to already support setting env vars on official Kafka images the way one would expect: https://github.com/mimaison/metrics-reporter/commit/78a7e4884d3d20c589d6798409d6e085a6666331#diff-2c4376994877e77f6cf5220a3603feeb95b19a458924486fe5b63c2c8150c8c2R39
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri closed pull request #17705: KAFKA-17953: Docker image fails when setting env vars
URL: https://github.com/apache/kafka/pull/17705
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think `testPrepareServerConfigsWithoutMountedFile` but the name is a bit misleading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #17705:
URL: https://github.com/apache/kafka/pull/17705#issuecomment-2482749013
Even [the example](https://github.com/apache/kafka/blob/trunk/docker/examples/README.md#using-environment-variables) is confusing and suggests we can pass a single value:
```
Command docker run --env CONFIG_NAME=CONFIG_VALUE -p 9092:9092 apache/kafka:latest can be used to provide environment variables to docker container
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-17953: Docker image fails when setting env vars [kafka]
Posted by "fvaleri (via GitHub)" <gi...@apache.org>.
fvaleri commented on code in PR #17705:
URL: https://github.com/apache/kafka/pull/17705#discussion_r1846123240
##########
core/src/test/scala/unit/kafka/docker/KafkaDockerWrapperTest.scala:
##########
@@ -109,25 +109,6 @@ class KafkaDockerWrapperTest {
assertEquals(expected, actual)
}
- @Test
- def testPrepareServerConfigsWithEmptyMountedFile(): Unit = {
Review Comment:
> Why do we remove this test?
This was testing the empty user config, where we were silently switching to default config. As explained in the patch description, I changed this behavior to make it fail so the user can quickly spot the configuration issue. Wdyt?
> I expect we should have a test for envVars, could you help add one?
I think the following tests are also testing the env var config mechanism. Let me see if I can add a new one or improve them.
- testGetServerConfigsFromEnv
- testPrepareServerConfigs
- testPrepareServerConfigsWithoutMountedFile
- testPrepareServerConfigsWithoutEnvVariables
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org