-
Notifications
You must be signed in to change notification settings - Fork 10
[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
wizhaoredhat
merged 9 commits into
ovn-kubernetes:main
from
thom311:th/node-name-cleanup
Jun 3, 2025
Merged
[th/node-name-cleanup] cleanups around node name #219
wizhaoredhat
merged 9 commits into
ovn-kubernetes:main
from
thom311:th/node-name-cleanup
Jun 3, 2025
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Collaborator
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
08f5897
to
3b3073d
Compare
The regex parsing doesn't work for my node name
|
033beec
to
00dd1d7
Compare
m-naught
reviewed
May 30, 2025
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>
033beec
to
d9e112b
Compare
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.