Skip to content

🐛 fix support for external types by allowing the domain be empty, and properly generate the sample for cert-manager. #4217

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 22, 2024
Merged

🐛 fix support for external types by allowing the domain be empty, and properly generate the sample for cert-manager. #4217

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions docs/book/src/reference/using_an_external_resource.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,22 +28,22 @@ kubebuilder create api --group <theirgroup> --version <theirversion> --kind <the
For example, if you're managing Certificates from Cert Manager:

```shell
kubebuilder create api --group certmanager --version v1 --kind Certificate --controller=true --resource=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io
kubebuilder create api --group certmanager --version v1 --kind Certificate --controller=true --resource=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=io
```

See the RBAC markers generated for this:
See the RBAC [markers][markers-rbac] generated for this:

```go
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/finalizers,verbs=update
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/finalizers,verbs=update
```

Also, the RBAC role:

```ymal
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates
verbs:
Expand All @@ -55,13 +55,13 @@ Also, the RBAC role:
- update
- watch
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/finalizers
verbs:
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/status
verbs:
Expand Down Expand Up @@ -126,15 +126,15 @@ For instance, to create a controller to manage Deployment the command would be l
create api --group apps --version v1 --kind Deployment --controller=true --resource=false
```

See the RBAC markers generated for this:
See the RBAC [markers][markers-rbac] generated for this:

```go
// +kubebuilder:rbac:groups=apps,resources=deployments,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=apps,resources=deployments/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=apps,resources=deployments/finalizers,verbs=update
```

Also, the RBAC for the above markers:
Also, the RBAC for the above [markers][markers-rbac]:

