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

Add defaults and validation for ServiceDefaults #320

Merged
merged 6 commits into from
Sep 15, 2020

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Sep 3, 2020

Changes proposed in this PR:

Add defaulting and validation for ServiceDefaults

Checklist:

  • Tests added

How I tested the PR:
• Used the image ashwinvenkatesh/consul-k8s:validation to deploy consul via helm with the controller enabled.
• Tried creating service default resource with
• Invalid mesh gateway mode
• Invalid expose path protocol (not in "http" and "http2") and path value (not prefixed by "/")
This led to webhook errors which indicated the invalid fields
• Combinations of various invalid values for the above fields
This led to webhook errors that listed every single invalid field

How I expect people to test the PR:

• You can retry the above scenarios with multiple invalid values to verify every invalid value is mentioned in the returned error from the webhook.

@thisisnotashwin thisisnotashwin force-pushed the servce-defaults-validation branch from 0f1acdf to 9ea49fa Compare September 3, 2020 17:32
@thisisnotashwin thisisnotashwin requested review from lkysow, a team and ishustava and removed request for a team September 8, 2020 20:40
@thisisnotashwin thisisnotashwin marked this pull request as ready for review September 8, 2020 20:40
@thisisnotashwin thisisnotashwin force-pushed the servce-defaults-validation branch from f2f7b1d to 03ada6a Compare September 8, 2020 20:45
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

The validating logic looks awesome!

I wasn't super sure if we need defaulting though. It seems like a duplication of logic that already exists in Consul, and so I'm not sure what is the value of having it in the controller as well. It also couples us to consul versions more tightly - if consul changes defaults, you'd need to update to the version of the controller/consul-k8s that has that default.

api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks good!

@ishustava ishustava added area/connect Related to Connect service mesh, e.g. injection type/enhancement New feature or request theme/crds labels Sep 10, 2020
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types_test.go Outdated Show resolved Hide resolved
api/v1alpha1/servicedefaults_types_test.go Outdated Show resolved Hide resolved
api/v1alpha1/types.go Outdated Show resolved Hide resolved
@thisisnotashwin thisisnotashwin force-pushed the servce-defaults-validation branch from d0989eb to d0a466c Compare September 14, 2020 22:28
api/v1alpha1/servicedefaults_types.go Outdated Show resolved Hide resolved
api/v1alpha1/types.go Outdated Show resolved Hide resolved
api/v1alpha1/types.go Outdated Show resolved Hide resolved
@thisisnotashwin thisisnotashwin force-pushed the servce-defaults-validation branch from 1e3740e to bc3686a Compare September 15, 2020 16:57
@thisisnotashwin thisisnotashwin merged commit 9cf39f6 into crd-controller-base Sep 15, 2020
@thisisnotashwin thisisnotashwin deleted the servce-defaults-validation branch September 15, 2020 17:54
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connect Related to Connect service mesh, e.g. injection theme/crds type/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants