Allow configuration of Calico host mount path

[uhthomas]

Description

Calico persists data to /etc/calico on the host but unfortunately some operating systems, like Talos, mount /etc/ as read-only. This change allows configuration of the host mount path.

Fixes #2444

I am aware there are no tests. I wanted to get a draft PR open to move the issue along. I'd like to understand if:

• This is the right way to make the change.
• If the field name is inline with expectations.
• If the field name is in the correct place.
• What the best way to test this change would be.

Thanks!

For PR author

• Tests for change.
• If changing pkg/apis/, run make gen-files
• If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

• Milestone set according to targeted release.
• Appropriate labels:
  • kind/bug if this is a bugfix.
  • kind/enhancement if this is a a new feature.
  • enterprise if this PR applies to Calico Enterprise only.

[CLAassistant]

All committers have signed the CLA.

[mgleung]

Hey @uhthomas , I may not be able to answer all of your questions above about the naming conventions and optimal setting location, but I should be able to help with some testing.

I think the first thing you'll want to do is run make test locally to run the UTs. After that, I think the easiest way to see if this works with regards to all of our components is to build your version of the operator make image and then deploy your created operator image into a test cluster (using the steps at https://docs.tigera.io/calico/3.25/getting-started/kubernetes/quickstart and modifying the operator image to point to yours would probably be easiest). You should see everything come up without any issue but it might be a good idea to also check that there's no strange error logs in the calico-node instances that boot. Let me know if you need any help with any help validating any of these steps.

[epleterte]

@uhthomas I've had a similar patch lying around for a couple of weeks, but wanted to test it first. Had some trouble building initially and haven't had time to pick it up, but just gave building another go with success. Did you get around to do any testing yet?

[ErikLundJensen]

Any progress?

[agmimidi]

Thanks for the contribution Thomas.
Any updates on when this will be part of an official release?

[ErikLundJensen]

We have tested you change to tigera-operator together with Calico v3.25.1 and it works as expected. Thanks for you contribution, we now need this to go into the next release of the tigera-opeartor.

[uhthomas]

Awesome, thanks for testing this change. Is there anything else which needs to be done for this to be merged?

[tmjd]

/sem-approve

[tmjd]

I think you'll need to rebase your changes on master to pick up updates in #2613 to address one issue in CI.
Also you need to update pkg/controller/utils/merge.go to account for the new field. You should be able to see the failure with go test ./pkg/controller/utils.

[uhthomas]

Thanks @tmjd, I updated pkg/controller/utils/merge.go.

As an aside: make gen-files does not work on macOS.

for x in $(find config/crd/bases/*); do sed -i -e '/creationTimestamp: null/d' -e '/^---/d' -e '/^\s*$/d' $x; done
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
sed: -e: No such file or directory
make: *** [manifests] Error 1

I tried fixing this, but it still doesn't work.

❯ for x in $(find config/crd/bases/*); do sed -i -e'/creationTimestamp: null/d' -e'/^---/d' -e'/^\s*$/d' $x; done
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory
sed: rename(): No such file or directory

I had to work around this by installing gnu-sed and then:

❯ for x in $(find config/crd/bases/*); do gsed -i -e '/creationTimestamp: null/d' -e '/^---/d' -e '/^\s*$/d' $x; done

[tmjd]

/sem-approve

[uhthomas]

@tmjd Looks like CI is happy, is this okay to merge?

[tmjd]

Sorry no response for a while. I've been trying to chat with people and figure out if this is what we need/want. Initially I was going to ask you to change the field name to be more specific but from looking at the PR I was compelled to ask a few more questions about what the directory is used for. The answer was that it is only for a configuration file that no one probably uses and isn't really the way we should be configuring features with an operator install.
This configuration file idea is a copy of how we handled FlexVol configuration, FlexVol consisted of a binary that was on a host system and so it needed a file based configuration. With CSI, which uses this host mount path, the code is containerized so we can provide configuration to the pod directly instead of needing to use a configuration file.
Hopefully that provides the context for what we should do with this PR.

So with all that info, instead of exposing a new configuration field to set a host path that no one is probably using, we should remove the volume mount so it is not present.

I'm open to hear if this is problematic for anyone that has been a part of this conversation (or anyone not yet part of it too).

[uhthomas]

Thanks for following up @tmjd. This sounds like a good thing to do, though I do see there are 169 references for "/etc/calico" across the tigera organisation. How much work would it be to remove the need for that path altogether?

[tmjd]

I don't think we need to worry about any of those other references. None of them are used as a host path volume with an operator install. Many of them are still the location where some files go but it is only inside of a pod (no volume mount) so it doesn't really matter what that path is.

I know calicoctl references that path for configuration but it is a binary that is expected to be a user utility, I'm not sure if that path is configurable (I think it is) but that is outside of the scope of the operator. You could check on calicoctl and make that path configurable if it is not yet.

[tmjd]

I'm closing this PR since #2654 was merged instead of this one.