Support the containers.conf container_name_as_hostname option#5943
Conversation
a433090 to
1991aae
Compare
run_common.go
Outdated
| // mapContainerNameToHostname returns the passed-in string with characters that | ||
| // don't match the pattern "[A-Za-z0-9][A-Za-z0-9._-]+" stripped out. | ||
| func mapContainerNameToHostname(containerName string) string { | ||
| const initialAllowedChars = "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789" |
There was a problem hiding this comment.
Would regex such as ^[A-Za-z0-9] and [A-Za-z0-9._-] be easier to maintain ? or could it be slow here ? I mean directly using regexp instead of string matching.
There was a problem hiding this comment.
That's potentially a lot of calls to FindStringIndex(), but it's worth a shot if the result is more readable.
Luap99
left a comment
There was a problem hiding this comment.
I think there needs to be a length limit like in podman or does buildah restrict a name length already earlier?
run_common.go
Outdated
| // don't match the pattern "[A-Za-z0-9][A-Za-z0-9._-]+" stripped out. | ||
| func mapContainerNameToHostname(containerName string) string { | ||
| const initialAllowedChars = "abcdefghijklmnopqrstuvwxyz" + "ABCDEFGHIJKLMNOPQRSTUVWXYZ" + "0123456789" | ||
| const allowedChars = initialAllowedChars + "._-" |
There was a problem hiding this comment.
_ is not a valid hostname char and must be stripped, the same is done in podman containers/podman@6fa234a
1991aae to
455f9d0
Compare
Nothing between setting one in the spec and handing it off to the runtime. Added one when we base it on the container name. |
When containers.conf has the "container_name_as_hostname" option set, use that value, with values that don't fit `[A-Za-z0-9][A-Za-z0-9.-]+` stripped out. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
455f9d0 to
72e2bf4
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, nalind The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgtm |
5868f44
into
containers:main
What type of PR is this?
/kind feature
What this PR does / why we need it:
When containers.conf has the "container_name_as_hostname" option set, use that value, with values that don't fit
[A-Za-z0-9][A-Za-z0-9._-]+stripped out.How to verify it
New integration test!
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?