You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "lucas-a-martins (via GitHub)" <gi...@apache.org> on 2024/02/02 18:06:08 UTC
[PR] Changes error message when using invalid `endpoint.url` [cloudstack]
lucas-a-martins opened a new pull request, #8603:
URL: https://github.com/apache/cloudstack/pull/8603
### Description
When validating the `endpoint.url` global setting, such as when using the `createKubernetesCluster` API, we encounter the exception message `Global setting endpoint.url has to be set to the Management Server's API endpoint`. This message lacks clarity about the problem to be solved and exposes information about the environment. Upon further analysis, it was found that this same message was used across several classes, repeating the same code in each one of them.
This pull request addresses this issue by replacing the exception with a generic one that does not expose any information about the environment. Also, it introduces a more detailed and explicit message to the logs, including the current value of `endpoint.url`. Furthermore, a new method has been implemented to validate the `endpoint.url` whenever necessary.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [x] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] build/CI
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
I tested by using CloudMonkey and tried to create a Kubernetes cluster. I implemented unit tests for the new method too.
Exception before changes:
```
(lucas) :cat: > create kubernetescluster name=test description=test zoneid=0ea94256-6969-46a8-b02e-b1604c81dd01 kubernetesversionid=ea7dc03b-40d0-4cfe-9144-4a9044cf88bd serviceofferingid=3eb8e0f1-d909-4bfe-aa55-2b9329920349 size=1
:see_no_evil: Error: (HTTP 530, error code 9999) Global setting endpoint.url has to be set to the Management Server's API end point
```
Exception after changes:
- New exception:
```
(lucas) :cat: > create kubernetescluster name=test description=test zoneid=0ea94256-6969-46a8-b02e-b1604c81dd01 kubernetesversionid=ea7dc03b-40d0-4cfe-9144-4a9044cf88bd serviceofferingid=3eb8e0f1-d909-4bfe-aa55-2b9329920349 size=1
:see_no_evil: Error: (HTTP 530, error code 9999) Unable to complete this operation. Contact your cloud admin.
```
- New log:
```
2024-01-31 19:16:41,219 ERROR [o.a.c.c.ApiServiceConfiguration] (qtp775386112-17:ctx-ff635773 ctx-96fc2faf) (logid:a8dc806e) Global setting endpoint.url cannot contain localhost or be blank. Current value: http://localhost:8080/client/api
```
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1930601077
@blueorangutan package
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1936168523
@lucas-a-martins do you want to target this for 4.20?
- if so see https://github.com/apache/cloudstack/pull/8603#issuecomment-1934208751
- if no we can retarget it.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2101040857
> > @lucas-a-martins I understand some error messages are unnecessary for normal users, not only this one, but also most other exceptions in vm the deployment, migration, etc. I agree we should display different error messages to normal users.
> > However I hope, as a root admin, we can get more information from error message on the UI and in the API response. Please bear in mind , not all root admins have access to the management server log and have the ability to find the root cause from massvie logs.
> > you can explore the options, for example in the API response or UI, display "Unable to complete this operation. Contact your cloud admin." (configurable) to normal users, and display the detailed error message to root admins. It could apply on ALL exceptions.
>
> Hey, @weizhouapache
>
> Considering these scenarios, I agree that having an exception to Root Admins makes sense and could be applied to all exceptions; however, this goes out of the scope of this PR. We have a discussion about patterns about logging (#8746) that could address these cases as well; we can discuss that there.
ok @lucas-a-martins
let's hold on this PR, and continue when we have a consensus in #8746 ?
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1928027262
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8536
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1930602636
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2103240845
> > @GutoVeronezi why do you think the new log is way more clear......
>
> The previous message says: `Global setting endpoint.url has to be set to the Management Server's API endpoint`; however, it validates if the setting has `localhost` in the name or it is blank. Without the change, operators will have to check the code to know that.
@GutoVeronezi
what I disagree is not the new error in the log (actually I like it), see below
LOGGER.error("Global setting [{}] cannot contain localhost or be blank. Current value: {}", ApiServletPath.key(), csUrl);
What I disagree is the message in the exception, see below
throw new InvalidParameterValueException("Unable to complete this operation. Contact your cloud admin.");
My suggestion is
- update the description of the global setting to emphasize it should not be empty or contain localhost
- update the message in the exception to include more information, for example "Unable to xxx because endpoint url is not configured correctly, please xxxx"
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1930578586
@weizhouapache, thanks you for your review. I've made changes to incorporate your suggestions in the latest commit (4cf74fc9aa08ecefa5c2c770512f7537f6c97b75), so if you could provide another review, I would greatly appreciate it!
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1952029900
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1953192395
<b>[SF] Trillian test result (tid-9288)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 42901 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8603-t9288-kvm-centos7.zip
Smoke tests completed. 129 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#discussion_r1479686180
##########
api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java:
##########
@@ -29,6 +33,20 @@ public class ApiServiceConfiguration implements Configurable {
"true", "Are the source checks on API calls enabled (true) or not (false)? See api.allowed.source.cidr.list", true, ConfigKey.Scope.Global);
public static final ConfigKey<String> ApiAllowedSourceCidrList = new ConfigKey<>(String.class, "api.allowed.source.cidr.list", "Advanced",
"0.0.0.0/0,::/0", "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.", true, ConfigKey.Scope.Account, null, null, null, null, null, ConfigKey.Kind.CSV, null);
+
+
+ public static void validateEndpointUrl() {
+ String csUrl = getApiServletPathValue();
+ if (StringUtils.isBlank(csUrl) || csUrl.contains("localhost") || csUrl.contains("127.0.0.1")) {
Review Comment:
can use `StringUtils.containsAny` instead
##########
api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java:
##########
@@ -18,8 +18,12 @@
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.Configurable;
+import com.cloud.exception.InvalidParameterValueException;
+import org.apache.commons.lang3.StringUtils;
+import org.apache.log4j.Logger;
Review Comment:
the new imports can be optimized.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1949000751
@blueorangutan package
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "GutoVeronezi (via GitHub)" <gi...@apache.org>.
GutoVeronezi commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2103064235
> ok @lucas-a-martins let's hold on this PR, and continue when we have a consensus in #8746 ?
I do not see why we need to wait that discussion to improve a log message. The previous message was misleading; the new log is way more clear. The idea of presenting different messages for operators and final users is interesting; however, is a different context from this PR. If we define something later, we can create another PR to address the concept.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1936322733
> @lucas-a-martins do you want to target this for 4.20?
>
I do. I'm just testing the code with log4j2.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "sureshanaparti (via GitHub)" <gi...@apache.org>.
sureshanaparti commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1926863935
@blueorangutan package
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2064670067
> To be honest, I think the old error message makes more sense for root admin
@weizhouapache,
One of the major issues with the old exception message is its lack of clarity. Even for root admins, it would be difficult to discern what action needs to be taken.
The new exception is more generic compared to the old one, as the intention is to give no environment information. To give any direction to operators, this PR adds a new log that is more objective, indicating to the operator where and what the problem is. IMO, the new message can also help prevent operators from making the same mistake again, as they will now know that `endpoint.url` can't be blank or `localhost`. The old message states `endpoint.url has to be set`, but if the `endpoint.url` is `localhost`, then it is already set, albeit with an invalid value, and the old message would not make sense.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2092439013
> @weizhouapache,
>
> The exception being unclear is intentional, as this is what users will see. I understand what you mean by showing an exclusive message to ROOT admins, but is this really necessary since they have access to the logs with a clear message? Especially considering that the current exception is unclear, so this wouldn't change. I can't see a benefit in adding a "less unclear but not clear enough" exception. Also, is there any case of an exception in the UI being different between Users and Root Admins in the same situation?
@lucas-a-martins
I understand some error messages are unnecessary for normal users, not only this one, but also most other exceptions in vm the deployment, migration, etc. I agree we should display different error messages to normal users.
However I hope, as a root admin, we can get more information from error message on the UI and in the API response. Please bear in mind , not all root admins have access to the management server log and have the ability to find the root cause from massvie logs.
you can explore the options, for example in the API response or UI, display "Unable to complete this operation. Contact your cloud admin." (configurable) to normal users, and display the detailed error message to root admins. It could apply on ALL exceptions.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1929209362
@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1926865773
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8603?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`fedcf66`)](https://app.codecov.io/gh/apache/cloudstack/commit/fedcf66de06911c1866353efa8b3a0bad998655a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 30.81% compared to head [(`8b4b9af`)](https://app.codecov.io/gh/apache/cloudstack/pull/8603?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 4.38%.
> Report is 24 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #8603 +/- ##
============================================
- Coverage 30.81% 4.38% -26.44%
============================================
Files 5341 361 -4980
Lines 375033 28699 -346334
Branches 54554 5004 -49550
============================================
- Hits 115564 1258 -114306
+ Misses 244185 27302 -216883
+ Partials 15284 139 -15145
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8603/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8603/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8603/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.38% <ø> (-0.02%)` | :arrow_down: |
| [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8603/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8603?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1927832919
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2091551541
> I know @lucas-a-martins
>
> but the new error message `Unable to complete this operation. Contact your cloud admin.` is much more unclear
>
> ```
> LOGGER.error("Global setting [{}] cannot contain localhost or be blank. Current value: {}", ApiServletPath.key(), csUrl);
> throw new InvalidParameterValueException("Unable to complete this operation. Contact your cloud admin.");
> ```
@weizhouapache,
The exception being unclear is intentional, as this is what users will see. I understand what you mean by showing an exclusive message to ROOT admins, but is this really necessary since they have access to the logs with a clear message? Especially considering that the current exception is unclear, so this wouldn't change. I can't see a benefit in adding a "less unclear but not clear enough" exception. Also, is there any case of an exception in the UI being different between Users and Root Admins in the same situation?
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1930688484
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8556
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1930608919
> @weizhouapache, thanks you for your review. I've made changes to incorporate your suggestions in the latest commit (4cf74fc9aa08ecefa5c2c770512f7537f6c97b75), so if you could provide another review, I would greatly appreciate it!
@lucas-a-martins
thanks for the update
Looks good
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1927831404
@blueorangutan package
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "GutoVeronezi (via GitHub)" <gi...@apache.org>.
GutoVeronezi commented on code in PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#discussion_r1491683963
##########
api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java:
##########
@@ -29,6 +34,20 @@ public class ApiServiceConfiguration implements Configurable {
"true", "Are the source checks on API calls enabled (true) or not (false)? See api.allowed.source.cidr.list", true, ConfigKey.Scope.Global);
public static final ConfigKey<String> ApiAllowedSourceCidrList = new ConfigKey<>(String.class, "api.allowed.source.cidr.list", "Advanced",
"0.0.0.0/0,::/0", "Comma separated list of IPv4/IPv6 CIDRs from which API calls can be performed. Can be set on Global and Account levels.", true, ConfigKey.Scope.Account, null, null, null, null, null, ConfigKey.Kind.CSV, null);
+
+
+ public static void validateEndpointUrl() {
+ String csUrl = getApiServletPathValue();
+ if (StringUtils.isBlank(csUrl) || StringUtils.containsAny(csUrl, "localhost", "127.0.0.1")) {
+ LOGGER.error(String.format("Global setting %s cannot contain localhost or be blank. Current value: %s", ApiServletPath.key(), csUrl));
Review Comment:
```suggestion
LOGGER.error("Global setting [{}] cannot contain localhost or be blank. Current value: {}", ApiServletPath.key(), csUrl);
```
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "GutoVeronezi (via GitHub)" <gi...@apache.org>.
GutoVeronezi commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2103167942
> @GutoVeronezi why do you think the new log is way more clear......
The previous message says: `Global setting endpoint.url has to be set to the Management Server's API endpoint`; however, it validates if the setting has `localhost` in the name or it is blank. Without the change, operators will have to check the code to know that.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1929205560
@blueorangutan test
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1931013176
<b>[SF] Trillian test result (tid-9096)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 48814 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8603-t9096-kvm-centos7.zip
Smoke tests completed. 122 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1949162146
Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8694
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2059932636
To be honest, I think the old error message makes more sense for root admin
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2064828116
> > To be honest, I think the old error message makes more sense for root admin
>
> @weizhouapache,
>
> One of the major issues with the old exception message is its lack of clarity. Even for root admins, it would be difficult to discern what action needs to be taken.
>
> The new exception is more generic compared to the old one, as the intention is to give no environment information. To give any direction to operators, this PR adds a new log that is more objective, indicating to the operator where and what the problem is. IMO, the new message can also help prevent operators from making the same mistake again, as they will now know that `endpoint.url` can't be blank or `localhost`. The old message states `endpoint.url has to be set`, but if the `endpoint.url` is `localhost`, then it is already set, albeit with an invalid value, and the old message would not make sense.
I know @lucas-a-martins
but the new error message `Unable to complete this operation. Contact your cloud admin.` is much more unclear
```
LOGGER.error("Global setting [{}] cannot contain localhost or be blank. Current value: {}", ApiServletPath.key(), csUrl);
throw new InvalidParameterValueException("Unable to complete this operation. Contact your cloud admin.");
```
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "JoaoJandre (via GitHub)" <gi...@apache.org>.
JoaoJandre commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1934208751
Hey, @lucas-a-martins please change your logger imports to use log4j2
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1952029461
@blueorangutan test
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-1949006281
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2103130265
> > ok @lucas-a-martins let's hold on this PR, and continue when we have a consensus in #8746 ?
>
> I do not see why we need to wait that discussion to improve a log message. The previous message was misleading; the new log is way more clear. The idea of presenting different messages for operators and final users is interesting; however, is a different context from this PR. If we define something later, we can create another PR to address the concept.
>
>
@GutoVeronezi
why do you think the new log is way more clear......
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Changes error message when using invalid `endpoint.url` [cloudstack]
Posted by "lucas-a-martins (via GitHub)" <gi...@apache.org>.
lucas-a-martins commented on PR #8603:
URL: https://github.com/apache/cloudstack/pull/8603#issuecomment-2101020576
> @lucas-a-martins I understand some error messages are unnecessary for normal users, not only this one, but also most other exceptions in vm the deployment, migration, etc. I agree we should display different error messages to normal users.
>
> However I hope, as a root admin, we can get more information from error message on the UI and in the API response. Please bear in mind , not all root admins have access to the management server log and have the ability to find the root cause from massvie logs.
>
> you can explore the options, for example in the API response or UI, display "Unable to complete this operation. Contact your cloud admin." (configurable) to normal users, and display the detailed error message to root admins. It could apply on ALL exceptions.
Hey, @weizhouapache
Considering these scenarios, I agree that having an exception to Root Admins makes sense and could be applied to all exceptions; however, this goes out of the scope of this PR. We have a discussion about patterns about logging (#8746) that could address these cases as well; we can discuss that there.
--
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@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org