Add configuration validation in clients by sarah-witt · Pull Request #59 · jenkinsci/datadog-plugin

sarah-witt · 2020-04-23T22:14:51Z

What does this PR do?

Leaves most validation up to the client (besides form validation in the jenkins UI)
Does not attempt validation when the last instance is the same as the attempted parameters
Does not continuously log the same error when trying to create the same instance as before

What inspired you to submit this pull request?
#55

Description of the Change

Removes some duplicate validation in GlobalConfiguration to the DatadogClients
Modifies getInstance so that it does not log the same message over and over again
Changes getPort and setPort to use Integers since port is an Integer

Alternate Designs

Only remove the log statements and do not refactor the code (we want the log statements at some point)

Possible Drawbacks

less validation, so may miss some errors that global config caught before

Verification Process

unit tests
run the instance locally and check logs to see that they are not overpopulated
TODO: more validation and cleanup, and addressing build failure

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
PR title must be written as a CHANGELOG entry (see why)
Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
PR should not have do-not-merge/ label attached.
If Applicable, issue must have kind/ and severity/ labels attached at least.

FlorianVeaux

I know it's still a draft, but I wanted to add a few comments already.

FlorianVeaux · 2020-04-24T08:51:53Z

+            instance.setLogIntakeConnectionBroken(true);
+            logger.severe("Connection broken, please double check both your Log Intake URL and Key");
+        }
+        return;


So validation will pass even though both the url and the logIntakeUrl are invalid (or the apiKey is invalid).
Maybe we should at least fail it metrics can't be submitted?

Also to come back to the UI part, we should be able to display a FormWarning if the plugin can't connect.

Oh that's a good idea. I agree there are more validations that can be performed.

FlorianVeaux · 2020-04-24T08:54:02Z

        return this.jenkinsVersion;
    }

