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

feat: support for central trait preserved #1564

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
8 changes: 4 additions & 4 deletions .secrets.baseline
Original file line number Diff line number Diff line change
Expand Up @@ -376,21 +376,21 @@
"filename": "pkg/client/fleetmanager/mocks/client_moq.go",
"hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c",
"is_verified": false,
"line_number": 584
"line_number": 646
},
{
"type": "Secret Keyword",
"filename": "pkg/client/fleetmanager/mocks/client_moq.go",
"hashed_secret": "0ff50155b4f57adeccae93f27dc23efe2a8b7824",
"is_verified": false,
"line_number": 585
"line_number": 647
},
{
"type": "Secret Keyword",
"filename": "pkg/client/fleetmanager/mocks/client_moq.go",
"hashed_secret": "5ce1b8d4fb9dae5c02b2017e39e7267a21cea37f",
"is_verified": false,
"line_number": 594
"line_number": 656
}
],
"pkg/client/iam/client_moq.go": [
Expand Down Expand Up @@ -586,5 +586,5 @@
}
]
},
"generated_at": "2024-02-05T19:02:34Z"
"generated_at": "2024-02-08T19:14:52Z"
}
124 changes: 124 additions & 0 deletions e2e/e2e_central_traits_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
package e2e

import (
"context"
"net/http"
"os"
"time"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"github.com/stackrox/acs-fleet-manager/internal/dinosaur/constants"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/api/admin/private"
"github.com/stackrox/acs-fleet-manager/pkg/client/fleetmanager"
fmImpl "github.com/stackrox/acs-fleet-manager/pkg/client/fleetmanager/impl"
)

var _ = Describe("central traits", Ordered, func() {

var client *fleetmanager.Client
var adminAPI fleetmanager.AdminAPI
var notes []string
var ctx = context.Background()

BeforeEach(func() {
SkipIf(!runCentralTests, "Skipping Central tests")
Expect(restoreDefaultGitopsConfig()).To(Succeed())

option := fmImpl.OptionFromEnv()
auth, err := fmImpl.NewStaticAuth(context.Background(), fmImpl.StaticOption{StaticToken: option.Static.StaticToken})
Expect(err).ToNot(HaveOccurred())
client, err = fmImpl.NewClient(fleetManagerEndpoint, auth)
Expect(err).ToNot(HaveOccurred())

adminStaticToken := os.Getenv("STATIC_TOKEN_ADMIN")
adminAuth, err := fmImpl.NewStaticAuth(context.Background(), fmImpl.StaticOption{StaticToken: adminStaticToken})
Expect(err).ToNot(HaveOccurred())
adminClient, err := fmImpl.NewClient(fleetManagerEndpoint, adminAuth)
Expect(err).ToNot(HaveOccurred())
adminAPI = adminClient.AdminAPI()

GinkgoWriter.Printf("Current time: %s\n", time.Now().String())
printNotes(notes)
})

It("should manage traits", func() {
central, _, err := adminAPI.CreateCentral(ctx, true, private.CentralRequestPayload{
CloudProvider: dpCloudProvider,
MultiAz: true,
Name: newCentralName(),
Region: dpRegion,
})
Expect(err).Should(Succeed())
id := central.Id

Eventually(assertCentralRequestProvisioning(ctx, client, id)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())

defer adminAPI.DeleteDbCentralById(ctx, id)

traits, _, err := adminAPI.GetCentralTraits(ctx, id)
Expect(err).ToNot(HaveOccurred(), "no error on no traits")
Expect(traits).To(BeEmpty(), "no traits yet")

_, err = adminAPI.PutCentralTrait(ctx, id, "test-trait")
Expect(err).ToNot(HaveOccurred(), "no error on adding test-trait")

traits, _, err = adminAPI.GetCentralTraits(ctx, id)
Expect(err).ToNot(HaveOccurred(), "no error on having traits")
Expect(traits).To(BeEquivalentTo([]string{"test-trait"}), "test-trait should be found")

_, err = adminAPI.PutCentralTrait(ctx, id, "test-trait-1")
Expect(err).ToNot(HaveOccurred(), "no error on adding test-trait-1")

_, err = adminAPI.PutCentralTrait(ctx, id, "test-trait-1")
Expect(err).ToNot(HaveOccurred(), "no error on adding test-trait-1 twice")

traits, _, err = adminAPI.GetCentralTraits(ctx, id)
Expect(err).ToNot(HaveOccurred(), "no error on having multiple traits")
Expect(traits).To(BeEquivalentTo([]string{"test-trait", "test-trait-1"}), "should have only two traits")

_, err = adminAPI.GetCentralTrait(ctx, id, "test-trait")
Expect(err).ToNot(HaveOccurred(), "no error on checking for existing trait")

_, err = adminAPI.GetCentralTrait(ctx, id, "test-trait-2")
Expect(err).To(HaveOccurred(), "error on checking for non-existing trait")
Copy link
Contributor

@stehessel stehessel Feb 13, 2024

Choose a reason for hiding this comment

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

We could assert 404 here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made it in #1661.


_, err = adminAPI.DeleteCentralTrait(ctx, id, "test-trait")
Expect(err).ToNot(HaveOccurred(), "no error on deleting test-trait")

_, err = adminAPI.DeleteCentralTrait(ctx, id, "test-trait")
Expect(err).ToNot(HaveOccurred(), "no error on deleting non-existing trait")

traits, _, err = adminAPI.GetCentralTraits(ctx, id)
Expect(err).ToNot(HaveOccurred(), "no error on retreiving traits")
Expect(traits).To(BeEquivalentTo([]string{"test-trait-1"}), "should have only one trait now")
})

It("should preserve preserved", func() {
central, _, err := adminAPI.CreateCentral(ctx, true, private.CentralRequestPayload{
CloudProvider: dpCloudProvider,
MultiAz: true,
Name: newCentralName(),
Region: dpRegion,
})
Expect(err).Should(Succeed())
Eventually(assertCentralRequestProvisioning(ctx, client, central.Id)).
WithTimeout(waitTimeout).
WithPolling(defaultPolling).
Should(Succeed())

_, err = adminAPI.PutCentralTrait(ctx, central.Id, constants.CentralTraitPreserved)
Expect(err).Should(Succeed())

resp, err := client.PublicAPI().DeleteCentralById(ctx, central.Id, true)
Expect(resp.StatusCode).To(Equal(http.StatusConflict))
Expect(err).To(HaveOccurred())

_, err = adminAPI.DeleteDbCentralById(ctx, central.Id)
Expect(err).Should(Succeed(), "AdminAPI should ignore the preserved trait")
})
})
4 changes: 4 additions & 0 deletions internal/dinosaur/constants/central.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,3 +103,7 @@ var ActiveStatuses = func() []CentralStatus {
}
return active
}()

