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

Allow most printable ASCII chars for service label key #2000

Merged
1 commit merged into from
Oct 17, 2022

Conversation

kb2ma
Copy link
Contributor

@kb2ma kb2ma commented Sep 1, 2022

Description

Updated the regex for valid characters for a service label to allow printable ASCII chars except space, double/single quotes, and backtick. These extended characters support apps like Traefik and caddy-docker-proxy. Characters still not supported likely are not useful as label keys and error prone.

Fixes #1949

Type of change

patch

How Has This Been Tested?

Updated 06-validation.spec.ts. Also tested by POSTing all valid chars to /v2/local/target-state.

@kb2ma kb2ma added area/compose-feature A feature which is part of the supported compose fields versionbot/pr-draft labels Sep 1, 2022
Copy link
Contributor

@cywang117 cywang117 left a comment

Choose a reason for hiding this comment

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

This LGTM! Do you have to add Closes: #1949 in the commit footer for bulldozer-bot to auto-close the issue on merge? Or is having it in the PR description sufficient? Since I'm not sure, I'd suggest adding it to the commit footer just in case.

@dfunckt
Copy link
Member

dfunckt commented Sep 21, 2022

Unless the Supervisor assigns labels to containers/volumes/etc. and balena-engine rejects them, it doesn't really need to validate them.

I think this in fact applies in general: data that only ever comes from the target state will necessarily be valid because tooling (eg. the CLI, builder, API, etc.) will have rejected invalid releases before they make it to the Supervisor.

@kb2ma
Copy link
Contributor Author

kb2ma commented Sep 21, 2022

Unless the Supervisor assigns labels to containers/volumes/etc. and balena-engine rejects them, it doesn't really need to validate them.

But the accepted character set on an older Supervisor version may not include characters from a newer balena-builder or balena-cli. So in a sense the Supervisor is acting as the guardian of the device. It knows best what the device should accept.

cc: @pipex

@pipex
Copy link
Contributor

pipex commented Sep 22, 2022

I agree with Ken, even though there are some validations on the CLI and backend, the user may be using an incompatible CLI version, so the supervisor should still do some validations (without going too crazy).

The supervisor also exposes the target-state endpoint that allows to bypass the CLI entirely, so validation is needed there for sure.

@dfunckt
Copy link
Member

dfunckt commented Sep 22, 2022

But the accepted character set on an older Supervisor version may not include characters from a newer balena-builder or balena-cli.

@kb2ma from a user’s perspective this will appear as failure to deploy a release. The correct way to introduce breaking changes is via contracts. Let’s chat in JF how to go about doing that.

@pipex I wasn’t aware about that endpoint but my point still stands, validation should happen in that endpoint and ideally using the same primitives/code the CLI and builder use.

@kb2ma kb2ma marked this pull request as draft September 22, 2022 11:13
Change-type: patch
Signed-off-by: Ken Bannister <kb2ma@runbox.com>
@kb2ma kb2ma marked this pull request as ready for review October 17, 2022 12:05
@kb2ma
Copy link
Contributor Author

kb2ma commented Oct 17, 2022

@balena-ci I self-certify!

@ghost ghost merged commit 7297b74 into master Oct 17, 2022
@ghost ghost deleted the accept_more_label_chars branch October 17, 2022 12:07
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/compose-feature A feature which is part of the supported compose fields
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove restrictions on docker labels
4 participants