Skip to content

Storage: add HMAC key support#8430

Merged
tseaver merged 15 commits intomasterfrom
storage-add-hmac-key-support-feature
Aug 7, 2019
Merged

Storage: add HMAC key support#8430
tseaver merged 15 commits intomasterfrom
storage-add-hmac-key-support-feature

Conversation

@tseaver
Copy link
Contributor

@tseaver tseaver commented Jun 19, 2019 •
edited
Loading

@jkwlui This PR is for merging the HMAC keys feature branch to master: you have already-reviewed the changes in PRs #8041, #8043, and #8045.

I still need to:

  • Add system tests exercising the CRUD cycle for HMACKeyMetadata resources.

Closes #7851.

@tseaver tseaver added the api: storage Issues related to the Cloud Storage API. label Jun 19, 2019
@tseaver tseaver requested a review from jkwlui June 19, 2019 16:41
@tseaver tseaver requested review from crwilcox and frankyn as code owners June 19, 2019 16:41
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 19, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 26, 2019
@tseaver tseaver force-pushed the storage-add-hmac-key-support-feature branch from 5b07516 to f5cbf13 Compare June 26, 2019 21:00
@tseaver
Copy link
Contributor Author

tseaver commented Jun 26, 2019 •
edited
Loading

@jkwlui Working on system tests: the POST to /projects/<CI project>/hmacKeys' fails with a 403, claiming that the service account is not in the project, which is clearly false (I verfied the email in the console, where it shows with the Owner` role for the project).

Update: I tweaked the system tests to use the client's credentials to discover the service account email, rather than querying for one. They now pass.

@tseaver tseaver force-pushed the storage-add-hmac-key-support-feature branch from 2cdf630 to e1b14af Compare July 2, 2019 18:57
@tseaver
Copy link
Contributor Author

tseaver commented Jul 2, 2019

Rebased to deal with out-of-date madness.

@tseaver tseaver force-pushed the storage-add-hmac-key-support-feature branch from e1b14af to e15ddac Compare July 2, 2019 19:23
@tseaver tseaver added needs work This is a pull request that needs a little love. and removed 🚨 This issue needs some love. labels Jul 2, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Jul 2, 2019

Storage Kokoro job fails due to flaky systest, #8552.

@tseaver tseaver mentioned this pull request Jul 8, 2019
Closed
8 tasks
@tseaver tseaver force-pushed the storage-add-hmac-key-support-feature branch from 4386da1 to 6cc2fc3 Compare July 9, 2019 15:58
@tseaver
Copy link
Contributor Author

tseaver commented Jul 9, 2019

Force-pushed to pick up the systest fix from #8617.

@tseaver
Copy link
Contributor Author

tseaver commented Jul 9, 2019

Ugh, Storage failure due to #8455.

@tseaver tseaver added do not merge Indicates a pull request not ready for merge, due to either quality or timing. and removed needs work This is a pull request that needs a little love. labels Jul 9, 2019
@tseaver tseaver force-pushed the storage-add-hmac-key-support-feature branch from 6cc2fc3 to 928959d Compare July 9, 2019 17:29
@tseaver
Copy link
Contributor Author

tseaver commented Jul 9, 2019

Rebased again to pick up systest fix from #8620.

jkwlui
jkwlui approved these changes Jul 9, 2019
View reviewed changes
Copy link
Member

@jkwlui jkwlui left a comment

Choose a reason for hiding this comment

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

Looks good! Please hold off from merging until the feature is off whitelist :)

@tseaver
Copy link
Contributor Author

tseaver commented Jul 12, 2019

@jkwlui No worries. Please ping this PR when the feature is off whitelist.

frankyn
frankyn reviewed Aug 6, 2019
View reviewed changes
@frankyn frankyn removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Aug 6, 2019
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 6, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 6, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Aug 6, 2019

Per discussion with @frankyn, I plan to add optional access_id and project_id arguments to the HMACKeyMetadata constructor, to allow fetching / updating / deleting key metadata without first listing keys.

@tseaver
Copy link
Contributor Author

tseaver commented Aug 6, 2019 •
edited
Loading

Further discussion leads to the need for a Client.get_hmac_key_metadata factory function (similar to Client.get_bucket): it would take access_id and optional project_id args and return an already-reloaded instance of HMACKeyMetadata.

frankyn
frankyn approved these changes Aug 7, 2019
View reviewed changes
Copy link
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Thanks @tseaver LGTM. Please merge when ready.

@tseaver tseaver merged commit 2eb2969 into master Aug 7, 2019
@tseaver tseaver deleted the storage-add-hmac-key-support-feature branch August 7, 2019 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crwilcox crwilcox Awaiting requested review from crwilcox

2 more reviewers

@frankyn frankyn frankyn approved these changes

@jkwlui jkwlui jkwlui approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Storage: HMAC Service Account key support

6 participants

@tseaver @frankyn @jkwlui @googlebot @yoshi-automation @yoshi-kokoro