-
Notifications
You must be signed in to change notification settings - Fork 17
Validation #67
base: master
Are you sure you want to change the base?
Validation #67
Conversation
92ae698
to
52ee0d6
Compare
actual fix to bash config line fix multiple id tests fix more tests fix aws test for webhook add condition for failing admission fix success unit test add create fail test get string for error add periods to tests fix tests with invalid endpoint add err check add awscred to test add require error on tests revert sourceurl error
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.
Minor changes. Can you also add documentation specifying what things should go in the webhook and what should go in the handler, that would be helpful.
.gitignore
Outdated
pkg/client | ||
vendor | ||
vendor.orig | ||
.DS_Store |
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 don't think we should include these...They are specific to your setup and so shouldn't be included in the git too.
Dockerfile
Outdated
COPY ./vck / | ||
CMD /vck | ||
COPY ./validation-webhook / | ||
CMD validation-webhook |
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.
CMD is not necessary because it's included in the deployment.
1. [OpenAPI v3 schema] - This is the easiest method but has little flexibility. By modifying `kube-volume-controller-crd.yml`, required fields for all sourceTypes and required types for fields can be set. | ||
|
||
|
||
2. [Validation Webhook] - This is a bit more complicated but has significantly more flexibility. By modifying `validation-webhook.go`, any validation rule can be specified in the `validateVolumeManager` and `validate<Source Type>` functions. |
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.
Maybe give an example? Like specify what is being done for validating the s3 source type.
@@ -0,0 +1,15 @@ | |||
apiVersion: v1 |
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.
This and other webhook yamls should also be under the {{- if .Values.webhook.install -}}
condition.
|
||
import ( | ||
"fmt" | ||
"testing" |
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 don't think this is being used.
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 think it is actually
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.
yes, it is.
}, | ||
} | ||
} | ||
log.Println("All Volume Manager(s) look good!") |
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.
glog here too.
errs = append(errs, "labels cannot be empty.") | ||
} | ||
|
||
if _, ok := vc.Options["server"]; !ok { |
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.
Can you also validate the value types?
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.
Isn't that done in the OpenAPI schema?
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.
no, like for example, the server should be a url, the timeout should be in the right format, etc.
errs = append(errs, "path has to be set in options.") | ||
} | ||
|
||
if vc.AccessMode == "" { |
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.
Can you check for specific AccessModes too?
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 believe this is also checked in the OpenAPI schema
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.
Yes, it's checking for general types, what I meant here is that each source type can only accept a subset of those access modes.
errs = append(errs, "sourceURL has to be a valid URL.") | ||
} | ||
|
||
if sourceURL, ok := vc.Options["endpointURL"]; ok { |
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 guess the other stuff can also be included here, like the timeoutForDataDownload, dataPath?
@@ -0,0 +1,29 @@ | |||
apiVersion: vck.intelai.org/v1alpha1 |
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.
Where is this file used?
No description provided.