✨ Set default value for corev1.Protocol type if not already specified

[munnerz]

Mitigation to help resolve #444 whilst waiting for kubernetes/enhancements#1928 to merge.

This patch will cause resources that embed e.g. ServiceSpec or PodSpec to have the default field set for the protocol field:

                          ports:
                            items:
                              properties:
                                appProtocol:
                                  type: string
                                name:
                                  type: string
                                nodePort:
                                  format: int32
                                  type: integer
                                port:
                                  format: int32
                                  type: integer
                                protocol:
                                  default: TCP
                                  type: string
                                targetPort:
                                  anyOf:
                                  - type: integer
                                  - type: string
                                  x-kubernetes-int-or-string: true
                              required:
                              - port
                              type: object
                            type: array
                            x-kubernetes-list-map-keys:
                            - port
                            - protocol
                            x-kubernetes-list-type: map

This will workaround the issue described in #444, and can be easily extended to new fields/types as they are discovered.

We should probably also consider fixing #445 as well, as this may cause some users that are not specifying // +kubebuilder:default annotations in their resources to begin specifying a default field (if they utilise this Protocol type), which will cause kubectl client side validation errors (which will require users to add --validate=false to their kubectl create usages)

/cc @pwittrock @tamalsaha

[k8s-ci-robot]

@munnerz: GitHub didn't allow me to request PR reviews from the following users: tamalsaha.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Mitigation to help resolve #444 whilst waiting for kubernetes/enhancements#1928 to merge.

This patch will cause resources that embed e.g. ServiceSpec or PodSpec to have the default field set for the protocol field:

                         ports:
                           items:
                             properties:
                               appProtocol:
                                 type: string
                               name:
                                 type: string
                               nodePort:
                                 format: int32
                                 type: integer
                               port:
                                 format: int32
                                 type: integer
                               protocol:
                                 default: TCP
                                 type: string
                               targetPort:
                                 anyOf:
                                 - type: integer
                                 - type: string
                                 x-kubernetes-int-or-string: true
                             required:
                             - port
                             type: object
                           type: array
                           x-kubernetes-list-map-keys:
                           - port
                           - protocol
                           x-kubernetes-list-type: map

This will workaround the issue described in #444, and can be easily extended to new fields/types as they are discovered.

We should probably also consider fixing #445 as well, as this may cause some users that are not specifying // +kubebuilder:default annotations in their resources to begin specifying a default field (if they utilise this Protocol type), which will cause kubectl client side validation errors (which will require users to add --validate=false to their kubectl create usages)

/cc @pwittrock @tamalsaha

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pwittrock]

This is great. Is this possible to unit test for?

[pwittrock]

Any idea why the tests are failing?

[munnerz]

I've updated the testdata (the CronJob resource used embeds a PodSpec, so this change causes the default field to be correctly generated).

In terms of testing specifically this change, we don't currently have any unit test for gen.go or known_types.go. All testing functionality is encompassed through the existing testdata directory with the single CronJob CRD (and is being exercised due to the embedded PodSpec)

The only behaviour that is not tested is ensuring that if // +kubebuilder:default is explicitly specified on this field (or any other marker that gets added once the KEP merges), that it takes precedence over this built-in. The only way we can test that is if the annotation/marker actually does exist on the upstream type, as k8s.io/api/core/v1 is hardcoded into the known_types file.

The only way I can think of to work around this is to have the integration test framework inject a 'copy' of this package/entry into KnownTypes during tests only, which could contain a type named Protocol and thus make the existing logic in that function usable against our fake test package. If you don't think that sounds too brittle/awful, I can put that together 😄

[munnerz]

Okay, I did some more messing around with this and concluded that adding a test to ensure that the default value, if explicit specified with a marker, overrides this built-in default is actually not particularly useful (and not trivial, as during the evaluation of the functions defined in known_types.go, the markers have not yet been processed so we are unable to determine whether an explicit default has been provided here).

Given that this default value is not going to change within k8s.io/api/core/v1, I think it is safe for us to always set this default value to TCP. #481 will help users running older Kubernetes versions (pre 1.18) that do not support default values either.

@pwittrock this is ready for review now and passing 😄

[pwittrock]

@munnerz Can you look into the failed tests?

[munnerz]

This PR seems to do the same thing: #440

[pwittrock]

This PR seems to do the same thing: #440

The comments on that PR don't seem like blockers. Do you have a preference for which PR we take?

[pwittrock]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, pwittrock

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pwittrock]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pugangxa]

It still does not work for me. if "crdVersions=v1" is specified default value is not generated, see #476

If I use v1beta1, then although default value is generated, but during apply it report error like:

* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[triton].properties[ports].items.properties[protocol].default: Forbidden: must not be set
* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[tensorflow].properties[ports].items.properties[protocol].default: Forbidden: must not be set
* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[onnx].properties[ports].items.properties[protocol].default: Forbidden: must not be set
* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[pytorch].properties[ports].items.properties[protocol].default: Forbidden: must not be set
* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[sklearn].properties[ports].items.properties[protocol].default: Forbidden: must not be set
* spec.validation.openAPIV3Schema.properties[spec].properties[predictor].properties[xgboost].properties[ports].items.properties[protocol].default: Forbidden: must not be set

Any ideas? Thanks~

[estroz]

I'm hoping this can be cherry-picked to fix controller-gen in v0.3 for users still wanting v1beta1 CRDs and webhooks by default (ex. in kubebuilder's go/v2 plugin).

/cherry-pick release-0.3