const (
CentralTraitPreserved = "preserved" // Preserve centrals from deletion.
)
19 changes: 19 additions & 0 deletions internal/dinosaur/pkg/services/dinosaur.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@ package services
import (
"context"
"fmt"
"net/http"
"sync"
"time"

"github.com/aws/aws-sdk-go/service/route53"
"github.com/golang/glog"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/constants"
dinosaurConstants "github.com/stackrox/acs-fleet-manager/internal/dinosaur/constants"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/api/dbapi"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/config"
Expand All @@ -27,6 +29,7 @@ import (
"github.com/stackrox/acs-fleet-manager/pkg/metrics"
"github.com/stackrox/acs-fleet-manager/pkg/services"
coreServices "github.com/stackrox/acs-fleet-manager/pkg/services/queryparser"
"github.com/stackrox/acs-fleet-manager/pkg/shared/utils/arrays"
)

var (
Expand Down Expand Up @@ -60,6 +63,11 @@ type CNameRecordStatus struct {
Status *string
}

var errPreservedCentral = func(id string) *errors.ServiceError {
return errors.NewErrorFromHTTPStatusCode(http.StatusConflict,
"central %q has %s trait, remove the trait to enable deletion", id, constants.CentralTraitPreserved)
Copy link
Contributor

@stehessel stehessel Feb 13, 2024

Choose a reason for hiding this comment

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

Could you please update the openapi spec to include 409 for delete?

}

// DinosaurService ...
//
//go:generate moq -out dinosaurservice_moq.go . DinosaurService
Expand Down Expand Up @@ -492,6 +500,10 @@ func (k *dinosaurService) RegisterDinosaurDeprovisionJob(ctx context.Context, id
}
metrics.IncreaseCentralTotalOperationsCountMetric(dinosaurConstants.CentralOperationDeprovision)

if arrays.Contains(dinosaurRequest.Traits, constants.CentralTraitPreserved) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a check here in addition to the one during deletion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the case when the function is called by the HTTP handler: the check allows for returning the error immediately, instead of marking the instance for deletion and fail to delete it later.

return errPreservedCentral(id)
}

deprovisionStatus := dinosaurConstants.CentralRequestStatusDeprovision

if executed, err := k.UpdateStatus(id, deprovisionStatus); executed {
Expand All @@ -512,6 +524,7 @@ func (k *dinosaurService) DeprovisionDinosaurForUsers(users []string) *errors.Se
Model(&dbapi.CentralRequest{}).
Where("owner IN (?)", users).
Where("status NOT IN (?)", dinosaurDeletionStatuses).
Where("traits IS NULL OR ? != ALL (traits)", dinosaurConstants.CentralTraitPreserved).
Copy link
Collaborator

@ludydoo ludydoo Mar 25, 2024

Choose a reason for hiding this comment

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

By curiosity, what does ? != ALL (traits) mean ? And why not use ("? NOT IN traits", CentralTraitPreserved)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a specific syntax for column of type array: https://www.postgresql.org/docs/current/arrays.html#ARRAYS-SEARCHING

Updates(map[string]interface{}{
"status": dinosaurConstants.CentralRequestStatusDeprovision,
"deletion_timestamp": now,
Expand Down Expand Up @@ -548,6 +561,7 @@ func (k *dinosaurService) DeprovisionExpiredDinosaurs() *errors.ServiceError {
}

dbConn = dbConn.Where("status NOT IN (?)", dinosaurDeletionStatuses)
dbConn.Where("traits IS NULL OR ? != ALL (traits)", dinosaurConstants.CentralTraitPreserved)

db := dbConn.Updates(map[string]interface{}{
"status": dinosaurConstants.CentralRequestStatusDeprovision,
Expand Down Expand Up @@ -575,6 +589,11 @@ func (k *dinosaurService) DeprovisionExpiredDinosaurs() *errors.ServiceError {
// If the force flag is true, then any errors prior to the final deletion of the CentralRequest will be logged as warnings
// but do not interrupt the deletion flow.
func (k *dinosaurService) Delete(centralRequest *dbapi.CentralRequest, force bool) *errors.ServiceError {
Copy link
Contributor

@stehessel stehessel Feb 13, 2024

Choose a reason for hiding this comment

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

Is the force option exposed by the console UI? It feels like we are mixing up different concepts a bit. What deletion scenarios are we trying to prevent?

  1. User deletes their own instance. Do we want to stop anyone from doing this? They will be confused what's going on because the traits are managed via the admin API. Traits will be generally unknown to users, so why expose them to users?
  2. (Accidental) deletion by admins? Admins control traits, but admin API ignores traits.
  3. Unwanted/early expiry of instances. Wouldn't it make more sense to prevent expiry in the first place, rather than stop the deprovision of an already expired instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, force is not exposed. In fact, force=true if only an admin deletes the central via /centrals/db/{id} endpoint. I was thinking that this would be the only way to delete a preserved instance, including users not being able to do so.

Your 3rd point makes sense. The trait should be called perpetual then. We could actually have both traits, but I think perpetual is more interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, force is not exposed. In fact, force=true if only an admin deletes the central via /centrals/db/{id} endpoint. I was thinking that this would be the only way to delete a preserved instance, including users not being able to do so.

I think users could interpret this as being forced to pay in case they use on-demand billing. So I don't think this is a good idea. If we want to expose it to users, it would be better to also mark their instance as preserved in the UI and let them be able to change the state / force delete it.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO both admin and user API calls to delete instances should be respected despite preserved trait. If user wants to delete an instance, they should be able to do that without additional steps (they already have to type in instance name in the UI to confirm). If an admin wants to delete a user instance, they most likely have a good reason for it.

if !force && arrays.Contains(centralRequest.Traits, constants.CentralTraitPreserved) {
err := errPreservedCentral(centralRequest.ID)
return errors.NewWithCause(err.Code, err, "cannot delete preserved central with no force")
}

dbConn := k.connectionFactory.New()

// if the we don't have the clusterID we can only delete the row from the database
Expand Down
71 changes: 69 additions & 2 deletions internal/dinosaur/pkg/services/dinosaur_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ package services

import (
"context"
"net/http"
"reflect"
"testing"

mocket "github.com/selvatico/go-mocket"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/constants"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/api/dbapi"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/config"
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/converters"
Expand Down Expand Up @@ -175,6 +177,69 @@ func Test_dinosaurService_Get(t *testing.T) {
}
}

func Test_dinosaurService_Delete_Preserved(t *testing.T) {
k := &dinosaurService{
connectionFactory: db.NewMockConnectionFactory(nil),
dinosaurConfig: &config.CentralConfig{
CentralLifespan: config.NewCentralLifespanConfig(),
},
}
preserved := buildCentralRequest(func(centralRequest *dbapi.CentralRequest) {
centralRequest.Traits = append(centralRequest.Traits, constants.CentralTraitPreserved)
})
svcErr := k.Delete(preserved, false)
assert.Equal(t, http.StatusConflict, svcErr.HTTPCode)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

This block feels like it should be extracted into a separate function.

authHelper, err := auth.NewAuthHelper(JwtKeyFile, JwtCAFile, "")
if err != nil {
t.Fatalf("failed to create auth helper: %s", err.Error())
}
account, err := authHelper.NewAccount(testUser, "", "", "")
if err != nil {
t.Fatal("failed to build a new account")
}

jwt, err := authHelper.CreateJWTWithClaims(account, nil)
if err != nil {
t.Fatalf("failed to create jwt: %s", err.Error())
}
ctx := context.TODO()
authenticatedCtx := auth.SetTokenInContext(ctx, jwt)

mocket.Catcher.Reset().
NewMock().
WithQuery(`SELECT * FROM "central_requests" WHERE id = $1 AND owner = $2`).
WithArgs(testID, testUser).
WithReply(converters.ConvertDinosaurRequest(preserved))

svcErr = k.RegisterDinosaurDeprovisionJob(authenticatedCtx, preserved.ID)
assert.Equal(t, http.StatusConflict, svcErr.HTTPCode)
}
svcErr = k.Delete(preserved, true)
assert.Nil(t, svcErr)
}

func Test_dinosaurService_DeprovisionDinosaurForUsersQuery(t *testing.T) {
k := &dinosaurService{
connectionFactory: db.NewMockConnectionFactory(nil),
dinosaurConfig: &config.CentralConfig{
CentralLifespan: config.NewCentralLifespanConfig(),
},
}

m := mocket.Catcher.Reset().NewMock().WithQuery(`UPDATE "central_requests" ` +
`SET "deletion_timestamp"=$1,"status"=$2,"updated_at"=$3 WHERE ` +
`owner IN ($4) AND ` +
`status NOT IN ($5,$6) AND ` +
`(traits IS NULL OR $7 != ALL (traits)) AND ` +
`"central_requests"."deleted_at" IS NULL`).
OneTime()

svcErr := k.DeprovisionDinosaurForUsers([]string{"user1"})
assert.Nil(t, svcErr)
assert.True(t, m.Triggered)
}

func Test_dinosaurService_DeprovisionExpiredDinosaursQuery(t *testing.T) {
k := &dinosaurService{
connectionFactory: db.NewMockConnectionFactory(nil),
Expand All @@ -186,7 +251,8 @@ func Test_dinosaurService_DeprovisionExpiredDinosaursQuery(t *testing.T) {
m := mocket.Catcher.Reset().NewMock().WithQuery(`UPDATE "central_requests" ` +
`SET "deletion_timestamp"=$1,"status"=$2,"updated_at"=$3 WHERE ` +
`(expired_at IS NOT NULL AND expired_at < $4 OR instance_type = $5 AND created_at <= $6) ` +
`AND status NOT IN ($7,$8) AND "central_requests"."deleted_at" IS NULL`).
`AND status NOT IN ($7,$8) AND (traits IS NULL OR $9 != ALL (traits)) ` +
`AND "central_requests"."deleted_at" IS NULL`).
OneTime()

svcErr := k.DeprovisionExpiredDinosaurs()
Expand All @@ -196,7 +262,8 @@ func Test_dinosaurService_DeprovisionExpiredDinosaursQuery(t *testing.T) {
m = mocket.Catcher.Reset().NewMock().WithQuery(`UPDATE "central_requests" ` +
`SET "deletion_timestamp"=$1,"status"=$2,"updated_at"=$3 WHERE ` +
`expired_at IS NOT NULL AND expired_at < $4 ` +
`AND status NOT IN ($5,$6) AND "central_requests"."deleted_at" IS NULL`).
`AND status NOT IN ($5,$6) AND (traits IS NULL OR $7 != ALL (traits)) ` +
`AND "central_requests"."deleted_at" IS NULL`).
OneTime()
k.dinosaurConfig.CentralLifespan.EnableDeletionOfExpiredCentral = false
svcErr = k.DeprovisionExpiredDinosaurs()
Expand Down
5 changes: 5 additions & 0 deletions pkg/client/fleetmanager/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ type AdminAPI interface {
DeleteDbCentralById(ctx context.Context, id string) (*http.Response, error)
CentralRotateSecrets(ctx context.Context, id string, centralRotateSecretsRequest admin.CentralRotateSecretsRequest) (*http.Response, error)
UpdateCentralNameById(ctx context.Context, id string, centralUpdateNameRequest admin.CentralUpdateNameRequest) (admin.Central, *http.Response, error)

GetCentralTraits(ctx context.Context, id string) ([]string, *http.Response, error)
GetCentralTrait(ctx context.Context, id string, trait string) (*http.Response, error)
PutCentralTrait(ctx context.Context, id string, trait string) (*http.Response, error)
DeleteCentralTrait(ctx context.Context, id string, trait string) (*http.Response, error)
}

// Client is a helper struct that wraps around the API clients generated from
Expand Down
Loading
Loading