Conversation
alpeb
left a comment
There was a problem hiding this comment.
Thanks @GTRekter , the implementation looks good to me, and thanks for the extended testing instructions 👍
I'd like to make sure I understand the problem, so let me rephrase things. In OCI (is that the same as OKE?) they have a CNI plugin that creates config files with the conflist-current extension. And your solution is then to have users create a symlink with an extension we support (conf or conflist), and thus this PR that adds support for symlinks. Is that correct?
Are you able to dig into OCI's docs and see if that extension is configurable, so that we get a full range of possible solutions?
Have you checked with the team that reported this issue whether they're able to create such symlinks? Also note that the symlinks would need to be created as soon as any new node is provisioned, so probably this solution would require for them to come up with additional automation.
Are there other platforms besides OCI where this is also an issue? If so, what extension do their files use? If we can come up with a list of extension names, we could just add them to the list we originally support, without having users to add the symlink.
(Also, please take care of the DCO 😉 )
| # back into the host mount (e.g. /etc/... -> /host/etc/...). | ||
| # We need to /etc -> /host/etc when the symlink points to an absolute path as | ||
| # net.d is the container’s filesystem, not the host mount, so when cp -L follows | ||
| # that absolute target it lands on a path that doesn’t. |
| file=$(resolve_cni_config_path "${file}") | ||
| tmp_file="$(mktemp -u /tmp/linkerd-cni.patch-candidate.XXXXXX)" | ||
| cp -fp "${file}" "${tmp_file}" | ||
| cp -fpL "${file}" "${tmp_file}" |
There was a problem hiding this comment.
${file} shouldn't be a symlink at this point, so why the -L?
In some cases, the CNI conflist file might be a symlink to a different file located in another directory or with a different extension (e.g., in OCI it uses .conflist-current).
Problem
Right now, the sync function loops over all files with the .conflist or .conf extension. This will:
Exclude symlink files
Exclude files with an extension other than .conflist or .conf
Challenge
We can’t use a simple wildcard after .conflist, because some CNIs create temporary files. That would create a lot of noise in the logs, since the Linkerd CNI binaries would try to modify temporary files that do not exist.
Solution
In both sync function and main flow, if a .conflist or .conf file is a symlink, we will resolve it and get the path to the original file. We will then append the Linkerd configuration to that file.
Tests
Local
simple-appnamespace and validate that the DELETE is called as expected