Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CLD-8628 #2] Add custom status code for installation name/dns conflicts + bonus #1091

Merged
merged 6 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ endif

.PHONY: unittest
unittest:
@echo $(CLOUD_DATABASE)
CLOUD_DATABASE=$(CLOUD_DATABASE) $(GO) test -failfast ./... ${TEST_FLAGS} -covermode=count -coverprofile=coverage.out

.PHONY: verify-mocks
Expand Down
18 changes: 17 additions & 1 deletion internal/api/installation.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"time"

"github.com/mattermost/mattermost-cloud/internal/common"
"github.com/mattermost/mattermost-cloud/internal/events"

"github.com/pkg/errors"

Expand Down Expand Up @@ -254,6 +255,13 @@ func handleCreateInstallation(c *Context, w http.ResponseWriter, r *http.Request

err = c.Store.CreateInstallation(&installation, annotations, dnsRecords)
if err != nil {
var uniqueErr *store.UniqueConstraintError
if errors.As(err, &uniqueErr) {
c.Logger.WithError(err).Error("domain name already in use")
w.WriteHeader(http.StatusConflict)
return
}

c.Logger.WithError(err).Error("failed to create installation")
w.WriteHeader(http.StatusInternalServerError)
return
Expand Down Expand Up @@ -661,7 +669,15 @@ func handleDeleteInstallation(c *Context, w http.ResponseWriter, r *http.Request
return
}

err = c.EventProducer.ProduceInstallationStateChangeEvent(installationDTO.Installation, oldState)
// Try to parse the user ID from the request context, so it can be passed along with the webhook event
actorID := ""
if value := r.Context().Value(ContextKeyUserID{}); value != nil {
if str, ok := value.(string); ok && str != "" {
actorID = str
}
}

err = c.EventProducer.ProduceInstallationStateChangeEvent(installationDTO.Installation, oldState, events.DataField{Key: "actor_id", Value: actorID})
Comment on lines +672 to +680
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I snuck this in while I was here. This passes the ID of the actor that initiated the deletion request, so that webhook listeners can easily detect who did the deletion. The main use case for this will be the Cloud Plugin. Currently, whenever an installation is deleted for any reason, a message "Cloud isntallation <> has been manually deleted and is now removed from the cloud plugin", which confuses users, as sometimes an automatic cleanup script deletes them. With this change, the Cloud Plugin will be able to tell if it itself made the deletion request, or if it was another system, and adjust the message sent accordingly.

if err != nil {
c.Logger.WithError(err).Error("Failed to create installation state change event")
}
Expand Down
40 changes: 40 additions & 0 deletions internal/api/installation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,46 @@ func TestCreateInstallation(t *testing.T) {
require.EqualError(t, err, "failed with status code 400")
})

t.Run("custom status code for conflict in DNS name", func(t *testing.T) {
envs := model.EnvVarMap{
"MM_TEST2": model.EnvVar{Value: "test2"},
}
installation, err := client.CreateInstallation(&model.CreateInstallationRequest{
OwnerID: "owner",
Version: "version",
DNS: "useddns.example.com",
Affinity: model.InstallationAffinityIsolated,
Annotations: []string{"my-annotation"},
PriorityEnv: envs,
})
require.NoError(t, err)
require.Equal(t, "owner", installation.OwnerID)
require.Equal(t, "version", installation.Version)
require.Equal(t, "mattermost/mattermost-enterprise-edition", installation.Image)
require.Equal(t, "useddns.example.com", installation.DNS) //nolint
require.Equal(t, "useddns.example.com", installation.DNSRecords[0].DomainName)
require.Equal(t, "useddns", installation.Name)
require.Equal(t, model.InstallationAffinityIsolated, installation.Affinity)
require.Equal(t, model.InstallationStateCreationRequested, installation.State)
require.Equal(t, model.DefaultCRVersion, installation.CRVersion)
require.Empty(t, installation.LockAcquiredBy)
require.EqualValues(t, 0, installation.LockAcquiredAt)
require.NotEqual(t, 0, installation.CreateAt)
require.EqualValues(t, 0, installation.DeleteAt)

_, err = client.CreateInstallation(&model.CreateInstallationRequest{
OwnerID: "owner",
Version: "version",
DNS: "useddns.example.com",
Affinity: model.InstallationAffinityIsolated,
Annotations: []string{"my-annotation"},
PriorityEnv: envs,
})

require.Error(t, err)
require.EqualError(t, err, "failed with status code 409")
})

t.Run("valid", func(t *testing.T) {
envs := model.EnvVarMap{
"MM_TEST2": model.EnvVar{Value: "test2"},
Expand Down
6 changes: 5 additions & 1 deletion internal/store/installation.go
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ func (sqlStore *SQLStore) CreateInstallation(installation *model.Installation, a

err = sqlStore.createInstallation(tx, installation)
if err != nil {
return errors.Wrap(err, "failed to create installation")
return err
}

// We can do bulk insert for better performance, but currently we do not expect more than 1 record.
Expand Down Expand Up @@ -480,6 +480,10 @@ func (sqlStore *SQLStore) createInstallation(db execer, installation *model.Inst
SetMap(insertsMap),
)
if err != nil {
if isUniqueConstraintViolation(err) {
return &UniqueConstraintError{}
}

return errors.Wrap(err, "failed to create installation")
}

Expand Down
4 changes: 4 additions & 0 deletions internal/store/installation_dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ func (sqlStore *SQLStore) createInstallationDNS(db execer, installationID string

_, err := sqlStore.execBuilder(db, query)
if err != nil {
if isUniqueConstraintViolation(err) {
return &UniqueConstraintError{}
}

return errors.Wrap(err, "failed to create installation DNS record")
}

Expand Down
16 changes: 16 additions & 0 deletions internal/store/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"

sq "github.com/Masterminds/squirrel"
"github.com/lib/pq"
"github.com/mattermost/mattermost-cloud/model"
)

Expand Down Expand Up @@ -35,3 +36,18 @@ func applyPagingFilter(builder sq.SelectBuilder, paging model.Paging, deleteAtTa

return builder
}

type UniqueConstraintError struct {
}

func (e *UniqueConstraintError) Error() string {
return "unique constraint violation"
}

// isUniqueConstraintViolation checks if the error is a unique constraint violation.
func isUniqueConstraintViolation(err error) bool {
if pgErr, ok := err.(*pq.Error); ok && pgErr.Code == "23505" {
return true
}
return false
}
Loading