Handle invalid ValidityDuration user input #253
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ArkaSaha30 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ArkaSaha30
force-pushed
the
fix-cert-duration-check
branch
from
685ced1 to
119b232
Compare
|
cc @neolit123 |
ArkaSaha30
force-pushed
the
fix-cert-duration-check
branch
2 times, most recently
from
783c3dc to
e6346e5
Compare
|
/test pull-etcd-operator-test-e2e |
internal/controller/utils.go
Outdated
| // Set default duration to 365 days for auto provider if not provided | ||
| var duration time.Duration | ||
| if autoConfig.ValidityDuration == "" { | ||
| duration = certInterface.DefaultAutoValidity | ||
| } else { | ||
| var err error | ||
| duration, err = time.ParseDuration(autoConfig.ValidityDuration) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse ValidityDuration: %w", err) | ||
| } |
There was a problem hiding this comment.
Please add a function something like below, and reuse it for both createCMCertificateConfig and createAutoCertificateConfig
func parseValidityDuration(customizedDuration string, defaultDuration time.Duration) (time.Duration, error)
There was a problem hiding this comment.
Sure, updated the PR.
ivanvc
left a comment
ivanvc
left a comment
There was a problem hiding this comment.
Thanks for the pull request, Arka. Other than Benjamin's comment and one observation I left, this looks great :)
pkg/certificate/auto/provider.go
Outdated
| log.Printf("calling SelfCert with hosts: %v", hosts) | ||
|
|
||
| tlsInfo, selfCertErr := transport.SelfCert(zap.NewNop(), tmpDir, hosts, uint(validity/DefaultValidity)) | ||
| tlsInfo, selfCertErr := transport.SelfCert(zap.NewNop(), tmpDir, hosts, uint(validity/interfaces.DefaultAutoValidity)) |
There was a problem hiding this comment.
I think that dividing by interfaces.DefaultAutoValidity may be error-prone in the future (i.e., if someone changes the DefaultAutoValidity to anything other than one year). I think this division needs to be fixed to be by 365 * 24 * time.Hour, as transport.SelfCert(...) expects this to be years.
It could be another constant defined in this function.
There was a problem hiding this comment.
Sure, updated the PR
There was a problem hiding this comment.
I'll debug and fix the failing test as well
This commit will handle a scenario of invalid cert-manager ValidityDuration user input and throw an error. In case, ValidityDuration is not defined by user it will default to 90days for cert-manager Signed-off-by: ArkaSaha30 <[email protected]>
ArkaSaha30
force-pushed
the
fix-cert-duration-check
branch
from
e6346e5 to
4adafe5
Compare
This commit will handle a scenario of invalid auto cert provider ValidityDuration user input and throw an error. In case, ValidityDuration is not defined by user it will default to 365days for auto cert provider Signed-off-by: ArkaSaha30 <[email protected]>
ArkaSaha30
force-pushed
the
fix-cert-duration-check
branch
from
4adafe5 to
4d9d86c
Compare
Signed-off-by: ArkaSaha30 <[email protected]>
|
@ArkaSaha30: The following test failed, say
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. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
4 participants
This PR will handle a scenario of invalid
ValidityDurationuser input for the following certificate providers and throw an error.In case
ValidityDurationis not defined by the user, it will default to the corresponding default values:Fixes: #251