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

Fix OpenShiftStrimziKafkaWithRegistryMessagingIT by filtering out invalid environment variables propagated to the OpenShift pod #1117

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented May 6, 2024

Summary

After quarkusio/quarkus#40225 we have seen a bit of change in the behavior. I suppose it is a right direction. We have following error:

Caused by: jakarta.enterprise.inject.spi.DeploymentException: java.lang.IllegalArgumentException: SRMSG00071: Invalid channel configuration -  the `connector` attribute must be set for channel `channel`
15:56:21,161 INFO  [app] 	at io.quarkus.smallrye.reactivemessaging.runtime.SmallRyeReactiveMessagingLifecycle.onApplicationStart(SmallRyeReactiveMessagingLifecycle.java:58)
15:56:21,162 INFO  [app] 	at io.quarkus.smallrye.reactivemessaging.runtime.SmallRyeReactiveMessagingLifecycle_Observer_onApplicationStart_qTrMuLFyQ1IvGfeSxRVitl6CCBQ.notify(Unknown Source)
15:56:21,162 INFO  [app] 	at io.quarkus.arc.impl.EventImpl$Notifier.notifyObservers(EventImpl.java:351)
15:56:21,163 INFO  [app] 	at io.quarkus.arc.impl.EventImpl$Notifier.notify(EventImpl.java:333)
15:56:21,163 INFO  [app] 	at io.quarkus.arc.impl.EventImpl.fire(EventImpl.java:80)
15:56:21,163 INFO  [app] 	at io.quarkus.arc.runtime.ArcRecorder.fireLifecycleEvent(ArcRecorder.java:155)
15:56:21,164 INFO  [app] 	at io.quarkus.arc.runtime.ArcRecorder.handleLifecycleEvents(ArcRecorder.java:106)
15:56:21,164 INFO  [app] 	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy_0(Unknown Source)
15:56:21,164 INFO  [app] 	at io.quarkus.deployment.steps.LifecycleEventsBuildStep$startupEvent1144526294.deploy(Unknown Source)

Which has been experienced in past quarkusio/quarkus#23383 and means that Quarkus recognized channel called channel without connector, however we did set this connector:

 - name: "MP_MESSAGING_INCOMING_CHANNEL_STOCK_PRICE_CONNECTOR"
   value: "smallrye-kafka"

defined inside pod env vars which is generated from mp.messaging.incoming.channel-stock-price.connector=smallrye-kafka set as system property. As you can see the expected name is channel-stock-price and not channel. This changed I guess because of how runtime properties are recorded. Correct generated name should be MP_MESSAGING_INCOMING__CHANNEL_STOCK_PRICE__CONNECTOR however even this format is not unambiguous.

That is why in the past I followed https://smallrye.io/smallrye-config/Main/config/environment-variables/ suggestion to put config properties also to the dotted config source with a lower priority. Problem is that we still set these "incorrect" environment properties even for non-Quarkus properties. For we know that Quarkus uses MP config implementation, but we don't know whether other projects doesn't consume environment variables directly.

Therefore we should let only Quarkus to propagate these dotted properties to the integrated libraries in their correct format. At least that's how I read it. I can be wrong, but this PR fixes FW OCP failure.

Please check the relevant options

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Dependency update
  • Refactoring
  • Breaking change (fix or feature that would cause existing functionality to change)
  • This change requires a documentation update
  • This change requires execution against OCP (use run tests phrase in comment)

Checklist:

  • Example scenarios has been updated / added
  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik
Copy link
Member Author

run tests

@michalvavrik michalvavrik marked this pull request as draft May 6, 2024 19:13
@michalvavrik michalvavrik force-pushed the feature/fix-ocp-kafka-registry-test-by-config-props branch from 21c07d7 to 3e05420 Compare May 6, 2024 19:58
@michalvavrik michalvavrik marked this pull request as ready for review May 6, 2024 19:59
@michalvavrik
Copy link
Member Author

run tests

@michalvavrik michalvavrik requested a review from rsvoboda May 7, 2024 08:16
@rsvoboda
Copy link
Member

rsvoboda commented May 7, 2024

Correct generated name should be MP_MESSAGING_INCOMING__CHANNEL_STOCK_PRICE__CONNECTOR however even this format is not unambiguous.

Is it MP Config spec issue or more SmallRye impl issue?

@michalvavrik
Copy link
Member Author

michalvavrik commented May 7, 2024

Correct generated name should be MP_MESSAGING_INCOMING__CHANNEL_STOCK_PRICE__CONNECTOR however even this format is not unambiguous.

Is it MP Config spec issue or more SmallRye impl issue?

The MP Config spec https://download.eclipse.org/microprofile/microprofile-config-3.1/microprofile-config-spec-3.1.pdf section 5.3.1. Environment Variables Mapping Rules says that Replace each character that is neither alphanumeric nor _ with. If you replace each character with the underscore, then you can you reliably revert it back? It's not one to one mapping.

I don't see this as an issue, in the past, instead of the dotted format I used double underscores and happily released FW just to find out that we have such ambiguous scenarios in TS. And https://smallrye.io/smallrye-config/Main/config/environment-variables/ says it clearly: Environment Variables format cannot represent the entire spectrum of common property names.

That said, this is just my interpretation of the issue, I can be wrong. All I can see written about this is quarkusio/quarkus#40225 (comment).

Copy link
Member

@rsvoboda rsvoboda left a comment

Choose a reason for hiding this comment

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

LGTM, this is probably the best we can do now.

@rsvoboda rsvoboda merged commit 5090252 into quarkus-qe:main May 7, 2024
10 checks passed
@michalvavrik michalvavrik deleted the feature/fix-ocp-kafka-registry-test-by-config-props branch May 7, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants