feat: create and update secrets through cli & api #129

arinda-arif · 2021-12-17T06:38:33Z

No description provided.

coveralls · 2021-12-17T07:08:11Z

Pull Request Test Coverage Report for Build 1744262013

53 of 65 (81.54%) changed or added relevant lines in 2 files are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.08%) to 71.49%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
models/project.go	0	2	0.0%
api/handler/v1beta1/secret.go	53	63	84.13%

Files with Coverage Reduction	New Missed Lines	%
models/project.go	1	50.0%

Totals
Change from base Build 1739126541:	0.08%
Covered Lines:	5025
Relevant Lines:	7029

💛 - Coveralls

cmd/secret.go

models/project.go

store/postgres/migrations/000016_update_replay_table_add_config_column.down.sql

kushsharma · 2021-12-21T05:46:49Z

I was thinking what if we have a single set command instead of create/update?
@sravankorumilli @arinda-arif

arinda-arif · 2021-12-21T08:15:00Z

I was thinking what if we have a single set command instead of create/update? @sravankorumilli @arinda-arif

I think having create/update gives benefits of having extra validation, for example:

prevent users from replacing existing secrets by mistake when creating
prevent creating unnecessary secrets when updating a wrong secret name by mistake

And the flexibility to differentiate who is authorized to create or update in the future.

kushsharma · 2022-01-03T04:21:18Z

I was thinking what if we have a single set command instead of create/update? @sravankorumilli @arinda-arif

I think having create/update gives benefits of having extra validation, for example:

prevent users from replacing existing secrets by mistake when creating

prevent creating unnecessary secrets when updating a wrong secret name by mistake

And the flexibility to differentiate who is authorized to create or update in the future.

Make sense, what if we solve this using a flag --update-only? I am trying to make APIs a little more simpler. What do you think?

arinda-arif · 2022-01-03T05:14:15Z

I was thinking what if we have a single set command instead of create/update? @sravankorumilli @arinda-arif

I think having create/update gives benefits of having extra validation, for example:

prevent users from replacing existing secrets by mistake when creating

prevent creating unnecessary secrets when updating a wrong secret name by mistake

And the flexibility to differentiate who is authorized to create or update in the future.

Make sense, what if we solve this using a flag --update-only? I am trying to make APIs a little more simpler. What do you think?

Got your point. I assume this is the behavior if we are having that flag:

optimus create secret -> will create a secret. if any existing secret is found, will not be updated.
optimus create secret --update-only -> will update a secret. if the secret is not found, will not be created.

Does create secret --update-only seems odd? should we use register instead or is there any other suggestion?
cc @sbchaos @sravankorumilli

sbchaos · 2022-01-03T05:47:20Z

Does create secret --update-only seems odd? should we use register instead or is there any other suggestion? cc @sbchaos @sravankorumilli

To me it sounds a bit odd that we are using create to update the value of secret. Register seems like a reasonable alternative to update/create.

ravisuhag · 2022-01-03T05:49:08Z

+1 to set as suggested by @kushsharma

arinda-arif · 2022-01-03T08:06:44Z

Agree, let's have set then. For the flag, should we go with --update? having --update-only seems to skip the flag means it can be either create/update, while we actually want to separate both behaviors to avoid the above possible issues.

cmd/secret.go

kushsharma · 2022-01-04T08:12:05Z

docs/docs/guides/manage-secrets.md

+
+## Updating a secret
+
+Updating an already existing secret in Optimus can be done passing the flag update-only to set command:


These examples are misleading, update-only flag is generally used when you explicitly only want to update a secret when it is already there and don't want to create it by mistake. I think we should write it as such.

store/postgres/migrations/000016_update_replay_table_add_config_column.down.sql

store/store.go

kushsharma · 2022-01-05T04:16:32Z

cmd/secret.go

