Add support for Personal Access Token Authentication #110

rporres · 2022-03-04T12:38:31Z

https://github.com/andygrunwald/go-jira supports natively Personal Token Authentication (PAT). This PR brings it to jiralert.

In order to make this a reality, I needed to do a few things

Add tests to config. Since I was going to touch it, I thought that more tests were going to be required
Upgrade go to 1.15 due to a go-jira dependency: https://github.com/golang-jwt/jwt
Upgrade go-jira to 1.15.1 version to support PAT
Add PAT support
Test PAT changes in config package

Since some of the changes are related/needed but not exactly what the PR says, I've separated the above actions in different commits. Please let me know if you want them in separate PRs.

Lastly, I haven't written lots of Go code in my life, so apologies in advance for any obvious mistakes or not proper practices this code may have 😅

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

This is temporary while prometheus-community/jiralert#110 is reviewed and hopefully merged. Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

bwplotka

This is great!

And for all tests - amazing work.

Couple of nits:

Consider moving all config checks to one place
Please add trailing dot to all comments (we follow Full sentence English)
Table tests are great but consider consistent, smaller implementation wit anonymous struct and in place test cases

bwplotka · 2022-03-11T08:36:06Z

cmd/jiralert/main.go

+			client, err = jira.NewClient(tp.Client(), conf.APIURL)
+		} else {
+			// Config should not allow us to get here, but just in case...
+			errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)


Suggested change

errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)

errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)

return

This section was removed in ccb0fda as a result of #110 (comment)

bwplotka · 2022-03-11T09:24:32Z

pkg/config/config.go

@@ -158,11 +159,18 @@ func (c *Config) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	// We want to set c to the defaults and then overwrite it with the input.
 	// To make unmarshal fill the plain data struct rather than calling UnmarshalYAML
 	// again, we have to hide it using a type indirection.
+
+	// TODO: Handle the empty defaults case


What do you mean?

If there is no defaults section, jiralert panics

panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x13c40b5] goroutine 1 [running]: gopkg.in/yaml%2ev2.handleErr(0xc000157d58) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/yaml.go:249 +0x85 panic(0x15b0f60, 0x1a250c0) /Users/rporresm/.goenv/versions/1.15.15/src/runtime/panic.go:969 +0x1b9 github.com/prometheus-community/jiralert/pkg/config.(*Config).UnmarshalYAML(0xc00010f500, 0xc00000d860, 0x0, 0x94b6b18) /Users/rporresm/git/github.com/rporres/jiralert/pkg/config/config.go:170 +0x75 gopkg.in/yaml%2ev2.(*decoder).callUnmarshaler(0xc00002f080, 0xc000175ea0, 0x94b6b18, 0xc00010f500, 0xc00010f500) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/decode.go:270 +0xa6 gopkg.in/yaml%2ev2.(*decoder).prepare(0xc00002f080, 0xc000175ea0, 0x1606360, 0xc00010f500, 0x199, 0x104e0a5, 0xc00000e140, 0x0, 0x0) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/decode.go:313 +0x1d0 gopkg.in/yaml%2ev2.(*decoder).unmarshal(0xc00002f080, 0xc000175ea0, 0x1606360, 0xc00010f500, 0x199, 0x0) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/decode.go:364 +0xe9 gopkg.in/yaml%2ev2.(*decoder).document(0xc00002f080, 0xc000175e30, 0x1606360, 0xc00010f500, 0x199, 0x13913a7) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/decode.go:384 +0x7c gopkg.in/yaml%2ev2.(*decoder).unmarshal(0xc00002f080, 0xc000175e30, 0x1606360, 0xc00010f500, 0x199, 0x199) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/decode.go:360 +0x250 gopkg.in/yaml%2ev2.unmarshal(0xc000132800, 0x3f4, 0x400, 0x15c02c0, 0xc00010f500, 0x400, 0x0, 0x0) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/yaml.go:148 +0x33d gopkg.in/yaml%2ev2.Unmarshal(...) /Users/rporresm/go/1.15.15/pkg/mod/gopkg.in/yaml.v2@v2.3.0/yaml.go:81 github.com/prometheus-community/jiralert/pkg/config.Load(0xc000132400, 0x3f4, 0x3f4, 0xc000132400, 0x3f4) /Users/rporresm/git/github.com/rporres/jiralert/pkg/config/config.go:53 +0x99 github.com/prometheus-community/jiralert/pkg/config.LoadFile(0x1657b30, 0x13, 0x16fb140, 0xc000286e10, 0x0, 0x0, 0xc000286f00, 0x0, 0x0, 0x0) /Users/rporresm/git/github.com/rporres/jiralert/pkg/config/config.go:67 +0x214 main.main() /Users/rporresm/git/github.com/rporres/jiralert/cmd/jiralert/main.go:74 +0x2a5

