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

fix: remove the pattern validation for ResourceMeta.MountPoint #8874

Merged
merged 1 commit into from
Feb 8, 2025

Conversation

abarbare
Copy link
Contributor

@abarbare abarbare commented Feb 3, 2025

Hello,

When playing with Kubeblock, I wanted to add a custom configMap to my Cluster using userResourceRefs it fails when I try to use a mountPoint with 2 directories.

Right now, the mountPoint option do not allow having a full path in the field with this error

spec.componentSpecs[0].userResourceRefs.configMapRefs[0].mountPoint in body should match '^/[a-z]([a-z0-9\-]*[a-z0-9])?$'

I'm not sure why it seemed to work when implementing the feature.

On my end using a configuration like that it exits with the previous error.

  componentSpecs:
  - componentDefRef: mysql
    ....
   userResourceRefs:
       secretRefs:
           name: my-secret-volume
           mountPoint: /ops/config
           ### asVolumeFrom defines the list of containers will be injected into volumeMounts.
           asVolumeFrom: 
              - mysql
           secret:
                SecretName:  my-secret
       configMapRefs:
           name: my-config-volume
           mountPoint: /ops/scripts
           ### asVolumeFrom defines the list of containers will be injected into volumeMounts.
           asVolumeFrom: 
              - mysql
           configMap:
                name:  my-config  

@abarbare abarbare requested review from leon-inf, Y-Rookie and a team as code owners February 3, 2025 17:41
@CLAassistant
Copy link

CLAassistant commented Feb 3, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the size/S Denotes a PR that changes 10-29 lines. label Feb 3, 2025
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Feb 3, 2025
@apecloud-bot apecloud-bot added pre-approve Fork PR Pre Approve Test and removed pre-approve Fork PR Pre Approve Test labels Feb 3, 2025
@shanshanying
Copy link
Contributor

shanshanying commented Feb 5, 2025

@sophon-zt PTAL.

And BTW, @abarbare what is the user defined secret/configmap for in your case ?

@abarbare
Copy link
Contributor Author

abarbare commented Feb 5, 2025

Thanks for the review.
I was looking at a way to extend Clickhouse addons. Clickhouse allows user to configure the storage part with custom xml tags. I had a look to existing configSpec but as it the config relies on custom tags, the configSpec validation seems to be hard to implem.
Using directly configMaps with userResourceRefs could be a quickwin 🙂

@github-actions github-actions bot removed the size/S Denotes a PR that changes 10-29 lines. label Feb 5, 2025
@apecloud-bot apecloud-bot removed the pre-approve Fork PR Pre Approve Test label Feb 5, 2025
@github-actions github-actions bot added the size/XXL Denotes a PR that changes 1000+ lines. label Feb 5, 2025
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Feb 5, 2025
@abarbare
Copy link
Contributor Author

abarbare commented Feb 5, 2025

I added as well the . to set up path like /etc/myservice/config.d
Let me know if something else is required on my side, happy to help 🙂

@apecloud-bot apecloud-bot removed the pre-approve Fork PR Pre Approve Test label Feb 5, 2025
@github-actions github-actions bot added size/S Denotes a PR that changes 10-29 lines. and removed size/XXL Denotes a PR that changes 1000+ lines. labels Feb 5, 2025
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Feb 5, 2025
@shanshanying
Copy link
Contributor

I added as well the . to set up path like /etc/myservice/config.d Let me know if something else is required on my side, happy to help 🙂

Hi @abarbare
may I know which KubeBlocks version you are working on?

@loomts
Copy link
Contributor

loomts commented Feb 6, 2025

Hello @abarbare !

I was looking at a way to extend Clickhouse addons. Clickhouse allows user to configure the storage part with custom xml tags.

Maybe this tip is not related to this PR, you can simply add other clickhouse configuration like https://github.com/apecloud/kubeblocks-addons/pull/1450/files ~

@shanshanying
Copy link
Contributor

shanshanying commented Feb 6, 2025

Hi @abarbare

I added as well the . to set up path like /etc/myservice/config.d Let me know if something else is required on my side, happy to help 🙂

Based on your requirements, instead of using 'userResourceRefs' API, I'd recommend the following alternative:

  1. Specify how volumes are mounted in cmpd.spec.runtime.containers.volumeMounts.
  2. Define the volume when creating the cluster.

Here is an example:

apiVersion: apps.kubeblocks.io/v1
kind: ComponentDefinition
spec:
  runtime:
    containers:
      volumeMounts:
           - name: client-config
             mountPath: /etc/clickhouse-client/
+          - name: udf-config                                  # specify path and name
+            mountPath: /etc/myservice/config.d/
apiVersion: apps.kubeblocks.io/v1
kind: Cluster
spec:
    componentSpecs:
+      volumes:
+        - name: udf-config                            # specify the volume. it will be merged 
+          secret:
+            secretName: udf-secret

