From 9725ae46cb5bc7316be5989dc72b2c5db0ae85cd Mon Sep 17 00:00:00 2001 From: Simon Baird Date: Wed, 18 Dec 2024 17:31:37 -0500 Subject: [PATCH 1/4] Don't use timeout when zero timeout specified When running in a Tekton task it's better if Tekton handles the timeout, so that's why we want to be able to do this. I decide to skip the context.WithTimeout entirely rather than just set it to 100 years or whatever. Ref: https://issues.redhat.com/browse/EC-1030 --- cmd/root/root_cmd.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/cmd/root/root_cmd.go b/cmd/root/root_cmd.go index e7f9d82cb..6db8b1a2f 100644 --- a/cmd/root/root_cmd.go +++ b/cmd/root/root_cmd.go @@ -73,11 +73,18 @@ func NewRootCmd() *cobra.Command { // set a custom message for context.DeadlineExceeded error context.DeadlineExceeded = customDeadlineExceededError{} - // Create a new context now that flags have been parsed so a custom timeout can be used. - ctx, cancel := context.WithTimeout(cmd.Context(), globalTimeout) + // Create a new context now that flags have been parsed so a + // custom timeout can be used and traces can be added + ctx := cmd.Context() + var cancel context.CancelFunc + if globalTimeout > 0 { + ctx, cancel = context.WithTimeout(ctx, globalTimeout) + log.Debugf("globalTimeout is %d", globalTimeout) + } else { + log.Debugf("globalTimeout is %d, no timeout used", globalTimeout) + } ctx = tracing.WithTrace(ctx, enabledTraces) cmd.SetContext(ctx) - log.Debugf("globalTimeout is %d", globalTimeout) var cpuprofile *os.File var tracefile *os.File From 0be9be84e3ebb433b21e2b021cc54ed3a066c760 Mon Sep 17 00:00:00 2001 From: Simon Baird Date: Wed, 18 Dec 2024 18:54:50 -0500 Subject: [PATCH 2/4] Show a string for globalTimeout in debug output It's a little more user friendly this way. --- cmd/root/root_cmd.go | 2 +- features/task_validate_image.feature | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/root/root_cmd.go b/cmd/root/root_cmd.go index 6db8b1a2f..82bb02974 100644 --- a/cmd/root/root_cmd.go +++ b/cmd/root/root_cmd.go @@ -79,7 +79,7 @@ func NewRootCmd() *cobra.Command { var cancel context.CancelFunc if globalTimeout > 0 { ctx, cancel = context.WithTimeout(ctx, globalTimeout) - log.Debugf("globalTimeout is %d", globalTimeout) + log.Debugf("globalTimeout is %s", time.Duration(globalTimeout)) } else { log.Debugf("globalTimeout is %d, no timeout used", globalTimeout) } diff --git a/features/task_validate_image.feature b/features/task_validate_image.feature index d57d2ff7a..f110b1fd9 100644 --- a/features/task_validate_image.feature +++ b/features/task_validate_image.feature @@ -314,7 +314,7 @@ Feature: Verify Enterprise Contract Tekton Tasks | IGNORE_REKOR | true | | TIMEOUT | 666s | Then the task should succeed - And the task logs for step "debug-log" should contain "globalTimeout is 666000000000" + And the task logs for step "debug-log" should contain "globalTimeout is 11m6s" Scenario: SSL_CERT_DIR environment variable is customized Given a working namespace From b7c7d0e742dc6d7044ebff83262d02f0966b0126 Mon Sep 17 00:00:00 2001 From: Simon Baird Date: Thu, 19 Dec 2024 15:16:51 -0500 Subject: [PATCH 3/4] task: Always run without a timeout Ignore the TIMEOUT param. Mention in the param description that it's now ignored and deprecated. Ref: https://issues.redhat.com/browse/EC-1030 --- docs/modules/ROOT/pages/verify-enterprise-contract.adoc | 5 ++--- features/task_validate_image.feature | 6 ++++-- .../0.1/verify-enterprise-contract.yaml | 9 ++++++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/modules/ROOT/pages/verify-enterprise-contract.adoc b/docs/modules/ROOT/pages/verify-enterprise-contract.adoc index 6c5fb032f..cd6c194b6 100644 --- a/docs/modules/ROOT/pages/verify-enterprise-contract.adoc +++ b/docs/modules/ROOT/pages/verify-enterprise-contract.adoc @@ -64,9 +64,8 @@ paths can be provided by using the `:` separator. + *Default*: `now` *EXTRA_RULE_DATA* (`string`):: Merge additional Rego variables into the policy data. Use syntax "key=value,key2=value2..." -*TIMEOUT* (`string`):: Timeout setting for `ec validate`. -+ -*Default*: `5m0s` +*TIMEOUT* (`string`):: This param is deprecated and will be removed in future. Its value is ignored. EC will be run without a timeout. (If you do want to apply a timeout use the Tekton task timeout.) + *WORKERS* (`string`):: Number of parallel workers to use for policy evaluation. + *Default*: `1` diff --git a/features/task_validate_image.feature b/features/task_validate_image.feature index f110b1fd9..82646248d 100644 --- a/features/task_validate_image.feature +++ b/features/task_validate_image.feature @@ -298,7 +298,9 @@ Feature: Verify Enterprise Contract Tekton Tasks Then the task should succeed And the task logs for step "debug-log" should contain "Using provided effective time 2020-01-01T00:00:00Z" - Scenario: Timeout is honored + # Previously we did allow a custom timeout to be set via the TIMEOUT param, but now it's ignored. + # (This test could be removed in the future, but let's keep it for now I guess.) + Scenario: Deprecated timeout param is ignored Given a working namespace And a key pair named "known" And an image named "acceptance/timeout" @@ -314,7 +316,7 @@ Feature: Verify Enterprise Contract Tekton Tasks | IGNORE_REKOR | true | | TIMEOUT | 666s | Then the task should succeed - And the task logs for step "debug-log" should contain "globalTimeout is 11m6s" + And the task logs for step "debug-log" should contain "globalTimeout is 0, no timeout used" Scenario: SSL_CERT_DIR environment variable is customized Given a working namespace diff --git a/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml b/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml index ebf738db5..07f2cd01f 100644 --- a/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml +++ b/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml @@ -127,8 +127,10 @@ spec: - name: TIMEOUT type: string - description: Timeout setting for `ec validate`. - default: "5m0s" + description: > + This param is deprecated and will be removed in future. Its value is ignored. EC will + be run without a timeout. (If you do want to apply a timeout use the Tekton task timeout.) + default: "" - name: WORKERS type: string @@ -224,7 +226,8 @@ spec: - "$(params.WORKERS)" # NOTE: The syntax below is required to negate boolean parameters - "--info=$(params.INFO)" - - "--timeout=$(params.TIMEOUT)" + # No timeout for EC, though Tekton may apply a timeout + - "--timeout=0" - "--strict=false" - "--show-successes" - "--effective-time=$(params.EFFECTIVE_TIME)" From 73f183e25caa526cf842911a97e6e6e7982fbc69 Mon Sep 17 00:00:00 2001 From: Simon Baird Date: Mon, 6 Jan 2025 12:43:03 -0500 Subject: [PATCH 4/4] task: Use arbitrary long timeout instead of zero The commentary explains the reasoning. It's not expected to be an issue for Konflux, but theoretically RHTAP users might have an older EC base image and could some day migrate to this task definition which would break the task. Decided to keep the commit separate rather than squash so we can easily revert it some day. Ref: https://issues.redhat.com/browse/EC-1030 --- features/task_validate_image.feature | 2 +- .../0.1/verify-enterprise-contract.yaml | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/features/task_validate_image.feature b/features/task_validate_image.feature index 82646248d..53d8debb6 100644 --- a/features/task_validate_image.feature +++ b/features/task_validate_image.feature @@ -316,7 +316,7 @@ Feature: Verify Enterprise Contract Tekton Tasks | IGNORE_REKOR | true | | TIMEOUT | 666s | Then the task should succeed - And the task logs for step "debug-log" should contain "globalTimeout is 0, no timeout used" + And the task logs for step "debug-log" should contain "globalTimeout is 100h0m0s" Scenario: SSL_CERT_DIR environment variable is customized Given a working namespace diff --git a/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml b/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml index 07f2cd01f..add06b5b5 100644 --- a/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml +++ b/tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml @@ -226,8 +226,11 @@ spec: - "$(params.WORKERS)" # NOTE: The syntax below is required to negate boolean parameters - "--info=$(params.INFO)" - # No timeout for EC, though Tekton may apply a timeout - - "--timeout=0" + # Fresh versions of ec support "--timeout=0" to indicate no timeout, but this would break + # the task if it's used with an older version of ec. In an abundance of caution, let's set + # an arbitrary high value instead of using 0 here. In future we can change it to 0. + # (The reason to not use an explicit timeout for ec is so Tekton can handle the timeouts). + - "--timeout=100h" - "--strict=false" - "--show-successes" - "--effective-time=$(params.EFFECTIVE_TIME)"