From 643319549ca92b696a89da28d46a5ee5533a7c87 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Tue, 19 Dec 2023 20:49:10 +0100 Subject: [PATCH 01/20] add rotate secret backup feature to admin API --- .secrets.baseline | 97 ++++++++++--------- .../pkg/api/admin/private/api/openapi.yaml | 7 +- .../pkg/api/admin/private/api_default.go | 2 +- .../model_central_rotate_secrets_request.go | 1 + .../dinosaur/pkg/handlers/admin_dinosaur.go | 8 ++ internal/dinosaur/pkg/services/dinosaur.go | 12 +++ .../pkg/services/dinosaurservice_moq.go | 50 ++++++++++ openapi/fleet-manager-private-admin.yaml | 6 +- 8 files changed, 130 insertions(+), 53 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index f06e9733b4..683e240b09 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -20,6 +20,9 @@ { "name": "CloudantDetector" }, + { + "name": "DiscordBotTokenDetector" + }, { "name": "GitHubTokenDetector" }, @@ -72,10 +75,6 @@ { "path": "detect_secrets.filters.allowlist.is_line_allowlisted" }, - { - "path": "detect_secrets.filters.common.is_baseline_file", - "filename": ".secrets.baseline" - }, { "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies", "min_level": 2 @@ -122,8 +121,7 @@ "filename": "config/jwks-file-static.json", "hashed_secret": "551c2aa179bc3c0e2e8176d4b458d077ed358e25", "is_verified": false, - "line_number": 8, - "is_secret": false + "line_number": 8 }, { "type": "Base64 High Entropy String", @@ -137,8 +135,7 @@ "filename": "config/jwks-file-static.json", "hashed_secret": "f05bf8a9b8521955a5fa259abd1d5a6d269273ec", "is_verified": false, - "line_number": 16, - "is_secret": false + "line_number": 16 }, { "type": "Base64 High Entropy String", @@ -152,8 +149,7 @@ "filename": "config/jwks-file-static.json", "hashed_secret": "e23d321e76d1144e48b7f1d05dfd0d5036031003", "is_verified": false, - "line_number": 24, - "is_secret": false + "line_number": 24 }, { "type": "Base64 High Entropy String", @@ -167,16 +163,14 @@ "filename": "config/jwks-file-static.json", "hashed_secret": "9b87ab16703bb0ccd78aee2f69bd0e604f7a42dc", "is_verified": false, - "line_number": 32, - "is_secret": false + "line_number": 32 }, { "type": "Base64 High Entropy String", "filename": "config/jwks-file-static.json", "hashed_secret": "3744e3d32aa35c3bb53d76d1832699b723f07812", "is_verified": false, - "line_number": 41, - "is_secret": false + "line_number": 41 } ], "config/jwks-file.json": [ @@ -192,8 +186,7 @@ "filename": "config/jwks-file.json", "hashed_secret": "551c2aa179bc3c0e2e8176d4b458d077ed358e25", "is_verified": false, - "line_number": 8, - "is_secret": false + "line_number": 8 }, { "type": "Base64 High Entropy String", @@ -207,8 +200,7 @@ "filename": "config/jwks-file.json", "hashed_secret": "f05bf8a9b8521955a5fa259abd1d5a6d269273ec", "is_verified": false, - "line_number": 16, - "is_secret": false + "line_number": 16 }, { "type": "Base64 High Entropy String", @@ -222,8 +214,7 @@ "filename": "config/jwks-file.json", "hashed_secret": "e23d321e76d1144e48b7f1d05dfd0d5036031003", "is_verified": false, - "line_number": 24, - "is_secret": false + "line_number": 24 }, { "type": "Base64 High Entropy String", @@ -237,8 +228,7 @@ "filename": "config/jwks-file.json", "hashed_secret": "9b87ab16703bb0ccd78aee2f69bd0e604f7a42dc", "is_verified": false, - "line_number": 32, - "is_secret": false + "line_number": 32 } ], "db_setup_docker.sql": [ @@ -247,8 +237,7 @@ "filename": "db_setup_docker.sql", "hashed_secret": "afc848c316af1a89d49826c5ae9d00ed769415f3", "is_verified": false, - "line_number": 1, - "is_secret": false + "line_number": 1 } ], "dev/env/manifests/shared/03-configmap-config.yaml": [ @@ -352,30 +341,43 @@ "line_number": 1531 } ], + "internal/dinosaur/pkg/services/dinosaurservice_moq.go": [ + { + "type": "Secret Keyword", + "filename": "internal/dinosaur/pkg/services/dinosaurservice_moq.go", + "hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c", + "is_verified": false, + "line_number": 1054 + }, + { + "type": "Secret Keyword", + "filename": "internal/dinosaur/pkg/services/dinosaurservice_moq.go", + "hashed_secret": "b41a19f9cb43a6475a32a4748e222e5e8f7dce2b", + "is_verified": false, + "line_number": 1055 + } + ], "pkg/client/iam/client_moq.go": [ { "type": "Secret Keyword", "filename": "pkg/client/iam/client_moq.go", "hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c", "is_verified": false, - "line_number": 649, - "is_secret": false + "line_number": 649 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/client_moq.go", "hashed_secret": "4595e0fe3be13544e523e5f6c1145f15007f7b58", "is_verified": false, - "line_number": 650, - "is_secret": false + "line_number": 650 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/client_moq.go", "hashed_secret": "539fbe365f6c0db26d473d85a736d318c2f565e5", "is_verified": false, - "line_number": 991, - "is_secret": false + "line_number": 991 } ], "pkg/client/iam/gocloak_moq.go": [ @@ -384,48 +386,42 @@ "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c", "is_verified": false, - "line_number": 9711, - "is_secret": false + "line_number": 9711 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "7f0b58c8f07c09a5ed45a784a8e1ea4d3e983d59", "is_verified": false, - "line_number": 9712, - "is_secret": false + "line_number": 9712 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "9b8b876c2782fa992fab14095267bb8757b9fabc", "is_verified": false, - "line_number": 13092, - "is_secret": false + "line_number": 13092 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "5baa61e4c9b93f3f0682250b6cf8331b7ee68fd8", "is_verified": false, - "line_number": 13095, - "is_secret": false + "line_number": 13095 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "eb1b883e199141e362a143c51178ab8f09c87751", "is_verified": false, - "line_number": 13716, - "is_secret": false + "line_number": 13716 }, { "type": "Secret Keyword", "filename": "pkg/client/iam/gocloak_moq.go", "hashed_secret": "1b46ecc8fb47b1b39a420f00f08dbd58e0313188", "is_verified": false, - "line_number": 14023, - "is_secret": false + "line_number": 14023 } ], "pkg/client/redhatsso/api/api/openapi.yaml": [ @@ -443,8 +439,7 @@ "filename": "pkg/shared/secrets/secrets_test.go", "hashed_secret": "a94a8fe5ccb19ba61c4c0873d391e987982fbbd3", "is_verified": false, - "line_number": 113, - "is_secret": false + "line_number": 113 } ], "templates/envoy-config-configmap.yml": [ @@ -549,8 +544,7 @@ "filename": "test/support/certs.json", "hashed_secret": "d59844c767c4c6c3840f8cabbc04b1e5ed2acc22", "is_verified": false, - "line_number": 8, - "is_secret": false + "line_number": 8 } ], "test/support/jwt_private_key.pem": [ @@ -559,10 +553,17 @@ "filename": "test/support/jwt_private_key.pem", "hashed_secret": "be4fc4886bd949b369d5e092eb87494f12e57e5b", "is_verified": false, - "line_number": 1, - "is_secret": false + "line_number": 1 } ] }, +<<<<<<< HEAD "generated_at": "2024-01-10T15:22:39Z" +======= +<<<<<<< HEAD + "generated_at": "2024-01-09T13:44:27Z" +======= + "generated_at": "2023-12-19T19:47:30Z" +>>>>>>> 7f3fa845 (add rotate secret backup feature to admin API) +>>>>>>> 6204164f (add rotate secret backup feature to admin API) } diff --git a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml index 644b9ae457..fe363d8ce4 100644 --- a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml +++ b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml @@ -396,7 +396,7 @@ paths: required: true responses: "200": - description: RHSSO client successfully rotated + description: Secret successfully rotated "401": content: application/json: @@ -422,7 +422,7 @@ paths: schema: $ref: '#/components/schemas/Error' description: Unexpected error occurred - summary: Rotate RHSSO client of a central tenant + summary: Rotate RHSSO client or Secret Backup of a central tenant /api/rhacs/v1/admin/centrals/{id}/restore: post: parameters: @@ -523,9 +523,12 @@ components: CentralRotateSecretsRequest: example: rotate_rhsso_client_credentials: true + rotate_secret_backup: true properties: rotate_rhsso_client_credentials: type: boolean + rotate_secret_backup: + type: boolean type: object Error: allOf: diff --git a/internal/dinosaur/pkg/api/admin/private/api_default.go b/internal/dinosaur/pkg/api/admin/private/api_default.go index f5c7f53336..a2c66fb2b8 100644 --- a/internal/dinosaur/pkg/api/admin/private/api_default.go +++ b/internal/dinosaur/pkg/api/admin/private/api_default.go @@ -143,7 +143,7 @@ func (a *DefaultApiService) ApiRhacsV1AdminCentralsIdRestorePost(ctx _context.Co } /* -ApiRhacsV1AdminCentralsIdRotateSecretsPost Rotate RHSSO client of a central tenant +ApiRhacsV1AdminCentralsIdRotateSecretsPost Rotate RHSSO client or Secret Backup of a central tenant - @param ctx _context.Context - for authentication, logging, cancellation, deadlines, tracing, etc. Passed from http.Request or context.Background(). - @param id The ID of record - @param centralRotateSecretsRequest Options for secret rotation diff --git a/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go b/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go index 923a99f7e5..64c7afd6b4 100644 --- a/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go +++ b/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go @@ -13,4 +13,5 @@ package private // CentralRotateSecretsRequest struct for CentralRotateSecretsRequest type CentralRotateSecretsRequest struct { RotateRhssoClientCredentials bool `json:"rotate_rhsso_client_credentials,omitempty"` + RotateSecretBackup bool `json:"rotate_secret_backup,omitempty"` } diff --git a/internal/dinosaur/pkg/handlers/admin_dinosaur.go b/internal/dinosaur/pkg/handlers/admin_dinosaur.go index 147ff87359..2b52290d87 100644 --- a/internal/dinosaur/pkg/handlers/admin_dinosaur.go +++ b/internal/dinosaur/pkg/handlers/admin_dinosaur.go @@ -231,6 +231,14 @@ func (h adminCentralHandler) RotateSecrets(w http.ResponseWriter, r *http.Reques return nil, svcErr } } + + if rotateSecretsRequest.RotateSecretBackup { + svcErr = h.service.RotateCentralSecretBackup(ctx, centralRequest) + if svcErr != nil { + return nil, svcErr + } + } + return nil, nil }, } diff --git a/internal/dinosaur/pkg/services/dinosaur.go b/internal/dinosaur/pkg/services/dinosaur.go index 4f3e7f7d4a..c6b0e2ce0a 100644 --- a/internal/dinosaur/pkg/services/dinosaur.go +++ b/internal/dinosaur/pkg/services/dinosaur.go @@ -107,6 +107,7 @@ type DinosaurService interface { VerifyAndUpdateDinosaurAdmin(ctx context.Context, dinosaurRequest *dbapi.CentralRequest) *errors.ServiceError Restore(ctx context.Context, id string) *errors.ServiceError RotateCentralRHSSOClient(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError + RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError } var _ DinosaurService = &dinosaurService{} @@ -170,6 +171,17 @@ func (k *dinosaurService) RotateCentralRHSSOClient(ctx context.Context, centralR return nil } +func (k *dinosaurService) RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError { + centralRequest.Secrets = nil // pragma: allowlist secret + + dbConn := k.connectionFactory.New() + if err := dbConn.Unscoped().Model(centralRequest).Select("secrets").Updates(centralRequest).Error; err != nil { + return errors.NewWithCause(errors.ErrorGeneral, err, "Unable to reset secrets for central request") + } + + return nil +} + // HasAvailableCapacityInRegion ... func (k *dinosaurService) HasAvailableCapacityInRegion(dinosaurRequest *dbapi.CentralRequest) (bool, *errors.ServiceError) { regionCapacity := int64(k.dataplaneClusterConfig.ClusterConfig.GetCapacityForRegion(dinosaurRequest.Region)) diff --git a/internal/dinosaur/pkg/services/dinosaurservice_moq.go b/internal/dinosaur/pkg/services/dinosaurservice_moq.go index d33b51fac6..68d1a793b8 100644 --- a/internal/dinosaur/pkg/services/dinosaurservice_moq.go +++ b/internal/dinosaur/pkg/services/dinosaurservice_moq.go @@ -88,6 +88,9 @@ var _ DinosaurService = &DinosaurServiceMock{} // RotateCentralRHSSOClientFunc: func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { // panic("mock out the RotateCentralRHSSOClient method") // }, +// RotateCentralSecretBackupFunc: func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { +// panic("mock out the RotateCentralSecretBackup method") +// }, // UpdateFunc: func(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError { // panic("mock out the Update method") // }, @@ -170,6 +173,9 @@ type DinosaurServiceMock struct { // RotateCentralRHSSOClientFunc mocks the RotateCentralRHSSOClient method. RotateCentralRHSSOClientFunc func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError + // RotateCentralSecretBackupFunc mocks the RotateCentralSecretBackup method. + RotateCentralSecretBackupFunc func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError + // UpdateFunc mocks the Update method. UpdateFunc func(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError @@ -297,6 +303,13 @@ type DinosaurServiceMock struct { // CentralRequest is the centralRequest argument value. CentralRequest *dbapi.CentralRequest } + // RotateCentralSecretBackup holds details about calls to the RotateCentralSecretBackup method. + RotateCentralSecretBackup []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // CentralRequest is the centralRequest argument value. + CentralRequest *dbapi.CentralRequest + } // Update holds details about calls to the Update method. Update []struct { // DinosaurRequest is the dinosaurRequest argument value. @@ -345,6 +358,7 @@ type DinosaurServiceMock struct { lockRegisterDinosaurJob sync.RWMutex lockRestore sync.RWMutex lockRotateCentralRHSSOClient sync.RWMutex + lockRotateCentralSecretBackup sync.RWMutex lockUpdate sync.RWMutex lockUpdateStatus sync.RWMutex lockUpdates sync.RWMutex @@ -1035,6 +1049,42 @@ func (mock *DinosaurServiceMock) RotateCentralRHSSOClientCalls() []struct { return calls } +// RotateCentralSecretBackup calls RotateCentralSecretBackupFunc. +func (mock *DinosaurServiceMock) RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { + if mock.RotateCentralSecretBackupFunc == nil { + panic("DinosaurServiceMock.RotateCentralSecretBackupFunc: method is nil but DinosaurService.RotateCentralSecretBackup was just called") + } + callInfo := struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest + }{ + Ctx: ctx, + CentralRequest: centralRequest, + } + mock.lockRotateCentralSecretBackup.Lock() + mock.calls.RotateCentralSecretBackup = append(mock.calls.RotateCentralSecretBackup, callInfo) + mock.lockRotateCentralSecretBackup.Unlock() + return mock.RotateCentralSecretBackupFunc(ctx, centralRequest) +} + +// RotateCentralSecretBackupCalls gets all the calls that were made to RotateCentralSecretBackup. +// Check the length with: +// +// len(mockedDinosaurService.RotateCentralSecretBackupCalls()) +func (mock *DinosaurServiceMock) RotateCentralSecretBackupCalls() []struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest +} { + var calls []struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest + } + mock.lockRotateCentralSecretBackup.RLock() + calls = mock.calls.RotateCentralSecretBackup + mock.lockRotateCentralSecretBackup.RUnlock() + return calls +} + // Update calls UpdateFunc. func (mock *DinosaurServiceMock) Update(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError { if mock.UpdateFunc == nil { diff --git a/openapi/fleet-manager-private-admin.yaml b/openapi/fleet-manager-private-admin.yaml index ec09862e7c..079ca518df 100644 --- a/openapi/fleet-manager-private-admin.yaml +++ b/openapi/fleet-manager-private-admin.yaml @@ -259,7 +259,7 @@ paths: $ref: 'fleet-manager.yaml#/components/schemas/Error' '/api/rhacs/v1/admin/centrals/{id}/rotate-secrets': post: - summary: Rotate RHSSO client of a central tenant + summary: Rotate RHSSO client or Secret Backup of a central tenant parameters: - $ref: "fleet-manager.yaml#/components/parameters/id" requestBody: @@ -271,7 +271,7 @@ paths: required: true responses: "200": - description: RHSSO client successfully rotated + description: Secret successfully rotated "401": description: Auth token is invalid content: @@ -454,6 +454,8 @@ components: properties: rotate_rhsso_client_credentials: type: boolean + rotate_secret_backup: + type: boolean securitySchemes: Bearer: From 665d482139a4dc6d32dd1f2ac84e4e546480d9d4 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Tue, 19 Dec 2023 20:54:15 +0100 Subject: [PATCH 02/20] do not make an unscoped DB request --- internal/dinosaur/pkg/services/dinosaur.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/dinosaur/pkg/services/dinosaur.go b/internal/dinosaur/pkg/services/dinosaur.go index c6b0e2ce0a..ca32f1ac9c 100644 --- a/internal/dinosaur/pkg/services/dinosaur.go +++ b/internal/dinosaur/pkg/services/dinosaur.go @@ -175,7 +175,7 @@ func (k *dinosaurService) RotateCentralSecretBackup(ctx context.Context, central centralRequest.Secrets = nil // pragma: allowlist secret dbConn := k.connectionFactory.New() - if err := dbConn.Unscoped().Model(centralRequest).Select("secrets").Updates(centralRequest).Error; err != nil { + if err := dbConn.Model(centralRequest).Select("secrets").Updates(centralRequest).Error; err != nil { return errors.NewWithCause(errors.ErrorGeneral, err, "Unable to reset secrets for central request") } From bf869c19242e16daf6c0f673841444f030b663fc Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Tue, 19 Dec 2023 21:04:20 +0100 Subject: [PATCH 03/20] fix secret baseline --- .secrets.baseline | 8 -------- 1 file changed, 8 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 683e240b09..dc7d8cf30b 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -557,13 +557,5 @@ } ] }, -<<<<<<< HEAD "generated_at": "2024-01-10T15:22:39Z" -======= -<<<<<<< HEAD - "generated_at": "2024-01-09T13:44:27Z" -======= - "generated_at": "2023-12-19T19:47:30Z" ->>>>>>> 7f3fa845 (add rotate secret backup feature to admin API) ->>>>>>> 6204164f (add rotate secret backup feature to admin API) } From b71140f7d280ec2206c6f88beb933572b7090e42 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Tue, 19 Dec 2023 21:22:13 +0100 Subject: [PATCH 04/20] call the flag reset instead of rotate --- .secrets.baseline | 6 +- .../pkg/api/admin/private/api/openapi.yaml | 4 +- .../model_central_rotate_secrets_request.go | 2 +- .../dinosaur/pkg/handlers/admin_dinosaur.go | 4 +- internal/dinosaur/pkg/services/dinosaur.go | 4 +- .../pkg/services/dinosaurservice_moq.go | 100 +++++++++--------- openapi/fleet-manager-private-admin.yaml | 2 +- 7 files changed, 61 insertions(+), 61 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index dc7d8cf30b..9263d09591 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -347,14 +347,14 @@ "filename": "internal/dinosaur/pkg/services/dinosaurservice_moq.go", "hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c", "is_verified": false, - "line_number": 1054 + "line_number": 982 }, { "type": "Secret Keyword", "filename": "internal/dinosaur/pkg/services/dinosaurservice_moq.go", - "hashed_secret": "b41a19f9cb43a6475a32a4748e222e5e8f7dce2b", + "hashed_secret": "d035c0406b3e8286d3427e91db3497e0e17f0f83", "is_verified": false, - "line_number": 1055 + "line_number": 983 } ], "pkg/client/iam/client_moq.go": [ diff --git a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml index fe363d8ce4..c94a381e6e 100644 --- a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml +++ b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml @@ -522,12 +522,12 @@ components: - $ref: '#/components/schemas/CentralList_allOf' CentralRotateSecretsRequest: example: + reset_secret_backup: true rotate_rhsso_client_credentials: true - rotate_secret_backup: true properties: rotate_rhsso_client_credentials: type: boolean - rotate_secret_backup: + reset_secret_backup: type: boolean type: object Error: diff --git a/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go b/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go index 64c7afd6b4..34b6412ca0 100644 --- a/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go +++ b/internal/dinosaur/pkg/api/admin/private/model_central_rotate_secrets_request.go @@ -13,5 +13,5 @@ package private // CentralRotateSecretsRequest struct for CentralRotateSecretsRequest type CentralRotateSecretsRequest struct { RotateRhssoClientCredentials bool `json:"rotate_rhsso_client_credentials,omitempty"` - RotateSecretBackup bool `json:"rotate_secret_backup,omitempty"` + ResetSecretBackup bool `json:"reset_secret_backup,omitempty"` } diff --git a/internal/dinosaur/pkg/handlers/admin_dinosaur.go b/internal/dinosaur/pkg/handlers/admin_dinosaur.go index 2b52290d87..31e0f9c261 100644 --- a/internal/dinosaur/pkg/handlers/admin_dinosaur.go +++ b/internal/dinosaur/pkg/handlers/admin_dinosaur.go @@ -232,8 +232,8 @@ func (h adminCentralHandler) RotateSecrets(w http.ResponseWriter, r *http.Reques } } - if rotateSecretsRequest.RotateSecretBackup { - svcErr = h.service.RotateCentralSecretBackup(ctx, centralRequest) + if rotateSecretsRequest.ResetSecretBackup { + svcErr = h.service.ResetCentralSecretBackup(ctx, centralRequest) if svcErr != nil { return nil, svcErr } diff --git a/internal/dinosaur/pkg/services/dinosaur.go b/internal/dinosaur/pkg/services/dinosaur.go index ca32f1ac9c..9780fbf4c8 100644 --- a/internal/dinosaur/pkg/services/dinosaur.go +++ b/internal/dinosaur/pkg/services/dinosaur.go @@ -107,7 +107,7 @@ type DinosaurService interface { VerifyAndUpdateDinosaurAdmin(ctx context.Context, dinosaurRequest *dbapi.CentralRequest) *errors.ServiceError Restore(ctx context.Context, id string) *errors.ServiceError RotateCentralRHSSOClient(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError - RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError + ResetCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError } var _ DinosaurService = &dinosaurService{} @@ -171,7 +171,7 @@ func (k *dinosaurService) RotateCentralRHSSOClient(ctx context.Context, centralR return nil } -func (k *dinosaurService) RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError { +func (k *dinosaurService) ResetCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError { centralRequest.Secrets = nil // pragma: allowlist secret dbConn := k.connectionFactory.New() diff --git a/internal/dinosaur/pkg/services/dinosaurservice_moq.go b/internal/dinosaur/pkg/services/dinosaurservice_moq.go index 68d1a793b8..6fe4140cf3 100644 --- a/internal/dinosaur/pkg/services/dinosaurservice_moq.go +++ b/internal/dinosaur/pkg/services/dinosaurservice_moq.go @@ -82,15 +82,15 @@ var _ DinosaurService = &DinosaurServiceMock{} // RegisterDinosaurJobFunc: func(ctx context.Context, dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError { // panic("mock out the RegisterDinosaurJob method") // }, +// ResetCentralSecretBackupFunc: func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { +// panic("mock out the ResetCentralSecretBackup method") +// }, // RestoreFunc: func(ctx context.Context, id string) *serviceError.ServiceError { // panic("mock out the Restore method") // }, // RotateCentralRHSSOClientFunc: func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { // panic("mock out the RotateCentralRHSSOClient method") // }, -// RotateCentralSecretBackupFunc: func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { -// panic("mock out the RotateCentralSecretBackup method") -// }, // UpdateFunc: func(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError { // panic("mock out the Update method") // }, @@ -167,15 +167,15 @@ type DinosaurServiceMock struct { // RegisterDinosaurJobFunc mocks the RegisterDinosaurJob method. RegisterDinosaurJobFunc func(ctx context.Context, dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError + // ResetCentralSecretBackupFunc mocks the ResetCentralSecretBackup method. + ResetCentralSecretBackupFunc func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError + // RestoreFunc mocks the Restore method. RestoreFunc func(ctx context.Context, id string) *serviceError.ServiceError // RotateCentralRHSSOClientFunc mocks the RotateCentralRHSSOClient method. RotateCentralRHSSOClientFunc func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError - // RotateCentralSecretBackupFunc mocks the RotateCentralSecretBackup method. - RotateCentralSecretBackupFunc func(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError - // UpdateFunc mocks the Update method. UpdateFunc func(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError @@ -289,6 +289,13 @@ type DinosaurServiceMock struct { // DinosaurRequest is the dinosaurRequest argument value. DinosaurRequest *dbapi.CentralRequest } + // ResetCentralSecretBackup holds details about calls to the ResetCentralSecretBackup method. + ResetCentralSecretBackup []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // CentralRequest is the centralRequest argument value. + CentralRequest *dbapi.CentralRequest + } // Restore holds details about calls to the Restore method. Restore []struct { // Ctx is the ctx argument value. @@ -303,13 +310,6 @@ type DinosaurServiceMock struct { // CentralRequest is the centralRequest argument value. CentralRequest *dbapi.CentralRequest } - // RotateCentralSecretBackup holds details about calls to the RotateCentralSecretBackup method. - RotateCentralSecretBackup []struct { - // Ctx is the ctx argument value. - Ctx context.Context - // CentralRequest is the centralRequest argument value. - CentralRequest *dbapi.CentralRequest - } // Update holds details about calls to the Update method. Update []struct { // DinosaurRequest is the dinosaurRequest argument value. @@ -356,9 +356,9 @@ type DinosaurServiceMock struct { lockPrepareDinosaurRequest sync.RWMutex lockRegisterDinosaurDeprovisionJob sync.RWMutex lockRegisterDinosaurJob sync.RWMutex + lockResetCentralSecretBackup sync.RWMutex lockRestore sync.RWMutex lockRotateCentralRHSSOClient sync.RWMutex - lockRotateCentralSecretBackup sync.RWMutex lockUpdate sync.RWMutex lockUpdateStatus sync.RWMutex lockUpdates sync.RWMutex @@ -977,6 +977,42 @@ func (mock *DinosaurServiceMock) RegisterDinosaurJobCalls() []struct { return calls } +// ResetCentralSecretBackup calls ResetCentralSecretBackupFunc. +func (mock *DinosaurServiceMock) ResetCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { + if mock.ResetCentralSecretBackupFunc == nil { + panic("DinosaurServiceMock.ResetCentralSecretBackupFunc: method is nil but DinosaurService.ResetCentralSecretBackup was just called") + } + callInfo := struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest + }{ + Ctx: ctx, + CentralRequest: centralRequest, + } + mock.lockResetCentralSecretBackup.Lock() + mock.calls.ResetCentralSecretBackup = append(mock.calls.ResetCentralSecretBackup, callInfo) + mock.lockResetCentralSecretBackup.Unlock() + return mock.ResetCentralSecretBackupFunc(ctx, centralRequest) +} + +// ResetCentralSecretBackupCalls gets all the calls that were made to ResetCentralSecretBackup. +// Check the length with: +// +// len(mockedDinosaurService.ResetCentralSecretBackupCalls()) +func (mock *DinosaurServiceMock) ResetCentralSecretBackupCalls() []struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest +} { + var calls []struct { + Ctx context.Context + CentralRequest *dbapi.CentralRequest + } + mock.lockResetCentralSecretBackup.RLock() + calls = mock.calls.ResetCentralSecretBackup + mock.lockResetCentralSecretBackup.RUnlock() + return calls +} + // Restore calls RestoreFunc. func (mock *DinosaurServiceMock) Restore(ctx context.Context, id string) *serviceError.ServiceError { if mock.RestoreFunc == nil { @@ -1049,42 +1085,6 @@ func (mock *DinosaurServiceMock) RotateCentralRHSSOClientCalls() []struct { return calls } -// RotateCentralSecretBackup calls RotateCentralSecretBackupFunc. -func (mock *DinosaurServiceMock) RotateCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *serviceError.ServiceError { - if mock.RotateCentralSecretBackupFunc == nil { - panic("DinosaurServiceMock.RotateCentralSecretBackupFunc: method is nil but DinosaurService.RotateCentralSecretBackup was just called") - } - callInfo := struct { - Ctx context.Context - CentralRequest *dbapi.CentralRequest - }{ - Ctx: ctx, - CentralRequest: centralRequest, - } - mock.lockRotateCentralSecretBackup.Lock() - mock.calls.RotateCentralSecretBackup = append(mock.calls.RotateCentralSecretBackup, callInfo) - mock.lockRotateCentralSecretBackup.Unlock() - return mock.RotateCentralSecretBackupFunc(ctx, centralRequest) -} - -// RotateCentralSecretBackupCalls gets all the calls that were made to RotateCentralSecretBackup. -// Check the length with: -// -// len(mockedDinosaurService.RotateCentralSecretBackupCalls()) -func (mock *DinosaurServiceMock) RotateCentralSecretBackupCalls() []struct { - Ctx context.Context - CentralRequest *dbapi.CentralRequest -} { - var calls []struct { - Ctx context.Context - CentralRequest *dbapi.CentralRequest - } - mock.lockRotateCentralSecretBackup.RLock() - calls = mock.calls.RotateCentralSecretBackup - mock.lockRotateCentralSecretBackup.RUnlock() - return calls -} - // Update calls UpdateFunc. func (mock *DinosaurServiceMock) Update(dinosaurRequest *dbapi.CentralRequest) *serviceError.ServiceError { if mock.UpdateFunc == nil { diff --git a/openapi/fleet-manager-private-admin.yaml b/openapi/fleet-manager-private-admin.yaml index 079ca518df..0e2ad75384 100644 --- a/openapi/fleet-manager-private-admin.yaml +++ b/openapi/fleet-manager-private-admin.yaml @@ -454,7 +454,7 @@ components: properties: rotate_rhsso_client_credentials: type: boolean - rotate_secret_backup: + reset_secret_backup: type: boolean securitySchemes: From 48931e39643776361b2b53028f79676375f43820 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Tue, 19 Dec 2023 21:30:58 +0100 Subject: [PATCH 05/20] add comment --- internal/dinosaur/pkg/services/dinosaur.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/dinosaur/pkg/services/dinosaur.go b/internal/dinosaur/pkg/services/dinosaur.go index 9780fbf4c8..6a9d418e00 100644 --- a/internal/dinosaur/pkg/services/dinosaur.go +++ b/internal/dinosaur/pkg/services/dinosaur.go @@ -107,6 +107,10 @@ type DinosaurService interface { VerifyAndUpdateDinosaurAdmin(ctx context.Context, dinosaurRequest *dbapi.CentralRequest) *errors.ServiceError Restore(ctx context.Context, id string) *errors.ServiceError RotateCentralRHSSOClient(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError + // ResetCentralSecretBackup resets the Secret field of centralReqest, which are the backed up secrets + // of a tenant. By resetting the field the next update will store new secrets which enables manual rotation. + // This is currently the only way to update secret backups, an automatic approach should be implemented + // to accomated for regular processes like central TLS cert rotation. ResetCentralSecretBackup(ctx context.Context, centralRequest *dbapi.CentralRequest) *errors.ServiceError } From a3b1f682325cf465554c603d45db25d06e63b87c Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Wed, 10 Jan 2024 16:42:57 +0100 Subject: [PATCH 06/20] add e2e test for secret backup rotation --- .secrets.baseline | 36 +++++++ e2e/e2e_test.go | 94 ++++++++++++++----- .../pkg/api/admin/private/api/openapi.yaml | 1 + .../pkg/api/admin/private/api_default.go | 4 +- openapi/fleet-manager-private-admin.yaml | 1 + pkg/client/fleetmanager/api_moq.go | 62 +++++++++++- pkg/client/fleetmanager/client.go | 1 + 7 files changed, 170 insertions(+), 29 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 9263d09591..3da243aa78 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -75,6 +75,10 @@ { "path": "detect_secrets.filters.allowlist.is_line_allowlisted" }, + { + "path": "detect_secrets.filters.common.is_baseline_file", + "filename": ".secrets.baseline" + }, { "path": "detect_secrets.filters.common.is_ignored_due_to_verification_policies", "min_level": 2 @@ -323,6 +327,15 @@ "line_number": 7 } ], + "e2e/e2e_test.go": [ + { + "type": "Secret Keyword", + "filename": "e2e/e2e_test.go", + "hashed_secret": "7f38822bc2b03e97325ff310099f457f6f788daf", + "is_verified": false, + "line_number": 267 + } + ], "fleetshard/pkg/central/cloudprovider/dbclient_moq.go": [ { "type": "Secret Keyword", @@ -357,6 +370,29 @@ "line_number": 983 } ], + "pkg/client/fleetmanager/api_moq.go": [ + { + "type": "Secret Keyword", + "filename": "pkg/client/fleetmanager/api_moq.go", + "hashed_secret": "44e17306b837162269a410204daaa5ecee4ec22c", + "is_verified": false, + "line_number": 567 + }, + { + "type": "Secret Keyword", + "filename": "pkg/client/fleetmanager/api_moq.go", + "hashed_secret": "0ff50155b4f57adeccae93f27dc23efe2a8b7824", + "is_verified": false, + "line_number": 568 + }, + { + "type": "Secret Keyword", + "filename": "pkg/client/fleetmanager/api_moq.go", + "hashed_secret": "5ce1b8d4fb9dae5c02b2017e39e7267a21cea37f", + "is_verified": false, + "line_number": 577 + } + ], "pkg/client/iam/client_moq.go": [ { "type": "Secret Keyword", diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index d61dc0ac91..96c33bdf64 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/rand" "encoding/hex" + "errors" "fmt" "net" "net/url" @@ -240,29 +241,12 @@ var _ = Describe("Central", Ordered, func() { // TODO(ROX-11368): Create test to check Central is correctly exposed It("should restore secrets and deployment on namespace delete", func() { - previousNamespace := corev1.Namespace{} - Expect(assertNamespaceExists(ctx, &previousNamespace, namespaceName)()). - To(Succeed()) - // Using managedDB false here because e2e don't run with managed postgresql secretBackup := k8s.NewSecretBackup(k8sClient, false) expectedSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) Expect(err).ToNot(HaveOccurred()) - previousCreationTime := previousNamespace.CreationTimestamp - Expect(k8sClient.Delete(ctx, &previousNamespace)). - NotTo(HaveOccurred()) - - Eventually(func() error { - newNamespace := corev1.Namespace{} - if err := k8sClient.Get(ctx, ctrlClient.ObjectKey{Name: namespaceName}, &newNamespace); err != nil { - return err - } - if previousCreationTime.Equal(&newNamespace.CreationTimestamp) { - return fmt.Errorf("namespace found but was not yet deleted") - } - return nil - }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) + deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) actualSecrets := map[string]*corev1.Secret{} Eventually(func() (err error) { @@ -270,13 +254,44 @@ var _ = Describe("Central", Ordered, func() { return err }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) - Expect(actualSecrets).ToNot(BeEmpty()) - Expect(len(actualSecrets)).To(Equal(len(expectedSecrets))) - for secretName := range expectedSecrets { // pragma: allowlist secret - actualData := actualSecrets[secretName].StringData - expectedData := expectedSecrets[secretName].StringData - Expect(actualData).To(Equal(expectedData)) + assertEqualSecrets(actualSecrets, expectedSecrets) + }) + + It("should delete and recreate secret backup for admin reset API", func() { + secretBackup := k8s.NewSecretBackup(k8sClient, false) + oldSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) + Expect(err).ToNot(HaveOccurred()) + Expect(oldSecrets).ToNot(BeEmpty()) + + // modify secrets to later test that the backup was updated succesfully + for _, secret := range oldSecrets { + secret.StringData["test"] = "modified" + err := k8sClient.Update(ctx, secret) + Expect(err).ToNot(HaveOccurred()) } + + _, err = adminAPI.CentralRotateSecrets(ctx, centralRequestID, private.CentralRotateSecretsRequest{ResetSecretBackup: true}) + Expect(err).ToNot(HaveOccurred()) + + // Wait for secrets to be backed up again + Eventually(func() error { + central, _, err := client.PrivateAPI().GetCentral(ctx, centralRequestID) + Expect(err).ToNot(HaveOccurred()) + if len(central.Metadata.Secrets) == 0 { + return errors.New("secrets backup is empty") + } + + return nil + }). + WithTimeout(defaultTimeout). + WithPolling(defaultPolling). + Should(Succeed()) + + deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) + + newSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) + Expect(err).ToNot(HaveOccurred()) + assertEqualSecrets(newSecrets, oldSecrets) }) It("should transition central to deprovisioning state", func() { @@ -487,3 +502,34 @@ func printNotes(notes []string) { GinkgoWriter.Println(note) } } + +func deleteNamespaceAndWaitForRecreation(ctx context.Context, namespaceName string, k8sClient ctrlClient.Client) { + previousNamespace := corev1.Namespace{} + Expect(assertNamespaceExists(ctx, &previousNamespace, namespaceName)()). + To(Succeed()) + + previousCreationTime := previousNamespace.CreationTimestamp + Expect(k8sClient.Delete(ctx, &previousNamespace)). + NotTo(HaveOccurred()) + + Eventually(func() error { + newNamespace := corev1.Namespace{} + if err := k8sClient.Get(ctx, ctrlClient.ObjectKey{Name: namespaceName}, &newNamespace); err != nil { + return err + } + if previousCreationTime.Equal(&newNamespace.CreationTimestamp) { + return fmt.Errorf("namespace found but was not yet deleted") + } + return nil + }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) +} + +func assertEqualSecrets(actualSecrets, expectedSecrets map[string]*corev1.Secret) { + Expect(actualSecrets).ToNot(BeEmpty()) + Expect(len(actualSecrets)).To(Equal(len(expectedSecrets))) + for secretName := range expectedSecrets { // pragma: allowlist secret + actualData := actualSecrets[secretName].StringData + expectedData := expectedSecrets[secretName].StringData + Expect(actualData).To(Equal(expectedData)) + } +} diff --git a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml index c94a381e6e..7baf3aff01 100644 --- a/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml +++ b/internal/dinosaur/pkg/api/admin/private/api/openapi.yaml @@ -380,6 +380,7 @@ paths: summary: Update `expired_at` central property /api/rhacs/v1/admin/centrals/{id}/rotate-secrets: post: + operationId: centralRotateSecrets parameters: - description: The ID of record in: path diff --git a/internal/dinosaur/pkg/api/admin/private/api_default.go b/internal/dinosaur/pkg/api/admin/private/api_default.go index a2c66fb2b8..d90b0ba4e4 100644 --- a/internal/dinosaur/pkg/api/admin/private/api_default.go +++ b/internal/dinosaur/pkg/api/admin/private/api_default.go @@ -143,12 +143,12 @@ func (a *DefaultApiService) ApiRhacsV1AdminCentralsIdRestorePost(ctx _context.Co } /* -ApiRhacsV1AdminCentralsIdRotateSecretsPost Rotate RHSSO client or Secret Backup of a central tenant +CentralRotateSecrets Rotate RHSSO client or Secret Backup of a central tenant - @param ctx _context.Context - for authentication, logging, cancellation, deadlines, tracing, etc. Passed from http.Request or context.Background(). - @param id The ID of record - @param centralRotateSecretsRequest Options for secret rotation */ -func (a *DefaultApiService) ApiRhacsV1AdminCentralsIdRotateSecretsPost(ctx _context.Context, id string, centralRotateSecretsRequest CentralRotateSecretsRequest) (*_nethttp.Response, error) { +func (a *DefaultApiService) CentralRotateSecrets(ctx _context.Context, id string, centralRotateSecretsRequest CentralRotateSecretsRequest) (*_nethttp.Response, error) { var ( localVarHTTPMethod = _nethttp.MethodPost localVarPostBody interface{} diff --git a/openapi/fleet-manager-private-admin.yaml b/openapi/fleet-manager-private-admin.yaml index 0e2ad75384..c8febe4ca6 100644 --- a/openapi/fleet-manager-private-admin.yaml +++ b/openapi/fleet-manager-private-admin.yaml @@ -259,6 +259,7 @@ paths: $ref: 'fleet-manager.yaml#/components/schemas/Error' '/api/rhacs/v1/admin/centrals/{id}/rotate-secrets': post: + operationId: centralRotateSecrets summary: Rotate RHSSO client or Secret Backup of a central tenant parameters: - $ref: "fleet-manager.yaml#/components/parameters/id" diff --git a/pkg/client/fleetmanager/api_moq.go b/pkg/client/fleetmanager/api_moq.go index 29b17c1e9f..b7541cb57c 100644 --- a/pkg/client/fleetmanager/api_moq.go +++ b/pkg/client/fleetmanager/api_moq.go @@ -490,6 +490,9 @@ var _ AdminAPI = &AdminAPIMock{} // // // make and configure a mocked AdminAPI // mockedAdminAPI := &AdminAPIMock{ +// CentralRotateSecretsFunc: func(ctx context.Context, id string, centralRotateSecretsRequest admin.CentralRotateSecretsRequest) (*http.Response, error) { +// panic("mock out the CentralRotateSecrets method") +// }, // CreateCentralFunc: func(ctx context.Context, async bool, centralRequestPayload admin.CentralRequestPayload) (admin.CentralRequest, *http.Response, error) { // panic("mock out the CreateCentral method") // }, @@ -506,6 +509,9 @@ var _ AdminAPI = &AdminAPIMock{} // // } type AdminAPIMock struct { + // CentralRotateSecretsFunc mocks the CentralRotateSecrets method. + CentralRotateSecretsFunc func(ctx context.Context, id string, centralRotateSecretsRequest admin.CentralRotateSecretsRequest) (*http.Response, error) + // CreateCentralFunc mocks the CreateCentral method. CreateCentralFunc func(ctx context.Context, async bool, centralRequestPayload admin.CentralRequestPayload) (admin.CentralRequest, *http.Response, error) @@ -517,6 +523,15 @@ type AdminAPIMock struct { // calls tracks calls to the methods. calls struct { + // CentralRotateSecrets holds details about calls to the CentralRotateSecrets method. + CentralRotateSecrets []struct { + // Ctx is the ctx argument value. + Ctx context.Context + // ID is the id argument value. + ID string + // CentralRotateSecretsRequest is the centralRotateSecretsRequest argument value. + CentralRotateSecretsRequest admin.CentralRotateSecretsRequest + } // CreateCentral holds details about calls to the CreateCentral method. CreateCentral []struct { // Ctx is the ctx argument value. @@ -541,9 +556,50 @@ type AdminAPIMock struct { LocalVarOptionals *admin.GetCentralsOpts } } - lockCreateCentral sync.RWMutex - lockDeleteDbCentralById sync.RWMutex - lockGetCentrals sync.RWMutex + lockCentralRotateSecrets sync.RWMutex + lockCreateCentral sync.RWMutex + lockDeleteDbCentralById sync.RWMutex + lockGetCentrals sync.RWMutex +} + +// CentralRotateSecrets calls CentralRotateSecretsFunc. +func (mock *AdminAPIMock) CentralRotateSecrets(ctx context.Context, id string, centralRotateSecretsRequest admin.CentralRotateSecretsRequest) (*http.Response, error) { + if mock.CentralRotateSecretsFunc == nil { + panic("AdminAPIMock.CentralRotateSecretsFunc: method is nil but AdminAPI.CentralRotateSecrets was just called") + } + callInfo := struct { + Ctx context.Context + ID string + CentralRotateSecretsRequest admin.CentralRotateSecretsRequest + }{ + Ctx: ctx, + ID: id, + CentralRotateSecretsRequest: centralRotateSecretsRequest, + } + mock.lockCentralRotateSecrets.Lock() + mock.calls.CentralRotateSecrets = append(mock.calls.CentralRotateSecrets, callInfo) + mock.lockCentralRotateSecrets.Unlock() + return mock.CentralRotateSecretsFunc(ctx, id, centralRotateSecretsRequest) +} + +// CentralRotateSecretsCalls gets all the calls that were made to CentralRotateSecrets. +// Check the length with: +// +// len(mockedAdminAPI.CentralRotateSecretsCalls()) +func (mock *AdminAPIMock) CentralRotateSecretsCalls() []struct { + Ctx context.Context + ID string + CentralRotateSecretsRequest admin.CentralRotateSecretsRequest +} { + var calls []struct { + Ctx context.Context + ID string + CentralRotateSecretsRequest admin.CentralRotateSecretsRequest + } + mock.lockCentralRotateSecrets.RLock() + calls = mock.calls.CentralRotateSecrets + mock.lockCentralRotateSecrets.RUnlock() + return calls } // CreateCentral calls CreateCentralFunc. diff --git a/pkg/client/fleetmanager/client.go b/pkg/client/fleetmanager/client.go index e7c7c9b293..c4a9833611 100644 --- a/pkg/client/fleetmanager/client.go +++ b/pkg/client/fleetmanager/client.go @@ -34,6 +34,7 @@ type AdminAPI interface { GetCentrals(ctx context.Context, localVarOptionals *admin.GetCentralsOpts) (admin.CentralList, *http.Response, error) CreateCentral(ctx context.Context, async bool, centralRequestPayload admin.CentralRequestPayload) (admin.CentralRequest, *http.Response, error) DeleteDbCentralById(ctx context.Context, id string) (*http.Response, error) + CentralRotateSecrets(ctx context.Context, id string, centralRotateSecretsRequest admin.CentralRotateSecretsRequest) (*http.Response, error) } var ( From 47fc0c5fa69acb3b218cede1086a0e28f50fe004 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Wed, 10 Jan 2024 17:32:33 +0100 Subject: [PATCH 07/20] use Data instead of StringData field of secrets --- e2e/e2e_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 96c33bdf64..8eab8b591e 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -265,7 +265,7 @@ var _ = Describe("Central", Ordered, func() { // modify secrets to later test that the backup was updated succesfully for _, secret := range oldSecrets { - secret.StringData["test"] = "modified" + secret.Data["test"] = []byte("modified") err := k8sClient.Update(ctx, secret) Expect(err).ToNot(HaveOccurred()) } @@ -528,8 +528,8 @@ func assertEqualSecrets(actualSecrets, expectedSecrets map[string]*corev1.Secret Expect(actualSecrets).ToNot(BeEmpty()) Expect(len(actualSecrets)).To(Equal(len(expectedSecrets))) for secretName := range expectedSecrets { // pragma: allowlist secret - actualData := actualSecrets[secretName].StringData - expectedData := expectedSecrets[secretName].StringData + actualData := actualSecrets[secretName].Data + expectedData := expectedSecrets[secretName].Data Expect(actualData).To(Equal(expectedData)) } } From a9c737031d077ac19ebd282eea09f9e1d863ff26 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 10:39:50 +0100 Subject: [PATCH 08/20] set higher timeout --- e2e/e2e_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 8eab8b591e..eb330afb4d 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -277,13 +277,13 @@ var _ = Describe("Central", Ordered, func() { Eventually(func() error { central, _, err := client.PrivateAPI().GetCentral(ctx, centralRequestID) Expect(err).ToNot(HaveOccurred()) - if len(central.Metadata.Secrets) == 0 { + if len(central.Metadata.SecretsStored) == 0 { return errors.New("secrets backup is empty") } return nil }). - WithTimeout(defaultTimeout). + WithTimeout(7 * time.Minute). WithPolling(defaultPolling). Should(Succeed()) From c5f4c28c13a80333af9a0c308df35745bd1470a6 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 11:18:48 +0100 Subject: [PATCH 09/20] higher polling interval --- e2e/e2e_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index eb330afb4d..079f663a02 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -284,7 +284,7 @@ var _ = Describe("Central", Ordered, func() { return nil }). WithTimeout(7 * time.Minute). - WithPolling(defaultPolling). + WithPolling(30 * time.Second). Should(Succeed()) deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) From 8319b0c9d2fb24e92353638a6dcac6770a679875 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 12:19:55 +0100 Subject: [PATCH 10/20] try without deletion of namespace before --- e2e/e2e_test.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 079f663a02..2515d9a016 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -240,24 +240,24 @@ var _ = Describe("Central", Ordered, func() { // TODO(ROX-11368): create test to check that Central and Scanner are healthy // TODO(ROX-11368): Create test to check Central is correctly exposed - It("should restore secrets and deployment on namespace delete", func() { - // Using managedDB false here because e2e don't run with managed postgresql - secretBackup := k8s.NewSecretBackup(k8sClient, false) - expectedSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) - Expect(err).ToNot(HaveOccurred()) + // It("should restore secrets and deployment on namespace delete", func() { + // // Using managedDB false here because e2e don't run with managed postgresql + // secretBackup := k8s.NewSecretBackup(k8sClient, false) + // expectedSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) + // Expect(err).ToNot(HaveOccurred()) - deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) + // deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) - actualSecrets := map[string]*corev1.Secret{} - Eventually(func() (err error) { - actualSecrets, err = secretBackup.CollectSecrets(ctx, namespaceName) // pragma: allowlist secret - return err - }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) + // actualSecrets := map[string]*corev1.Secret{} + // Eventually(func() (err error) { + // actualSecrets, err = secretBackup.CollectSecrets(ctx, namespaceName) // pragma: allowlist secret + // return err + // }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) - assertEqualSecrets(actualSecrets, expectedSecrets) - }) + // assertEqualSecrets(actualSecrets, expectedSecrets) + // }) - It("should delete and recreate secret backup for admin reset API", func() { + It("should rotate and restore secret backup for admin reset API", func() { secretBackup := k8s.NewSecretBackup(k8sClient, false) oldSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) Expect(err).ToNot(HaveOccurred()) From f8fbb759f7d4c14db9445317f61c17921d6badfb Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 14:32:16 +0100 Subject: [PATCH 11/20] try what happens if eventually does not call the api --- e2e/e2e_test.go | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 2515d9a016..719531590c 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -274,21 +274,17 @@ var _ = Describe("Central", Ordered, func() { Expect(err).ToNot(HaveOccurred()) // Wait for secrets to be backed up again + waitCount := 0 Eventually(func() error { - central, _, err := client.PrivateAPI().GetCentral(ctx, centralRequestID) - Expect(err).ToNot(HaveOccurred()) - if len(central.Metadata.SecretsStored) == 0 { - return errors.New("secrets backup is empty") + if waitCount < 6 { + waitCount++ + return errors.New("did not wait enough") } - return nil - }). - WithTimeout(7 * time.Minute). - WithPolling(30 * time.Second). - Should(Succeed()) - deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) + }).WithTimeout(defaultTimeout).WithPolling(10 * time.Second) + deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) newSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) Expect(err).ToNot(HaveOccurred()) assertEqualSecrets(newSecrets, oldSecrets) From d22008164174c10b36717ad22d1bdbe9daa22412 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 15:51:06 +0100 Subject: [PATCH 12/20] try with very long timeout --- e2e/e2e_test.go | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index 719531590c..fd71c26855 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -240,24 +240,24 @@ var _ = Describe("Central", Ordered, func() { // TODO(ROX-11368): create test to check that Central and Scanner are healthy // TODO(ROX-11368): Create test to check Central is correctly exposed - // It("should restore secrets and deployment on namespace delete", func() { - // // Using managedDB false here because e2e don't run with managed postgresql - // secretBackup := k8s.NewSecretBackup(k8sClient, false) - // expectedSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) - // Expect(err).ToNot(HaveOccurred()) + It("should restore secrets and deployment on namespace delete", func() { + // Using managedDB false here because e2e don't run with managed postgresql + secretBackup := k8s.NewSecretBackup(k8sClient, false) + expectedSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) + Expect(err).ToNot(HaveOccurred()) - // deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) + deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) - // actualSecrets := map[string]*corev1.Secret{} - // Eventually(func() (err error) { - // actualSecrets, err = secretBackup.CollectSecrets(ctx, namespaceName) // pragma: allowlist secret - // return err - // }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) + actualSecrets := map[string]*corev1.Secret{} + Eventually(func() (err error) { + actualSecrets, err = secretBackup.CollectSecrets(ctx, namespaceName) // pragma: allowlist secret + return err + }).WithTimeout(waitTimeout).WithPolling(defaultPolling).Should(Succeed()) - // assertEqualSecrets(actualSecrets, expectedSecrets) - // }) + assertEqualSecrets(actualSecrets, expectedSecrets) + }) - It("should rotate and restore secret backup for admin reset API", func() { + It("should delete and recreate secret backup for admin reset API", func() { secretBackup := k8s.NewSecretBackup(k8sClient, false) oldSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) Expect(err).ToNot(HaveOccurred()) @@ -274,17 +274,21 @@ var _ = Describe("Central", Ordered, func() { Expect(err).ToNot(HaveOccurred()) // Wait for secrets to be backed up again - waitCount := 0 Eventually(func() error { - if waitCount < 6 { - waitCount++ - return errors.New("did not wait enough") + central, _, err := client.PrivateAPI().GetCentral(ctx, centralRequestID) + Expect(err).ToNot(HaveOccurred()) + if len(central.Metadata.SecretsStored) == 0 { + return errors.New("secrets backup is empty") } - return nil - }).WithTimeout(defaultTimeout).WithPolling(10 * time.Second) + return nil + }). + WithTimeout(20 * time.Minute). + WithPolling(10 * time.Second). + Should(Succeed()) deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) + newSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) Expect(err).ToNot(HaveOccurred()) assertEqualSecrets(newSecrets, oldSecrets) From 5aa5d58e89cf2caef1cf80d88059b9966d5762f4 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 16:46:35 +0100 Subject: [PATCH 13/20] add some logs to see what fleetshard is doing --- fleetshard/pkg/central/reconciler/reconciler.go | 1 + fleetshard/pkg/runtime/runtime.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 0c5e287e45..67543b1fab 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -141,6 +141,7 @@ type CentralReconciler struct { // TODO(sbaumer): Should an initial ManagedCentral be added on reconciler creation? func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private.ManagedCentral) (*private.DataPlaneCentralStatus, error) { // Only allow to start reconcile function once + glog.Infof("Attempt to start reconcile but busy: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) if !atomic.CompareAndSwapInt32(r.status, FreeStatus, BlockedStatus) { return nil, ErrBusy } diff --git a/fleetshard/pkg/runtime/runtime.go b/fleetshard/pkg/runtime/runtime.go index 77047d42ef..cb6764c9b1 100644 --- a/fleetshard/pkg/runtime/runtime.go +++ b/fleetshard/pkg/runtime/runtime.go @@ -156,6 +156,7 @@ func (r *Runtime) Start() error { } if features.TargetedOperatorUpgrades.Enabled() { + glog.Infof("Start target operator install") err := r.upgradeOperator(list) if err != nil { err = errors.Wrapf(err, "Upgrading operator") @@ -168,6 +169,7 @@ func (r *Runtime) Start() error { reconciledCentralCountCache = int32(len(list.Items)) logger.InfoChangedInt32(&reconciledCentralCountCache, "Received central count changed: received %d centrals", reconciledCentralCountCache) for _, central := range list.Items { + glog.Infof("Try to start reconciliation: %s", central.Id) if _, ok := r.reconcilers[central.Id]; !ok { r.reconcilers[central.Id] = centralReconciler.NewCentralReconciler(r.k8sClient, r.client, central, r.dbProvisionClient, postgres.InitializeDatabase, r.secretCipher, r.encryptionKeyGenerator, reconcilerOpts) From e0e775cea0e06196e0bd7eeab8b9d9e6d3d07f63 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 17:19:34 +0100 Subject: [PATCH 14/20] more logs --- fleetshard/pkg/central/reconciler/reconciler.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 67543b1fab..068d5a5df4 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -141,17 +141,20 @@ type CentralReconciler struct { // TODO(sbaumer): Should an initial ManagedCentral be added on reconciler creation? func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private.ManagedCentral) (*private.DataPlaneCentralStatus, error) { // Only allow to start reconcile function once - glog.Infof("Attempt to start reconcile but busy: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) + glog.Infof("Start reconcile func: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) if !atomic.CompareAndSwapInt32(r.status, FreeStatus, BlockedStatus) { + glog.Infof("Start reconcile but busy: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) return nil, ErrBusy } defer atomic.StoreInt32(r.status, FreeStatus) + glog.Infof("Start getInstanceConfig: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) central, err := r.getInstanceConfig(&remoteCentral) if err != nil { return nil, err } + glog.Infof("Start centralChanged: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) changed, err := r.centralChanged(remoteCentral) if err != nil { return nil, errors.Wrap(err, "checking if central changed") @@ -159,6 +162,7 @@ func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private needsReconcile := r.needsReconcile(changed, central) if !needsReconcile && r.shouldSkipReadyCentral(remoteCentral) { + glog.Infof("skip reconcile: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) return nil, ErrCentralNotChanged } From ec7b924dc2b7d5a02782e4dc64bab51518ea9292 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 18:32:14 +0100 Subject: [PATCH 15/20] add an hour timer to give time to debug --- .secrets.baseline | 4 ++-- e2e/e2e_test.go | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 3da243aa78..5c77b5650e 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -333,7 +333,7 @@ "filename": "e2e/e2e_test.go", "hashed_secret": "7f38822bc2b03e97325ff310099f457f6f788daf", "is_verified": false, - "line_number": 267 + "line_number": 272 } ], "fleetshard/pkg/central/cloudprovider/dbclient_moq.go": [ @@ -593,5 +593,5 @@ } ] }, - "generated_at": "2024-01-10T15:22:39Z" + "generated_at": "2024-01-11T17:30:26Z" } diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index fd71c26855..dd029273c8 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -105,6 +105,11 @@ var _ = Describe("Central", Ordered, func() { Expect(resp.Status).To(Equal(statusAccepted)) }) + It("should wait for an hour", func() { + tick := time.NewTimer(1 * time.Hour) + <-tick.C + }) + It("should transition central request state to provisioning", func() { Eventually(assertCentralRequestProvisioning(ctx, client, centralRequestID)). WithTimeout(waitTimeout). From 649aa868ac6a594ac98abd93a5f674ee7cf48b1e Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 18:41:40 +0100 Subject: [PATCH 16/20] should always reconcile if secretsstored is empty --- .secrets.baseline | 4 ++-- e2e/e2e_test.go | 5 ----- fleetshard/pkg/central/reconciler/reconciler.go | 9 +++++++-- fleetshard/pkg/runtime/runtime.go | 1 - 4 files changed, 9 insertions(+), 10 deletions(-) diff --git a/.secrets.baseline b/.secrets.baseline index 5c77b5650e..2128910f24 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -333,7 +333,7 @@ "filename": "e2e/e2e_test.go", "hashed_secret": "7f38822bc2b03e97325ff310099f457f6f788daf", "is_verified": false, - "line_number": 272 + "line_number": 267 } ], "fleetshard/pkg/central/cloudprovider/dbclient_moq.go": [ @@ -593,5 +593,5 @@ } ] }, - "generated_at": "2024-01-11T17:30:26Z" + "generated_at": "2024-01-11T17:41:29Z" } diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index dd029273c8..fd71c26855 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -105,11 +105,6 @@ var _ = Describe("Central", Ordered, func() { Expect(resp.Status).To(Equal(statusAccepted)) }) - It("should wait for an hour", func() { - tick := time.NewTimer(1 * time.Hour) - <-tick.C - }) - It("should transition central request state to provisioning", func() { Eventually(assertCentralRequestProvisioning(ctx, client, centralRequestID)). WithTimeout(waitTimeout). diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 068d5a5df4..2ed0e5c25c 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -159,7 +159,7 @@ func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private if err != nil { return nil, errors.Wrap(err, "checking if central changed") } - needsReconcile := r.needsReconcile(changed, central) + needsReconcile := r.needsReconcile(changed, central, remoteCentral.Metadata.SecretsStored) if !needsReconcile && r.shouldSkipReadyCentral(remoteCentral) { glog.Infof("skip reconcile: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) @@ -1598,10 +1598,15 @@ func (r *CentralReconciler) shouldSkipReadyCentral(remoteCentral private.Managed isRemoteCentralReady(&remoteCentral) } -func (r *CentralReconciler) needsReconcile(changed bool, central *v1alpha1.Central) bool { +func (r *CentralReconciler) needsReconcile(changed bool, central *v1alpha1.Central, storedSecrets []string) bool { + if !r.areSecretsStored(storedSecrets) { + return true + } + if changed { return true } + forceReconcile, ok := central.Labels["rhacs.redhat.com/force-reconcile"] return ok && forceReconcile == "true" } diff --git a/fleetshard/pkg/runtime/runtime.go b/fleetshard/pkg/runtime/runtime.go index cb6764c9b1..366b27137e 100644 --- a/fleetshard/pkg/runtime/runtime.go +++ b/fleetshard/pkg/runtime/runtime.go @@ -156,7 +156,6 @@ func (r *Runtime) Start() error { } if features.TargetedOperatorUpgrades.Enabled() { - glog.Infof("Start target operator install") err := r.upgradeOperator(list) if err != nil { err = errors.Wrapf(err, "Upgrading operator") From 75cf586df4a05f1a8e2f6e21e883b142e3b2f38e Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 18:42:36 +0100 Subject: [PATCH 17/20] remove unnecessary logs --- fleetshard/pkg/central/reconciler/reconciler.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 2ed0e5c25c..950f530654 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -141,20 +141,17 @@ type CentralReconciler struct { // TODO(sbaumer): Should an initial ManagedCentral be added on reconciler creation? func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private.ManagedCentral) (*private.DataPlaneCentralStatus, error) { // Only allow to start reconcile function once - glog.Infof("Start reconcile func: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) if !atomic.CompareAndSwapInt32(r.status, FreeStatus, BlockedStatus) { glog.Infof("Start reconcile but busy: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) return nil, ErrBusy } defer atomic.StoreInt32(r.status, FreeStatus) - glog.Infof("Start getInstanceConfig: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) central, err := r.getInstanceConfig(&remoteCentral) if err != nil { return nil, err } - glog.Infof("Start centralChanged: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) changed, err := r.centralChanged(remoteCentral) if err != nil { return nil, errors.Wrap(err, "checking if central changed") From 63cc1ceba58747684038e4b68af27b55764d81e3 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 19:24:35 +0100 Subject: [PATCH 18/20] remove debug logs, fix unit tests --- e2e/e2e_test.go | 2 +- fleetshard/pkg/central/reconciler/reconciler.go | 2 -- fleetshard/pkg/central/reconciler/reconciler_test.go | 1 + fleetshard/pkg/runtime/runtime.go | 1 - 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index fd71c26855..c60fdb55de 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -283,7 +283,7 @@ var _ = Describe("Central", Ordered, func() { return nil }). - WithTimeout(20 * time.Minute). + WithTimeout(waitTimeout). WithPolling(10 * time.Second). Should(Succeed()) diff --git a/fleetshard/pkg/central/reconciler/reconciler.go b/fleetshard/pkg/central/reconciler/reconciler.go index 950f530654..4b0872faed 100644 --- a/fleetshard/pkg/central/reconciler/reconciler.go +++ b/fleetshard/pkg/central/reconciler/reconciler.go @@ -142,7 +142,6 @@ type CentralReconciler struct { func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private.ManagedCentral) (*private.DataPlaneCentralStatus, error) { // Only allow to start reconcile function once if !atomic.CompareAndSwapInt32(r.status, FreeStatus, BlockedStatus) { - glog.Infof("Start reconcile but busy: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) return nil, ErrBusy } defer atomic.StoreInt32(r.status, FreeStatus) @@ -159,7 +158,6 @@ func (r *CentralReconciler) Reconcile(ctx context.Context, remoteCentral private needsReconcile := r.needsReconcile(changed, central, remoteCentral.Metadata.SecretsStored) if !needsReconcile && r.shouldSkipReadyCentral(remoteCentral) { - glog.Infof("skip reconcile: %s/%s", remoteCentral.Metadata.Namespace, remoteCentral.Metadata.Name) return nil, ErrCentralNotChanged } diff --git a/fleetshard/pkg/central/reconciler/reconciler_test.go b/fleetshard/pkg/central/reconciler/reconciler_test.go index 7bcda8cf5c..0988331e35 100644 --- a/fleetshard/pkg/central/reconciler/reconciler_test.go +++ b/fleetshard/pkg/central/reconciler/reconciler_test.go @@ -346,6 +346,7 @@ func TestReconcileLastHashNotUpdatedOnError(t *testing.T) { central: private.ManagedCentral{}, resourcesChart: resourcesChart, encryptionKeyGenerator: cipher.AES256KeyGenerator{}, + secretBackup: k8s.NewSecretBackup(fakeClient, false), } _, err := r.Reconcile(context.TODO(), simpleManagedCentral) diff --git a/fleetshard/pkg/runtime/runtime.go b/fleetshard/pkg/runtime/runtime.go index 366b27137e..77047d42ef 100644 --- a/fleetshard/pkg/runtime/runtime.go +++ b/fleetshard/pkg/runtime/runtime.go @@ -168,7 +168,6 @@ func (r *Runtime) Start() error { reconciledCentralCountCache = int32(len(list.Items)) logger.InfoChangedInt32(&reconciledCentralCountCache, "Received central count changed: received %d centrals", reconciledCentralCountCache) for _, central := range list.Items { - glog.Infof("Try to start reconciliation: %s", central.Id) if _, ok := r.reconcilers[central.Id]; !ok { r.reconcilers[central.Id] = centralReconciler.NewCentralReconciler(r.k8sClient, r.client, central, r.dbProvisionClient, postgres.InitializeDatabase, r.secretCipher, r.encryptionKeyGenerator, reconcilerOpts) From 0c878dd748052d9b858ce95a33863202cb898381 Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 20:00:25 +0100 Subject: [PATCH 19/20] fix hash test --- fleetshard/pkg/central/reconciler/reconciler_test.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/fleetshard/pkg/central/reconciler/reconciler_test.go b/fleetshard/pkg/central/reconciler/reconciler_test.go index 0988331e35..ce56d59a80 100644 --- a/fleetshard/pkg/central/reconciler/reconciler_test.go +++ b/fleetshard/pkg/central/reconciler/reconciler_test.go @@ -177,6 +177,15 @@ func centralDBPasswordSecretObject() *v1.Secret { } } +func centralEncryptionKeySecretObject() *v1.Secret { + return &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: centralEncryptionKeySecretName, + Namespace: centralNamespace, + }, + } +} + func conditionForType(conditions []private.DataPlaneCentralStatusConditions, conditionType string) (*private.DataPlaneCentralStatusConditions, bool) { for _, c := range conditions { if c.Type == conditionType { @@ -371,11 +380,12 @@ func TestReconcileLastHashSetOnSuccess(t *testing.T) { centralDeploymentObject(), centralTLSSecretObject(), centralDBPasswordSecretObject(), + centralEncryptionKeySecretObject(), ) managedCentral := simpleManagedCentral managedCentral.RequestStatus = centralConstants.CentralRequestStatusReady.String() - + managedCentral.Metadata.SecretsStored = r.secretBackup.GetWatchedSecrets() expectedHash, err := util.MD5SumFromJSONStruct(&managedCentral) require.NoError(t, err) From a09c8168f6bfaa97f3f653e12362177e7e46d1cf Mon Sep 17 00:00:00 2001 From: Johannes Malsam Date: Thu, 11 Jan 2024 20:52:54 +0100 Subject: [PATCH 20/20] add eventually to collect secrets to allow some time to recover --- e2e/e2e_test.go | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index c60fdb55de..ec69915b94 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -289,8 +289,16 @@ var _ = Describe("Central", Ordered, func() { deleteNamespaceAndWaitForRecreation(ctx, namespaceName, k8sClient) - newSecrets, err := secretBackup.CollectSecrets(ctx, namespaceName) - Expect(err).ToNot(HaveOccurred()) + var newSecrets map[string]*corev1.Secret + Eventually(func() error { + secrets, err := secretBackup.CollectSecrets(ctx, namespaceName) + if err != nil { + return err + } + newSecrets = secrets // pragma: allowlist secret + return nil + }).WithTimeout(1 * time.Minute).WithPolling(10).Should(Succeed()) + assertEqualSecrets(newSecrets, oldSecrets) })