Clarified comment in b7f35ac

bwplotka · 2022-03-11T09:25:14Z

cmd/jiralert/main.go

+			client, err = jira.NewClient(tp.Client(), conf.APIURL)
+		} else {
+			// Config should not allow us to get here, but just in case...
+			errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)


I would consider having one place of checking config. Either here or in unmarshal, not mixed?

Fair enough. Fixed in ccb0fda

pkg/config/config.go

pkg/config/config_test.go

bwplotka · 2022-03-11T09:31:08Z

pkg/config/config_test.go

+	type testCase struct {
+		receiverTestConfigMandatoryFields []string
+		errorMessage                      string
+	}
+
+	mandatory := mandatoryReceiverFields()
+
+	// Test cases
+	// * missing user
+	// * missing password
+	// * specifying user/password and PAT auth
+	testTable := []testCase{
+		{
+			removeFromStrSlice(mandatory, "User"),
+			`missing authentication in receiver "Name"`,
+		},
+		{
+			removeFromStrSlice(mandatory, "Password"),
+			`missing authentication in receiver "Name"`,
+		},
+		{
+			append(mandatory, "PersonalAccessToken"),
+			"bad auth config in defaults section: user/password and PAT authentication are mutually exclusive",
+		},
+	}
+
+	minimalReceiverTestConfig := newReceiverTestConfig([]string{"Name"}, []string{})
+	for _, test := range testTable {
+		defaultsConfig := newReceiverTestConfig(test.receiverTestConfigMandatoryFields, []string{})
+		config := testConfig{
+			Defaults:  defaultsConfig,
+			Receivers: []*receiverTestConfig{minimalReceiverTestConfig},
+			Template:  "jiralert.tmpl",
+		}
+
+		configErrorTestRunner(t, config, test.errorMessage)
+	}
+}


This is great!

But there is one pattern we could reuse here to simplify code (and have consistency).

Suggested change

type testCase struct {

receiverTestConfigMandatoryFields []string

errorMessage string

}

mandatory := mandatoryReceiverFields()

// Test cases

// * missing user

// * missing password

// * specifying user/password and PAT auth

testTable := []testCase{

{

removeFromStrSlice(mandatory, "User"),

`missing authentication in receiver "Name"`,

},

{

removeFromStrSlice(mandatory, "Password"),

`missing authentication in receiver "Name"`,

},

{

append(mandatory, "PersonalAccessToken"),

"bad auth config in defaults section: user/password and PAT authentication are mutually exclusive",

},

}

minimalReceiverTestConfig := newReceiverTestConfig([]string{"Name"}, []string{})

for _, test := range testTable {

defaultsConfig := newReceiverTestConfig(test.receiverTestConfigMandatoryFields, []string{})

config := testConfig{

Defaults: defaultsConfig,

Receivers: []*receiverTestConfig{minimalReceiverTestConfig},

Template: "jiralert.tmpl",

}

configErrorTestRunner(t, config, test.errorMessage)

}

}

mandatory := mandatoryReceiverFields()

minimalReceiverTestConfig := newReceiverTestConfig([]string{"Name"}, []string{})

