feat(secrets): add secret metadata #206

ecrupper · 2021-10-29T16:33:58Z

Adding more metadata for secrets including the created time, user that created the secret, last updated time, and the build id of the last build where it was used

codecov · 2021-10-29T16:35:03Z

Codecov Report

Merging #206 (4a01d3f) into master (8e493b9) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #206      +/-   ##
==========================================
+ Coverage   96.66%   96.70%   +0.04%     
==========================================
  Files          53       53              
  Lines        5509     5589      +80     
==========================================
+ Hits         5325     5405      +80     
  Misses        137      137              
  Partials       47       47

Impacted Files	Coverage Δ
database/secret.go	`100.00% <100.00%> (ø)`
library/secret.go	`100.00% <100.00%> (ø)`

library/secret.go

jbrockopp · 2021-10-29T17:37:27Z

library/secret.go

@@ -26,6 +26,10 @@ type Secret struct {
 	Images       *[]string `json:"images,omitempty"`
 	Events       *[]string `json:"events,omitempty"`
 	AllowCommand *bool     `json:"allow_command,omitempty"`
+	CreateTime   *string   `json:"create_time,omitempty"`
+	CreatedBy    *string   `json:"created_by,omitempty"`
+	UpdateTime   *string   `json:"update_time,omitempty"`


Thoughts on making this UpdatedAt?

I think that is a more common naming convention for a field like this.

Hilariously, updated_at seems to be a Postgres trigger keyword (at least in the testing DB), which causes it to override any user input and instead put the .Now() timestamp in the field. Any thoughts on a different name? CreatedTs and UpdatedTs maybe? Open to suggestions

I think this is actually related to the Golang library we're using for this:

https://github.com/go-gorm/gorm

I found these docs which appear to describe the issue you're referencing:

https://gorm.io/docs/models.html#Creating-Updating-Time-Unix-Milli-Nano-Seconds-Tracking

This issue might be related?

go-gorm/gorm#4827

library/secret.go

wass3r

Thanks for the PR! Few additional things:

Assuming that CreatedBy and UpdatedBy will reference User IDs, we should match the type, ie. change from string to int64.
I think we'll need to add these new fields to /database/secret.go as well
No action for this PR, but just a reminder that we will need to have an accompanying PR that adds the fields in https://github.com/go-vela/server/tree/master/database as well

jbrockopp

Thanks for updating this with the previous changes! 🎉

One more thing I noticed as I was reviewing:

database/secret_test.go

kneal · 2021-11-02T16:52:28Z

Just a clarifying question, LastBuildID is that going to be the last build to pull the secret?

ecrupper · 2021-11-02T16:55:31Z

Just a clarifying question, LastBuildID is that going to be the last build to pull the secret?

Yes that is the current plan

kneal · 2021-11-02T17:00:35Z

Cool! Just checking, I generally recommend linking the issue in the body of the PR. Helps with that extra context and create circular deps to the PR to issue etc.

ecrupper · 2021-11-02T18:28:41Z

Cool! Just checking, I generally recommend linking the issue in the body of the PR. Helps with that extra context and create circular deps to the PR to issue etc.

Absolutely! It was an internal request that wasn't duplicated to the community issue board yet. Here it is for reference: https://github.com/go-vela/community/issues/428

- adding functionality for this attribute increases scope by a large margin - will start with this smaller change (timestamps and authors)

ecrupper · 2021-11-04T18:25:01Z

https://github.com/go-vela/server/pull/526

Draft PR in the go-vela/server that would accompany this one

jbrockopp · 2021-11-09T13:55:15Z

database/secret.go

+	CreatedAt    sql.NullInt64  `sql:"created_at"`
+	CreatedBy    sql.NullInt64  `sql:"created_by"`
+	UpdatedAt    sql.NullInt64  `sql:"updated_at"`
+	UpdatedBy    sql.NullInt64  `sql:"updated_by"`


Is it safe to assume that CreatedBy and UpdatedBy are int64 types since they'll store the user's ID?

types/database/user.go

Lines 48 to 58 in 911ffbe

// User is the database representation of a user.

type User struct {

ID sql.NullInt64 `sql:"id"`

Name sql.NullString `sql:"name"`

RefreshToken sql.NullString `sql:"refresh_token"`

Token sql.NullString `sql:"token"`

Hash sql.NullString `sql:"hash"`

Favorites pq.StringArray `sql:"favorites" gorm:"type:varchar(5000)"`

Active sql.NullBool `sql:"active"`

Admin sql.NullBool `sql:"admin"`

}

IIRC, they were string types earlier which is why I'm asking.

Yes after taking a look at that User struct, I thought it best to change those to int64 types. Originally I thought it might be stored as a name, but I believe int64 is best.

@ecrupper ok thanks for the extra information 👍

Out of curiosity, could you elaborate on why int64 is the approach we're choosing?

I think I tend to agree that from the data storage perspective, int64 is better than string for the database.

However, my concern with this approach has to do with data access and usage patterns.

Today, there isn't a great way to query a user by their ID in Vela.

So if I'm looking at a secret with the UI/CLI/API, I would only be able to see the user's ID who created/updated it.

To actually see what user created/updated the secret, I'd have to ask an admin "Who is user <id> in Vela?"

Perhaps one way to address this is to add a new endpoint to Vela which allows looking a user up by their ID?

Otherwise we could consider storing the user's Name rather than ID in the secret object?

What are your thoughts on this?

That's a great point. Originally I was thinking in terms of consistency (if there's a User table with IDs as their primary key, it would make sense to have any reference to a User in another table be their ID). However, after considering the end user as you said, I agree that it should be a string. Not only would it save the UI an extra API call, but considering the CLI, running another command to find the name associated with a User ID seems tedious. I'll make that change!

