Skip to content
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

feat: update chart to enable crs and topology features #139

Conversation

salasberryfin
Copy link
Contributor

@salasberryfin salasberryfin commented Sep 21, 2023

What this PR does / why we need it:

This PR updates the chart so that when it deploys the CoreProvider it also sets the environment variables for the features:

  • CRS
  • Topology

These features are currently set by manually creating a secret with the desired values for the environment variables CLUSTER_TOPOLOGY and EXP_CLUSTER_RESOURCE_SET. This secret will now be created as part of the chart installation and both features will default to true. These can otherwise be disabled passing the parameters to Helm:

--set cluster-api-operator.cluster-api.secretConfig.name="user-secret-name"
--set cluster-api-operator.cluster-api.secretConfig.namespace="user-secret-namespace

When both these parameters are provided, the secret is expected to exist in the namespace and the user must manage its contents, including (but not limited to) CLUSTER_TOPOLOGY and EXP_CLUSTER_RESOURCE_SET.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #130

Special notes for your reviewer:

README is updated with the new values and a new issue is created to track rancher-turtles-docs updates rancher/turtles-docs#27.

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@salasberryfin salasberryfin requested a review from a team as a code owner September 21, 2023 15:03
@salasberryfin salasberryfin force-pushed the update-chart-to-enable-feature-flags branch from e31fa65 to e14033a Compare September 22, 2023 08:46
@salasberryfin salasberryfin changed the title WIP: feat: update chart to enable crs and topology features feat: update chart to enable crs and topology features Sep 22, 2023
@salasberryfin salasberryfin force-pushed the update-chart-to-enable-feature-flags branch 2 times, most recently from f2bd358 to ed9b78b Compare September 22, 2023 09:41
@@ -30,9 +30,6 @@ var (
//go:embed resources/testdata/fleet-capi-test.yaml
fleetCAPITestdata []byte

//go:embed resources/config/capi-providers-secret.yaml
capiProvidersSecret []byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? This secret is a “patch” on top of existing resource as well, and is used for providing additional env depending on type of test. Like in e2e for azure PR, it adds all azure env secrets.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was removed just because I did not see it being used other than in the Adding CAPI variables secret section of initRancherTurtles, which is no longer required, but I have no problem in keeping it if it's needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be no longer required a secret content should be fully customizable. Right now you can only set 2 feature flags.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we give the user the possibility to use their own secret, do we still need to let them customize the content of the "default" secret?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functionality used in e2e tests. There is a PR implementing test case for azure, it was opened a couple days prior to that. This is where the secret is used and extended.

@@ -68,4 +64,17 @@ data:
- apiGroups: ["rke-machine.cattle.io"]
resources: ["*"]
verbs: ["*"]
---
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a user would want to use his own secret, already created in the cluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a flag to use an existing secret. The default behavior would be to create the pre-configured secret but the user can pass this parameter to avoid this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The simplest way how to achieve the goal - to not create anything if name and namespace is not set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initial idea was to use an opt-out strategy rather than opt-in, so the secret would be created by default and the values for these two features set to true. They would be able to explicitly turn them off.

@@ -30,9 +30,6 @@ var (
//go:embed resources/testdata/fleet-capi-test.yaml
fleetCAPITestdata []byte

//go:embed resources/config/capi-providers-secret.yaml
capiProvidersSecret []byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this to be no longer required a secret content should be fully customizable. Right now you can only set 2 feature flags.

@alexander-demicev
Copy link
Member

In my opinion, what we should do here is to create a default secret in the core provider namespace with CRS and topology set to true if the user doesn't provide secret name and namespace. If these values are provided we do nothing and let users do it themselves. We have to document this behavior and that we ask CRS and topology to be enabled if they create a secret manually.

@Danil-Grigorev
Copy link
Contributor

In my opinion, what we should do here is to create a default secret in the core provider namespace with CRS and topology set to true if the user doesn't provide secret name and namespace. If these values are provided we do nothing and let users do it themselves. We have to document this behavior and that we ask CRS and topology to be enabled if they create a secret manually.

This information is provided in the docs in several places. For example: https://docs.rancher-turtles.com/docs/getting-started/install_capi_operator#install-manually-with-helm-alternative

IMO there are 3 different use-cases happening there.

  1. User wants to adopt existing secret.
  2. User wants to create a new secret with default set of values provided by us.
  3. User wants to create a new secret with a custom set of values provided by them.

Solution described is better then “always” enforcing secret creation, yet the full set of use-cases could be covered with an addition of gauge value for helm, something like secret-create: true. True by default will create a secret in a specified namespace/name. False, will lead to adoption of secret in the namespace/name.

In addition to that, the cluster-api-operator helm chart has another set of flags for secret handing, which in part overlaps with existing templating, I think this is slightly misleading.

The other part - secret data. As helm values are just yaml, we can define something like in the values.yaml:

secret-data: |
  CLUSTER_TOPOLOGY: "true"
  EXP_CLUSTER_RESOURCE_SET: "true"

and then reference it in the secret with stringData: {{ .Values.secret-data }} or similar (templating may be wrong).

I’m ok with any solution provided which is documented and won’t cause confusion, but removing secret patching functionality in tests is not yet addressed - #139 (comment)

@salasberryfin
Copy link
Contributor Author

I would be more in favor of @alexander-demicev's proposal. I think this is a simpler solution and gives the user the freedom to manage their own secret if they want to do so, which I would say covers both point 2 and 3 in @Danil-Grigorev's list of use cases.

The deletion of the secret patching functionality was just an initial clean-up proposal which I'll undo. This could use a user-managed secret (current status) with any custom environment variables/feature flags.

@salasberryfin salasberryfin force-pushed the update-chart-to-enable-feature-flags branch 3 times, most recently from 9b8422f to 6f809db Compare September 25, 2023 14:37
@salasberryfin salasberryfin force-pushed the update-chart-to-enable-feature-flags branch 3 times, most recently from 191e894 to adde2ff Compare September 25, 2023 15:26
@salasberryfin
Copy link
Contributor Author

Updated the changes to support two scenarios:

  • User does not provide name or namespace: create default secret with crs and topology enabled.
  • User passes name and namespace: do not create secret and use user-managed with custom content. This is the current behavior.

Also, created issue rancher/turtles-docs#27 to track documentation updates.

Danil-Grigorev
Danil-Grigorev previously approved these changes Sep 26, 2023
richardcase
richardcase previously approved these changes Sep 26, 2023
@salasberryfin
Copy link
Contributor Author

There was a condition missing in core-provider.yaml when setting CoreProvider https://github.com/rancher-sandbox/rancher-turtles/blob/d73fd08d01e71aa21ecdecfa23d0269f94cfc3d6/charts/rancher-turtles/templates/core-provider.yaml#L24 It should now be fixed

@alexander-demicev alexander-demicev merged commit 0e28a41 into rancher:main Sep 26, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the chart to enable the CRS & Topology features
4 participants