Skip to content
This repository has been archived by the owner on Mar 6, 2023. It is now read-only.

Allow AcmeTimeout to be configured with OPENSHIFT_ACME_TIMEOUT env #86

Closed
wants to merge 2 commits into from

Conversation

seandilda
Copy link
Contributor

I had problems running against an internal ACME server where the certs weren't always available within 60 seconds and hard coded timeout caused the request to fail.

This change allows the timeout to be changed per deployment.

@openshift-ci-robot
Copy link
Collaborator

Hi @seandilda. Thanks for your PR.

I'm waiting for a tnozicka or openshift 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.

pkg/cmd/cmd.go Outdated
@@ -96,6 +97,7 @@ func NewOpenShiftAcmeCommand(in io.Reader, out, err io.Writer) *cobra.Command {
rootCmd.PersistentFlags().StringP(Flag_ExposerListenIP, "", "0.0.0.0", "Listen address for http-01 server")
rootCmd.PersistentFlags().StringP(Flag_SelfNamespace_Key, "", "", "Namespace where this controller and associated objects are deployed to. Defaults to current namespace if this program is running inside of the cluster.")
rootCmd.PersistentFlags().StringP(Flag_DefaultRouteTermination_Key, "", string(routev1.InsecureEdgeTerminationPolicyRedirect), "Default TLS termination of the route.")
rootCmd.PersistentFlags().Int32P(Flag_Timeout, "", 60, "Timeout for talking to ACME server")
Copy link
Owner

Choose a reason for hiding this comment

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

pls make it typed using DurationVarP

@tnozicka
Copy link
Owner

/ok-to-test

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@LorbusChris
Copy link

friendly ping @seandilda, would you mind adressing the requested changes and rebasing?
/remove-lifecycle stale

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: seandilda
To complete the pull request process, please assign tnozicka
You can assign the PR to them by writing /assign @tnozicka 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

@seandilda
Copy link
Contributor Author

/assign @tnozicka

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@seandilda
Copy link
Contributor Author

/remove-lifecycle rotten

@tnozicka Any updates on this?

@tnozicka
Copy link
Owner

tnozicka commented Dec 2, 2019

@seandilda thanks for the PR, I've checked the V2 rewrite and there is a similar change incorporated which should cover the scenario

To avoid conflicts with the rewrite, closing in favor of https://github.com/tnozicka/openshift-acme/pull/92/files#diff-7de04b860f288545dc225de20a47af16R127

/close

@openshift-ci-robot
Copy link
Collaborator

@tnozicka: Closed this PR.

In response to this:

@seandilda thanks for the PR, I've checked the V2 rewrite and there is a similar change incorporated which should cover the scenario

To avoid conflicts with the rewrite, closing in favor of https://github.com/tnozicka/openshift-acme/pull/92/files#diff-7de04b860f288545dc225de20a47af16R127

/close

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 subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants