Changes error message when using invalid `endpoint.url` #8603

lucas-a-martins · 2024-02-02T18:06:06Z

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

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

sureshanaparti · 2024-02-05T12:16:57Z

@blueorangutan package

codecov · 2024-02-05T12:17:46Z

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (49cecae) 30.37% compared to head (6b86329) 30.76%.
Report is 30 commits behind head on main.

Files	Patch %	Lines
...bernetes/cluster/KubernetesClusterManagerImpl.java	0.00%	3 Missing ⚠️
...loud/network/lb/LoadBalancingRulesManagerImpl.java	0.00%	2 Missing ⚠️
...r/actionworkers/KubernetesClusterActionWorker.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8603      +/-   ##
============================================
+ Coverage     30.37%   30.76%   +0.39%     
- Complexity    32633    33078     +445     
============================================
  Files          5352     5353       +1     
  Lines        374419   374605     +186     
  Branches      54609    54633      +24     
============================================
+ Hits         113719   115244    +1525     
+ Misses       245523   244085    -1438     
- Partials      15177    15276      +99

Flag	Coverage Δ
simulator-marvin-tests	`24.61% <7.14%> (+0.46%)`	⬆️
uitests	`4.38% <ø> (-0.01%)`	⬇️
unit-tests	`16.44% <57.14%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

weizhouapache · 2024-02-05T18:57:41Z

@blueorangutan package

blueorangutan · 2024-02-05T18:58:03Z

@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.

blueorangutan · 2024-02-05T20:19:15Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8536

DaanHoogland · 2024-02-06T10:23:53Z

@blueorangutan test

blueorangutan · 2024-02-06T10:26:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland

clgtm

weizhouapache

overall code lgtm
left 2 minor comments

api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java

lucas-a-martins · 2024-02-06T19:04:14Z

@weizhouapache, thanks for your review. I've made changes to incorporate your suggestions in the latest commit (4cf74fc), so if you could provide another review, I would greatly appreciate it!

weizhouapache · 2024-02-06T19:18:56Z

@blueorangutan package

blueorangutan · 2024-02-06T19:20:03Z

@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.

weizhouapache · 2024-02-06T19:24:38Z

@weizhouapache, thanks you for your review. I've made changes to incorporate your suggestions in the latest commit (4cf74fc), so if you could provide another review, I would greatly appreciate it!

@lucas-a-martins
thanks for the update
Looks good

blueorangutan · 2024-02-06T20:21:15Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8556

blueorangutan · 2024-02-07T00:24:35Z

[SF] Trillian test result (tid-9096)
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

JoaoJandre · 2024-02-08T14:14:10Z

Hey, @lucas-a-martins please change your logger imports to use log4j2

DaanHoogland · 2024-02-09T15:51:09Z

@lucas-a-martins do you want to target this for 4.20?

if so see Changes error message when using invalid endpoint.url #8603 (comment)
if no we can retarget it.

lucas-a-martins · 2024-02-09T17:36:04Z

@lucas-a-martins do you want to target this for 4.20?

@DaanHoogland I do. I'm just testing the code with log4j2.

api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java

…guration.java Co-authored-by: Daniel Augusto Veronezi Salvador <[email protected]>

weizhouapache · 2024-02-16T18:04:23Z

@blueorangutan package

blueorangutan · 2024-02-16T18:06:03Z

@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.

blueorangutan · 2024-02-16T19:15:06Z

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8694

DaanHoogland · 2024-02-19T09:25:48Z

@blueorangutan test

blueorangutan · 2024-02-19T09:26:03Z

@DaanHoogland a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2024-02-19T21:45:01Z

[SF] Trillian test result (tid-9288)
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

bernardodemarco

LGTM, I manually tested the proposed changes in a local environment.

CloudMonkey:

(lab) 🐱 > createKubernetesCluster name=dummy-kubernetes-cluster zoneid=0ea94256-6969-46a8-b02e-b1604c81dd01
🙈 Error: (HTTP 530, error code 9999) Unable to complete this operation. Contact your cloud admin.

Management server logs:

2024-04-16T20:22:30,881 ERROR [o.a.c.c.ApiServiceConfiguration] (qtp1759482496-19:[ctx-9ff4e912, ctx-4928e0cd]) (logid:6f2b30d3) Global setting [endpoint.url] cannot contain localhost or be blank. Current value: http://localhost:8080/client/api

weizhouapache · 2024-04-16T21:13:54Z

To be honest, I think the old error message makes more sense for root admin

lucas-a-martins · 2024-04-18T17:30:25Z

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.

weizhouapache · 2024-04-18T18:19:05Z

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.");

lucas-a-martins · 2024-05-02T20:56:17Z

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?

weizhouapache · 2024-05-03T07:12:36Z

@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.

GutoVeronezi

CLGTM

Thanks, @lucas-a-martins. The previous message was misleading; the new log is way more clear.

lucas-a-martins · 2024-05-08T17:01:13Z

@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.

weizhouapache · 2024-05-08T17:13:16Z

@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 ?

GutoVeronezi · 2024-05-09T17:07:43Z

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.

weizhouapache · 2024-05-09T17:41:39Z

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......

For root admins, no information is given by the new error message, it makes no sense.

As a workaround, can you display the error message on UI for regular users? If your customers use UI not API. @lucas-a-martins

GutoVeronezi · 2024-05-09T18:06:12Z

@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.

weizhouapache · 2024-05-09T18:57:27Z

@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"

kiranchavala · 2024-09-23T08:56:01Z

@lucas-a-martins any update on this

Could you please take a look at @weizhouapache suggestion

lucas.martins.scclouds added 6 commits January 29, 2024 16:57

Fix validate endpoint url message

07ef20b

Fix message and log error

c272eac

Using new get

f89f884

Implement unit tests

d87fa1f

Add license

92e59c7

Add newline at end of file

8b4b9af

boring-cyborg bot added component:api component:kubernetes component:networking labels Feb 2, 2024

DaanHoogland approved these changes Feb 6, 2024

View reviewed changes

DaanHoogland added the status:needs-testing label Feb 6, 2024

weizhouapache reviewed Feb 6, 2024

View reviewed changes

api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java Show resolved Hide resolved

api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java Outdated Show resolved Hide resolved

Optimize imports and add containsAny (Wei suggestions)

4cf74fc

Merge branch 'main' into fix-endpoint-url-message

4f18607

Change to log4j2

5343763

rohityadavcloud added this to the 4.20.0.0 milestone Feb 13, 2024

GutoVeronezi reviewed Feb 15, 2024

View reviewed changes

api/src/main/java/org/apache/cloudstack/config/ApiServiceConfiguration.java Outdated Show resolved Hide resolved

Update api/src/main/java/org/apache/cloudstack/config/ApiServiceConfi…

6b86329

…guration.java Co-authored-by: Daniel Augusto Veronezi Salvador <[email protected]>

bernardodemarco approved these changes Apr 16, 2024

View reviewed changes

GutoVeronezi approved these changes May 3, 2024

View reviewed changes

JoaoJandre modified the milestones: 4.20.0.0, 4.21.0.0 Sep 10, 2024

Changes error message when using invalid endpoint.url #8603

Are you sure you want to change the base?

Changes error message when using invalid endpoint.url #8603

Conversation

lucas-a-martins commented Feb 2, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

sureshanaparti commented Feb 5, 2024

codecov bot commented Feb 5, 2024 • edited Loading

Codecov Report

weizhouapache commented Feb 5, 2024

blueorangutan commented Feb 5, 2024

blueorangutan commented Feb 5, 2024

DaanHoogland commented Feb 6, 2024

blueorangutan commented Feb 6, 2024

DaanHoogland left a comment

Choose a reason for hiding this comment

weizhouapache left a comment

Choose a reason for hiding this comment

lucas-a-martins commented Feb 6, 2024 • edited Loading

weizhouapache commented Feb 6, 2024

blueorangutan commented Feb 6, 2024

weizhouapache commented Feb 6, 2024

blueorangutan commented Feb 6, 2024

blueorangutan commented Feb 7, 2024

JoaoJandre commented Feb 8, 2024

DaanHoogland commented Feb 9, 2024

lucas-a-martins commented Feb 9, 2024 • edited Loading

weizhouapache commented Feb 16, 2024

blueorangutan commented Feb 16, 2024

blueorangutan commented Feb 16, 2024

DaanHoogland commented Feb 19, 2024

blueorangutan commented Feb 19, 2024

blueorangutan commented Feb 19, 2024

bernardodemarco left a comment

Choose a reason for hiding this comment

weizhouapache commented Apr 16, 2024

lucas-a-martins commented Apr 18, 2024

weizhouapache commented Apr 18, 2024

lucas-a-martins commented May 2, 2024

weizhouapache commented May 3, 2024

GutoVeronezi left a comment

Choose a reason for hiding this comment

lucas-a-martins commented May 8, 2024

weizhouapache commented May 8, 2024

GutoVeronezi commented May 9, 2024

weizhouapache commented May 9, 2024 • edited Loading

GutoVeronezi commented May 9, 2024

weizhouapache commented May 9, 2024

kiranchavala commented Sep 23, 2024

Changes error message when using invalid `endpoint.url` #8603

Changes error message when using invalid `endpoint.url` #8603

codecov bot commented Feb 5, 2024 •

edited

Loading

lucas-a-martins commented Feb 6, 2024 •

edited

Loading

lucas-a-martins commented Feb 9, 2024 •

edited

Loading

weizhouapache commented May 9, 2024 •

edited

Loading