+)
+
+var (
+	secretTimeout = time.Minute * 15


I think this is too long of a timeout to recognize the operation has occurred a problem. I guess 2 min should be enough.

Sure, will update it to 2 minutes

kushsharma · 2022-01-05T05:04:08Z

api/handler/v1/runtime.go

+			return nil, status.Errorf(codes.Internal, "%s: failed to update secret %s", err.Error(), req.GetSecretName())
+		}
+	} else {
+		if err := secretRepo.Save(ctx, models.ProjectSecretItem{


wouldn't the current implementation of Save will fail this if the secret is already registered? I was thinking the behavior should be

if the set is called and it doesn't exist, it will create it

if the set is called and it does exist, it will update it

if the set is called with --update-only flag and it doesn't exist, it will fail

if the set is called with --update-only flag and it does exist, it will update

The current behaviour will update an existing secret at time of create.
If this is not going to be a problem, then I can keep the current behaviour.

Sorry didn't get what you said, can you rephrase it, please? Is the current behavior match with those 4 points?

Existing behaviour (without change on this PR), save will create a new secret, if it exists, it will update the secret.
It might be a problem when the update of existing secret is unintentional at the time of create.

If we encounter this problem very less(unintentional update) then we can retain the existing behaviour.
Otherwise it will be better to be intentional about create or update of secrets.

Co-authored-by: Sandeep Bhardwaj<sandeep.bhardwaj2307@gmail.com>

sbchaos · 2022-01-10T08:22:23Z

api/handler/v1beta1/runtime.go

 	}, nil
 }

-func (sv *RuntimeServiceServer) RegisterSecret(ctx context.Context, req *pb.RegisterSecretRequest) (*pb.RegisterSecretResponse, error) {


Moving secrets management to separate file as runtime file is currently very big.
We can split it later as part of a refactor to jobs, resources and different apis.

cmd/secret.go

kushsharma · 2022-01-20T08:02:16Z

cmd/secret.go

+	if updateSecretResponse.Success {
+		l.Info(coloredSuccess("Secret updated"))
+	} else {
+		return errors.New(fmt.Sprintf("Request failed for updating secret %s: %s", req.SecretName,


You should not start error with a capital letter as it could be embedded part of some other error. This is done in couple of places in this file

Changed the error messages to lowercase

kushsharma · 2022-01-20T08:02:57Z

cmd/secret.go

+		return errors.Wrapf(err, "Request failed for updating secret %s", req.SecretName)
+	}
+
+	if updateSecretResponse.Success {


In what case this could be false?

This check is not required.

kushsharma · 2022-01-20T08:06:48Z

cmd/secret.go

+	secretRequestTimeout, secretRequestCancel := context.WithTimeout(context.Background(), secretTimeout)
+	defer secretRequestCancel()
+
+	l.Info("please wait...")


Should we use a spinner here? and in Update call?

Using spinner

kushsharma · 2022-01-20T08:11:28Z

store/postgres/secret_repository.go

 		return errors.Wrap(err, "unable to find secret by name")
 	}
-	resource, err := Secret{}.FromSpec(spec, repo.project, repo.hash)
+	return errors.New("secret already exist")


We should generalize this error to resource already exists or something similar to store.ErrResourceNotFound or should get rid of this logic of checking if it exists and only insert the secret? Can you check if gorm throws an error if it already exists?
Because currently, I think we are leaking our implementation detail out of an interface.

Gorm will throw an error like

\x1b[31;1m /../optimus/store/postgres/secret_repository.go:135 \x1b[35;1mERROR: duplicate key value violates unique constraint \"secret_project_id_name_key\" (SQLSTATE 23505)" ....(prints whole query with values)

Which will return the implementation (and query) details back in the api, so we will need to replace the returned error.

Can you please explain how are we leaking implementation details out of interface ?

checking explicitly is fine & we don't need to rely on gorm error.

sbchaos · 2022-01-25T08:05:29Z

Merging the PR, we can still have the conversation if needed.

Any required changes will be made to the followup PR.

arinda-arif marked this pull request as draft December 17, 2021 06:38

kushsharma reviewed Dec 21, 2021

View reviewed changes

cmd/secret.go Show resolved Hide resolved

models/project.go Outdated Show resolved Hide resolved

store/postgres/migrations/000016_update_replay_table_add_config_column.down.sql Show resolved Hide resolved

arinda-arif marked this pull request as ready for review January 3, 2022 04:28

arinda-arif force-pushed the secret-management branch from e95ed6a to 0127c4f Compare January 3, 2022 04:51

sbchaos force-pushed the secret-management branch from 0127c4f to 64968f7 Compare January 4, 2022 05:05

kushsharma reviewed Jan 4, 2022

View reviewed changes

sbchaos force-pushed the secret-management branch 4 times, most recently from 18d81f1 to 9a8a89b Compare January 4, 2022 11:38

kushsharma reviewed Jan 5, 2022

View reviewed changes

sbchaos force-pushed the secret-management branch 3 times, most recently from 7d6d278 to c07d01d Compare January 6, 2022 08:58

feat: add command to set secrets

52d19d1

Co-authored-by: Sandeep Bhardwaj<sandeep.bhardwaj2307@gmail.com>

sbchaos force-pushed the secret-management branch from c07d01d to 52d19d1 Compare January 6, 2022 09:01

refactor(optimus): add updateSeret api and add confirm for update

3830036

sbchaos reviewed Jan 10, 2022

View reviewed changes

sbchaos added 2 commits January 12, 2022 18:10

feat(optimus): add foreign key constraint for namespace

d21b307

fix(optimus): fix test and orm for foreign key constraint

17c5313

arinda-arif commented Jan 13, 2022

View reviewed changes

cmd/secret.go Outdated Show resolved Hide resolved

arinda-arif commented Jan 13, 2022

View reviewed changes

cmd/secret.go Outdated Show resolved Hide resolved

arinda-arif commented Jan 13, 2022

View reviewed changes

cmd/secret.go Outdated Show resolved Hide resolved

arinda-arif assigned sbchaos and arinda-arif Jan 13, 2022

feat(optimus): improve formatting of error messages

6be7f72

kushsharma reviewed Jan 20, 2022

View reviewed changes

sbchaos added 4 commits January 24, 2022 15:37

Merge branch 'main' into secret-management

e2ecee7

feat(optimus): use spinner instead of log message

9c55914

feat(optimus): remove check for success flag and lowercase errors

107957d

feat(optimus): update proton commit

6eb3cf1

sbchaos merged commit 500e3cd into main Jan 25, 2022

sbchaos deleted the secret-management branch January 25, 2022 08:08

sravankorumilli linked an issue Feb 16, 2022 that may be closed by this pull request

User should be able to create/update a secret through apis & cli. #123

Closed

4 tasks


		## Updating a secret

		Updating an already existing secret in Optimus can be done passing the flag update-only to set command:

feat: create and update secrets through cli & api #129

feat: create and update secrets through cli & api #129

Uh oh!

Conversation

arinda-arif commented Dec 17, 2021

Uh oh!

coveralls commented Dec 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1744262013

💛 - Coveralls

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kushsharma commented Dec 21, 2021

Uh oh!

arinda-arif commented Dec 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kushsharma commented Jan 3, 2022

Uh oh!

arinda-arif commented Jan 3, 2022

Uh oh!

sbchaos commented Jan 3, 2022

Uh oh!

ravisuhag commented Jan 3, 2022

Uh oh!

arinda-arif commented Jan 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbchaos commented Jan 25, 2022

Uh oh!

Reviewers

Assignees

Labels

coveralls commented Dec 17, 2021 •

edited

Loading

arinda-arif commented Dec 21, 2021 •

edited

Loading

arinda-arif commented Jan 3, 2022 •

edited

Loading