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.

Sign up for GitHub

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

Jump to bottom

Adding TLS options to the sign logic. #162

Merged
merged 1 commit into from Dec 4, 2022
Merged

Adding TLS options to the sign logic. #162

merged 1 commit into from Dec 4, 2022

Conversation

ybettan
Copy link
Contributor

@ybettan ybettan commented Nov 24, 2022 •
edited

Adding TLS options to the sign logic.

When checking if the signed image already exist or when pulling the
image to sign, we should be able to set some TLS options.

This flow follows the same logic we have for builds.

In additions, this commit also update the type of the
ModuleLoaderContainerSpec.RegistryTLS API so we use the default values
when not specified.

Signed-off-by: Yoni Bettan yonibettan@gmail.com

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ybettan

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 cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 24, 2022
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2022
@ybettan
Copy link
Contributor Author

ybettan commented Nov 24, 2022

/cc @chr15p

@k8s-ci-robot
Copy link
Contributor

@ybettan: GitHub didn't allow me to request PR reviews from the following users: chr15p.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @chr15p

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.

@ybettan
Copy link
Contributor Author

ybettan commented Nov 24, 2022

This PR is following on #86 (comment)

@ybettan
Copy link
Contributor Author

ybettan commented Nov 24, 2022

/hold
Adding UT for the new additions.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2022
@ybettan
Copy link
Contributor Author

ybettan commented Nov 24, 2022

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022 •
edited

Codecov Report

Base: 73.92% // Head: 74.07% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (f617407) compared to base (dedee81).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #162      +/-   ##
==========================================
+ Coverage   73.92%   74.07%   +0.15%     
==========================================
  Files          26       26              
  Lines        2558     2573      +15     
==========================================
+ Hits         1891     1906      +15     
  Misses        582      582              
  Partials       85       85
Impacted Files Coverage Δ
internal/build/job/maker.go 90.14% <100.00%> (+0.04%) ⬆️
internal/module/helper.go 100.00% <100.00%> (ø)
internal/sign/job/signer.go 93.18% <100.00%> (+1.28%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ybettan
Copy link
Contributor Author

ybettan commented Nov 25, 2022

/hold
This PR should be rebased on top op #163

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 25, 2022
@ybettan ybettan force-pushed the tls-sign branch 2 times, most recently from 9123d8e to 91dce79 Compare November 27, 2022 07:24
@ybettan
Copy link
Contributor Author

ybettan commented Nov 27, 2022

Waiting to see the resolution of #164

@ybettan
Copy link
Contributor Author

ybettan commented Nov 30, 2022

Fixes #164

@ybettan ybettan force-pushed the tls-sign branch 2 times, most recently from 39d8b73 to d0537d3 Compare November 30, 2022 14:56
@ybettan
Copy link
Contributor Author

ybettan commented Nov 30, 2022 •
edited

@qbarrand @yevgeny-shnaidman PTAL.

@ybettan
Copy link
Contributor Author

ybettan commented Nov 30, 2022

/unhold

1 similar comment
@ybettan
Copy link
Contributor Author

ybettan commented Dec 1, 2022

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 1, 2022
@ybettan
Copy link
Contributor Author

ybettan commented Dec 1, 2022

Will be rebased on top #156 once merged.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022
@netlify
Copy link

netlify bot commented Dec 4, 2022 •
edited

✅ Deploy Preview for kubernetes-sigs-kmm ready!

Name Link
🔨 Latest commit f617407
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-kmm/deploys/638ca37aaeafa0000acbfcde
😎 Deploy Preview https://deploy-preview-162--kubernetes-sigs-kmm.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2022
yevgeny-shnaidman
yevgeny-shnaidman reviewed Dec 4, 2022
View reviewed changes
internal/module/helper.go Outdated Show resolved Hide resolved
internal/sign/job/signer.go Show resolved Hide resolved
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

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

ybettan commented Dec 4, 2022

/hold
Wait for CI

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2022
@ybettan
Adding TLS options to the sign logic.
f617407 
When checking if the `signed` image already exist or when pulling the
image to sign, we should be able to set some TLS options.

This flow follows the same logic we have for builds.

In additions, this commit also update the type of the
`ModuleLoaderContainerSpec.RegistryTLS` API so we use the default values
when not specified.

Signed-off-by: Yoni Bettan <yonibettan@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 Dec 4, 2022
@ybettan
Copy link
Contributor Author

ybettan commented Dec 4, 2022

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2022
@yevgeny-shnaidman
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 33fce24 into kubernetes-sigs:main Dec 4, 2022
@ybettan ybettan deleted the tls-sign branch December 4, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yevgeny-shnaidman yevgeny-shnaidman yevgeny-shnaidman left review comments

@qbarrand qbarrand Awaiting requested review from qbarrand

Assignees

@yevgeny-shnaidman yevgeny-shnaidman

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ybettan @k8s-ci-robot @codecov-commenter @yevgeny-shnaidman