Fix acme account deletion without provider change #3664

zyclonite · 2018-07-24T11:26:24Z

What does this PR do?

prevent traefik from deleting the acme.account without provider change
fixes issue #3650

SantoDE

LGTM 👼

Good catch. Well done!

mmatur

Thanks for your contribution. Few remarks.
Could you also add some tests. You could maybe extract this new things into a function to be able to test more easily

mmatur · 2018-07-24T11:52:43Z

provider/acme/provider.go

+	if p.account != nil && p.account.Registration != nil {
+		aru, err := url.Parse(p.account.Registration.URI)
+		if err != nil {
+			log.Error("Unable to parse account.Registration URL")


You need to return an error

mmatur · 2018-07-24T11:52:47Z

provider/acme/provider.go

+		}
+		cau, err := url.Parse(p.CAServer)
+		if err != nil {
+			log.Error("Unable to parse CAServer URL")


You need to return an error

mmatur · 2018-07-24T11:53:32Z

acme/acme.go

+		if account != nil && account.Registration != nil {
+			aru, err := url.Parse(account.Registration.URI)
+			if err != nil {
+				log.Error("Unable to parse account.Registration URL")


You need to return an error

mmatur · 2018-07-24T11:53:36Z

acme/acme.go

+			}
+			cau, err := url.Parse(a.CAServer)
+			if err != nil {
+				log.Error("Unable to parse CAServer URL")


You need to return an error

mmatur · 2018-07-24T11:56:36Z

provider/acme/provider.go

+			log.Error("Unable to parse CAServer URL")
+		}
+		if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {
+			log.Error("Account does not match the current CAServer")


Could you please change the level of this log and improve the message with something like that

log.Info("Account URI does not match the current CAServer. The account will be reset")

mmatur · 2018-07-24T11:56:41Z

provider/acme/provider.go

+			log.Error("Unable to parse CAServer URL")
+		}
+		if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {
+			log.Error("Account does not match the current CAServer")


Could you please change the level of this log and improve the message with something like that

log.Info("Account URI does not match the current CAServer. The account will be reset")

mmatur · 2018-07-24T11:56:51Z

acme/acme.go

+				log.Error("Unable to parse CAServer URL")
+			}
+			if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {
+				log.Error("Account does not match the current CAServer")


Could you please change the level of this log and improve the message with something like that

log.Info("Account URI does not match the current CAServer. The account will be reset")

mmatur · 2018-07-24T11:56:52Z

acme/acme.go

+				log.Error("Unable to parse CAServer URL")
+			}
+			if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {
+				log.Error("Account does not match the current CAServer")


Could you please change the level of this log and improve the message with something like that

log.Info("Account URI does not match the current CAServer. The account will be reset")

mmatur · 2018-07-24T11:59:01Z

provider/acme/provider.go

+		if err != nil {
+			log.Error("Unable to parse CAServer URL")
+		}
+		if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {


As you don't return error before - cau and aru can be nil

mmatur · 2018-07-24T11:59:35Z

acme/acme.go

+			if err != nil {
+				log.Error("Unable to parse CAServer URL")
+			}
+			if strings.Compare(cau.Hostname(), aru.Hostname()) != 0 {


As you don't return error before - cau and aru can be nil

zyclonite · 2018-07-24T12:57:25Z

i have no proper go dev environment, it's a bit trail and error with simply a text editor...

as i do not know how to properly write tests in go, it would be nice if someone could support me here

mmatur

Thanks for the fix.

You can start to add a function in provider/acme/provider_test.go

func TestIsAccountMatchingCaServer(t *testing.T) {
	testCases := []struct {
		desc       string
		accountURI string
		serverURI  string
		expected   bool
	}{
		{
			desc:       "description",
			accountURI: "http://powpow.com/",
			serverURI:  "http://powpow2.com/",
			expected:   false,
		},
		{
			desc:       "description 2",
			accountURI: "http://powpow.com/",
			serverURI:  "http://powpow.com/",
			expected:   true,
		},
	}

	for _, test := range testCases {
		test := test
		t.Run(test.desc, func(t *testing.T) {
			t.Parallel()

			result := isAccountMatchingCaServer(test.accountURI, test.serverURI)

			assert.Equal(t, test.expected, result)
		})
	}
}

mmatur · 2018-07-24T14:07:43Z

acme/acme.go

+		log.Infof("Unable to parse CAServer URL : %v", err)
+		return false
+	}
+	return strings.Compare(cau.Hostname(), aru.Hostname()) != 0


You should replace by

return cau.Hostname() == aru.Hostname()

mmatur · 2018-07-24T14:08:57Z

provider/acme/provider.go

+		log.Infof("Unable to parse CAServer URL : %v", err)
+		return false
+	}
+	return strings.Compare(cau.Hostname(), aru.Hostname()) != 0


You should replace by

return cau.Hostname() == aru.Hostname()

mmatur · 2018-07-25T16:05:03Z

acme/acme.go

@@ -183,7 +184,8 @@ func (a *ACME) leadershipListener(elected bool) error {
 		account := object.(*Account)
 		account.Init()
 		// Reset Account values if caServer changed, thus registration URI can be updated
-		if account != nil && account.Registration != nil && !strings.HasPrefix(account.Registration.URI, a.CAServer) {
+		if account != nil && account.Registration != nil && isAccountMatchingCaServer(account.Registration.URI, a.CAServer) {


It's should be !isAccountMatchingCaServer(account.Registration.URI, a.CAServer) instead of isAccountMatchingCaServer(account.Registration.URI, a.CAServer) no ?

mmatur · 2018-07-25T16:05:10Z

provider/acme/provider.go

@@ -130,7 +131,8 @@ func (p *Provider) Init(_ types.Constraints) error {
 	}

 	// Reset Account if caServer changed, thus registration URI can be updated
-	if p.account != nil && p.account.Registration != nil && !strings.HasPrefix(p.account.Registration.URI, p.CAServer) {
+	if p.account != nil && p.account.Registration != nil && isAccountMatchingCaServer(p.account.Registration.URI, p.CAServer) {


It's should be !isAccountMatchingCaServer(account.Registration.URI, a.CAServer) instead of isAccountMatchingCaServer(account.Registration.URI, a.CAServer) no ?

true, my mistake

mmatur · 2018-07-25T16:07:19Z

provider/acme/provider_test.go

+			expected:   false,
+		},
+		{
+			desc:       "malformed server url",


Could you please add a test with 2 malformed url
Like that:

{ desc: "malformed account & server url", accountURI: "/test.example.com/acme/acct/1234567", serverURI: "/test.example.com/acme/acct/1234567", expected: false, },

problem is, it's not really malformed for the parser.. i'll find a different approach

mmatur

LGTM.
Great jobs @zyclonite 👏

nmengin

LGTM

…letion

fix acme.account deletion without provider change

2ee8ab2

traefiker added area/acme size/S status/0-needs-triage labels Jul 24, 2018

traefiker added this to the 1.7 milestone Jul 24, 2018

mmatur changed the title ~~fix acme.account deletion without provider change~~ Fix acme account deletion without provider change Jul 24, 2018

mmatur added status/2-needs-review kind/bug/fix a bug fix and removed status/0-needs-triage labels Jul 24, 2018

SantoDE approved these changes Jul 24, 2018

View reviewed changes

mmatur requested changes Jul 24, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 24, 2018

zyclonite added 3 commits July 24, 2018 14:17

change loglevel and return errors

8dbc846

move acme account and server matching to function

5b5c1e0

fix linter issues

d8de05e

mmatur requested changes Jul 24, 2018

View reviewed changes

add tests and small fixes

980f4a1

mmatur removed the contributor/waiting-for-corrections label Jul 25, 2018

mmatur requested changes Jul 25, 2018

View reviewed changes

traefiker added the contributor/waiting-for-corrections label Jul 25, 2018

zyclonite and others added 2 commits July 25, 2018 18:31

Merge branch 'v1.7' into fix_acme_account_deletion

bb82e05

fix negated if statements, fix malformed url test

541c357

emilevauge removed the contributor/waiting-for-corrections label Jul 26, 2018

emilevauge requested a review from nmengin July 30, 2018 15:05

mmatur approved these changes Jul 31, 2018

View reviewed changes

nmengin approved these changes Jul 31, 2018

View reviewed changes

nmengin added status/3-needs-merge and removed status/2-needs-review labels Jul 31, 2018

traefiker added the bot/merge-retry-1 label Jul 31, 2018

traefiker added status/4-merge-in-progress and removed bot/merge-retry-1 labels Jul 31, 2018

Merge remote-tracking branch 'upstream/v1.7' into fix_acme_account_de…

1991a51

…letion

traefiker removed the status/4-merge-in-progress label Jul 31, 2018

traefiker merged commit 838dd8c into traefik:v1.7 Jul 31, 2018

traefiker removed the status/3-needs-merge label Jul 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix acme account deletion without provider change #3664

Fix acme account deletion without provider change #3664

zyclonite commented Jul 24, 2018

SantoDE left a comment

mmatur left a comment

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 24, 2018

zyclonite commented Jul 24, 2018

mmatur left a comment

mmatur Jul 24, 2018

mmatur Jul 24, 2018

mmatur Jul 25, 2018

mmatur Jul 25, 2018

zyclonite Jul 25, 2018

mmatur Jul 25, 2018

zyclonite Jul 25, 2018

mmatur left a comment

nmengin left a comment

Fix acme account deletion without provider change #3664

Fix acme account deletion without provider change #3664

Conversation

zyclonite commented Jul 24, 2018

What does this PR do?

SantoDE left a comment

Choose a reason for hiding this comment

mmatur left a 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

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

zyclonite commented Jul 24, 2018

mmatur left a 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

Choose a reason for hiding this comment

mmatur left a comment

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment