Fix Rancher chart features value

[anmazzotti]

Fixes #1009

This PR only fixes the CATTLE_FEATURES notation that is currently ineffective.

[salasberryfin]

Thanks @anmazzotti. Doesn't this make the whole embedded-capi-disabled test suite obsolete?

[anmazzotti]

I am not exactly familiar with this test suite, but the behavior is unchanged.

The behavior seems to be that we let Rancher run the embedded CAPI. I did not really think this was possible, so now I'm confused. Is this supported?

[cpinjani]

I think for the suite to cover embedded-capi-disabled scenario and until rancher/rancher#41724 is fixed, we can perform steps:

1. Install Rancher (embedded-capi is enabled)
2. As mentioned in docs:

• Disable the embedded-cluster-api feature in Rancher
• Delete the mutating-webhook-configuration and validating-webhook-configuration webhooks that are no longer needed

3. Install turtles chart to run tests

[anmazzotti]

@salasberryfin Following your comment I kept the code as it is, just fixed the CATTLE_FEATURES usage, and disabled the test until the Rancher issue is fixed. Note that I tried to use the DontRunLabel, however this is going to fail during the BeforeSuite when the cluster is initialized.

For this reason I commented out the GHA workflow job instead.

[salasberryfin]

Thanks @anmazzotti. LGTM

[Danil-Grigorev]

rancher/rancher#41724 does not seem to get a fix any time soon (if ever) @anmazzotti. We are handling this by applying Feature resource change directly. Can you please update the PR to reflect this? I’m confused by the issue and PR.

Is the purpose to disable embedded capi test scenario? Can the be better clarified?

[anmazzotti]

@Danil-Grigorev I'm happy to clarify but I'm confused as well now.
I'm not the author of these tests, but my educated guess is that the embedded_capi_disabled test is simply verifying that everything works when the pre-install chart hook from Turtles does not disable the Rancher embedded-cluster-api feature.

There is no "embedded capi test scenario" that I know of. Using "embedded capi" from Rancher is not a supported setup.
To be more specific, the only thing this test is validating is this line here only: https://github.com/rancher/turtles/blob/main/charts/rancher-turtles/templates/pre-install-job.yaml#L1

This assumes Rancher can be installed without embedded-cluster-api, but it's not the case because of the upstream issue you linked.

I just recently tweaked the documentation to recommend users to rely on the Turtles chart to disable this Rancher feature. So in my opinion this test is now covering a not recommended scenario and could be eliminated.

I also wonder what was the idea with this setup also. Why are we even considering not turning off this Rancher feature?
Is it possible to use Turtles with the Rancher embedded capi?

If not then I'd say the embedded-capi Turtles chart value should also be removed. There are no scenarios where this can be used. If Rancher is installed (can Rancher not be installed?) then the embedded-cluster-api feature must be turned off always:

apiVersion: management.cattle.io/v3
kind: Feature
metadata:
  name: embedded-cluster-api
  annotations:
    "helm.sh/hook": pre-install
    "helm.sh/hook-weight": "1"
spec:
  value: false

[Danil-Grigorev]

I don't think that disabling the test is the right approach here. It is verifying that we don't break provisioning v2 implementation by managing CAPI with turtles. It may be the only test scenario to identify regression once a new API version of CAPI will be released - v1beta2. Historically was used to verify that gone v1alpha3 version does not affect provisioning v2 functionality. Currently fixed by aligning versions of CAPI in rancher and turtles - 1.9, but we'd prefer to have it latest. This TC is the one which can ensure this.

[Danil-Grigorev]

It shouldn't be possible to use turtles with embedded CAPI enabled, but it would be awesome to make it work, by using CAPI operator withing rancher/rancher as well (instead of current provisioning mechanism)

[anmazzotti]

@salasberryfin @Danil-Grigorev I just fixed the CATTLE_FEATURES usage.
This means the test will fail, but from the office hours I understand it will be removed, so I'll leave it to that.

[furkatgofurov7]

LGTM