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

Aws secretsmanager additions #6381

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

mpechner-akasa
Copy link

@mpechner-akasa mpechner-akasa commented Nov 29, 2024

This adds the ability to specify a secretKey in the awsSecretManager TriggerAuthentication. This will allow parsing of secrets that contain Key/Value pairs (returned in JSON format).

Resubmission of #6031

Provide a description of what has been changed

Checklist

Fixes #5940

Relates to #

@mpechner-akasa mpechner-akasa requested a review from a team as a code owner November 29, 2024 18:28
@mpechner-akasa mpechner-akasa force-pushed the aws-secretsmanager-additions branch from a4701ef to 23175d8 Compare November 29, 2024 20:09
Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Looking good, thanks. We should also update docs

@mpechner-akasa
Copy link
Author

#5940

docs. kedacore/keda-docs#1508

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Really nice job! some comments inline

Comment on lines 285 to 325
func TestAwsSecretManager(t *testing.T) {
var useJSONSecretFormat = false
require.NotEmpty(t, awsAccessKeyID, "TF_AWS_ACCESS_KEY env variable is required for AWS Secret Manager test")
require.NotEmpty(t, awsSecretAccessKey, "TF_AWS_SECRET_KEY env variable is required for AWS Secret Manager test")

// Create the secret in AWS
err := createAWSSecret(t, useJSONSecretFormat)
assert.NoErrorf(t, err, "cannot create AWS Secret Manager secret - %s", err)

// Create kubernetes resources for PostgreSQL server
kc := GetKubernetesClient(t)
data, postgreSQLtemplates := getPostgreSQLTemplateData()

CreateKubernetesResources(t, kc, testNamespace, data, postgreSQLtemplates)

assert.True(t, WaitForStatefulsetReplicaReadyCount(t, kc, postgreSQLStatefulSetName, testNamespace, 1, 60, 3),
"replica count should be %d after 3 minutes", 1)

createTableSQL := "CREATE TABLE task_instance (id serial PRIMARY KEY,state VARCHAR(10));"
psqlCreateTableCmd := fmt.Sprintf("psql -U %s -d %s -c \"%s\"", postgreSQLUsername, postgreSQLDatabase, createTableSQL)

ok, out, errOut, err := WaitForSuccessfulExecCommandOnSpecificPod(t, postgresqlPodName, testNamespace, psqlCreateTableCmd, 60, 3)
assert.True(t, ok, "executing a command on PostreSQL Pod should work; Output: %s, ErrorOutput: %s, Error: %s", out, errOut, err)

// Create kubernetes resources for testing
data, templates := getTemplateData(useJSONSecretFormat)

KubectlApplyMultipleWithTemplate(t, data, templates)
assert.True(t, WaitForDeploymentReplicaReadyCount(t, kc, deploymentName, testNamespace, minReplicaCount, 60, 3),
"replica count should be %d after 3 minutes", minReplicaCount)

testScaleOut(t, kc, data)

// cleanup
KubectlDeleteMultipleWithTemplate(t, data, templates)
DeleteKubernetesResources(t, testNamespace, data, postgreSQLtemplates)

// Delete the secret in AWS
err = deleteAWSSecret(t)
assert.NoErrorf(t, err, "cannot delete AWS Secret Manager secret - %s", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we create a function for the test and pass the required changes? 'TestAwsSecretManager' and 'TestAwsSecretManagerJSONFormat' are almost the same, maybe we can create a function like testAwsSecretManager with the changes as arguments (useJSONSecretFormat , the templates, etc)

Choose a reason for hiding this comment

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

Do you want testAwsSecretManager refactored? Or a new function?

Copy link
Member

Choose a reason for hiding this comment

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

My proposal is to create a new function that can be called with both configurations to reduce the amount of duplicated code, the name was tentative and just a suggestion

Choose a reason for hiding this comment

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

Ok thanks.

@@ -247,11 +276,54 @@ spec:
)

func TestAwsSecretManager(t *testing.T) {
var useJSONSecretFormat = false
Copy link
Member

Choose a reason for hiding this comment

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

same as the other test

@mpechner-akasa mpechner-akasa force-pushed the aws-secretsmanager-additions branch 2 times, most recently from 36f2dcc to a6b4067 Compare December 16, 2024 22:38
Signed-off-by: michael pechner <[email protected]>
Signed-off-by: michael pechner <[email protected]>
…-akasa/keda into aws-secretsmanager-additions
…ect.

Will remove  REMOVETestAwsSecretManagerJSONFormat and change aws_secret_manager_pod_identity.go once I have changed this file as expected.

Signed-off-by: michael pechner <[email protected]>
// Local imports
	. "github.com/kedacore/keda/v2/tests/helper"

Signed-off-by: michael pechner <[email protected]>
Signed-off-by: michael pechner <[email protected]>
…the the helper.

Fixed type Ti -> T

Signed-off-by: michael pechner <[email protected]>
AwsSecretManager() does not return anything, so fixed the calling test

Signed-off-by: michael pechner <[email protected]>
added test code back and making AwsSecretmanager() return nil
Signed-off-by: michael pechner <[email protected]>
Signed-off-by: michael pechner <[email protected]>
Signed-off-by: michael pechner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants