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

Improve the generation init-tasks by database migration on Kubernetes/OpenShift #33409

Merged
merged 2 commits into from
May 26, 2023

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented May 16, 2023

This pull request includes the following changes:

  • Change default image for init container waiters to groundnuty/k8s-wait-for:no-root-v1.7 which is compatible with OpenShift because it's rootless
  • Allow users to disable the generation of an init task by specifying it by name.

Fix #33097

@quarkus-bot

This comment has been minimized.

@Sgitario
Copy link
Contributor Author

It seems that - Allow users to modify the init container image to use. needs to be done on Dekorate side, so I've removed the commit from here.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@@ -20,40 +21,47 @@

public class InitTaskProcessor {

private static final String INIT_CONTAINER_WAITER_NAME = "init";
private static final String INIT_CONTAINER_WAITER_DEFAULT_IMAGE = "groundnuty/k8s-wait-for:no-root-v1.7";
Copy link
Member

Choose a reason for hiding this comment

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

I know we used it before your patch but I didn't notice it. Are we sure using this image is safe? I'm a bit worried about it.

/cc @maxandersen @cescoffier

Copy link
Member

Choose a reason for hiding this comment

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

Never heard about it.... We need to have a deeper look.

The fact that it uses Alpine is not a great start...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since this image seems to be commonly used by the community for this purpose and also it was already used before these changes, I think the way to proceed is to open a new issue to investigate how secure it is and the alternatives.

@cescoffier @gsmet @iocanel do you think we can merge these changes that improve this existing feature or do you prefer put it hold off?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't hold off this pull request because of the image. We need to track it in a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

What image was used before ? Yeah. We should not be using/proposing alpine unless absolutely no other option.

What is the requirements for the image and why not use the base images we otherwise use here ? (That would mean 0 mb extra to fetch :)

Copy link
Contributor

@iocanel iocanel left a comment

Choose a reason for hiding this comment

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

LGTM

Added an optional suggestion. Which can be part of this or a follow up request.

Regarding the image, I think that it needs to be tracked / addressed by a different issue / pr.

@quarkus-bot
Copy link

quarkus-bot bot commented May 26, 2023

Failing Jobs - Building f982d5e

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: integration-tests/rest-client-reactive 

📦 integration-tests/rest-client-reactive

io.quarkus.it.rest.client.wronghost.ExternalWrongHostTestCase.restClientRejected line 28 - More details - Source on GitHub

java.lang.AssertionError: 
1 expectation failed.
Response body doesn't match expectation.

@Sgitario
Copy link
Contributor Author

The test failure is unrelated. Merging.

@Sgitario Sgitario merged commit c80fef6 into quarkusio:main May 26, 2023
@Sgitario Sgitario deleted the 33097_new branch May 26, 2023 14:12
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone May 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quarkus with liquibase on Kubernetes
5 participants