+    private static boolean isCollectBuildLogEnabled(){


I don't like that we need to access the config from here.

This is also currently in DogstatsdClient. I'm not sure the best way to check for logs being enabled.

FlorianVeaux · 2020-05-04T11:45:46Z

-
-            return status;
-        }catch(Exception e){
-            // Intercept all FormException instances.


I'm not sure what the effect of this is. Are we sure the change is safe ?

Hm I'm not completely sure, I know FormExceptions are the most common, and they should be thrown, but maybe unexpected errors should be caught and logged.

I added it back- I agree that having this safeguard is more important than the cleanliness of the code.

In theory java should let you know about all exceptions and force you to catch or rethrow them except if they are unchecked exception like RuntimeExceptions.
So I think you should be able to ensure that all exceptions are caught?

FlorianVeaux · 2020-05-04T11:47:42Z

-
-            DatadogUtilities.severe(logger, e, null);
+        if(client != null) {
+            // There are no reasons at this point client should be null.


Let's update the comment here, client is expected to be null if connection is impossible.

And let's reverse the condition:

if(client == null) { return null } client.setDefaultIntakeConnectionBroken(false); client.setLogIntakeConnectionBroken(false); // Persist global configuration information save(); return status;

I also think that instead of returning status here we should return true. And at the top of the method do this:

if(!super.configure(req, formData)){ return false }

FlorianVeaux · 2020-05-04T12:11:09Z


-    private static DatadogClient instance;
+    private static DatadogClient instance = null;
+    private static boolean failedLastValidation = false;


Let's have a comment to tell why it's useful.

FlorianVeaux · 2020-05-04T12:21:45Z

+        DatadogHttpClient newInstance = new DatadogHttpClient(url, logIntakeUrl, apiKey);
+        DatadogHttpClient httpInstance = (DatadogHttpClient) instance;
+        if (httpInstance != null && httpInstance.equals(newInstance)) {
+            if (DatadogHttpClient.failedLastValidation) {


We will need a way to retry validation every X minutes. What if failedLastValidation is true because of a network failure ? You'd require a restart of Jenkins to fix

Hmm that's true. I will look into a way to do this.

We still need this to be addressed

I know, I haven't thought of a great approach yet. Do you have any ideas?

So, thinking it through, in the case of a network failure, it wouldn't require a restart of jenkins, it would just require entering different credentials (and then entering the correct ones). It isn't the best solution, but I'm not sure how likely this scenario is.

FlorianVeaux · 2020-05-04T12:27:32Z

+            DatadogHttpClient.lastInstance = new DatadogHttpClient(url, logIntakeUrl, apiKey);
+            if (enableValidations) {
+                try {
+                    validateCongiguration(url, logIntakeUrl, apiKey);


Reopening: this should not be a static method.

FlorianVeaux · 2020-05-04T12:29:26Z

+                DatadogHttpClient.instance = newInstance;
+                try {
+                    validateCongiguration(url, logIntakeUrl, apiKey);
+                } catch(RuntimeException e){


Let's not use unchecked exception for validation.

FlorianVeaux · 2020-05-04T12:30:51Z

        this.logIntakeUrl = logIntakeUrl;
    }

+    public static void validateCongiguration(String url, String logIntakeUrl, Secret apiKey) throws RuntimeException {


Typo: validateConfiguration*

Should not be static

Let's use checked exception instead

FlorianVeaux · 2020-05-04T12:32:07Z

+    }
+
+    @Override
+    public int hashCode() {


Auto generated ? Let's indicate it

I based it off of the method from our other hashCode methods; it was recommended to override hashCode since I overrode equals

FlorianVeaux · 2020-05-04T12:45:05Z

 import org.datadog.jenkins.plugins.datadog.util.TagsUtil;
+import org.apache.commons.lang.StringUtils;

 import java.io.*;


Same comments as for DatadogHttpClient

FlorianVeaux · 2020-05-05T08:41:44Z

+            if (enableValidations) {
+                DatadogHttpClient.instance = newInstance;
+                try {
+                    newInstance.validateConfiguration(url, logIntakeUrl, apiKey);


Suggested change

newInstance.validateConfiguration(url, logIntakeUrl, apiKey);

newInstance.validateConfiguration();

You don't need the args because it's not static anymore

ofek

LGTM

jetersen

I do not see how this PR fixes the issue of using the LogLevel.INFO

jetersen · 2020-05-19T09:30:06Z

    <groupId>org.jenkins-ci.plugins</groupId>
    <artifactId>plugin</artifactId>
-    <version>3.56</version> <!-- or whatever the newest version available is -->
+    <version>4.1</version> <!-- or whatever the newest version available is -->


This seems ill adviced:
https://github.com/jenkinsci/plugin-pom/blob/88cb710c1b7244bf5f885e16fc9f0baef1ef2661/pom.xml#L53
Please at least specify a Jenkins.version property

Hi, @jetersen!
This change was to just update the parent pom version. It was also included in this PR as well, which is ready to be merged: https://github.com/jenkinsci/datadog-plugin/pull/57/files#diff-600376dffeb79835ede4a0b285078036R8 Adding a minimum jenkins version is out of the scope for this PR, but it’s a good idea that we should do in a later PR.

sarah-witt · 2020-05-19T15:56:39Z

I do not see how this PR fixes the issue of using the LogLevel.INFO

Thanks for the review @jetersen! I agree this PR doesn’t solve the issue of the continuous DatadogCountersPublisher logs- this was only meant to resolve the null api key logs issue. Can we use #63 to address the DatadogCountersPublisher issue?

jetersen · 2020-05-19T18:57:26Z

+        return DatadogUtilities.getDatadogGlobalDescriptor() != null &&
+                DatadogUtilities.getDatadogGlobalDescriptor().isCollectBuildLogs();


Your doing a lookup twice 😓 and your using an more expensive get in the method called.

You should use:

ExtensionList.lookupSingleton(DatadogGlobalConfiguration.class);

Also the method could be moved to the DatadogGlobalConfiguration class, basically moving it to where it belongs.
See here for example:
https://github.com/jenkinsci/azure-keyvault-plugin/blob/ef3a5eb4413f7ecd47d6532735604c52fceaaef0/src/main/java/org/jenkinsci/plugins/azurekeyvaultplugin/AzureKeyVaultGlobalConfiguration.java#L241-L243

That sounds like a good idea, so we should refactor getDatadogGlobalDescriptor() and move it to DatadogGlobalConfiguration?

There is definitely an opportunity to clean this up, but let's cover it in a separate PR.

@sarah-witt I see you never picked up this suggestion 😅

Some changes were implemented here

Ah you kept DatadogUtilities.getDatadogGlobalDescriptor 😅 Logically I would prefer DatadogGlobalConfiguration.get()

…into sarah/validate-config

mgarabed

🔥 LGTM - tested this out locally and only see a single log line when the API key is invalid.

* Add configuration validation * Create .equals method and use synchronization * Remove try catch in configure and enforce use of POST * Simplify logic in configure * Don't create instance if config is invalid * Remove IOException * Throw form exception * Remove exception and post * Fiz style * Check for zero as port * Use parseInt instead of Number Utils * Remove zero check * Update pom * Fix style of imports * Add back extra safeguards for exceptions in configuration * Update exception type and use assertThrows in tests * Validation does not need parameters * Reset config for testing * Remove reset * Remove unneeded resetConfig * Remove unneeded validation * Move hostname validation * Style * Refactor validate url * Simplify equals methods * Update equals and remove imports * Update instance type

Add configuration validation

4d47023

sarah-witt requested a review from FlorianVeaux April 23, 2020 22:16

FlorianVeaux reviewed Apr 24, 2020

View reviewed changes

sarah-witt added 8 commits April 24, 2020 23:16

Create .equals method and use synchronization

0a41af3

Remove try catch in configure and enforce use of POST

ca51179

Simplify logic in configure

0b9b070

Don't create instance if config is invalid

320700b

Remove IOException

fc2f051

Throw form exception

e52f7ac

Remove exception and post

602ae08

Fiz style

41dc9b0

sarah-witt changed the title ~~Add configuration validation~~ Add configuration validation in clients Apr 28, 2020

sarah-witt added 5 commits April 28, 2020 11:20

Check for zero as port

58b6b63

Use parseInt instead of Number Utils

93c5451

Remove zero check

8634375

Update pom

01b0a92

Fix style of imports

428d62c

sarah-witt marked this pull request as ready for review April 29, 2020 14:59

FlorianVeaux reviewed May 4, 2020

View reviewed changes

sarah-witt added 2 commits May 4, 2020 14:26

Add back extra safeguards for exceptions in configuration

f8da50b

Update exception type and use assertThrows in tests

0f17ee0

FlorianVeaux reviewed May 5, 2020

View reviewed changes

sarah-witt added 7 commits May 6, 2020 16:35

Validation does not need parameters

9763d84

Reset config for testing

a1de9b8

Remove reset

ecd5b61

Remove unneeded resetConfig

4b338d5

Remove unneeded validation

835878c

Move hostname validation

5fa7d21

Style

40bb705

ofek previously approved these changes May 13, 2020

View reviewed changes

jetersen reviewed May 19, 2020

View reviewed changes

sarah-witt mentioned this pull request May 19, 2020

override getNormalLoggingLevel to set log Level.Fine #63

Merged

6 tasks

jetersen reviewed May 19, 2020

View reviewed changes

Update POM

b926636

sarah-witt dismissed ofek’s stale review via b926636 June 17, 2020 18:00

mgarabed reviewed Jun 22, 2020

View reviewed changes

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/DatadogGlobalConfiguration.java Outdated

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java

sarah-witt added 2 commits June 23, 2020 11:07

Refactor validate url

cd8ee72

Merge branch 'master' of https://github.com/jenkinsci/datadog-plugin …

7357a80

…into sarah/validate-config

mgarabed reviewed Jun 23, 2020

View reviewed changes

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java Outdated

Comment thread src/main/java/org/datadog/jenkins/plugins/datadog/clients/DatadogHttpClient.java Outdated

sarah-witt added 3 commits June 23, 2020 13:20

Simplify equals methods

63edcf4

Update equals and remove imports

1a63e28

Update instance type

8e7040b

mgarabed approved these changes Jun 23, 2020

View reviewed changes

sarah-witt merged commit 76cbe15 into master Jun 24, 2020

sarah-witt deleted the sarah/validate-config branch June 24, 2020 14:31

This was referenced Jun 24, 2020

Update failedlastValidation if client passes validation #81

Merged

Don't fail validation if log connection is broken #82

Merged

mgarabed mentioned this pull request Jun 30, 2020

Update config access through lookupSingleton method #85

Merged

6 tasks

FlorianVeaux added the changelog/Fixed Fixed features results into a bug fix version bump label Jul 6, 2020

	newInstance.validateConfiguration(url, logIntakeUrl, apiKey);
	newInstance.validateConfiguration();

		return DatadogUtilities.getDatadogGlobalDescriptor() != null &&
		DatadogUtilities.getDatadogGlobalDescriptor().isCollectBuildLogs();

Uh oh!

Conversation

sarah-witt commented Apr 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Description of the Change

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

Uh oh!

FlorianVeaux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sarah-witt Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sarah-witt commented Apr 23, 2020 •

edited

Loading

sarah-witt Jun 17, 2020 •

edited

Loading

jetersen May 19, 2020 •

edited

Loading

jetersen May 19, 2020 •

edited

Loading