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

Add attribute support for certificate subject #129

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add attribute support for certificate subject #129

wants to merge 1 commit into from

Conversation

cornfeedhobo
Copy link
Contributor

@cornfeedhobo cornfeedhobo commented Dec 22, 2022 •
edited
Loading

@jetstack-bot jetstack-bot added the dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. label Dec 22, 2022
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cornfeedhobo
To complete the pull request process, please assign wallrj after the PR has been reviewed.
You can assign the PR to them by writing /assign @wallrj in a comment when ready.

The full list of commands accepted by this bot can be found 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

@jetstack-bot jetstack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 22, 2022
@jetstack-bot
Copy link
Contributor

Hi @cornfeedhobo. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jetstack-bot jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 22, 2022
@jetstack-bot jetstack-bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Dec 22, 2022
SgtCoDFish
SgtCoDFish requested changes Jan 16, 2023
View reviewed changes
Copy link
Member

@SgtCoDFish SgtCoDFish left a comment

Choose a reason for hiding this comment

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

Thanks for this!

As mentioned on slack, I don't have a tonne of time at the moment but I've added a couple of comments which I hope are useful!

pkg/requestgen/generator.go
if err != nil {
return nil, fmt.Errorf("%q: %w", v, err)
}
*k = strings.Split(e, ",")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: strings.Split might not be suitable here, since several of these fields can reasonably contain commas.

I remember that you commented on a PR implementing a similar feature in cert-manager: cert-manager/cert-manager#4502 (comment)

I think we're likely to need a similar approach here as I suggested in there. CSV parsing seems like a good solution.

Example test string:

`"1725 Slough Avenue, Suite 200, Scranton Business Park","10 Downing Street, Westminster",Something Else`

Should produce exactly 3 street address entries

Copy link
Member

Choose a reason for hiding this comment

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

@cornfeedhobo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@inteon Hey, I unfortunately don't have the time to revisit this and my team ended up scraping the associated project. Anyone is welcome to pick up where I left off, otherwise it will probably be a few months until I can carve out time.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for letting us know!

pkg/apis/validation/validation_test.go
Comment on lines +211 to +221
"correct literal-subject should not error": {
attr: map[string]string{
csiapi.IssuerNameKey: "test-issuer",
csiapi.LiteralSubjectKey: literalSubject,
csiapi.CAFileKey: "ca.crt",
csiapi.CertFileKey: "crt.tls",
csiapi.KeyFileKey: "key.tls",
csiapi.DNSNamesKey: "foo.bar.com",
csiapi.KeyEncodingKey: "PKCS8",
},
expErr: nil,
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: this will need many more test cases IMO, to ensure that the splitting functionality works as expected!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SgtCoDFish If I may push back, I felt like if I added too many permutations, I'd essentially be testing github.com/cert-manager/cert-manager/pkg/util/pki.ParseSubjectStringToRawDerBytes, because that's doing the heavy lifting in this specific case. That said, if you think there is a branch of this logic that could use a better test, I'm open to anything.

@SgtCoDFish
Copy link
Member

/ok-to-test

@jetstack-bot jetstack-bot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2023
@cornfeedhobo
Copy link
Contributor Author

Small update. I'll be out for a few weeks but am still tracking this. Looking forward to having more time in a few weeks.

@cornfeedhobo
Copy link
Contributor Author

This PR is ready, but I'm waiting for cert-manager to cut a new release so I don't have to duplicate the CVS functions that are available in pkg/util.

@inteon
Copy link
Member

inteon commented Sep 17, 2023

Update: #129 (comment)

@jetstack-bot
Copy link
Contributor

@cornfeedhobo: 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-cert-manager-csi-driver-test a6bb5a8 link true /test pull-cert-manager-csi-driver-test

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.

@nzbr nzbr mentioned this pull request Mar 27, 2024
Merged
@jetstack-bot jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 27, 2024
@jetstack-bot
Copy link
Contributor

PR needs rebase.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inteon inteon inteon left review comments

@SgtCoDFish SgtCoDFish SgtCoDFish requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test 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.

Support all subject attributes
4 participants
@cornfeedhobo @jetstack-bot @SgtCoDFish @inteon