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

Add support to prefix for envFrom #39782

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

mcruzdev
Copy link
Contributor

@mcruzdev mcruzdev commented Mar 29, 2024

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/kubernetes labels Mar 29, 2024
@mcruzdev mcruzdev marked this pull request as ready for review March 29, 2024 12:37
@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch from d5a146f to 1c38724 Compare March 29, 2024 12:38

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@gsmet gsmet marked this pull request as draft April 2, 2024 08:47
@gsmet gsmet requested a review from iocanel April 2, 2024 08:47
@gsmet
Copy link
Member

gsmet commented Apr 2, 2024

Converting to draft as it's using a snapshot version.

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch from 03196a5 to 3481619 Compare April 11, 2024 23:18
@mcruzdev mcruzdev marked this pull request as ready for review April 11, 2024 23:18
@mcruzdev
Copy link
Contributor Author

Hi @gsmet I think that we can review now, thank you @iocanel for release the dekorate version :)

This comment has been minimized.

This comment has been minimized.

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch 2 times, most recently from b91d285 to 364d6ac Compare April 12, 2024 02:28

This comment has been minimized.

This comment has been minimized.

@mcruzdev
Copy link
Contributor Author

I will fix the import sort soon...

This comment has been minimized.

This comment has been minimized.

Copy link

github-actions bot commented Apr 13, 2024

🙈 The PR is closed and the preview is expired.

@@ -164,7 +164,7 @@
<kotlin.coroutine.version>1.8.0</kotlin.coroutine.version>
<azure.toolkit-lib.version>0.27.0</azure.toolkit-lib.version>
<kotlin-serialization.version>1.6.2</kotlin-serialization.version>
<dekorate.version>4.1.2</dekorate.version> <!-- Please check with Java Operator SDK team before updating -->
<dekorate.version>4.1.3</dekorate.version> <!-- Please check with Java Operator SDK team before updating -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gsmet, should this change be on separated pull request?

Copy link
Contributor

Choose a reason for hiding this comment

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

@metacosm I assume the version bump is fine with you?

Copy link
Contributor

@metacosm metacosm Jun 21, 2024

Choose a reason for hiding this comment

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

I actually don't know and didn't get a chance to check… That version of dekorate uses an older sundrio version than the one used in the Kubernetes client and I have no idea about compatibility. I would have preferred this upgrade to be isolated from this PR if at all possible and be consulted before merging it, as is mentioned in the POM file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough.

When you get please chance, please check the version and see if everything works as expected.

Thanks!

Copy link
Contributor Author

@mcruzdev mcruzdev Jun 21, 2024

Choose a reason for hiding this comment

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

This was necessary because dekorate did not support prefix for env, I had to implement this on the dekorate side.

dekorateio/dekorate#1289

Without this version, this PR breaks.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@metacosm
Copy link
Contributor

Hi @metacosm, to do this one will be necessary to change the EnvVarsConfig POJO.

Removing the List<String> secrets and adding something like mapping Map<String, EnvVarFromKeyConfig> mapping;.

The secrets will need a change:

From: quarkus.kubernetes.env.secrets=my-secret,my-other-secret to:

quarkus.kubernetes.env.secrets.my-secret.name=my-secret
quarkus.kubernetes.env.secrets.my-secret.prefix=DBZ_

or you can follow what @iocanel suggested!

Let me know if I am wrong 😃

You're right.

Maybe we could use the mapping configuration to our advantage here, though, with something like:

quarkus.kubernetes.env.mapping."DB1_".as-prefix-for-secret=my-secret
quarkus.kubernetes.env.mapping."CM1_".as-prefix-for-configmap=my-cm

@mcruzdev
Copy link
Contributor Author

mcruzdev commented Jun 17, 2024

Hi @metacosm Sorry for delay... I am back now :)

I think that using this one:

quarkus.kubernetes.env.mapping."DB1_".as-prefix-for-secret=my-secret

We need to set the with-key property to use, like this:

quarkus.kubernetes.env.mapping."DB1_".with-key=secret-key-here

It means that to set the prefix we need to set the with-key. two properties to set the prefix, what do you think about @iocanel suggestion?

quarkus.kubernetes.env.prefix."MY_PREFIX_".fromSecret=my-secret

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch 2 times, most recently from 01a5444 to ad70cb1 Compare June 17, 2024 23:58

This comment has been minimized.

This comment has been minimized.

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch from ad70cb1 to 69dc609 Compare June 18, 2024 10:22
@mcruzdev mcruzdev requested a review from iocanel June 18, 2024 10:22

This comment has been minimized.

@metacosm
Copy link
Contributor

It means that to set the prefix we need to set the with-key. two properties to set the prefix, what do you think about @iocanel suggestion?

You're right again :)

quarkus.kubernetes.env.prefix."MY_PREFIX_".fromSecret=my-secret

I guess that's indeed the best solution but then I'd do:

quarkus.kubernetes.env.prefixes."MY_PREFIX_".for-secret=my-secret

If we wanted to be more fluent, we could do:

quarkus.kubernetes.env.using."MY_PREFIX_".as-prefix-for-secret=my-secret

but I guess that would be less legible on the ConfigGroup in the code…

@mcruzdev
Copy link
Contributor Author

mcruzdev commented Jun 18, 2024

I loved the last sample!

quarkus.kubernetes.env.using."MY_PREFIX_".as-prefix-for-secret

But as I said, I think that:

quarkus.kubernetes.env.prefixes."MY_PREFIX_".for-secret=my-secret

Is better for both (customer and code)

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch 2 times, most recently from 18473d9 to 31e87fa Compare June 18, 2024 15:19

This comment has been minimized.

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch 2 times, most recently from a8acf83 to cf2872a Compare June 18, 2024 17:15

This comment has been minimized.

This comment has been minimized.

@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch from cf2872a to 3c8c451 Compare June 20, 2024 15:04
@mcruzdev mcruzdev force-pushed the feature/fromEnvPrefix branch from 3c8c451 to 58575e7 Compare June 20, 2024 15:05
Copy link

quarkus-bot bot commented Jun 20, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 58575e7.

✅ 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 21

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

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/oidc-client-wiremock

io.quarkus.it.keycloak.OidcClientTest.testEchoAndRefreshTokensWithConcurrency - History

  • Assertion condition defined as a Lambda expression in io.quarkus.it.keycloak.OidcClientTest null within 2 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a Lambda expression in io.quarkus.it.keycloak.OidcClientTest null within 2 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.keycloak.OidcClientTest.testEchoAndRefreshTokensWithConcurrency(OidcClientTest.java:125)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

@geoand
Copy link
Contributor

geoand commented Jun 21, 2024

@iocanel can you have a look at this?

Thanks

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

@iocanel iocanel merged commit fab0ec5 into quarkusio:main Jun 21, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.13 - main milestone Jun 21, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/documentation area/kubernetes kind/enhancement New feature or request triage/flaky-test
Projects
Development

Successfully merging this pull request may close these issues.

Kubernetes extension: Support env vars from secrets with a prefix
5 participants