Extract route/ingress configuration into a specific config group #13068

glefloch · 2020-11-02T15:52:36Z

This branch introduce a new config group for route/ingress specific configuration.

To configure a route, we can use the following configuration:

quarkus.openshift.route.expose=true
quarkus.openshift.route.host=example.com
quarkus.openshift.route.secure=true

For kubernetes, an ingress can be configured using the same config group, such as:

quarkus.kubernetes.ingress.expose=true
quarkus.kubernetes.ingress.host=example.com
quarkus.kubernetes.ingress.secure=true

This will add the same dekorate properties as before.
The only feature in this branch is the secure property which is enable by default and add the following annotation:

kubernetes.io/tls-acme: true

close #12630

glefloch · 2020-11-02T15:54:27Z

If everything is ok, I will push a migration note in the wiki

geoand · 2020-11-02T15:57:42Z

Very nice, thanks!

@iocanel do you want to take a look?

@gsmet this probably interests you as well

gsmet · 2020-11-02T16:05:14Z

...etes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/ExpositionConfig.java

+     * If true, TLS will be enabled on the exposed service
+     */
+    @ConfigItem(defaultValue = "true")
+    boolean secure;


Any chance we could handle the annotations added to the route while we're at it?

At the moment, I added them globally but I think it makes sense to be able to define annotations specific to the route?

Note that I'm not an OpenShift expert so better check with @geoand and @iocanel if it makes sense :).

Can you remind me again what annotation you needed on the Route?

Looking at dekorate, I'm not sure we can configure annotation for a specific type of object. But may be wrong.
I added the kubernetes.io/tls-acme annotation to enable tls. But as mentionned by @gsmet in an other comment, enabling tls may have more consequences than just adding an annotation (like certificate, key, ...)

So I needed an annotation for defining the secure route so it's mostly covered now :).

It does make sense!
And dekorate does support adding Labels and Annotations to specific resource types. See: dekorateio/dekorate#644

gsmet · 2020-11-02T16:07:13Z

.../vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesConfigUtil.java

