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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ public final class KubernetesInitContainerBuildItem extends MultiBuildItem {
private final boolean sharedEnvironment;
private final boolean sharedFilesystem;

public static KubernetesInitContainerBuildItem create(String image) {
return new KubernetesInitContainerBuildItem("init", null, image, Collections.emptyList(), Collections.emptyList(),
public static KubernetesInitContainerBuildItem create(String name, String image) {
return new KubernetesInitContainerBuildItem(name, null, image, Collections.emptyList(), Collections.emptyList(),
Collections.emptyMap(), false, false);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package io.quarkus.kubernetes.deployment;

import io.quarkus.runtime.annotations.ConfigGroup;
import io.quarkus.runtime.annotations.ConfigItem;

@ConfigGroup
public class InitTaskConfig {
/**
* If true, the init task will be generated. Otherwise, the init task resource generation will be skipped.
*/
@ConfigItem(defaultValue = "true")
public boolean enabled;

/**
* The init task image to use by the init-container.
*/
@ConfigItem(defaultValue = "groundnuty/k8s-wait-for:no-root-v1.7")
public String image;
}
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package io.quarkus.kubernetes.deployment;

import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;

import io.dekorate.kubernetes.config.EnvBuilder;
import io.dekorate.kubernetes.decorator.AddEnvVarDecorator;
Expand All @@ -20,48 +20,59 @@

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 :)


static void process(
String target, // kubernetes, openshift, etc.
String name,
ContainerImageInfoBuildItem image, List<InitTaskBuildItem> initTasks,
ContainerImageInfoBuildItem image,
List<InitTaskBuildItem> initTasks,
Map<String, InitTaskConfig> initTasksConfig,
BuildProducer<KubernetesJobBuildItem> jobs,
BuildProducer<KubernetesInitContainerBuildItem> initContainers,
BuildProducer<KubernetesEnvBuildItem> env,
BuildProducer<KubernetesRoleBuildItem> roles,
BuildProducer<KubernetesRoleBindingBuildItem> roleBindings,
BuildProducer<DecoratorBuildItem> decorators) {

initTasks.forEach(task -> {
initContainers.produce(KubernetesInitContainerBuildItem.create("groundnuty/k8s-wait-for:1.3")
.withTarget(target)
.withArguments(Arrays.asList("job", task.getName())));

jobs.produce(KubernetesJobBuildItem.create(image.getImage())
.withName(task.getName())
.withTarget(target)
.withEnvVars(task.getTaskEnvVars())
.withCommand(task.getCommand())
.withArguments(task.getArguments())
.withSharedEnvironment(task.isSharedEnvironment())
.withSharedFilesystem(task.isSharedFilesystem()));
boolean generateRoleForJobs = false;
for (InitTaskBuildItem task : initTasks) {
InitTaskConfig config = initTasksConfig.get(task.getName());
if (config == null || config.enabled) {
generateRoleForJobs = true;
jobs.produce(KubernetesJobBuildItem.create(image.getImage())
.withName(task.getName())
.withTarget(target)
.withEnvVars(task.getTaskEnvVars())
.withCommand(task.getCommand())
.withArguments(task.getArguments())
.withSharedEnvironment(task.isSharedEnvironment())
.withSharedFilesystem(task.isSharedFilesystem()));

task.getAppEnvVars().forEach((k, v) -> {
decorators.produce(new DecoratorBuildItem(target,
new AddEnvVarDecorator(ApplicationContainerDecorator.ANY, name, new EnvBuilder()
.withName(k)
.withValue(v)
.build())));
task.getAppEnvVars().forEach((k, v) -> {
decorators.produce(new DecoratorBuildItem(target,
new AddEnvVarDecorator(ApplicationContainerDecorator.ANY, name, new EnvBuilder()
.withName(k)
.withValue(v)
.build())));
});

});
initContainers.produce(KubernetesInitContainerBuildItem.create(INIT_CONTAINER_WAITER_NAME,
config == null ? INIT_CONTAINER_WAITER_DEFAULT_IMAGE : config.image)
.withTarget(target)
.withArguments(List.of("job", task.getName())));
}
}

if (generateRoleForJobs) {
roles.produce(new KubernetesRoleBuildItem("view-jobs", Collections.singletonList(
new PolicyRule(
Collections.singletonList("batch"),
Collections.singletonList("jobs"),
List.of("get"))),
target));
roleBindings.produce(new KubernetesRoleBindingBuildItem(null, "view-jobs", false, target));

});
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -340,11 +340,25 @@ public enum DeploymentResourceKind {
* Flag to enable init task externalization.
* When enabled (default), all initialization tasks
* created by extensions, will be externalized as Jobs.
* In addition the deployment will wait for these jobs.
* In addition, the deployment will wait for these jobs.
*
* @Deprecated use {@link #initTasks} configuration instead
*/
@Deprecated(since = "3.1", forRemoval = true)
@ConfigItem(defaultValue = "true")
boolean externalizeInit;

/**
* Init tasks configuration.
*
* The init tasks are automatically generated by extensions like Flyway to perform the database migration before staring
* up the application.
*
* This property is only taken into account if `quarkus.kubernetes.externalize-init` is true.
*/
@ConfigItem
Map<String, InitTaskConfig> initTasks;

/**
* Switch used to control whether non-idempotent fields are included in generated kubernetes resources to improve
* git-ops compatibility
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -568,11 +568,25 @@ public EnvVarsConfig getEnv() {
* Flag to enable init task externalization.
* When enabled (default), all initialization tasks
* created by extensions, will be externalized as Jobs.
* In addition the deployment will wait for these jobs.
* In addition, the deployment will wait for these jobs.
*
* @Deprecated use {@link #initTasks} configuration instead
*/
@Deprecated(since = "3.1", forRemoval = true)
@ConfigItem(defaultValue = "true")
boolean externalizeInit;

/**
* Init tasks configuration.
*
* The init tasks are automatically generated by extensions like Flyway to perform the database migration before staring
* up the application.
*
* This property is only taken into account if `quarkus.openshift.externalize-init` is true.
*/
@ConfigItem
Map<String, InitTaskConfig> initTasks;

/**
* Switch used to control whether non-idempotent fields are included in generated kubernetes resources to improve
* git-ops compatibility
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -384,8 +384,8 @@ void externalizeInitTasks(
BuildProducer<DecoratorBuildItem> decorators) {
final String name = ResourceNameUtil.getResourceName(config, applicationInfo);
if (config.externalizeInit) {
InitTaskProcessor.process(OPENSHIFT, name, image, initTasks, jobs, initContainers, env, roles, roleBindings,
decorators);
InitTaskProcessor.process(OPENSHIFT, name, image, initTasks, config.initTasks,
jobs, initContainers, env, roles, roleBindings, decorators);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -293,8 +293,8 @@ void externalizeInitTasks(
BuildProducer<DecoratorBuildItem> decorators) {
final String name = ResourceNameUtil.getResourceName(config, applicationInfo);
if (config.externalizeInit) {
InitTaskProcessor.process(KUBERNETES, name, image, initTasks, jobs, initContainers, env, roles, roleBindings,
decorators);
InitTaskProcessor.process(KUBERNETES, name, image, initTasks, config.initTasks,
jobs, initContainers, env, roles, roleBindings, decorators);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public void assertGeneratedResources() throws IOException {
assertThat(t.getSpec()).satisfies(podSpec -> {
assertThat(podSpec.getInitContainers()).singleElement().satisfies(container -> {
assertThat(container.getName()).isEqualTo("init");
assertThat(container.getImage()).isEqualTo("groundnuty/k8s-wait-for:no-root-v1.7");
});

});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package io.quarkus.it.kubernetes;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.io.IOException;
import java.nio.file.Path;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;

import org.jboss.shrinkwrap.api.ShrinkWrap;
import org.jboss.shrinkwrap.api.spec.JavaArchive;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.fabric8.kubernetes.api.model.apps.Deployment;
import io.fabric8.kubernetes.api.model.batch.v1.Job;
import io.fabric8.kubernetes.api.model.rbac.RoleBinding;
import io.quarkus.bootstrap.model.AppArtifact;
import io.quarkus.builder.Version;
import io.quarkus.test.ProdBuildResults;
import io.quarkus.test.ProdModeTestResults;
import io.quarkus.test.QuarkusProdModeTest;

public class KubernetesWithFlywayInitWithJobDisabledTest {

private static final String NAME = "kubernetes-with-flyway-with-job-disabled";

@RegisterExtension
static final QuarkusProdModeTest config = new QuarkusProdModeTest()
.setArchiveProducer(() -> ShrinkWrap.create(JavaArchive.class).addClasses(GreetingResource.class))
.setApplicationName(NAME)
.setApplicationVersion("0.1-SNAPSHOT")
.overrideConfigKey("quarkus.kubernetes.init-tasks.\"" + NAME + "-flyway-init\".enabled", "false")
.setForcedDependencies(Arrays.asList(
new AppArtifact("io.quarkus", "quarkus-kubernetes", Version.getVersion()),
new AppArtifact("io.quarkus", "quarkus-flyway", Version.getVersion())));

@ProdBuildResults
private ProdModeTestResults prodModeTestResults;

@Test
public void assertGeneratedResources() throws IOException {
final Path kubernetesDir = prodModeTestResults.getBuildDir().resolve("kubernetes");
assertThat(kubernetesDir)
.isDirectoryContaining(p -> p.getFileName().endsWith("kubernetes.json"))
.isDirectoryContaining(p -> p.getFileName().endsWith("kubernetes.yml"));
List<HasMetadata> kubernetesList = DeserializationUtil
.deserializeAsList(kubernetesDir.resolve("kubernetes.yml"));

Optional<Deployment> deployment = kubernetesList.stream()
.filter(d -> "Deployment".equals(d.getKind())
&& NAME.equals(d.getMetadata().getName()))
.map(d -> (Deployment) d).findAny();

assertTrue(deployment.isPresent());
assertThat(deployment).satisfies(j -> j.isPresent());
assertThat(deployment.get()).satisfies(d -> {
assertThat(d.getMetadata()).satisfies(m -> {
assertThat(m.getName()).isEqualTo(NAME);
});

assertThat(d.getSpec()).satisfies(deploymentSpec -> {
assertThat(deploymentSpec.getTemplate()).satisfies(t -> {
assertThat(t.getSpec()).satisfies(podSpec -> {
assertThat(podSpec.getInitContainers()).noneSatisfy(container -> {
assertThat(container.getName()).isEqualTo("init");
});
});
});
});
});

Optional<Job> job = kubernetesList.stream()
.filter(j -> "Job".equals(j.getKind()) && (NAME + "-flyway-init").equals(j.getMetadata().getName()))
.map(j -> (Job) j)
.findAny();
assertFalse(job.isPresent());

Optional<RoleBinding> roleBinding = kubernetesList.stream().filter(
r -> r instanceof RoleBinding && (NAME + "-view-jobs").equals(r.getMetadata().getName()))
.map(r -> (RoleBinding) r).findFirst();
assertFalse(roleBinding.isPresent());
}
}