fwiw, i proposed user id initially to be consistent with the repo object in the repos table where we use int64 for user id. another instance where user name could be beneficial when trying to figure out who "owns" a repo.

in the future we might consider embedding the user struct in the response similar to how github does it for some of it's objects (eg. check response here: https://docs.github.com/en/rest/reference/issues#get-an-issue).

(waiting for @JordanSussman to chime in with the obligatory GraphQL commentary 😄)

thanks for the work and revisions @ecrupper !

(waiting for @JordanSussman to chime in with the obligatory GraphQL commentary 😄)

Don't tempt me with a good time! Would love to add a v2 API with graphql and/or parameters to allow parsed output.

library/secret.go

…es into feat114/add-secret-metadata

jbrockopp

LGTM

kneal

LGTM 🐬

ecrupper and others added 3 commits October 19, 2021 16:12

added new constant for repository events for github webhook

61e6f81

Merge branch 'master' into master

6d8fcd6

adding metadata to secret

d3a9491

ecrupper requested a review from a team as a code owner October 29, 2021 16:33

remove event repository constant

1c28be5

JordanSussman reviewed Oct 29, 2021

View reviewed changes

library/secret.go Show resolved Hide resolved

jbrockopp requested changes Oct 29, 2021

View reviewed changes

changing names and adding updatedby field

976a247

wass3r requested changes Oct 31, 2021

View reviewed changes

wass3r changed the title ~~Feat: add secret metadata~~ feat(secrets): add secret metadata Oct 31, 2021

ecrupper added 2 commits November 1, 2021 10:33

user ids instead of strings for created and updated by

e3a7fd3

add same fields to the database type

dc15682

jbrockopp requested changes Nov 2, 2021

View reviewed changes

database/secret_test.go Outdated Show resolved Hide resolved

change timestamps type to int64

1652201

kneal assigned ecrupper Nov 2, 2021

kneal added the feature Indicates a new feature label Nov 2, 2021

Remove LastBuildId field for now

fee96b4

- adding functionality for this attribute increases scope by a large margin - will start with this smaller change (timestamps and authors)

jbrockopp requested changes Nov 9, 2021

View reviewed changes

