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

Add staging directory for kms #111980

Merged
merged 1 commit into from Sep 26, 2022
Merged

Add staging directory for kms #111980

merged 1 commit into from Sep 26, 2022

Conversation

aramase
Copy link
Member

@aramase aramase commented Aug 23, 2022

Signed-off-by: Anish Ramasekar anish.ramasekar@gmail.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add the staging directory for the kms repo. See kubernetes/org#3630 for details.

Which issue(s) this PR fixes:

part of kubernetes/org#3630
Fixes #111921

Special notes for your reviewer:

I used #102153 as a template to make these changes.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP] https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3299-kms-v2-improvements

/sig auth
/assign @enj @ritazh

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Aug 23, 2022
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/auth Categorizes an issue or PR as relevant to SIG Auth. labels Aug 23, 2022
@k8s-ci-robot
Copy link
Contributor

Please note that we're already in Test Freeze for the release-1.25 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.25.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Aug 23 19:46:32 UTC 2022.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/release-eng Issues or PRs related to the Release Engineering subproject labels Aug 23, 2022
@k8s-ci-robot k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Aug 23, 2022
@k8s-ci-robot k8s-ci-robot added area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/test sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 23, 2022
@aramase aramase marked this pull request as ready for review August 23, 2022 22:26
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2022
@k8s-ci-robot k8s-ci-robot requested review from aojea, caesarxuchao and a team August 23, 2022 22:27
@aramase
Copy link
Member Author

aramase commented Sep 22, 2022

I'm not that familiar with the publishing bot guts, but my inclination would be to keep the staging repos consistent with k/k and each other

thanks @liggitt, that resolved the issue. The PR is ready for review.

@ameukam could we change the default branch name to master in the staging repo to be consistent with all existing staging repos? I think the rename to main would be part of the bigger effort for all the staging repos.

@nikhita
Copy link
Member

nikhita commented Sep 23, 2022

publishing-bot has master baked in at a few places, so we'll need to stick to master for all staging repos here.

@aramase can you please create an issue in the k/org repo to get the kms repo ready for publishing and assign it to me? We'll need a few more changes -- like ensuring that the repo has an initial empty commit, etc.

@BenTheElder
Copy link
Member

BenTheElder commented Sep 23, 2022

I added a comment to the KEP for master->main for this repo, it's probably best to switch staging repos to main as part of that KEP rather than ahead of Kubernetes, so the branches continue to align closely: kubernetes/enhancements#2853

# DO NOT REPORT SECURITY VULNERABILITIES DIRECTLY TO THESE NAMES, FOLLOW THE
# INSTRUCTIONS AT https://kubernetes.io/security/

aramase
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure this is supposed to be match OWNERS, i.e. sig-auth-encryption-at-rest-approvers.

Copy link
Member Author

Choose a reason for hiding this comment

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

AFAIK, sig-auth-encryption-at-rest-approvers is only relevant in k/k because the alias is defined here. If this alias is repo specific, we need to re define the alias in the staging dir, so the staging github repo has it or else the staging github repo will reference an alias that needs to be looked up in k/k.

staging/publishing/import-restrictions.yaml Outdated Show resolved Hide resolved
staging/publishing/import-restrictions.yaml Outdated Show resolved Hide resolved
limitations under the License.
*/

// Package kms contains the proto definitions for the kms API.
Copy link
Member

Choose a reason for hiding this comment

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

I suppose this is true right now but it will change in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's right! I was thinking we can keep the comment relevant, so we can update this comment as we add more to this. WDYT?

staging/src/k8s.io/kms/README.md Outdated Show resolved Hide resolved
@aramase
Copy link
Member Author

aramase commented Sep 23, 2022

publishing-bot has master baked in at a few places, so we'll need to stick to master for all staging repos here.

@aramase can you please create an issue in the k/org repo to get the kms repo ready for publishing and assign it to me? We'll need a few more changes -- like ensuring that the repo has an initial empty commit, etc.

@nikhita The kms repo issue is still open in k/org - kubernetes/org#3630. I've assigned that to you. Is it ok to merge this PR before the branch rename from main to master is complete in the staging repo?

@enj
Copy link
Member

enj commented Sep 26, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@enj
Copy link
Member

enj commented Sep 26, 2022

This needs approval from @liggitt and the staging repo needs to be switched to use master which I believe is being handled by @nikhita.

@liggitt
Copy link
Member

liggitt commented Sep 26, 2022

replicate the content of staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/v2alpha1/OWNERS to staging/src/k8s.io/kms/apis/OWNERS to make sure these packages show up as packages needing API review

lgtm otherwise (once the publishing bot CI is green)

- Moves kms proto apis to the staging repo
- Updates generate and verify kms proto scripts to check staging repo

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@aramase
Copy link
Member Author

aramase commented Sep 26, 2022

replicate the content of staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/envelope/kmsv2/v2alpha1/OWNERS to staging/src/k8s.io/kms/apis/OWNERS to make sure these packages show up as packages needing API review

lgtm otherwise (once the publishing bot CI is green)

Added the OWNERS file for api reviews.

@liggitt publishing bot CI failure is because there is no kms directory in the k/k master branch?

I0923 16:09:47.572947     152 validate.go:37] validating directories exist in the kubernetes branch
E0923 16:09:48.196706     152 main.go:46] Error : kms not found in branch master
E0923 16:09:48.197253     152 main.go:46] Error : kms not found in branch master

Would the tests pass until we merge this PR?

@k8s-ci-robot
Copy link
Contributor

@aramase: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-publishing-bot-validate c3794e2 link false /test pull-publishing-bot-validate

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@liggitt
Copy link
Member

liggitt commented Sep 26, 2022

@liggitt publishing bot CI failure is because there is no kms directory in the k/k master branch?

I0923 16:09:47.572947     152 validate.go:37] validating directories exist in the kubernetes branch
E0923 16:09:48.196706     152 main.go:46] Error : kms not found in branch master
E0923 16:09:48.197253     152 main.go:46] Error : kms not found in branch master

Would the tests pass until we merge this PR?

hmm, I don't know if it's because

will defer to folks who know the publishing bot better than me to answer that

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, enj, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit a54d8d8 into kubernetes:master Sep 26, 2022
SIG Auth Old automation moved this from Needs Triage to Closed / Done Sep 26, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Sep 26, 2022
@aramase aramase deleted the kms branch September 26, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Archived in project
Status: Done
SIG Auth Old
Closed / Done
Development

Successfully merging this pull request may close these issues.

[KMSv2] Create new staging repo k8s.io/kms
8 participants