for _, test := range []struct {

receiverTestConfigMandatoryFields []string

errorMessage string

}{

{

// Missing user.

removeFromStrSlice(mandatory, "User"),

`missing authentication in receiver "Name"`,

},

{

// Missing password.

removeFromStrSlice(mandatory, "Password"),

`missing authentication in receiver "Name"`,

},

{

// Specifying user/password and PAT auth.

append(mandatory, "PersonalAccessToken"),

"bad auth config in defaults section: user/password and PAT authentication are mutually exclusive",

},

} {

defaultsConfig := newReceiverTestConfig(test.receiverTestConfigMandatoryFields, []string{})

config := testConfig{

Defaults: defaultsConfig,

Receivers: []*receiverTestConfig{minimalReceiverTestConfig},

Template: "jiralert.tmpl",

}

configErrorTestRunner(t, config, test.errorMessage)

}

}

Fixed this everywhere in b2861cf

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

jankaluza · 2022-03-14T07:03:45Z

Can we release new version once this is merged and tested? There is a move from basic auth to PAT in my company and we rely on jiralert for monitoring...

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres · 2022-03-14T12:04:51Z

This should be ready for a new review, @bwplotka. I've added separate commits to address comments separately. Happy to rearrange commits whenever this is ready to go.

bwplotka

Thanks, amazing job!

Added tests on config

8f971eb

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres mentioned this pull request Mar 4, 2022

Update Go dependencies #109

Closed

rporres added a commit to rporres/container-images that referenced this pull request Mar 7, 2022

Build jiralert from app-sre PAT fork

54c25ba

This is temporary while prometheus-community/jiralert#110 is reviewed and hopefully merged. Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres mentioned this pull request Mar 7, 2022

Build jiralert from app-sre PAT fork app-sre/container-images#36

Merged

rporres added 5 commits March 10, 2022 12:54

moved to 1.15

ed064f7

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Upgrade go-jira

6e84e0e

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Changes to support PAT authentication

42906bc

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

addded PAT

dc5dfb5

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Added tests for Personal Access Token config

9eff971

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres force-pushed the pat-support-1.15 branch from 25e3a96 to 9eff971 Compare March 10, 2022 11:55

bwplotka reviewed Mar 11, 2022

View reviewed changes

rporres added 3 commits March 13, 2022 23:19

Removed redundant check from main

ccb0fda

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Add missing trailing dots in config_test.go

8b572bf

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Clarified TODO comment

b7f35ac

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

rporres force-pushed the pat-support-1.15 branch from d2c1992 to b7f35ac Compare March 13, 2022 22:34

rporres added 2 commits March 13, 2022 23:35

added trailing dots in config.go

99428f0

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

Used anonymous struct and in place test cases

b2861cf

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

User/Password can be overriden separately

107bc8f

Signed-off-by: Rafa Porres Molina <rporresm@redhat.com>

bwplotka approved these changes Mar 15, 2022

View reviewed changes

bwplotka merged commit 0c7c40f into prometheus-community:master Mar 15, 2022

rporres deleted the pat-support-1.15 branch March 15, 2022 12:31

julianfbeck mentioned this pull request Mar 8, 2023

Adds Documentation for PAT usage #155

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Personal Access Token Authentication #110

Add support for Personal Access Token Authentication #110

rporres commented Mar 4, 2022 •

edited

Loading

bwplotka left a comment

bwplotka Mar 11, 2022

rporres Mar 13, 2022 •

edited

Loading

bwplotka Mar 11, 2022

rporres Mar 13, 2022

rporres Mar 13, 2022

bwplotka Mar 11, 2022

rporres Mar 13, 2022

bwplotka Mar 11, 2022

rporres Mar 13, 2022

jankaluza commented Mar 14, 2022

rporres commented Mar 14, 2022

bwplotka left a comment

	errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)
	errorHandler(w, http.StatusInternalServerError, fmt.Errorf("missing JIRA auth config"), conf.Name, &data, logger)
	return

Add support for Personal Access Token Authentication #110

Add support for Personal Access Token Authentication #110

Conversation

rporres commented Mar 4, 2022 • edited Loading

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rporres Mar 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jankaluza commented Mar 14, 2022

rporres commented Mar 14, 2022

bwplotka left a comment

Choose a reason for hiding this comment

rporres commented Mar 4, 2022 •

edited

Loading

rporres Mar 13, 2022 •

edited

Loading