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: make secretKeyRef a required field in plugins' configFrom #5103

Merged
merged 3 commits into from
Nov 14, 2023

Conversation

randmonkey
Copy link
Contributor

@randmonkey randmonkey commented Nov 7, 2023

What this PR does / why we need it:

remove the incorrect usage of annotations // +kubebuilder:validation:Optional on type defintions and reserve one // +kubebuilder:validation:Optional in packages v1 and v1beta1 (in file docs.go).

Which issue this PR fixes:

fixes #5061

Special notes for your reviewer:

The // +kubebuilder:validation:Optional comments above types are first imported in this PR: #1757. I do not know the purpose here, but kept the generated CRDs identical to previous versions. @rainest Do you have more context of that PR?

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@randmonkey randmonkey requested a review from a team as a code owner November 7, 2023 09:33
@randmonkey randmonkey self-assigned this Nov 7, 2023
@randmonkey randmonkey added this to the KIC v3.1.0 milestone Nov 7, 2023
@randmonkey randmonkey marked this pull request as draft November 7, 2023 09:51
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b4718ab) 77.8% compared to head (350d71b) 77.7%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #5103     +/-   ##
=======================================
- Coverage   77.8%   77.7%   -0.1%     
=======================================
  Files        168     168             
  Lines      18907   18906      -1     
