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

Invalid step name in conditions #3393

Closed
afrittoli opened this issue Oct 15, 2020 · 1 comment · Fixed by #3394
Closed

Invalid step name in conditions #3393

afrittoli opened this issue Oct 15, 2020 · 1 comment · Fixed by #3394
Assignees
@mattmoor
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@afrittoli
Copy link
Member

Expected Behavior

The generated step name for conditions is valid

Actual Behavior

If the parent name is long enough, it may become too long:

TestPipelineLevelFinally_OneDAGTaskFailed_Failure: kubelogs.go:197: E 09:59:06.501 tekton-pipelines-controller-655cb7594d-49ktz [pipelinerun/pipelinerun.go:211] [arendelle-7drp7/pipeline-level-finally-one-d-a-g-task-failed-failure-ynidsuge] Reconcile error: error creating ConditionCheck container called pipeline-level-finally-one-d-a-g-task-failed-failure-ynid-rz5k6 for PipelineTask dagtask3 from PipelineRun pipeline-level-finally-one-d-a-g-task-failed-failure-ynidsuge: admission webhook "validation.webhook.pipeline.tekton.dev" denied the request: validation failed: invalid value "condition-check-pipeline-level-finally-one-d-a-g-task-failed-failure-kmahgbvf": spec.taskspec.steps[0].name Task step name must be a valid DNS Label, For more info refer to https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names

Steps to Reproduce the Problem

  1. See https://tekton-releases.appspot.com/build/tekton-prow/pr-logs/pull/tektoncd_pipeline/3392/pull-tekton-pipeline-integration-tests/1316675745287770115/

Additional Info

The code that builds the name is

pipeline/pkg/reconciler/pipelinerun/resources/conditionresolution.go

Line 99 in bd885f8

rcc.Condition.Spec.Check.Name = unnamedCheckNamePrefix + rcc.Condition.Name
.

We may be able to use kmeta.ChildName to fix the issue.

  • Tekton Pipeline version:

v0.17.1
@afrittoli afrittoli added the kind/bug Categorizes issue or PR as related to a bug. label Oct 15, 2020
@afrittoli afrittoli mentioned this issue Oct 15, 2020
Merged
2 tasks
@mattmoor
Copy link
Member

I can take a look at this if nobody else is. I'm seeing it a lot and I dislodged the bug.

/assign

mattmoor added a commit to mattmoor/pipeline that referenced this issue Oct 15, 2020
@mattmoor
Use kmeta.ChildName to create DNS-safe names.
e62f1a3 
My recent change uncovered a latent bug where especially long test names result in especially long container names, which fails certain validations.  Unfortunately it slipped in because the test was expected to fail.

This leverages an upstream helper we created to solve ~exactly this problem.  You get DNS-safe short names based on an input name and a suffix, which are deterministic, and are friendly for suitably short inputs.

Fixes: tektoncd#3393
@mattmoor mattmoor mentioned this issue Oct 15, 2020
Merged
4 tasks
mattmoor added a commit to mattmoor/pipeline that referenced this issue Oct 15, 2020
@mattmoor
Ensure ConditionCheck container names are DNS-safe.
85e7f5b 
My recent change uncovered a latent bug where especially long test names result in especially long container names, which fails certain validations.  Unfortunately it slipped in because the test was expected to fail.

This leverages the names.SimpleNameGenerator.RestrictLength helper to solve this problem.

Fixes: tektoncd#3393
tekton-robot pushed a commit that referenced this issue Oct 15, 2020
@mattmoor @tekton-robot
Ensure ConditionCheck container names are DNS-safe.
d876923 
My recent change uncovered a latent bug where especially long test names result in especially long container names, which fails certain validations.  Unfortunately it slipped in because the test was expected to fail.

This leverages the names.SimpleNameGenerator.RestrictLength helper to solve this problem.

Fixes: #3393
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mattmoor mattmoor

Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make ConditionCheck container names DNS-safe mattmoor/pipeline
2 participants
@afrittoli @mattmoor