```yaml
- apiGroups:
Expand Down Expand Up @@ -176,4 +176,4 @@ See an example:
```go
kubebuilder create webhook --group core --version v1 --kind Pod --programmatic-validation
```

[markers-rbac]: ./markers/rbac.md
8 changes: 0 additions & 8 deletions pkg/plugins/golang/v4/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,14 +146,6 @@ func (p *createAPISubcommand) InjectResource(res *resource.Resource) error {
}
}

// Ensure that if any external API flag is set, both must be provided.
if len(p.options.ExternalAPIPath) != 0 || len(p.options.ExternalAPIDomain) != 0 {
if len(p.options.ExternalAPIPath) == 0 || len(p.options.ExternalAPIDomain) == 0 {
return errors.New("Both '--external-api-path' and '--external-api-domain' must be " +
"specified together when referencing an external API.")
}
}

Copy link
Member Author

@camilamacedo86 camilamacedo86 Oct 22, 2024

Choose a reason for hiding this comment

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

Hi @fischor,

In my opinion, groups should not be empty when dealing with non-core types in scaffolds. The concepts should stay aligned with Kubernetes itself. We cannot have Kubebuilder diverging from core Kubernetes principles. However, in order to not block others scenarios we should not make mandatory users provide the Domain.

What are your thoughts?

Copy link

@fischor fischor Oct 22, 2024

Choose a reason for hiding this comment

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

I am refering to the "group" (--group) as used in kubebuilder, not the Kubernetes apiGroup.
The apiGroup will not be empty if you do --domain cert-manager.io --group "", it will be cert-manager.io then.

My thoughts are that is does not make sense to have --domain and --group for external APIs, since these are kubebuilder specific entities and not all external APIs use that scheme. Maybe just --api-group is better to specify the (full) apiGroup for external APIs.

p.options.UpdateResource(p.resource, p.config)

if err := p.resource.Validate(); err != nil {
Expand Down
8 changes: 4 additions & 4 deletions test/testdata/generate.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ function scaffold_test_project {
$kb create api --group crew --version v1 --kind Admiral --plural=admirales --controller=true --resource=true --namespaced=false --make=false
$kb create webhook --group crew --version v1 --kind Admiral --plural=admirales --defaulting
# Controller for External types
$kb create api --group certmanager --version v1 --kind Certificate --controller=true --resource=false --make=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io
$kb create api --group "cert-manager" --version v1 --kind Certificate --controller=true --resource=false --make=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=io
# Webhook for External types
$kb create webhook --group certmanager --version v1 --kind Issuer --defaulting --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io
$kb create webhook --group "cert-manager" --version v1 --kind Issuer --defaulting --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=io
# Webhook for Core type
$kb create webhook --group core --version v1 --kind Pod --defaulting
fi
Expand Down Expand Up @@ -76,9 +76,9 @@ function scaffold_test_project {
$kb create api --group foo --version v1 --kind Bar --controller=true --resource=true --make=false
$kb create api --group fiz --version v1 --kind Bar --controller=true --resource=true --make=false
# Controller for External types
$kb create api --group certmanager --version v1 --kind Certificate --controller=true --resource=false --make=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io
$kb create api --group "cert-manager" --version v1 --kind Certificate --controller=true --resource=false --make=false --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=io
# Webhook for External types
$kb create webhook --group certmanager --version v1 --kind Issuer --defaulting --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=cert-manager.io
$kb create webhook --group "cert-manager" --version v1 --kind Issuer --defaulting --external-api-path=github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1 --external-api-domain=io
# Webhook for Core type
$kb create webhook --group core --version v1 --kind Pod --programmatic-validation
fi
Expand Down
8 changes: 4 additions & 4 deletions testdata/project-v4-multigroup/PROJECT
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,15 +127,15 @@ resources:
path: sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/api/fiz/v1
version: v1
- controller: true
domain: cert-manager.io
domain: io
external: true
group: certmanager
group: cert-manager
kind: Certificate
path: github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1
version: v1
- domain: cert-manager.io
- domain: io
external: true
group: certmanager
group: cert-manager
kind: Issuer
path: github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1
version: v1
Expand Down
4 changes: 2 additions & 2 deletions testdata/project-v4-multigroup/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ import (
shipv1beta1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/api/ship/v1beta1"
shipv2alpha1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/api/ship/v2alpha1"
appscontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/apps"
certmanagercontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/certmanager"
certmanagercontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/cert-manager"
crewcontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/crew"
examplecomcontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/example.com"
fizcontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/fiz"
foocontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/foo"
foopolicycontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/foo.policy"
seacreaturescontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/sea-creatures"
shipcontroller "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/controller/ship"
webhookcertmanagerv1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/webhook/certmanager/v1"
webhookcertmanagerv1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/webhook/cert-manager/v1"
webhookcorev1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/webhook/core/v1"
webhookcrewv1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/webhook/crew/v1"
webhookexamplecomv1alpha1 "sigs.k8s.io/kubebuilder/testdata/project-v4-multigroup/internal/webhook/example.com/v1alpha1"
Expand Down
2 changes: 1 addition & 1 deletion ...s/cainjection_in_certmanager_issuers.yaml → .../cainjection_in_cert-manager_issuers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ kind: CustomResourceDefinition
metadata:
annotations:
cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
name: issuers.certmanager.cert-manager.io
name: issuers.cert-manager.io
2 changes: 1 addition & 1 deletion ...tches/webhook_in_certmanager_issuers.yaml → ...ches/webhook_in_cert-manager_issuers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: issuers.certmanager.cert-manager.io
name: issuers.cert-manager.io
spec:
conversion:
strategy: Webhook
Expand Down
6 changes: 3 additions & 3 deletions testdata/project-v4-multigroup/config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ rules:
- patch
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates
verbs:
Expand All @@ -58,13 +58,13 @@ rules:
- update
- watch
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/finalizers
verbs:
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/status
verbs:
Expand Down
4 changes: 2 additions & 2 deletions testdata/project-v4-multigroup/config/webhook/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ webhooks:
service:
name: webhook-service
namespace: system
path: /mutate-certmanager-cert-manager-io-v1-issuer
path: /mutate-cert-manager-io-v1-issuer
failurePolicy: Fail
name: missuer-v1.kb.io
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
apiVersions:
- v1
operations:
Expand Down
10 changes: 5 additions & 5 deletions testdata/project-v4-multigroup/dist/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,7 +1177,7 @@ rules:
- patch
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates
verbs:
Expand All @@ -1189,13 +1189,13 @@ rules:
- update
- watch
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/finalizers
verbs:
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/status
verbs:
Expand Down Expand Up @@ -1820,12 +1820,12 @@ webhooks:
service:
name: project-v4-multigroup-webhook-service
namespace: project-v4-multigroup-system
path: /mutate-certmanager-cert-manager-io-v1-issuer
path: /mutate-cert-manager-io-v1-issuer
failurePolicy: Fail
name: missuer-v1.kb.io
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
apiVersions:
- v1
operations:
Expand Down
8 changes: 4 additions & 4 deletions ...ler/certmanager/certificate_controller.go → ...er/cert-manager/certificate_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ type CertificateReconciler struct {
Scheme *runtime.Scheme
}

// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/finalizers,verbs=update
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/finalizers,verbs=update

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
Expand All @@ -57,6 +57,6 @@ func (r *CertificateReconciler) Reconcile(ctx context.Context, req ctrl.Request)
func (r *CertificateReconciler) SetupWithManager(mgr ctrl.Manager) error {
return ctrl.NewControllerManagedBy(mgr).
For(&certmanagerv1.Certificate{}).
Named("certmanager-certificate").
Named("cert-manager-certificate").
Complete(r)
}
0 ...ertmanager/certificate_controller_test.go → ...rt-manager/certificate_controller_test.go
View file
Open in desktop
File renamed without changes.
0 ...rnal/controller/certmanager/suite_test.go → ...nal/controller/cert-manager/suite_test.go
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion .../webhook/certmanager/v1/issuer_webhook.go → ...webhook/cert-manager/v1/issuer_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func SetupIssuerWebhookWithManager(mgr ctrl.Manager) error {

// TODO(user): EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN!

// +kubebuilder:webhook:path=/mutate-certmanager-cert-manager-io-v1-issuer,mutating=true,failurePolicy=fail,sideEffects=None,groups=certmanager.cert-manager.io,resources=issuers,verbs=create;update,versions=v1,name=missuer-v1.kb.io,admissionReviewVersions=v1
// +kubebuilder:webhook:path=/mutate-cert-manager-io-v1-issuer,mutating=true,failurePolicy=fail,sideEffects=None,groups=cert-manager.io,resources=issuers,verbs=create;update,versions=v1,name=missuer-v1.kb.io,admissionReviewVersions=v1

// IssuerCustomDefaulter struct is responsible for setting default values on the custom resource of the
// Kind Issuer when those are created or updated.
Expand Down
0 ...ook/certmanager/v1/issuer_webhook_test.go → ...ok/cert-manager/v1/issuer_webhook_test.go
View file
Open in desktop
File renamed without changes.
0 ...hook/certmanager/v1/webhook_suite_test.go → ...ook/cert-manager/v1/webhook_suite_test.go
View file
Open in desktop
File renamed without changes.
8 changes: 4 additions & 4 deletions testdata/project-v4/PROJECT
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ resources:
defaulting: true
webhookVersion: v1
- controller: true
domain: cert-manager.io
domain: io
external: true
group: certmanager
group: cert-manager
kind: Certificate
path: github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1
version: v1
- domain: cert-manager.io
- domain: io
external: true
group: certmanager
group: cert-manager
kind: Issuer
path: github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1
version: v1
Expand Down
2 changes: 1 addition & 1 deletion testdata/project-v4/config/crd/patches/cainjection_in_issuers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,4 @@ kind: CustomResourceDefinition
metadata:
annotations:
cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
name: issuers.certmanager.cert-manager.io
name: issuers.cert-manager.io
2 changes: 1 addition & 1 deletion testdata/project-v4/config/crd/patches/webhook_in_issuers.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: issuers.certmanager.cert-manager.io
name: issuers.cert-manager.io
spec:
conversion:
strategy: Webhook
Expand Down
6 changes: 3 additions & 3 deletions testdata/project-v4/config/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ metadata:
name: manager-role
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates
verbs:
Expand All @@ -17,13 +17,13 @@ rules:
- update
- watch
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/finalizers
verbs:
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/status
verbs:
Expand Down
4 changes: 2 additions & 2 deletions testdata/project-v4/config/webhook/manifests.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ webhooks:
service:
name: webhook-service
namespace: system
path: /mutate-certmanager-cert-manager-io-v1-issuer
path: /mutate-cert-manager-io-v1-issuer
failurePolicy: Fail
name: missuer-v1.kb.io
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
apiVersions:
- v1
operations:
Expand Down
10 changes: 5 additions & 5 deletions testdata/project-v4/dist/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,7 @@ metadata:
name: project-v4-manager-role
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates
verbs:
Expand All @@ -417,13 +417,13 @@ rules:
- update
- watch
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/finalizers
verbs:
- update
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
resources:
- certificates/status
verbs:
Expand Down Expand Up @@ -694,12 +694,12 @@ webhooks:
service:
name: project-v4-webhook-service
namespace: project-v4-system
path: /mutate-certmanager-cert-manager-io-v1-issuer
path: /mutate-cert-manager-io-v1-issuer
failurePolicy: Fail
name: missuer-v1.kb.io
rules:
- apiGroups:
- certmanager.cert-manager.io
- cert-manager.io
apiVersions:
- v1
operations:
Expand Down
6 changes: 3 additions & 3 deletions testdata/project-v4/internal/controller/certificate_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,9 @@ type CertificateReconciler struct {
Scheme *runtime.Scheme
}

// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=certmanager.cert-manager.io,resources=certificates/finalizers,verbs=update
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=cert-manager.io,resources=certificates/finalizers,verbs=update

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the cluster closer to the desired state.
Expand Down
Loading
Loading