+                    expositionConfig.host.ifPresent(
+                            host -> unPrefixed.put(DEKORATE_PREFIX + platformConfiguration.getTargetPlatformName() + ".host",
+                                    host));
+                    if (expositionConfig.secure) {


This doesn't have more consequences? Asking that because it's a specific checkbox in the UI.

Asking because I remember trying to define the annotation afterwards and things didn't work and George recreated the route entirely and it worked. So I wonder if secure is more than just that.

Yes that's possible. There is not much about tls in dekorate neither, maybe that would be a good use case. WDYT @iocanel?

AFAIR, the particular file is meant to be used to provide support for the old style configuration. So, I think that we can entirely ignore that for this pull request. In fact, I think we have it long enough that we should probably get rid of it soon.

gsmet · 2020-11-02T16:09:48Z

...etes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesConfig.java

@@ -92,6 +92,8 @@

    /**
     * The host under which the application is going to be exposed
+     * 
+     * @deprecated Use the {@code quarkus.kubernetes.ingress.host} instead


I wonder if we should try to add an automated warning when using deprecated configuration. Similar to the one we have for unused properties.

IMO, that's food for thoughts for later improvements but as for this PR is concerned, I wonder if we should at least add a warning when using these old configurations properties? Wondering also if we should handle the case when there is a conflict? I think the new ones should be prioritized but we should probably have a warning too.

I updated the code such as, the old properties are read only if new ones are not set. and in that case, there is a warning.

iocanel

If I understand things correctly the pull requests, implements the feature via the KubernetsConfigUtil. This utility is mostly used to for backwards compatibility reasons and is likely to be removed in the future.

The recommended approach to functionality like this is by using:

KubernetesProcessor
OpenshiftProcessor

and provide the necessary Configuratiors and Decroators.

For example: I would use a Configurator to set the expose and host and possibly the labels.

If the difference between configurators and decorators is not obvious. The former modifies the Configuration object we pass to decroate. The latter acts directly on the generated resources.

iocanel · 2020-11-12T14:18:30Z

docs/src/main/asciidoc/deploying-to-kubernetes.adoc

@@ -611,7 +610,9 @@ The table below describe all the available configuration options.
 | quarkus.kubernetes.liveness-probe                  | Probe                                     |             | ( see Probe )
 | quarkus.kubernetes.readiness-probe                 | Probe                                     |             | ( see Probe )
 | quarkus.kubernetes.sidecars                        | Map<String, Container>                    |             |
-| quarkus.kubernetes.expose                          | boolean                                   |             | false


We might want to keep this around for backwards compatibility reasons.
Don't have a very strong opinion about it. It`s what we use to do in related cases.

Never mind, just noticed that its only removed from the documentation.

iocanel · 2020-11-12T14:21:22Z

...etes/vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/ExpositionConfig.java

+     * If true, TLS will be enabled on the exposed service
+     */
+    @ConfigItem(defaultValue = "true")
+    boolean secure;


It does make sense!
And dekorate does support adding Labels and Annotations to specific resource types. See: dekorateio/dekorate#644

iocanel · 2020-11-12T14:27:48Z

.../vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesConfigUtil.java

+                    expositionConfig.host.ifPresent(
+                            host -> unPrefixed.put(DEKORATE_PREFIX + platformConfiguration.getTargetPlatformName() + ".host",
+                                    host));
+                    if (expositionConfig.secure) {


AFAIR, the particular file is meant to be used to provide support for the old style configuration. So, I think that we can entirely ignore that for this pull request. In fact, I think we have it long enough that we should probably get rid of it soon.

iocanel · 2020-11-12T14:28:04Z

.../vanilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesConfigUtil.java

        for (String generator : EXPOSABLE_GENERATORS) {
            boolean unprefixedExpose = config.getOptionalValue(generator + "." + EXPOSE_PROPERTY_NAME, Boolean.class)
                    .orElse(false);
            boolean prefixedExpose = config
                    .getOptionalValue(QUARKUS_PREFIX + generator + "." + EXPOSE_PROPERTY_NAME, Boolean.class)
                    .orElse(false);
            if (unprefixedExpose || prefixedExpose) {
+                if (generator == KUBERNETES) {
+                    log.warn("Usage of quarkus.kubernetes.expose is deprecated in favor of quarkus.kubernetes.ingress.expose");


glefloch · 2020-11-12T20:30:06Z

Thanks for your review @iocanel, I will update the code.

glefloch · 2020-11-16T13:17:56Z

@iocanel I pushed a new version of the code. As you suggested, I implemented a custom Configurator to handle the expose and host.
And I used a decorator to handle route/ingress specific annotation.

glefloch · 2020-11-23T09:24:02Z

@iocanel could you review the new version of the code?

glefloch · 2020-12-14T14:49:02Z

@iocanel I just fixed conflicts, could you have a look ?

geoand · 2021-02-26T10:38:17Z

I would like to have this fixed.

@glefloch can you rebase please so @iocanel can take another look?

Thanks

glefloch · 2021-02-26T11:12:15Z

Yes for sure @geoand. It should be good now :)

geoand · 2021-02-26T11:16:55Z

Thanks!

iocanel

LGTM

geoand · 2021-02-26T11:41:27Z

...anilla/deployment/src/main/java/io/quarkus/kubernetes/deployment/KubernetesCommonHelper.java

-import static io.quarkus.kubernetes.deployment.Constants.QUARKUS_ANNOTATIONS_BUILD_TIMESTAMP;
-import static io.quarkus.kubernetes.deployment.Constants.QUARKUS_ANNOTATIONS_COMMIT_ID;
-import static io.quarkus.kubernetes.deployment.Constants.QUARKUS_ANNOTATIONS_VCS_URL;
+import static io.quarkus.kubernetes.deployment.Constants.*;


Can we have the star imports removed please?

yes, and I reconfigured my IDE so that should not happen again.

Excellent, thanks 👍

glefloch · 2021-03-01T12:30:38Z

@geoand should we merge this ?

geoand · 2021-03-01T12:33:35Z

Absolutely!

glefloch requested review from geoand, iocanel and gsmet and removed request for geoand November 2, 2020 15:52

gsmet reviewed Nov 2, 2020

View reviewed changes

glefloch force-pushed the fix/12630 branch from c5ac630 to de0ec01 Compare November 3, 2020 08:26

iocanel requested changes Nov 12, 2020

View reviewed changes

glefloch force-pushed the fix/12630 branch from de0ec01 to e1c09bd Compare November 16, 2020 13:16

ghost added area/documentation area/kubernetes labels Nov 16, 2020

glefloch force-pushed the fix/12630 branch 3 times, most recently from 6caecc2 to f787595 Compare November 16, 2020 13:54

glefloch force-pushed the fix/12630 branch from f787595 to 76103c0 Compare December 14, 2020 14:48

glefloch force-pushed the fix/12630 branch from 76103c0 to 9273085 Compare February 26, 2021 11:11

geoand requested a review from iocanel February 26, 2021 11:17

iocanel approved these changes Feb 26, 2021

View reviewed changes

geoand reviewed Feb 26, 2021

View reviewed changes

glefloch force-pushed the fix/12630 branch from 9273085 to 736fcce Compare February 26, 2021 12:05

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 26, 2021

Extract route/ingress configuration into a specific object

8068cb2

glefloch force-pushed the fix/12630 branch from 736fcce to 8068cb2 Compare February 26, 2021 12:14

geoand merged commit db322d5 into quarkusio:master Mar 1, 2021

quarkus-bot bot added this to the 1.13 - master milestone Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract route/ingress configuration into a specific config group #13068

Extract route/ingress configuration into a specific config group #13068

glefloch commented Nov 2, 2020

glefloch commented Nov 2, 2020

geoand commented Nov 2, 2020

gsmet Nov 2, 2020

geoand Nov 2, 2020

glefloch Nov 2, 2020

gsmet Nov 2, 2020

iocanel Nov 12, 2020

gsmet Nov 2, 2020

gsmet Nov 2, 2020

glefloch Nov 2, 2020

iocanel Nov 12, 2020

gsmet Nov 2, 2020

glefloch Nov 3, 2020

iocanel left a comment

iocanel Nov 12, 2020

iocanel Nov 12, 2020

iocanel Nov 12, 2020

iocanel Nov 12, 2020

iocanel Nov 12, 2020

glefloch commented Nov 12, 2020

glefloch commented Nov 16, 2020

glefloch commented Nov 23, 2020

glefloch commented Dec 14, 2020

geoand commented Feb 26, 2021

glefloch commented Feb 26, 2021

geoand commented Feb 26, 2021

iocanel left a comment

geoand Feb 26, 2021

glefloch Feb 26, 2021 •

edited

Loading

geoand Feb 26, 2021

glefloch commented Mar 1, 2021

geoand commented Mar 1, 2021

Extract route/ingress configuration into a specific config group #13068

Extract route/ingress configuration into a specific config group #13068

Conversation

glefloch commented Nov 2, 2020

glefloch commented Nov 2, 2020

geoand commented Nov 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iocanel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefloch commented Nov 12, 2020

glefloch commented Nov 16, 2020

glefloch commented Nov 23, 2020

glefloch commented Dec 14, 2020

geoand commented Feb 26, 2021

glefloch commented Feb 26, 2021

geoand commented Feb 26, 2021

iocanel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefloch Feb 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glefloch commented Mar 1, 2021

geoand commented Mar 1, 2021

glefloch Feb 26, 2021 •

edited

Loading