CFE-849: Add new DNSNameResolver CRD #1524
Conversation
|
@arkadeepsen: This pull request references CFE-849 which is a valid jira issue. In response to this:
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. |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @arkadeepsen! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/L
arkadeepsen
force-pushed
the
egressfirewalldnsname
branch
2 times, most recently
from
83838c3
to
2d7a3cb
Compare
|
@JoelSpeed PTAL |
arkadeepsen
force-pushed
the
egressfirewalldnsname
branch
from
2d7a3cb
to
30e3b95
Compare
| Name: FeatureGateEgressFirewallDNSName, | ||
| }, | ||
| OwningJiraComponent: "dns", | ||
| ResponsiblePerson: "miciah", |
There was a problem hiding this comment.
@Miciah Just checking you're cool with being the owner listed on this?
There was a problem hiding this comment.
I had initially added @Miciah's name as the apigroup was chosen as dns.openshift.io. Though. as of now it is decided to go with network.openshift.io as the apigroup.
dns/.codegen.yaml
Outdated
There was a problem hiding this comment.
I would slim this down to just the ones that you need, so that would just end up being
schemapatch:
requiredFeatureSets:
- ""
- "Default"
- "TechPreviewNoUpgrade"
- "CustomNoUpgrade"
There was a problem hiding this comment.
You should have a CustomNoUpgrade version of this CRD as well else you risk breaking CustomNoUpgrade use cases
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| api-approved.openshift.io: https://github.com/openshift/api/pull/xxx |
There was a problem hiding this comment.
1524 needs to be subbed in here
dns/v1alpha1/doc.go
Outdated
| // +k8s:defaulter-gen=TypeMeta | ||
| // +k8s:openapi-gen=true | ||
|
|
||
| // +groupName=dns.openshift.io |
There was a problem hiding this comment.
Can you remind me, what group is the EgressFirewall in? Was the group name discussed on the enhancement somewhere?
There was a problem hiding this comment.
It was decided in a meeting with @Miciah , @danwinship and @npinaeva that the group will be network.openshift.io and the name of the CRD will be DNSNameResolver. I will post the changes. openshift/enhancements#1335 (comment)
| // dnsName is the resolved DNS name matching the name field of EgressFirewallDNSNameSpec. | ||
| // +kubebuilder:validation:Pattern=^(\*\.)?([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.$ | ||
| DNSName string `json:"dnsName"` | ||
| // The IP addresses associated with the DNS name used in a EgressFirewall rule. |
There was a problem hiding this comment.
Should start with the name of the field and explain what it is
| // The IP addresses associated with the DNS name used in a EgressFirewall rule. | ||
| // +listType=set | ||
| IPs []string `json:"ips"` | ||
| // Minimum time-to-live value among all the IP addresses. |
There was a problem hiding this comment.
Should start with the name of the field
| // +listType=set | ||
| IPs []string `json:"ips"` | ||
| // Minimum time-to-live value among all the IP addresses. | ||
| TTL int64 `json:"ttl"` |
There was a problem hiding this comment.
I currently have no idea what unit this is, any reason not to use a duration string?
| // Minimum time-to-live value among all the IP addresses. | ||
| TTL int64 `json:"ttl"` | ||
| // Timestamp when the last DNS lookup was successfully completed. | ||
| LastLookupTime metav1.Time `json:"lastLookupTime"` |
There was a problem hiding this comment.
Is this required? Will need to be a pointer as the json marshal for the zero value is not what you want, ever
| // retryCounter keeps the count of how many times the DNS lookup failed for the dnsName field. | ||
| RetryCounter int `json:"retryCounter"` |
There was a problem hiding this comment.
Maybe resolutionFailures? Retry counter to me suggests it might be hard to comprehend, what does it mean when zero? When would it increment to 1?
Need to expand the godoc here as well, to explain what this is counting and what happens if it reaches a certain threshold
There was a problem hiding this comment.
I will add the details
openshift-merge-robot
added
the
needs-rebase
openshift-ci
bot
added
size/XL
arkadeepsen
force-pushed
the
egressfirewalldnsname
branch
from
70bc0a0
to
ad5ffab
Compare
openshift-merge-robot
removed
the
needs-rebase
|
/retitle Add new DNSNameResolver CRD |
openshift-ci
bot
changed the title
|
@arkadeepsen: No Jira issue is referenced in the title of this pull request. In response to this:
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. |
openshift-ci-robot
removed
the
jira/valid-reference
|
/retitle CFE-849: Add new DNSNameResolver CRD |
openshift-ci
bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@arkadeepsen: This pull request references CFE-849 which is a valid jira issue. In response to this:
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. |
|
@JoelSpeed I have incorporated your suggestions. I have also changed the name of the CRD to |
| // resolvedNames contains a list of matching DNS names and their corresponding IP addresses | ||
| // along with TTL and last DNS lookup time. | ||
| // +optional | ||
| ResolvedNames []DNSNameResolverStatusItem `json:"resolvedNames,omitempty"` |
There was a problem hiding this comment.
if we decide to patch status, we may need more options for this field like listType and patchStrategy?
… name. Fix wordings of tests and godoc. Add space before godoc of each field.
…ensure each label is a maximum of 63 characters. Update godoc for conditions to add details about Degraded condition.
|
|
||
| type DNSNameResolverResolvedAddress struct { | ||
| // ip is an IP address associated with the dnsName. The validity of the IP address expires after | ||
| // lastLookupTime + ttlSeconds. To refresh the information a DNS lookup will be performed on the |
There was a problem hiding this comment.
nit:
| // lastLookupTime + ttlSeconds. To refresh the information a DNS lookup will be performed on the | |
| // lastLookupTime + ttlSeconds. To refresh the information, a DNS lookup will be performed upon the |
| // ip is an IP address associated with the dnsName. The validity of the IP address expires after | ||
| // lastLookupTime + ttlSeconds. To refresh the information a DNS lookup will be performed on the | ||
| // expiration of the IP address's validity. If the information is not refreshed then it will be | ||
| // removed with a grace period after the expiration of the IP address's validity. |
There was a problem hiding this comment.
Can you add the duration of the grace period? Even if we aren't sure yet how long that is, you can state that it may change in the future.
| // removed with a grace period after the expiration of the IP address's validity. | |
| // removed after observing a grace period of __ seconds, and after the expiration of the IP address's validity. |
There was a problem hiding this comment.
This was discussed previously, with @npinaeva and @JoelSpeed. We had decided to remove the specifics about the grace period from here: #1524 (comment)
There was a problem hiding this comment.
@arkadeepsen - if you can get this into the docs without putting it into the godocs, that's fine. Seems like an extra amount of work to both provide docs through the godocs, and also write something up separately.
There was a problem hiding this comment.
The information about the grace period will be added in the TP docs for the feature.
| } | ||
|
|
||
| // DNSName is used for validation of a DNS name. | ||
| // +kubebuilder:validation:Pattern=`^(\*\.)?([a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?\.){2,}$` |
There was a problem hiding this comment.
I'm wondering if we could break down this validation at all into CEL and then use that to enforce the different rules and apply better error messages. Will have a think about this.
There was a problem hiding this comment.
@JoelSpeed The dnsName field is used inside the resolvedNames field. We will probably hit the cost exceeds budget issue with CEL validations that we faced while trying to add the same for the ip field: #1524 (comment)
There was a problem hiding this comment.
I'm also a little worried that this might cause issues for DNS names that are accepted by the library-go validation for Route but not accepted here. In a Route, we had to allow for a one-label host name: https://github.com/openshift/library-go/blob/master/pkg/route/routeapihelpers/routeapihelpers.go#L63
There was a problem hiding this comment.
This should not be a problem, as while creating the CR the OVN-K master will correctly convert a DNS name into this format. Even the EgressFirewall DNS name accepts DNS names without trailing periods, however while creating the corresponding DNSNameResolver CR, OVN-K master will add the trailing period. The more strict regex is added as we want to eliminate the chances of creating more than one CR for the same DNS name.
There was a problem hiding this comment.
Ahh yes, we can't do this because resolvedAddresses is an unbounded list. I don't think given previous conversation that we can bound resolvedAddresses, so nothing to do here
|
A few remaining comments that @JoelSpeed can decide, but otherwise: |
|
@arkadeepsen: This pull request references CFE-849 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
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. |
|
@JoelSpeed PLMK if I need to change anything in the PR. If not, it would be great if you can approve the PR. |
|
/lgtm |
openshift-ci
bot
added
the
approved
|
/retest-required |
|
/retest-required |
|
@arkadeepsen: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arkadeepsen, candita, JoelSpeed, rfredette 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:
Approvers can indicate their approval by writing |
Added the new CRD as per the enhancement proposal openshift/enhancements#1335