-
Notifications
You must be signed in to change notification settings - Fork 202
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
Add webhookconfiguration options to tektonConfig additional options #2129
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
Copyright 2024 The Tekton Authors | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
||
package v1alpha1 | ||
|
||
import ( | ||
"fmt" | ||
|
||
"k8s.io/apimachinery/pkg/util/sets" | ||
"knative.dev/pkg/apis" | ||
) | ||
|
||
var ( | ||
validatePipelineWebhookConfigurationFailurePolicy = sets.NewString("Ignore", "Fail") | ||
validatePipelineWebhookConfigurationSideEffects = sets.NewString("NoneOnDryRun", "None", "Unknown", "Some") | ||
) | ||
|
||
func (w *WebhookConfigurationOptions) validate(path string) (errs *apis.FieldError) { | ||
if w.FailurePolicy != nil && !validatePipelineWebhookConfigurationFailurePolicy.Has(string(*w.FailurePolicy)) { | ||
errs = errs.Also(apis.ErrInvalidValue(*w.FailurePolicy, fmt.Sprintf("%s.webhookconfigurationoptions.failurePolicy", path))) | ||
} | ||
if w.SideEffects != nil && !validatePipelineWebhookConfigurationSideEffects.Has(string(*w.SideEffects)) { | ||
errs = errs.Also(apis.ErrInvalidValue(*w.SideEffects, fmt.Sprintf("%s.webhookconfigurationoptions.sideEffects", path))) | ||
} | ||
return errs | ||
} | ||
|
||
func (op *AdditionalOptions) validate(path string) (errs *apis.FieldError) { | ||
if op.WebhookConfigurationOptions != nil { | ||
for _, webhookConfig := range op.WebhookConfigurationOptions { | ||
return webhookConfig.validate(path) | ||
} | ||
} | ||
return errs | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -115,6 +115,12 @@ func (tc *TektonConfig) Validate(ctx context.Context) (errs *apis.FieldError) { | |
|
||
errs = errs.Also(tc.Spec.Pipeline.PipelineProperties.validate("spec.pipeline")) | ||
|
||
errs = errs.Also(tc.Spec.Pipeline.Options.validate("spec.pipeline.options")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should be doing here the config options one, and for others we should in respective valdations. I think we should validate this for all components There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
errs = errs.Also(tc.Spec.Hub.Options.validate("spec.hub.options")) | ||
errs = errs.Also(tc.Spec.Dashboard.Options.validate("spec.dashboard.options")) | ||
errs = errs.Also(tc.Spec.Chain.Options.validate("spec.chain.options")) | ||
errs = errs.Also(tc.Spec.Trigger.Options.validate("spec.trigger.options")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dont we need to have one for config like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @piyush-garg TektonConfigSpec struct doestnt have Options There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ohk, thanks |
||
|
||
return errs.Also(tc.Spec.Trigger.TriggersProperties.validate("spec.trigger")) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ limitations under the License. | |
package v1alpha1 | ||
|
||
import ( | ||
admissionregistrationv1 "k8s.io/api/admissionregistration/v1" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
duckv1 "knative.dev/pkg/apis/duck/v1" | ||
) | ||
|
@@ -143,6 +144,13 @@ type OptionalPipelineProperties struct { | |
DefaultResolverType string `json:"default-resolver-type,omitempty"` | ||
} | ||
|
||
// WebhookOptions defines options for webhooks | ||
type WebhookConfigurationOptions struct { | ||
FailurePolicy *admissionregistrationv1.FailurePolicyType `json:"failurePolicy,omitempty"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think we should some validations for them like
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"` | ||
SideEffects *admissionregistrationv1.SideEffectClass `json:"sideEffects,omitempty"` | ||
} | ||
|
||
// PipelineMetricsProperties defines the fields which are configurable for | ||
// metrics | ||
type PipelineMetricsProperties struct { | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
--- | ||
apiVersion: admissionregistration.k8s.io/v1 | ||
kind: ValidatingWebhookConfiguration | ||
metadata: | ||
name: validation.webhook.pipeline.tekton.dev | ||
labels: | ||
app.kubernetes.io/component: webhook | ||
app.kubernetes.io/instance: default | ||
app.kubernetes.io/part-of: tekton-pipelines | ||
pipeline.tekton.dev/release: "v0.58.0" | ||
webhooks: | ||
- admissionReviewVersions: ["v1"] | ||
clientConfig: | ||
service: | ||
name: tekton-pipelines-webhook | ||
namespace: tekton-pipelines | ||
failurePolicy: Fail | ||
sideEffects: None | ||
name: validation.webhook.pipeline.tekton.dev | ||
|
||
--- | ||
apiVersion: admissionregistration.k8s.io/v1 | ||
kind: MutatingWebhookConfiguration | ||
metadata: | ||
name: webhook.pipeline.tekton.dev | ||
labels: | ||
app.kubernetes.io/component: webhook | ||
app.kubernetes.io/instance: default | ||
app.kubernetes.io/part-of: tekton-pipelines | ||
pipeline.tekton.dev/release: "v0.58.0" | ||
webhooks: | ||
- admissionReviewVersions: ["v1"] | ||
clientConfig: | ||
service: | ||
name: tekton-pipelines-webhook | ||
namespace: tekton-pipelines | ||
failurePolicy: Fail | ||
sideEffects: None | ||
name: webhook.pipeline.tekton.dev |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
--- | ||
apiVersion: admissionregistration.k8s.io/v1 | ||
kind: ValidatingWebhookConfiguration | ||
metadata: | ||
creationTimestamp: null | ||
name: validation.webhook.pipeline.tekton.dev | ||
labels: | ||
app.kubernetes.io/component: webhook | ||
app.kubernetes.io/instance: default | ||
app.kubernetes.io/part-of: tekton-pipelines | ||
pipeline.tekton.dev/release: "v0.58.0" | ||
webhooks: | ||
- admissionReviewVersions: ["v1"] | ||
clientConfig: | ||
service: | ||
name: tekton-pipelines-webhook | ||
namespace: tekton-pipelines | ||
failurePolicy: Ignore | ||
timeoutSeconds: 10 | ||
sideEffects: Unknown | ||
name: validation.webhook.pipeline.tekton.dev | ||
|
||
--- | ||
apiVersion: admissionregistration.k8s.io/v1 | ||
kind: MutatingWebhookConfiguration | ||
metadata: | ||
creationTimestamp: null | ||
name: webhook.pipeline.tekton.dev | ||
labels: | ||
app.kubernetes.io/component: webhook | ||
app.kubernetes.io/instance: default | ||
app.kubernetes.io/part-of: tekton-pipelines | ||
pipeline.tekton.dev/release: "v0.58.0" | ||
webhooks: | ||
- admissionReviewVersions: ["v1"] | ||
clientConfig: | ||
service: | ||
name: tekton-pipelines-webhook | ||
namespace: tekton-pipelines | ||
failurePolicy: Fail | ||
timeoutSeconds: 10 | ||
sideEffects: None | ||
name: webhook.pipeline.tekton.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see webhookConfigurationOptions is supported for both Validation and Mutation can we update this example to show that as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does not make a difference, at the end it is just name