=======================================
- Hits       14714   14697     -17     
- Misses      3360    3373     +13     
- Partials     833     836      +3     
Files Coverage Δ
internal/admission/validator.go 73.2% <ø> (+0.7%) ⬆️
internal/dataplane/kongstate/kongstate.go 86.7% <100.0%> (+<0.1%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@randmonkey randmonkey force-pushed the fix/incorrect_validation_optional_note branch from 57bd585 to 3b9ab51 Compare November 7, 2023 10:00
@randmonkey randmonkey marked this pull request as ready for review November 7, 2023 10:12
@pmalek
Copy link
Member

pmalek commented Nov 7, 2023

How about removing this code marker altogether and make the fields not optional by default. This way, if a field is to be optional we can always add it.

We can make this change:

diff --git a/pkg/apis/configuration/v1/configsource.go b/pkg/apis/configuration/v1/configsource.go
index 2ba4ea73b..149e0bf58 100644
--- a/pkg/apis/configuration/v1/configsource.go
+++ b/pkg/apis/configuration/v1/configsource.go
@@ -19,10 +19,10 @@ type NamespacedConfigSource struct {
 type SecretValueFromSource struct {
 	// The secret containing the key.
 	// +kubebuilder:validation:Required
-	Secret string `json:"name,omitempty"`
+	Secret string `json:"name"`
 	// The key containing the value.
 	// +kubebuilder:validation:Required
-	Key string `json:"key,omitempty"`
+	Key string `json:"key"`
 }
 
 // NamespacedSecretValueFromSource represents the source of a secret value specifying the secret namespace.
@@ -30,11 +30,11 @@ type SecretValueFromSource struct {
 type NamespacedSecretValueFromSource struct {
 	// The namespace containing the secret.
 	// +kubebuilder:validation:Required
-	Namespace string `json:"namespace,omitempty"`
+	Namespace string `json:"namespace"`
 	// The secret containing the key.
 	// +kubebuilder:validation:Required
-	Secret string `json:"name,omitempty"`
+	Secret string `json:"name"`
 	// The key containing the value.
 	// +kubebuilder:validation:Required
-	Key string `json:"key,omitempty"`
+	Key string `json:"key"`
 }
diff --git a/pkg/apis/configuration/v1/kongclusterplugin_types.go b/pkg/apis/configuration/v1/kongclusterplugin_types.go
index 94e1058ae..2a1d60c95 100644
--- a/pkg/apis/configuration/v1/kongclusterplugin_types.go
+++ b/pkg/apis/configuration/v1/kongclusterplugin_types.go
@@ -29,7 +29,6 @@ import (
 // +kubebuilder:resource:scope=Cluster,shortName=kcp,categories=kong-ingress-controller
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
-// +kubebuilder:validation:Optional
 // +kubebuilder:printcolumn:name="Plugin-Type",type=string,JSONPath=`.plugin`,description="Name of the plugin"
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`,description="Age"
 // +kubebuilder:printcolumn:name="Disabled",type=boolean,JSONPath=`.disabled`,description="Indicates if the plugin is disabled",priority=1
@@ -64,7 +63,7 @@ type KongClusterPlugin struct {
 
 	// PluginName is the name of the plugin to which to apply the config.
 	// +kubebuilder:validation:Required
-	PluginName string `json:"plugin,omitempty"`
+	PluginName string `json:"plugin"`
 
 	// RunOn configures the plugin to run on the first or the second or both
 	// nodes in case of a service mesh deployment.
diff --git a/pkg/apis/configuration/v1/kongconsumer_types.go b/pkg/apis/configuration/v1/kongconsumer_types.go
index eb36e3f58..8c04e6594 100644
--- a/pkg/apis/configuration/v1/kongconsumer_types.go
+++ b/pkg/apis/configuration/v1/kongconsumer_types.go
@@ -26,7 +26,6 @@ import (
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:resource:shortName=kc,categories=kong-ingress-controller
-// +kubebuilder:validation:Optional
 // +kubebuilder:printcolumn:name="Username",type=string,JSONPath=`.username`,description="Username of a Kong Consumer"
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`,description="Age"
 // +kubebuilder:printcolumn:name="Programmed",type=string,JSONPath=`.status.conditions[?(@.type=="Programmed")].status`
diff --git a/pkg/apis/configuration/v1/kongingress_types.go b/pkg/apis/configuration/v1/kongingress_types.go
index 3e171b9fc..26b7121cc 100644
--- a/pkg/apis/configuration/v1/kongingress_types.go
+++ b/pkg/apis/configuration/v1/kongingress_types.go
@@ -27,7 +27,6 @@ import (
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:resource:shortName=ki,categories=kong-ingress-controller
-// +kubebuilder:validation:Optional
 // +kubebuilder:validation:XValidation:rule="!has(self.proxy)", message="'proxy' field is no longer supported, use Service's annotations instead"
 // +kubebuilder:validation:XValidation:rule="!has(self.route)", message="'route' field is no longer supported, use Ingress' annotations instead"
 
diff --git a/pkg/apis/configuration/v1/kongplugin_types.go b/pkg/apis/configuration/v1/kongplugin_types.go
index 886f8480e..10eee7bdd 100644
--- a/pkg/apis/configuration/v1/kongplugin_types.go
+++ b/pkg/apis/configuration/v1/kongplugin_types.go
@@ -28,7 +28,6 @@ import (
 // +kubebuilder:subresource:status
 // +kubebuilder:storageversion
 // +kubebuilder:resource:shortName=kp,categories=kong-ingress-controller
-// +kubebuilder:validation:Optional
 // +kubebuilder:printcolumn:name="Plugin-Type",type=string,JSONPath=`.plugin`,description="Name of the plugin"
 // +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`,description="Age"
 // +kubebuilder:printcolumn:name="Disabled",type=boolean,JSONPath=`.disabled`,description="Indicates if the plugin is disabled",priority=1
@@ -64,7 +63,7 @@ type KongPlugin struct {
 
 	// PluginName is the name of the plugin to which to apply the config.
 	// +kubebuilder:validation:Required
-	PluginName string `json:"plugin,omitempty"`
+	PluginName string `json:"plugin"`
 
 	// RunOn configures the plugin to run on the first or the second or both
 	// nodes in case of a service mesh deployment.

to not introduce any user facing changes but flip the behavior for developers.

@pmalek
Copy link
Member

pmalek commented Nov 7, 2023

I've created #5105 to review all CRD level validations and admission webhook code which would be converted into the former.

With change from this PR making a field required is impossible (see the example in #5105). I'd suggest to make the change from #5103 (comment), close #5061 and then solve #5105.

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

As far as the original PR I likely just misunderstood how the annotation was supposed to be used. AFAICT from re-reading it, the initial versions of our Kubebuilder-based CRDs were lacking validation present in the original CRDs, and that PR added them, e.g. the original required fields were not originally present when we were migrating to Kubebuilder. The main goal of that PR was to add the required field markers, and I can't recall what, if any purpose there was to the +kubebuilder:validation:Optional tags.

The Kubebuilder doc does indicate that the package-level annotation indicates that fields defined by it are optional by default, and that only the fields with the validation:Required tag are required, which isn't wrong--the fields without a required tag like Config in a Plugin type are indeed optional--but it sounds like that's unnecessary if the JSON serialization hits mark it omitempty, and the package-level default introduces other problems.

@pmalek I'm not quite sure I follow the comment. If I read it correctly, we shouldn't make the change proposed here, or at least not until changing the omitemptys on the relevant fields? The comment indicates that the change here would actually prevent us from making fields we want to require required, but I'm somewhat confused since the changes here don't actually result in changes to the generated resources (and CI does not indicate they're out of date).

@pmalek
Copy link
Member

pmalek commented Nov 7, 2023

I just tried re-introducing my changes I mentioned here and on last sync and it seems that indeed, it's possible to just slap the +kubebuilder:validation:Required on a field and then it will work regardless of the // +kubebuilder:validation:Optional being set on a package 🤦 🤷

diff --git a/config/crd/bases/configuration.konghq.com_kongconsumers.yaml b/config/crd/bases/configuration.konghq.com_kongconsumers.yaml
index 4e407f191..d1898d2ef 100644
--- a/config/crd/bases/configuration.konghq.com_kongconsumers.yaml
+++ b/config/crd/bases/configuration.konghq.com_kongconsumers.yaml
@@ -151,6 +151,8 @@ spec:
           username:
             description: Username is a Kong cluster-unique username of the consumer.
             type: string
+        required:
+        - username
         type: object
     served: true
     storage: true
diff --git a/pkg/apis/configuration/v1/kongconsumer_types.go b/pkg/apis/configuration/v1/kongconsumer_types.go
index eb36e3f58..47cd8f3ae 100644
--- a/pkg/apis/configuration/v1/kongconsumer_types.go
+++ b/pkg/apis/configuration/v1/kongconsumer_types.go
@@ -37,6 +37,7 @@ type KongConsumer struct {
 	metav1.ObjectMeta `json:"metadata,omitempty"`
 
 	// Username is a Kong cluster-unique username of the consumer.
+	// +kubebuilder:validation:Required
 	Username string `json:"username,omitempty"`
 
 	// CustomID is a Kong cluster-unique existing ID for the consumer - useful for mapping

Anyway, the change in this PR doesn't introcuce a user facing change but keeps the "optional" by default. I'd suggest us go the other way. Just add the optional where is indeed needed and remove it from the package level.

@randmonkey
Copy link
Contributor Author

If you added omitempty in JSON tag of a field, the // +kubebuilder:validation:Required comment will not take effect, unless you have a // +kubebuilder:validation:Optional comment on the package level: kubernetes-sigs/controller-tools#599.
So I removed the omitempty notation in JSON tags of required fields. After the change, these fields of CRDs become required:
KongConsumer.Username
KongPlugin.ConfigFrom.SecretKeyRef (if ConfigFrom is non-empty)
KongClusterPlugin.ConfigFrom.SecretKeyRef (if ConfigFrom is non-empty)

@randmonkey randmonkey force-pushed the fix/incorrect_validation_optional_note branch from 3b9ab51 to 5972452 Compare November 8, 2023 08:05
@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 8, 2023
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

Apart leaving in // +kubebuilder:validation:Required code markers I'd check the secretKeyRef change. I'm not sure if making the field required wouldn't cause the enclosing CRD (KongClusterPlugin and KongPlugin) fail validation if config is used (instead of configFrom which uses secretKeyRef 🤔

I'd also change the PR title as that's not only about the validation tags now but also about changing the CRD validations.

@randmonkey randmonkey force-pushed the fix/incorrect_validation_optional_note branch from 6a045a5 to 350d71b Compare November 10, 2023 03:34
@pmalek pmalek changed the title fix: remove invalid validation:Optional annotations fix: make secretKeyRef a required field in plugins' configFrom Nov 14, 2023
Copy link
Member

@pmalek pmalek left a comment

Choose a reason for hiding this comment

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

I've changed the title of this PR to more reflect what's being changed. Feel free to amend it if you still feel we can improve it.

Leaving this for @randmonkey to decide if he'd like to merge it with the updated title.

Copy link
Contributor

@czeslavo czeslavo left a comment

Choose a reason for hiding this comment

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

I believe that by changing secretKeyRef to a required field we also do not close doors to a later potential extension of ConfigFrom allowing the use of some different source of config, e.g. configMapKeyRef. If we'd like to do so in the future, we may then drop the required condition and use CEL to require one of *KeyRef to be present.

@rainest rainest merged commit 21abc80 into main Nov 14, 2023
35 checks passed
@rainest rainest deleted the fix/incorrect_validation_optional_note branch November 14, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

+kubebuilder:validation:Optional is used on types which is not supported by kubebuilder
4 participants