@abarbare
Copy link
Contributor Author

abarbare commented Feb 6, 2025

Hi @abarbare
may I know which KubeBlocks version you are working on?

I was working on 0.9.2 but starting to have a look to pre-released of v1 at the moment.

Maybe this tip is not related to this PR, you can simply add other clickhouse configuration like https://github.com/apecloud/kubeblocks-addons/pull/1450/files ~

I tried to use as far as possible standard addons. May be this is not the best practices. Regarding your experience of Kubeblocks, is it a standard way to customize deployment to update and install customized version of the addons to match specific requirements?

Based on your requirements, instead of using 'userResourceRefs' API, I'd recommend the following alternative:

Indeed, I tried it first this way but as the previous answer, it requires to update the ComponentDefinition of the addons in first place and therefore run a custom version of it instead of the foss version.

Excuse me for my lack of knowledge; I'm new to the Kubeblock ecosystem and trying to understand the best practices for its usage and how to extend it the more standard way.

@loomts
Copy link
Contributor

loomts commented Feb 7, 2025

Maybe this tip is not related to this PR, you can simply add other clickhouse configuration like https://github.com/apecloud/kubeblocks-addons/pull/1450/files ~

I tried to use as far as possible standard addons. May be this is not the best practices. Regarding your experience of Kubeblocks, is it a standard way to customize deployment to update and install customized version of the addons to match specific requirements?

Sure, Kubeblocks are designed to be flexible - you're always welcome to fork and customize our addons to meet specific requirements. I'll be adding a configuration customization guide to the ClickHouse addon's README soon to make this even clearer.

Indeed, I tried it first this way but as the previous answer, it requires to update the ComponentDefinition of the addons in first place and therefore run a custom version of it instead of the foss version.

I understand your concern about updating the ComponentDefinition and running a custom version instead of the FOSS one. Based on my experience, I will customize like this:

  1. git clone git@github.com:apecloud/kubeblocks-addons.git
  2. cd kubeblocks-addons and modify the clickHouse addon according to https://doc.crds.dev/github.com/apecloud/kubeblocks@v0.9.2
  3. helm list -A -> see clickhouse and ch-cluster
  4. helm uninstall clickhouse && helm uninstall ch-cluster -> uninstall addons chart and deployed cluster
  5. helm upgrade -i clickhouse addons/clickhouse && helm upgrade -i ch-cluster addons-cluster/clickhouse-cluster

@shanshanying
Copy link
Contributor

HI @abarbare

Many addons are contributed by the community. Feel free to submit a PR to the KubeBlocks Addon Repo with changes on your branch and state clearly why you made the changs and hwo pepole can use it.

@abarbare
Copy link
Contributor Author

abarbare commented Feb 7, 2025

Thank you both for your answers. I'll continue this way then and prepare a custom addon for this need.

Still I think this PR is still required as the / is not allowed in userResourceRefs as we speak. I can rollback the adding of the . if you want.

@shanshanying
Copy link
Contributor

hi @abarbare

we suggest simply removing the pattern checking for the api.

As this api is deprecated in v1, we will pick the commit to release 0.9 once merged.

@apecloud-bot apecloud-bot removed the pre-approve Fork PR Pre Approve Test label Feb 7, 2025
@github-actions github-actions bot added size/XS Denotes a PR that changes 0-9 lines. and removed size/S Denotes a PR that changes 10-29 lines. labels Feb 7, 2025
@apecloud-bot apecloud-bot added the pre-approve Fork PR Pre Approve Test label Feb 7, 2025
@zjx20 zjx20 changed the title fix: allow / in mountPoint fix: remove the pattern validation for ResourceMeta.MountPoint Feb 7, 2025
Copy link
Contributor

@zjx20 zjx20 left a comment

Choose a reason for hiding this comment

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

Thanks!

@zjx20 zjx20 merged commit e80c30c into apecloud:main Feb 8, 2025
19 of 20 checks passed
@github-actions github-actions bot added this to the Release 0.9.3 milestone Feb 8, 2025
@zjx20
Copy link
Contributor

zjx20 commented Feb 8, 2025

/cherry-pick release-1.0-beta

@apecloud-bot
Copy link
Collaborator

🤖 says: cherry pick action finished successfully 🎉!
See: https://github.com/apecloud/kubeblocks/actions/runs/13211434043

apecloud-bot pushed a commit that referenced this pull request Feb 8, 2025
@zjx20
Copy link
Contributor

zjx20 commented Feb 8, 2025

/cherry-pick release-0.9

@apecloud-bot
Copy link
Collaborator

🤖 says: cherry pick action finished successfully 🎉!
See: https://github.com/apecloud/kubeblocks/actions/runs/13211443267

apecloud-bot pushed a commit that referenced this pull request Feb 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pre-approve Fork PR Pre Approve Test size/XS Denotes a PR that changes 0-9 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants