Add internalDNSRecords field #2460
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @cybertron! Some important instructions when contributing to openshift/api: |
|
Skipping CI for Draft Pull Request. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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-sigs/prow repository. |
openshift-ci
bot
added
the
size/XXL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: cybertron The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| // api, api-int, and ingress. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:Enum=Enabled;Disabled | ||
| // +openshift:validation:featureGate=OnPremInternalDNSRecords |
There was a problem hiding this comment.
AFAIK, this isn't a real marker. Let's remove this.
There was a problem hiding this comment.
Heh, yeah I was throwing stuff at the wall when I had trouble with the feature gate.
| @@ -1026,6 +1033,15 @@ type BareMetalPlatformStatus struct { | |||
| // +optional | |||
| LoadBalancer *BareMetalPlatformLoadBalancer `json:"loadBalancer,omitempty"` | |||
|
|
|||
| // internalDNSRecords determines whether we deploy with internal records enabled for | |||
| // api, api-int, and ingress. | |||
| // +kubebuilder:validation:Optional | |||
There was a problem hiding this comment.
This is essentially a duplicate of the +optional marker below. We prefer the use of the +optional marker so let's remove this one.
| // +openshift:validation:featureGate=OnPremInternalDNSRecords | ||
| // +openshift:enable:FeatureGate=OnPremInternalDNSRecords | ||
| // +optional | ||
| InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"` |
There was a problem hiding this comment.
Because this is an optional field and the zero value is invalid, this should have omitempty.
| type InternalDNSRecordsType string | ||
|
|
||
| const ( | ||
| InternalDNSRecordsDisabled InternalDNSRecordsType = "Disabled" | ||
| InternalDNSRecordsEnabled InternalDNSRecordsType = "Enabled" | ||
| ) |
There was a problem hiding this comment.
Normally we try to avoid the terminology Enabled and Disabled where possible because the terms can often be overloaded and cause confusion.
What if instead of naming the field this applies to internalDNSRecords, what if we named it something like dnsRecordsPolicy (or maybe dnsRecordsType? not sure which one is better) and we had Internal and External as the options?
| // internalDNSRecords determines whether we deploy with internal records enabled for | ||
| // api, api-int, and ingress. |
There was a problem hiding this comment.
Please include validation constraints in the GoDoc here so that this is more end-user friendly. This is the text used in our generated API documentation and what users will see when they use something like oc explain ... so we should make sure it reads appropriately as end-user documentation.
Some good guidelines for things to take into consideration for inclusion in the GoDoc are here: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#write-user-readable-documentation-in-godoc
| // internalDNSRecords determines whether we deploy with internal records enabled for | ||
| // api, api-int, and ingress. | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:Enum=Enabled;Disabled | ||
| // +openshift:validation:featureGate=OnPremInternalDNSRecords | ||
| // +openshift:enable:FeatureGate=OnPremInternalDNSRecords | ||
| // +optional | ||
| InternalDNSRecords InternalDNSRecordsType `json:"internalDNSRecords"` | ||
|
|
There was a problem hiding this comment.
This looks like we have only added this to the BareMetalPlatformStatus type? Is this because the OpenShift installer will end up setting this value at install time?
There was a problem hiding this comment.
If I understand the EP correctly as well, this sounds like this should only be possible to set when loadBalancer is set to UserManaged?
Do we need some additional validation logic (maybe a CEL expression) to enforce that?
There was a problem hiding this comment.
Yes, this will be populated by the installer. I have a validation in the installer to ensure it isn't set when it shouldn't be, but I can move that here if it would be better.
I should also note that this is only a partial version of the change. Because these are per-platform types we'll need to apply the same change to the other on-prem platforms once we know what it should look like.
No description provided.