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.

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

invalid: metadata.labels for createBy when SSO subject does not start with alphanumeric charecters #4880

Closed
ermeaney opened this issue Jan 14, 2021 · 5 comments · Fixed by #4881
Assignees
Labels
Milestone

Comments

@ermeaney
Copy link
Contributor

ermeaney commented Jan 14, 2021

Summary

Submitting a workflow from the UI when SSO is enabled fails and throws a kubernetes validation error

Diagnostics

What Kubernetes provider are you using?
EKS v1.18
What version of Argo Workflows are you running?
reproduced in 2.12.2 and 2.12.4

The error we receive is

Unsuccessful HTTP response: Workflow.argoproj.io "legacyjobs-ghbc4" is invalid: metadata.labels: Invalid value: "_Cjks_k4UFqRHnV8IEbPNXVrhpAeJZa6It-6cxYHGD8": a valid label must be an empty string or consist of alphanumeric characters, '-', '_' or '.', and must start and end with an alphanumeric character (e.g. 'MyValue', or 'my_value', or '12345', regex used for validation is '(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?'):  Reload

where _Cjks_k4UFqRHnV8IEbPNXVrhpAeJZa6It-6cxYHGD8 is the subject name in user info

looking at argo/workflow/creator/creator.go

It appears that some of this was addressed in a recent PR, but a change to the dnsFriendly function something like this would probably address the problem we are seeing

func dnsFriendly(s string) string {
	value := regexp.MustCompile("[^-_.a-z0-9A-Z]").ReplaceAllString(s, "-")
        value = regexp.MustCompile("^[^a-z0-9A-Z]*").ReplaceAllString(value, "")
	if len(value) > 63 {
		value = value[len(value)-63:]
	}
	value = strings.TrimLeft(value, "-")
	return value
}

Since that should satisfy the regex used for validation in the above error message


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@alexec
Copy link
Contributor

alexec commented Jan 14, 2021

would you like to submit a PR to fix?

@alexec alexec added this to the v2.12 milestone Jan 14, 2021
@alexec
Copy link
Contributor

alexec commented Jan 14, 2021

https://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hostname-or-ip-address

I think we must remove underscores completely, and trim hyphens from both ends. This bit of code has produced many bugs for its size.

@alexec alexec self-assigned this Jan 14, 2021
@ermeaney
Copy link
Contributor Author

I can see if I can work out a PR, I am seeing if I can get the compile to run so I can try and write some tests for this also.

@ermeaney
Copy link
Contributor Author

Added a PR with fixes to clean the beginning and ends of the creator label, and added tests for it

Let me know if there is anything else

alexec pushed a commit that referenced this issue Jan 14, 2021
igned-off-by: Eric Meaney <emeaney@motus.com>
@simster7 simster7 mentioned this issue Jan 19, 2021
17 tasks
simster7 pushed a commit that referenced this issue Jan 19, 2021
Signed-off-by: Eric Meaney <emeaney@motus.com>
@simster7
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants