Skip to content

[th/node-name-cleanup] cleanups around node name #219

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

Merged
merged 9 commits into from
Jun 3, 2025

Conversation

thom311
Copy link
Collaborator

@thom311 thom311 commented May 23, 2025

  • ktoolbox: bump to latest version
  • task: simplify selection of pod template and rename pod files
  • task: add "Task.render_pod_file()" helper
  • task: avoid redundant Task.out_file_yaml state
  • tftbase: add str_sanitize() function for sanitizing string
  • task: sanitize unusual characters in the pod name
  • task: make "node_name_sanitized" a property
  • testConfig: validate ConfNodeBase.name to be a valid DNS name
  • manifests: quote the {node,pod}_name Jinja variables in YAML

@thom311 thom311 force-pushed the th/node-name-cleanup branch 2 times, most recently from 08f5897 to 3b3073d Compare May 27, 2025 10:11
@m-naught
Copy link
Contributor

The regex parsing doesn't work for my node name

[FATAL] failed (exit 1); err='The Pod "sriov-pod-qs_ognrd_
ocars2_olab-server-5201" is invalid: 
\n* metadata.name: Invalid value: "sriov-pod-qs_ognrd_ocars2_olab-server-5201": a lowercase RFC 1123 subdomain must consis
t of lower case alphanumeric characters, \'-\' or \'.\', and must start and
 end with an alphanumeric character (e.g. \'example.com\', regex used for validation
 is \'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\')\n* spec.containers[0].name: 
Invalid value: "sriov-pod-qs_ognrd_ocars2_olab-server-520
1": a lowercase RFC 1123 label must consist of lower case alphanumeric characters or
 \'-\', and must start and end with an alphanumeric character (e.g. \'my-na
me\',  or \'123-abc\', regex used for validation is \'[a-z0-9]([-a-z0-9]*[a-z0-9])?\')\n'

@thom311 thom311 force-pushed the th/node-name-cleanup branch 2 times, most recently from 033beec to 00dd1d7 Compare May 27, 2025 15:26
thom311 added 9 commits May 30, 2025 12:24
Signed-off-by: Thomas Haller <thaller@redhat.com>
The "if" checks had duplicated code. For example, they all called
tftbase.get_manifest(), which can be done at the end of the block.

Also, we already chose a unique and suitable name for the pod. For the
generated YAML file, we should just use the same name. Previously, the
generated name would often differ from the pod name for no reason.

Signed-off-by: Thomas Haller <thaller@redhat.com>
There are two cases for calling Task.render_file():

- we render the file of the pod itself, that is, using
  self.in_file_template and self.out_file_yaml.
- we render other resources, where the caller specifies
  the in and out file parameters.

In no case, does the caller only want to specify either the
in_file_template or the out_file_yaml parameter.

This is confusing. Add Task.render_pod_file().

Also, change the log string for plugin pods from "Server Pod Yaml"
to "Plugin Pod Yaml".

Signed-off-by: Thomas Haller <thaller@redhat.com>
The property is always directly dependent on the pod_name.
Don't track it separately. Calculate it when needed.

Signed-off-by: Thomas Haller <thaller@redhat.com>
Preserves ASCII lower case letters and digits. But other characters
are escaped using "-".

The goal is that the input unambiguously maps to a result string (which
is reversible in theory).

Also, leading and trailing '-' are escaped too, because those are not
valid in a DNS label.

We will use this for the pod-name.

Signed-off-by: Thomas Haller <thaller@redhat.com>
Use tftbase.str_sanitize() for this.

Signed-off-by: Thomas Haller <thaller@redhat.com>
This seems more pythonic. The getter has no side effects and is cheap to
evaluate.

Also, it is based on the "node_name". That property is based on
immutable attributes and thus yields the identical reference on each
call. This makes "node_name" stateless and simple to reason about. As
such, "node_name_sanitized" also gives always an equal value, albeit a
newly allocated string reference.

Signed-off-by: Thomas Haller <thaller@redhat.com>
We use the node name to derive the pod_name. For that usage we have
"node_name_sanitize", which handles any characters.

Otherwise, we also use the node name for the pod selector:

  nodeSelector:
    kubernetes.io/hostname: {{ node_name }}

This hostname is usually the same as the node name. The node name must
be a valid DNS name. Technically, it seems you could configure the
hostname to differ from the node name, and then the hostname is not
required to be a valid DNS name. But let's not support that.

So this change breaks support for systems that rename the hostname of a
node to no longer be a valid DNS name. Just don't do that. Not
supported.

What this gives is that we know that the node name does not contain
bogus characters. For example, our naive Jinja2 replacement in the pod
files is not aware that it needs to insert a YAML string. Now we at
least know that the node name won't contain characters like '"' or '['
that would break the YAML.

Signed-off-by: Thomas Haller <thaller@redhat.com>
Jinja is not aware of what it is templating. So it does not know that we
want to set a string in YAML, or how to quote that.

Luckily, we already ensure that the node name is a valid DNS name. The
form of the pod name is even more strict. So they cannot contain quotes
that could break our YAML. Still, a node name "1" would be interpreted
as a plain number in YAML. We always want a YAML string.

Quote the value in the template.

In this case, this is safe, because we know that the node name and the
pod name only contain certain characters.

Signed-off-by: Thomas Haller <thaller@redhat.com>
@thom311 thom311 force-pushed the th/node-name-cleanup branch from 033beec to d9e112b Compare June 3, 2025 10:06
@wizhaoredhat
Copy link
Collaborator

LGTM

@wizhaoredhat wizhaoredhat merged commit c234f6b into ovn-kubernetes:main Jun 3, 2025
5 checks passed
@thom311 thom311 deleted the th/node-name-cleanup branch June 3, 2025 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants