{"action":"created","issue":{"url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/147","repository_url":"https://api.github.com/repos/kubernetes-sigs/cluster-api","labels_url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/147/labels{/name}","comments_url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/147/comments","events_url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/147/events","html_url":"https://github.com/kubernetes-sigs/cluster-api/issues/147","id":321740224,"node_id":"MDU6SXNzdWUzMjE3NDAyMjQ=","number":147,"title":"Allow provider-specific validation of ProviderConfig in a Cluster resource","user":{"login":"csrwng","id":5834976,"node_id":"MDQ6VXNlcjU4MzQ5NzY=","avatar_url":"https://avatars1.githubusercontent.com/u/5834976?v=4","gravatar_id":"","url":"https://api.github.com/users/csrwng","html_url":"https://github.com/csrwng","followers_url":"https://api.github.com/users/csrwng/followers","following_url":"https://api.github.com/users/csrwng/following{/other_user}","gists_url":"https://api.github.com/users/csrwng/gists{/gist_id}","starred_url":"https://api.github.com/users/csrwng/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/csrwng/subscriptions","organizations_url":"https://api.github.com/users/csrwng/orgs","repos_url":"https://api.github.com/users/csrwng/repos","events_url":"https://api.github.com/users/csrwng/events{/privacy}","received_events_url":"https://api.github.com/users/csrwng/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":4,"created_at":"2018-05-09T21:56:15Z","updated_at":"2018-10-16T18:35:51Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"### The Problem:\r\nCurrently the cluster API server validates only the known fields of a Cluster resource. ProviderConfig is taken at face value and left for the Machine controller to interpret. Validation is needed to ensure that the data passed in ProviderConfig is well-formed. There are 2 use cases for this:\r\n- **ProviderConfig only references an external resource** Although we don't yet have fields for it, the Cluster could simply reference an external resource as its provider config. In this case, whatever API server owns the resource can handle that resource's validation.\r\n- **ProviderConfig is embedded in the resource** This is what's allowed today. Something would have to intercept the cluster resource and validate the embedded config, or the cluster API apiserver would have to call out to something to validate it.\r\n\r\nGiven that an external ProviderConfig can be validated by an external server/controller, the following will only deal with the case in which a ProviderConfig is embedded in a Cluster resource.\r\n\r\n### Possible Solutions:\r\n- **_A separate controller validates the ProviderConfig_** - A provider-specific controller could watch Cluster resources as they're created and set their status appropriately if the embedded ProviderConfig is found to be invalid.\r\n\r\n  **Pros**: \r\n  - Nothing to be done. Whoever implements a cloud provider can write their own controller that \r\n performs validation.\r\n\r\n  **Cons**:\r\n  - Validation is done late and will not give the client immediate feedback on a badly formed ProviderConfig\r\n\r\n\r\n- **_Cluster API apiserver validates the ProviderConfig itself_** - Assuming that the ProviderConfig is backed by a CRD, the API server could look up the CRD definition, and if it includes a validation definition, reuse the CRD validation code to validate the resource. CRD validation is now a [gated feature](https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/#create-a-customresourcedefinition) in Kubernetes 1.10. \r\n\r\n  **Pros**: \r\n  - Validation done at the same time other resource validation happens.\r\n\r\n  **Cons**:\r\n  - Validation is limited to what CRD validation allows\r\n  - There is likely a good bit of work integrating code from CRD validation into the cluster API code.\r\n  - Assumes the ProviderConfig is a CRD. If the resource type is backed by another API server,  this type of validation is still not supported.\r\n\r\n\r\n- **_Cluster API apiserver delegates to dynamic admission control to validate_** - [Dynamic admission webhooks](https://kubernetes.io/docs/admin/extensible-admission-controllers/) allow dynamically registering external HTTP endpoints that will be called with an API resource on admission and give them a chance to validate the resource before it makes it to the API server's regular validation logic. This requires that the apiserver be configured to include the ValidatingAdmissionWebhook admission control via command line argument. \r\n\r\n  **Pros**: \r\n  - No additional code required. The deployment of the apiserver would need its command arguments modified to include the additional admission control.\r\n  - Validation done on admission, so the user would get immediate feedback of resources that fail validation.\r\n  - Supports all kinds of embedded data. Not required to be backed by a CRD or API server.\r\n\r\n  **Cons**:\r\n  - A cloud provider implementation that wants to have its own validation would need to provide its own webhook and register it with the clusterapi server. A starter repo for creating your own webhook provider can be found here: https://github.com/openshift/generic-admission-server\r\n\r\n\r\nMany thanks to @deads2k who provided a good bit of information on this.\r\n"},"comment":{"url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/comments/430349301","html_url":"https://github.com/kubernetes-sigs/cluster-api/issues/147#issuecomment-430349301","issue_url":"https://api.github.com/repos/kubernetes-sigs/cluster-api/issues/147","id":430349301,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMDM0OTMwMQ==","user":{"login":"alvaroaleman","id":6496100,"node_id":"MDQ6VXNlcjY0OTYxMDA=","avatar_url":"https://avatars2.githubusercontent.com/u/6496100?v=4","gravatar_id":"","url":"https://api.github.com/users/alvaroaleman","html_url":"https://github.com/alvaroaleman","followers_url":"https://api.github.com/users/alvaroaleman/followers","following_url":"https://api.github.com/users/alvaroaleman/following{/other_user}","gists_url":"https://api.github.com/users/alvaroaleman/gists{/gist_id}","starred_url":"https://api.github.com/users/alvaroaleman/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alvaroaleman/subscriptions","organizations_url":"https://api.github.com/users/alvaroaleman/orgs","repos_url":"https://api.github.com/users/alvaroaleman/repos","events_url":"https://api.github.com/users/alvaroaleman/events{/privacy}","received_events_url":"https://api.github.com/users/alvaroaleman/received_events","type":"User","site_admin":false},"created_at":"2018-10-16T18:35:51Z","updated_at":"2018-10-16T18:35:51Z","author_association":"MEMBER","body":"I started to toy around a little with the `Webhook` functionality of Kubebuilder by [trying to let it do Defaulting + Validation for `MachineDeployments`.](https://github.com/alvaroaleman/cluster-api/compare/master...alvaroaleman:webhook-generated)\r\n\r\nWhile I got it ultimately working, there are some issues:\r\n\r\n* This functionality is not part of a released kubebuilder version yet, one must build it from master\r\n* The cert bootstrapping is not really usable yet, Kubebuilder will generate a secret with cert+key, but it fails to start up without that cert+key being already present. I worked around this by temporarily disabling the secret volume mount and doing a `touch` on the respective files so it generates the secret, then re-deploying it with the mount enabled again\r\n* There some to be some issues with the generated patches, on my first attempt I got an `Internal error occurred: jsonpatch add operation does not apply: doc is missing path: /spec/strategy/type`. Adding the `spec.strategy.type` field with an empty string value fixed that and then the rest of the defaulting worked properly\r\n\r\nCC @Lion-Wei "}}
{"actor":{"display_login":"alvaroaleman"}}