add client cert support

[britcey]

Signed-off-by: Ben Ritcey [email protected]

This adds the ability to specify client certificates to use when talking to Jira - depending on the config, that may be sufficient to identify the account creating the Jira issues.

I left user/password as required - in the case where the client cert is used for authentication, they'll often just be ignored (depends on server config).

• I didn't see a clean way to unset user_cert/user_key in the individual receivers if set in the default, so I made the value of 'none' magical. I'm open to any better suggestions.

• what's the correct way to set insecure_skip_verify boolean based on the default? As-is, you'd have to specify it per receiver.

[britcey]

Looks like it's the use of the deprecated client.Authentication.SetBasicAuth that's causing tests to fail:

cmd/jiralert/main.go:118:4: SA1019: client.Authentication.SetBasicAuth is deprecated: Use BasicAuthTransport instead (staticcheck)

but AFAICT you can't use BasicAuthTransport with TLSConfig, etc.

I will, however, confirm that with the go-jira project.

[bwplotka]

Overall looks good, some comments only (:

[bwplotka]

This should be default, so let's remove it

[britcey]

Do we want to indicate that insecure_skip_verify is an option in the config, e.g.,

# insecure_skip_verify: true

?

[bwplotka]

none? hmmm, don't get this, why we need this?

[britcey]

This is what I was referring to in the first bullet point of the PR comment -

if you set a CAFile (or KeyFile/CertFile) in the defaults but didn't want it set in one of the specific receivers, you can't just set it to blank in the receiver config - the current config logic is that "if it's blank in the receiver config, use the default".

So I went with a "magic" value of "none" - I don't love that sort of magic, but I'm not sure what else to do short of totally rewriting the config logic. AFAICT, you can't set 'nil' in YAML - setting it to 'null' or '~' just results in an empty string.

[krasi-georgiev]

I would expect the following to work:

Marshal the default config
Then marshal the receiver config into the initialized default struct - all fields that are missing in the receiver's YAML will not override the defaults. All fields that are present in the receiver's config will override the default config.

Example receiver uses the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file
		APIURL = url2

Example receiver overrides the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: file2
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file2
		APIURL = url2

Example receiver overrides the defaults and doesn't use cert:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: ""
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = ""
		APIURL = url2

In other words, when the field doesn't exist in the receiver config use the default and in all other cases use the field value from the receiver config.

Also, let's add a test for this to make it clear and make sure we don't break this logic in the feature

[britcey]

(apologies for the big gap in responding - other work got in the way)

This may be my Go inexperience showing, but I believe that won't work as-is, since unmarshaling YAML with missing fields just initializes those fields to the empty string - it doesn't differentiate between "not specified" and "blank".

https://play.golang.org/p/_4ih2Xomnit

However, if I make them string pointers, the pointers will get initialized to nil if not set, which I can use to detect "not specified" vs. "blank". This appears to be one of the cases when using string pointers vs. strings makes sense: https://dhdersch.github.io/golang/2016/01/23/golang-when-to-use-string-pointers.html

Let me know if that sounds like a reasonable approach.

[krasi-georgiev]

I would expect the following to work:

Marshal the default config
Then marshal the receiver config into the initialized default struct - all fields that are missing in the receiver's YAML will not override the defaults. All fields that are present in the receiver's config will override the default config.

Example receiver uses the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file
		APIURL = url2

Example receiver overrides the defaults:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: file2
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = file2
		APIURL = url2

Example receiver overrides the defaults and doesn't use cert:
	defaults:
	  api_url: url
	  cert_file: file
	receivers:
	  cert_file: ""
	  api_url: url2
	
	Receiver Parsed:
	Receiver{
		CAFile = ""
		APIURL = url2

In other words, when the field doesn't exist in the receiver config use the default and in all other cases use the field value from the receiver config.

Also, let's add a test for this to make it clear and make sure we don't break this logic in the feature