You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "squakez (via GitHub)" <gi...@apache.org> on 2024/08/09 09:44:16 UTC
[PR] feat(core): auto cloud native properties [camel]
squakez opened a new pull request, #15063:
URL: https://github.com/apache/camel/pull/15063
Introduced camel.main.cloudPropertiesLocation which, if exists, would look into the related csv directories to find properties as possibly set in cloud native environments such as Kubernetes.
Closes CAMEL-21036
# Description
<!--
- Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
-->
# Target
- [ ] I checked that the commit is targeting the correct branch (note that Camel 3 uses `camel-3.x`, whereas Camel 4 uses the `main` branch)
# Tracking
- [ ] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it).
<!--
# *Note*: trivial changes like, typos, minor documentation fixes and other small items do not require a JIRA issue. In this case your pull request should address just this issue, without pulling in other changes.
-->
# Apache Camel coding standards and style
- [ ] I checked that each commit in the pull request has a meaningful subject line and body.
<!--
If you're unsure, you can format the pull request title like `[CAMEL-XXX] Fixes bug in camel-file component`, where you replace `CAMEL-XXX` with the appropriate JIRA issue.
-->
- [ ] I have run `mvn clean install -DskipTests` locally and I have committed all auto-generated changes
<!--
You can run the aforementioned command in your module so that the build auto-formats your code. This will also be verified as part of the checks and your PR may be rejected if if there are uncommited changes after running `mvn clean install -DskipTests`.
You can learn more about the contribution guidelines at https://github.com/apache/camel/blob/main/CONTRIBUTING.md
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2284123177
Okay we can document this more clearly that the scan is intended for text based properties
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1713876642
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,44 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+ final OrderedLocationProperties cp = new OrderedLocationProperties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(loc, file.getFileName().toString(), val);
+ } catch (IOException e) {
+ LOG.warn("Some error happened while reading property from cloud configuration file {}",
+ file, e);
+ e.printStackTrace();
Review Comment:
Remove this the stacktrace is already logged in the WARN
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1711237173
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,43 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+
+ final Properties cp = new Properties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(file.getFileName().toString(), val);
+ } catch (IOException e) {
+ // Do nothing or should we throw a RuntimeException?
Review Comment:
No, there is no easy way to check that. However, given the user is setting this configuration on purpose, should be aware of what he/she is doing. As it's an auto configuration I'd be more inclined to just skip the failing configuration, maybe adding a log line to report that.
The file name is not expected. Typically, the file name is straight from the secret/configmap key. In the unit test I had to add a .txt as there was some maven process running validation and looking for licenses. However, it should work correctly without file extension.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2277704201
Users may have binary certificates as configmaps so I wonder if there should be a xxxBinary option so you can use another folder for binary files.
Or some other option where you can list the file names that are binary
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1711238528
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,43 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+
+ final Properties cp = new Properties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(file.getFileName().toString(), val);
+ } catch (IOException e) {
+ // Do nothing or should we throw a RuntimeException?
+ }
+ }
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ }
+ }
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (overridProperties == null) {
+ return cp;
+ }
+ Properties mergedProperties = new Properties(overridProperties);
Review Comment:
I'll have a look at this then, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1711161864
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,43 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+
+ final Properties cp = new Properties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(file.getFileName().toString(), val);
+ } catch (IOException e) {
+ // Do nothing or should we throw a RuntimeException?
Review Comment:
I am not sure if we should throw an exception here. This may happen for instance if the user is setting a binary configuration.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2284079844
I made some test with some binary file, and it does not throw any exception. The presence of any binary file would be parsed normally, although it would not be probably something useful to have into app properties. The exception throwing is a generic one provided in all file IO signatures.
However, this is something that should not be expected. In order to enable this feature, the op team needs to mount any given secret/configmap in the application pod in a given folder (ie, /etc/camel/conf.d/_configmaps) and enable the parsing via setting `camel.main.cloud-properties-location= /etc/camel/conf.d/_configmaps` setting. I'd expect they mount the binary certificate under any different location if they don't want the property to be scanned as an application properties. Anything they are eventually mounting in those folders is because they want the runtime to scan as a property.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1711238220
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -415,6 +421,15 @@ protected void configurePropertiesService(CamelContext camelContext) throws Exce
if (op != null) {
pc.setOverrideProperties(op);
}
+
+ Optional<String> cloudLocations = pc.resolveProperty(MainConstants.CLOUD_PROPERTIES_LOCATION);
+ if (cloudLocations.isPresent()) {
+ LOG.info("Cloud properties location: {}", cloudLocations);
+ final Properties kp = tryLoadCloudProperties(op, cloudLocations.get());
+ if (kp != null) {
+ pc.setOverrideProperties(kp);
Review Comment:
The `tryLoadCloudProperties` is taking that overrideProperties and merge it, so, we should be safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2284218734
Not sure to understand this error:
```
There are uncommitted changes
HEAD detached at pull/15063/merge
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git restore <file>..." to discard changes in working directory)
modified: catalog/camel-catalog/src/generated/resources/org/apache/camel/catalog/main/camel-main-configuration-metadata.json
no changes added to commit (use "git add" and/or "git commit -a")
```
Is that file something I have to skip to pass the validation by any chance?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on code in PR #15063:
URL: https://github.com/apache/camel/pull/15063#discussion_r1711193275
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -415,6 +421,15 @@ protected void configurePropertiesService(CamelContext camelContext) throws Exce
if (op != null) {
pc.setOverrideProperties(op);
}
+
+ Optional<String> cloudLocations = pc.resolveProperty(MainConstants.CLOUD_PROPERTIES_LOCATION);
+ if (cloudLocations.isPresent()) {
+ LOG.info("Cloud properties location: {}", cloudLocations);
+ final Properties kp = tryLoadCloudProperties(op, cloudLocations.get());
+ if (kp != null) {
+ pc.setOverrideProperties(kp);
Review Comment:
There is some code above that also set some OverrideProperties - so you would override what may have been added before. So I think you need to check if something exists, and then putAll in that case.
##########
core/camel-main/src/main/java/org/apache/camel/main/DefaultConfigurationProperties.java:
##########
@@ -2727,4 +2728,16 @@ public T withStartupRecorderDir(String startupRecorderDir) {
return (T) this;
}
+ public String getCloudPropertiesLocation() {
+ return cloudPropertiesLocation;
+ }
+
+ /**
+ * Sets the locations (comma separated values) where to find properties configuration as defined for cloud native
+ * environments such as Kubernetes.
+ */
+ public void setCloudPropertiesLocation(String cloudPropertiesLocation) {
+ this.cloudPropertiesLocation = cloudPropertiesLocation;
+ }
+
Review Comment:
You should also add the `withXXX` method for this new option.
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,43 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+
+ final Properties cp = new Properties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(file.getFileName().toString(), val);
+ } catch (IOException e) {
+ // Do nothing or should we throw a RuntimeException?
+ }
+ }
+ return FileVisitResult.CONTINUE;
+ }
+ });
+ }
+ }
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ if (overridProperties == null) {
+ return cp;
+ }
+ Properties mergedProperties = new Properties(overridProperties);
Review Comment:
Camel has a Location based properties so we know where they are from that we use for reporting to users. See `OrderedLocationProperties`
##########
core/camel-main/src/main/java/org/apache/camel/main/BaseMainSupport.java:
##########
@@ -433,6 +448,43 @@ private Properties tryLoadProperties(
return ip;
}
+ private static Properties tryLoadCloudProperties(
+ Properties overridProperties, String cloudPropertiesLocations)
+ throws IOException {
+
+ final Properties cp = new Properties();
+ try {
+ String[] locations = cloudPropertiesLocations.split(",");
+ for (String loc : locations) {
+ Path confPath = Paths.get(loc);
+ if (Files.exists(confPath) && Files.isDirectory(confPath)) {
+ Files.walkFileTree(confPath, new SimpleFileVisitor<Path>() {
+ @Override
+ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
+ if (!Files.isDirectory(file)) {
+ try {
+ String val = new String(Files.readAllBytes(file));
+ cp.put(file.getFileName().toString(), val);
+ } catch (IOException e) {
+ // Do nothing or should we throw a RuntimeException?
Review Comment:
Yeah I am not sure if its possible to know if its binary vs text (I guess there is no good naming convention).
Also I wonder if they file name should have extension stripped as they key should be without the extension should it not?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "davsclaus (via GitHub)" <gi...@apache.org>.
davsclaus commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2285342856
You need to include the generated files in the catalog folder, you can run from root folder
```
camel install -P fastinstall
```
And then include updated files such as in the catalog folder in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #15063:
URL: https://github.com/apache/camel/pull/15063
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(core): auto cloud native properties [camel]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15063:
URL: https://github.com/apache/camel/pull/15063#issuecomment-2277566970
:star2: Thank you for your contribution to the Apache Camel project! :star2:
:robot: CI automation will test this PR automatically.
:camel: Apache Camel Committers, please review the following items:
* First-time contributors **require MANUAL approval** for the GitHub Actions to run
* You can use the command `/component-test (camel-)component-name1 (camel-)component-name2..` to request a test from the test bot.
* You can label PRs using `build-all`, `build-dependents`, `skip-tests` and `test-dependents` to fine-tune the checks executed by this PR.
* Build and test logs are available in the Summary page. **Only** [Apache Camel committers](https://camel.apache.org/community/team/#committers) have access to the summary.
* :warning: Be careful when sharing logs. Review their contents before sharing them publicly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org