Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote Development Mode docker permissions #41029

Merged

Conversation

vsevel
Copy link
Contributor

@vsevel vsevel commented Jun 6, 2024

Fixes #40502

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/platform Issues related to definition and interaction with Quarkus Platform labels Jun 6, 2024
@quarkus-bot

This comment has been minimized.

Copy link

github-actions bot commented Jun 6, 2024

🙈 The PR is closed and the preview is expired.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I added a question that I want us to sort out before merging. Feel free to push back if I'm confused :).

Comment on lines 86 to 89
# Remote Development Mode
ENV QUARKUS_LAUNCH_DEVMODE=true
RUN chmod o+rw -R /deployments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really something we want to include in the default Dockerfile?

I could be convinced to provide a specific remote dev mode Dockerfile if it has value but the default one should be as generic as possible and allowing the app to write in /deployment is not something we want if not in remote dev mode.

One option could be to comment it out but given they are very different use cases and you might need to use remote dev mode and then to build a container and it could be dangerous if you forgot to adjust it before building the prod container.

Copy link
Contributor Author

@vsevel vsevel Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowing the app to write in /deployment is not something we want if not in remote dev mode.

of course. I am glad you picked it up. and that would be a security risk. I meant to leave those 2 lines commented out, as options.

it could be dangerous if you forgot to adjust it before building the prod container.

regarding the second point, yes there is a risk that you would forget those in your real application. the only thing protecting us from that would be to have a separate dockerfile. but that seemed overkill to me. I guess we could add a precision in the comment (do not leave uncommented when building the prod container), and leave the responsibilty to the user. another more sophisticated way that we do not promote by default is generating the dockerfile based on properties you pick up from your pom, and/or a specific profile -Premote-dev. that way you could make sure that this section is only added when you explicitly add the specific profile.

also initially I only added the doc clarification. may be that is sufficient (and less risky).

finally I was not sure I needed to update extensions/container-image/container-image-docker-common/deployment/src/test/resources/ubi* files. I did it for consistency.

so what do you think?

  • doc only?
  • doc+qute template?
  • doc+qute template+ubi test files?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah so for me either we do doc only or we provide a separate Dockerfile. I'm not against the latter if you think it has value. Especially since if you are using remote dev, there's a good chance you will want two separate ones for the reason below.

we could add a precision in the comment (do not leave uncommented when building the prod container)

This is too dangerous. When you have to add this sort of comment, you can be sure it will go wrong for someone somewhere :).
Or even get pushed to Git because you were working on a large patch and it went unnoticed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think doc only is sufficient. I will make the change. thanks for the feedback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also added For security reasons, this option should not be added to the production docker file. in the doc.
and reverted the other changes.

@vsevel vsevel force-pushed the feature/remote_dev_mode_docker_permissions branch from 5d3a952 to 2a47b97 Compare June 6, 2024 14:49
@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 6, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 2a47b97.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/micrometer/deployment

io.quarkus.micrometer.deployment.binder.VertxTcpMetricsTest.testTcpMetrics - History

  • event executor terminated - java.util.concurrent.RejectedExecutionException
java.util.concurrent.RejectedExecutionException: event executor terminated
	at io.netty.util.concurrent.SingleThreadEventExecutor.reject(SingleThreadEventExecutor.java:934)
	at io.netty.util.concurrent.SingleThreadEventExecutor.offerTask(SingleThreadEventExecutor.java:351)
	at io.netty.util.concurrent.SingleThreadEventExecutor.addTask(SingleThreadEventExecutor.java:344)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:836)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute0(SingleThreadEventExecutor.java:827)
	at io.netty.util.concurrent.SingleThreadEventExecutor.execute(SingleThreadEventExecutor.java:817)
	at io.vertx.core.impl.EventLoopExecutor.execute(EventLoopExecutor.java:35)

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testFruits - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.kafka.KafkaConnectorTest expected: <6> but was: <5> within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.untilAsserted(ConditionFactory.java:790)
	at io.quarkus.it.kafka.KafkaConnectorTest.testFruits(KafkaConnectorTest.java:63)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@vsevel vsevel force-pushed the feature/remote_dev_mode_docker_permissions branch from 2a47b97 to 8831f06 Compare June 7, 2024 13:27
@quarkus-bot

This comment has been minimized.

@gsmet gsmet force-pushed the feature/remote_dev_mode_docker_permissions branch from 8831f06 to 5224f69 Compare June 11, 2024 16:42
@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes triage/backport labels Jun 11, 2024
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 11, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 5224f69.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gsmet gsmet merged commit ac69c08 into quarkusio:main Jun 11, 2024
5 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 11, 2024
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone Jun 11, 2024
@gsmet gsmet modified the milestones: 3.12 - main, 3.11.2 Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/platform Issues related to definition and interaction with Quarkus Platform kind/bugfix triage/flaky-test
Projects
Development

Successfully merging this pull request may close these issues.

Remote development mode: access denied on /deployments files
2 participants