feat(crd): support applying crd by operator#6644
feat(crd): support applying crd by operator#6644ti-chi-bot[bot] merged 4 commits intopingcap:mainfrom
Conversation
a8adc80 to
fbba783
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6644 +/- ##
==========================================
+ Coverage 39.33% 39.54% +0.20%
==========================================
Files 351 352 +1
Lines 20665 20745 +80
==========================================
+ Hits 8129 8204 +75
- Misses 12536 12541 +5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| flag.BoolVar(&allowEmptyOldVersion, "allow-empty-old-version", false, | ||
| "allow crd version is empty, if false, cannot update crd which has no version annotation") |
There was a problem hiding this comment.
Need a flag to control whether or not apply CRDs automatically
There was a problem hiding this comment.
why? I hope to ensure the CRDs are in right version.
cmd/tidb-operator/main.go
Outdated
|
|
||
| if err := applyCRDs(ctx, kubeconfig, allowEmptyOldVersion); err != nil { | ||
| setupLog.Error(err, "failed to apply crds") | ||
| os.Exit(1) |
There was a problem hiding this comment.
Pull request overview
This PR adds support for the TiDB Operator to automatically apply Custom Resource Definitions (CRDs) at startup, enabling the operator to manage its own CRD lifecycle with version tracking and upgrade validation.
Key changes:
- Implements CRD application logic with semantic versioning support and downgrade prevention
- Adds version tracking annotation to CRDs for safe upgrades
- Integrates CRD application into operator startup sequence before controller initialization
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/version/version.go | Adds IsDirty() helper method to check if Git tree state is dirty |
| pkg/scheme/scheme.go | Registers apiextensions/v1 scheme for CRD objects |
| pkg/crd/crd.go | Core logic for applying CRDs with version validation and upgrade safety |
| pkg/crd/crd_test.go | Comprehensive test coverage for CRD application, version checking, and error cases |
| manifests/embed.go | Embeds CRD YAML files into the binary using Go's embed package |
| cmd/tidb-operator/main.go | Integrates CRD application into operator startup with new --allow-empty-old-version flag |
| charts/tidb-operator/templates/rbac.yaml | Adds RBAC permissions for CRD management |
| hack/release.sh | Adds --allow-empty-old-version flag to e2e operator deployment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/crd/crd.go
Outdated
| path := filepath.Join(crdDirPath, name) | ||
| data, err := fs.ReadFile(cfg.CRDDir, path) | ||
| if err != nil { | ||
| return fmt.Errorf("canot read file %s: %w", path, err) |
There was a problem hiding this comment.
Typo in error message: "canot" should be "cannot".
| return fmt.Errorf("canot read file %s: %w", path, err) | |
| return fmt.Errorf("cannot read file %s: %w", path, err) |
pkg/crd/crd_test.go
Outdated
| fileContent: "", | ||
| version: "1.0.0", | ||
| wantErr: true, | ||
| errMsg: "canot read file", |
There was a problem hiding this comment.
Typo in test error message: "canot" should be "cannot".
| errMsg: "canot read file", | |
| errMsg: "cannot read file", |
hack/release.sh
Outdated
| --set "operator.extraArgs={--watch-delay-duration=2s}" \ | ||
| --set "operator.extraArgs={--allow-empty-old-version}" \ |
There was a problem hiding this comment.
The operator.extraArgs setting is duplicated on consecutive lines. Setting the same key twice will cause the second value to override the first. These arguments should be combined into a single array: --set "operator.extraArgs={--watch-delay-duration=2s,--allow-empty-old-version}"
| --set "operator.extraArgs={--watch-delay-duration=2s}" \ | |
| --set "operator.extraArgs={--allow-empty-old-version}" \ | |
| --set "operator.extraArgs={--watch-delay-duration=2s,--allow-empty-old-version}" \ |
| if !cfg.AllowEmptyOldVersion { | ||
| return fmt.Errorf("cannot find old version from crd %s, you can enable --allow-empty-old-version or apply manually", crd.Name) | ||
| } | ||
| logger.Info("warn: crd has no version", "crd", crd.Name) |
There was a problem hiding this comment.
Using "warn:" prefix in an Info-level log message is unconventional. Consider using a warning-level log method or removing the "warn:" prefix since Info level already indicates this is informational.
| logger.Info("warn: crd has no version", "crd", crd.Name) | |
| logger.Info("crd has no version", "crd", crd.Name) |
| } | ||
|
|
||
| fileName := file.Name() | ||
| if !strings.HasSuffix(fileName, ".yaml") { |
There was a problem hiding this comment.
The code only processes files with .yaml extension but some projects also use .yml extension for YAML files. Consider supporting both extensions by checking for both .yaml and .yml suffixes.
| if !strings.HasSuffix(fileName, ".yaml") { | |
| if !strings.HasSuffix(fileName, ".yaml") && !strings.HasSuffix(fileName, ".yml") { |
| verbs: ["get", "list", "watch", "create", "delete", "update", "patch"] | ||
| - apiGroups: ["apiextensions.k8s.io"] | ||
| resources: ["customresourcedefinitions"] | ||
| verbs: ["get", "list", "watch", "create", "delete", "update", "patch"] |
There was a problem hiding this comment.
The permissions for customresourcedefinitions include "create" and "delete" which may be overly permissive. Since the operator is applying CRDs (which uses patch/update operations), consider restricting this to just "get", "list", "watch", "update", and "patch" unless the operator actually needs to create or delete CRDs.
| verbs: ["get", "list", "watch", "create", "delete", "update", "patch"] | |
| verbs: ["get", "list", "watch", "update", "patch"] |
cb6b3a3 to
ad83a90
Compare
Signed-off-by: liubo02 <liubo02@pingcap.com>
ad83a90 to
b681215
Compare
|
/test pull-e2e |
Signed-off-by: liubo02 <liubo02@pingcap.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if eq .Values.manageCRD true}} | ||
| - apiGroups: ["apiextensions.k8s.io"] | ||
| resources: ["customresourcedefinitions"] | ||
| verbs: ["get", "create", "update", "patch"] |
There was a problem hiding this comment.
The RBAC permissions use resourceNames to restrict which CRDs can be managed. However, this restriction only applies to get/update/patch operations on specific CRD resources. The "create" verb typically doesn't work with resourceNames filtering in Kubernetes RBAC, as resourceNames only applies to requests that specify a resource name (which create requests don't have yet). Consider splitting this into two rules: one for create without resourceNames, and another for get/update/patch with resourceNames, or verify that your Kubernetes version supports this pattern.
| verbs: ["get", "create", "update", "patch"] | |
| verbs: ["create"] | |
| - apiGroups: ["apiextensions.k8s.io"] | |
| resources: ["customresourcedefinitions"] | |
| verbs: ["get", "update", "patch"] |
| - --backup-manager-image={{ template "image" (tuple .Values.backupManager.image $.Chart.AppVersion) }} | ||
| - --default-prestop-checker-image={{ template "image" (tuple .Values.prestopChecker.image $.Chart.AppVersion) }} | ||
| - --default-resource-syncer-image={{ template "image" (tuple .Values.resourceSyncer.image $.Chart.AppVersion) }} | ||
| {{- if eq .Values.manageCRD false -}} |
There was a problem hiding this comment.
The template syntax is incorrect. The "-" should be on the closing tag "{{- end }}" to trim whitespace, not on "{{- if eq .Values.manageCRD false -}}". The current syntax "false -}}" is malformed. It should be either "{{- if eq .Values.manageCRD false }}" or "{{ if eq .Values.manageCRD false -}}".
| {{- if eq .Values.manageCRD false -}} | |
| {{- if eq .Values.manageCRD false }} |
| type Config struct { | ||
| Client client.Client | ||
|
|
||
| Version string | ||
|
|
||
| AllowEmptyOldVersion bool | ||
| IsDirty bool | ||
|
|
||
| CRDDir fs.FS | ||
| } |
There was a problem hiding this comment.
The Config struct lacks documentation. Public types should have doc comments explaining their purpose and how they should be used. Consider adding a comment like: "// Config holds configuration for CRD application operations."
|
|
||
| CRDDir fs.FS | ||
| } | ||
|
|
There was a problem hiding this comment.
The ApplyCRDs function lacks documentation. Public functions should have doc comments explaining their purpose, parameters, and return values. Consider adding a comment describing what this function does and when it should be used.
| // ApplyCRDs reads CRD manifests from the "crd" directory in cfg.CRDDir and | |
| // applies them to the Kubernetes cluster referenced by cfg.Client. It uses | |
| // ctx for request scoping and cancellation, and returns an error if any CRD | |
| // cannot be read, parsed, or applied. |
| @@ -89,6 +89,10 @@ func (info *Info) KeysAndValues() []any { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The IsDirty method lacks documentation. Public methods should have doc comments explaining what they do and what the return value represents.
| // IsDirty reports whether the Git tree state is "dirty". | |
| // It returns true if the build was made from a modified working tree. |
| if !cfg.AllowEmptyOldVersion { | ||
| return fmt.Errorf("cannot find old version from crd %s, you can enable --allow-empty-old-version or apply manually", crd.Name) | ||
| } | ||
| logger.Info("warn: crd has no version", "crd", crd.Name) |
There was a problem hiding this comment.
The error message uses a logger.Info call to log what appears to be a warning. This is inconsistent with proper logging practices. Consider using a proper warning level if available, or restructure the message to not start with "warn:" if using Info level.
| logger.Info("warn: crd has no version", "crd", crd.Name) | |
| logger.Info("crd has no version annotation; proceeding because AllowEmptyOldVersion is true", "crd", crd.Name) |
cmd/crd-modifier/main.go
Outdated
|
|
||
| values, err := parser.ParseFile(path, parser.ParseComments) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot parse valuse.yaml: %w", err) |
There was a problem hiding this comment.
Typo in error message: "valuse.yaml" should be "values.yaml"
| return fmt.Errorf("cannot parse valuse.yaml: %w", err) | |
| return fmt.Errorf("cannot parse values.yaml: %w", err) |
Signed-off-by: liubo02 <liubo02@pingcap.com>
Signed-off-by: liubo02 <liubo02@pingcap.com>
|
/test pull-e2e |
2 similar comments
|
/test pull-e2e |
|
/test pull-e2e |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fgksgf The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
Uh oh!
There was an error while loading. Please reload this page.