ecrupper and others added 4 commits November 9, 2021 09:47

fixed comment typos

a9dc68b

Merge branch 'master' into feat114/add-secret-metadata

d1961bc

change createdby and updatedby types to string

93bfed0

Merge branch 'feat114/add-secret-metadata' of github.com:ecrupper/typ…

118f266

…es into feat114/add-secret-metadata

jbrockopp approved these changes Nov 12, 2021

View reviewed changes

Merge branch 'master' into feat114/add-secret-metadata

4d31c64

ecrupper and others added 2 commits November 16, 2021 08:29

Merge branch 'master' into feat114/add-secret-metadata

f76b560

Merge branch 'master' into feat114/add-secret-metadata

4a01d3f

wass3r approved these changes Nov 16, 2021

View reviewed changes

kneal approved these changes Nov 17, 2021

View reviewed changes

ecrupper merged commit 4dc404f into go-vela:master Nov 17, 2021

jbrockopp mentioned this pull request Jan 18, 2022

fix(secret/vault): update strategy to refresh token go-vela/server#570

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secrets): add secret metadata #206

feat(secrets): add secret metadata #206

ecrupper commented Oct 29, 2021

codecov bot commented Oct 29, 2021 •

edited

Loading

jbrockopp Oct 29, 2021

ecrupper Oct 29, 2021

ecrupper Nov 2, 2021 •

edited

Loading

jbrockopp Nov 3, 2021

wass3r left a comment

jbrockopp left a comment

kneal commented Nov 2, 2021

ecrupper commented Nov 2, 2021

kneal commented Nov 2, 2021

ecrupper commented Nov 2, 2021

ecrupper commented Nov 4, 2021

jbrockopp Nov 9, 2021

ecrupper Nov 9, 2021

jbrockopp Nov 12, 2021 •

edited

Loading

ecrupper Nov 12, 2021

wass3r Nov 16, 2021

JordanSussman Nov 16, 2021 •

edited

Loading

jbrockopp left a comment

kneal left a comment

	// User is the database representation of a user.
	type User struct {
	ID sql.NullInt64 `sql:"id"`
	Name sql.NullString `sql:"name"`
	RefreshToken sql.NullString `sql:"refresh_token"`
	Token sql.NullString `sql:"token"`
	Hash sql.NullString `sql:"hash"`
	Favorites pq.StringArray `sql:"favorites" gorm:"type:varchar(5000)"`
	Active sql.NullBool `sql:"active"`
	Admin sql.NullBool `sql:"admin"`
	}

feat(secrets): add secret metadata #206

feat(secrets): add secret metadata #206

Conversation

ecrupper commented Oct 29, 2021

codecov bot commented Oct 29, 2021 • edited Loading

Codecov Report

jbrockopp Oct 29, 2021

Choose a reason for hiding this comment

ecrupper Oct 29, 2021

Choose a reason for hiding this comment

ecrupper Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

jbrockopp Nov 3, 2021

Choose a reason for hiding this comment

wass3r left a comment

Choose a reason for hiding this comment

jbrockopp left a comment

Choose a reason for hiding this comment

kneal commented Nov 2, 2021

ecrupper commented Nov 2, 2021

kneal commented Nov 2, 2021

ecrupper commented Nov 2, 2021

ecrupper commented Nov 4, 2021

jbrockopp Nov 9, 2021

Choose a reason for hiding this comment

ecrupper Nov 9, 2021

Choose a reason for hiding this comment

jbrockopp Nov 12, 2021 • edited Loading

Choose a reason for hiding this comment

ecrupper Nov 12, 2021

Choose a reason for hiding this comment

wass3r Nov 16, 2021

Choose a reason for hiding this comment

JordanSussman Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

jbrockopp left a comment

Choose a reason for hiding this comment

kneal left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2021 •

edited

Loading

ecrupper Nov 2, 2021 •

edited

Loading

jbrockopp Nov 12, 2021 •

edited

Loading

JordanSussman Nov 16, 2021 •

edited

Loading