Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

CFE-748: Improve CoreDNS Integration with EgressFirewall #1335

Merged
merged 21 commits into from
Sep 11, 2023
Merged

CFE-748: Improve CoreDNS Integration with EgressFirewall #1335

Changes from 6 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
3626860
This enhancement proposes to improve the integration between CoreDNS …
arkadeepsen Feb 6, 2023
86cd179
Fixed errors with adding details of author, reviewers, approvers, api…
arkadeepsen Feb 6, 2023
78520f5
Added details in the Workflow Description, Drawback and Test Plan sec…
arkadeepsen Feb 9, 2023
35292b2
Added details about the api-group
arkadeepsen Feb 9, 2023
08681c3
Minor fixes
arkadeepsen Feb 9, 2023
bb769bf
Added content to the Implementation Details section
arkadeepsen Feb 9, 2023
f3809fc
Incorporated the comments in all sections except the Alternatives sec…
arkadeepsen May 3, 2023
48bbc96
Updated Alternatives section.
arkadeepsen May 11, 2023
ad5499d
Fixed lint error
arkadeepsen May 16, 2023
88cd4dd
Adding more details about the workflow. Design updated considering ov…
arkadeepsen Jun 28, 2023
fba7fa9
Updated the CRD name, apigroup and plugin name. Added pseudocodes for…
arkadeepsen Jul 20, 2023
6854ffc
Updated workflow diagram
arkadeepsen Jul 20, 2023
6afefe1
Added the workflow for update events in cluster manager
arkadeepsen Jul 25, 2023
132057e
Added the details about a new flag to enable the DNSNameResolver CRD …
arkadeepsen Jul 26, 2023
72b8056
Updated the EP to account for different TTL and last lookup time for …
arkadeepsen Aug 3, 2023
84d572b
Fixed lint error
arkadeepsen Aug 3, 2023
02dc287
Linked the DNS lookup failure section in the Proposal section.
arkadeepsen Aug 8, 2023
b7dc6ae
Update DNSNameResolver api information
arkadeepsen Aug 21, 2023
494a4b6
Pulled in the changes from the API PR. Update regex for validating DN…
arkadeepsen Sep 6, 2023
a73edd7
Remove DNS names after 5 lookup failure and all ips expired. Send req…
arkadeepsen Sep 6, 2023
a99f116
Fix: ResolutionFailures should be greater than or equal to 5 when rem…
arkadeepsen Sep 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
363 changes: 363 additions & 0 deletions enhancements/dns/coredns-egressfirewall-integration.md
View file
Open in desktop
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,363 @@
---
title: coredns-egressfirewall-integration
authors:
- '@arkadeepsen'
reviewers:
- '@Miciah'
- '@danwinship'
- '@JoelSpeed'
- '@TrilokGeer'
approvers:
- '@Miciah'
- '@danwinship'
api-approvers:
- '@JoelSpeed'
creation-date: 2023-01-31
last-updated: 2023-01-31
tracking-link:
- https://issues.redhat.com/browse/CFE-748
see-also:
- N.A.
replaces:
- N.A.
superseded-by:
- N.A.
---

# Improve CoreDNS Integration with EgressFirewall

## Summary

This enhancement improves the integration of CoreDNS with EgressFirewall. With this improved
integration, EgressFirewall will be able to better support DNS names whose IPs change dynamically
and also will be able to provide support for wildcard DNS names.

## Motivation

Currently, EgressFirewall (OVN-K master) does a DNS lookup of a DNS name based on a default TTL or the
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
TTL of the previous lookup. OVN-K master then updates the underlying ``AddressSet`` for the DNS name
referenced by the corresponding ACL rule of the EgressFirewall rule containing the DNS name. However,
if a pod, belonging to the same Namespace as that of the EgressFirewall, does a DNS lookup and is able
to get the response before the OVN-K master then the pod will be incorrectly allowed/denied access to
the DNS name. The integration between CoreDNS and EgressFirewall needs to be improved to avoid such a
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
scenario.

If an administrator wants to specifically allow/deny access to all subdomains then currently the
administrator has to add all subdomains in the EgressFirewall rules. This becomes difficult when
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
subdomains are dynamically added/removed as each one has to be added/removed individually from the
EgressFirewall rules. Currently, wildcard DNS names are not supported in EgressFirewall. However,
even if the support is added to EgressFirewall, the integration between CoreDNS and EgressFirewall
needs to be improved as wildcard DNS names cannot be directly looked up through a DNS query.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved


### User Stories

* As an OpenShift cluster administrator, I want to add regular DNS names to EgressFirewall rules, so that I can allow/deny
access to them even if the IPs associated with the corresponding DNS records change dynamically.
* As an OpenShift cluster administrator, I want to add wildcard DNS names to EgressFirewall rules, so that I can
allow/deny access to all the subdomains belonging to the wildcard DNS names.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* As an OpenShift engineer, I want to add a new Custom Resource, so that IPs and TTLs of the DNS names which are used
in EgressFirewall rules can be tracked.
* As an OpenShift engineer, I want to add a new plugin to CoreDNS, so that DNS lookups of DNS names used in EgressFirewall
rules can be inspected and the current IPs and TTLs can be tracked in the corresponding new CR.
* As an OpenShift engineer, I want to modify Cluster DNS operator, so that CoreDNS can be deployed with the
new plugin enabled.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

### Goals

* Support update of the ``AddressSet`` referenced by the ACL rule corresponding to an EgressFirewall rule containing
a DNS name if the IPs associated with it change dynamically.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* Support usage of wildcard DNS names in EgressFirewall rules.
* Create CRs for each unique DNS name used in the EgressFirewall rules and use the CRs to track the current
IPs and the corresponding TTL information.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

### Non-Goals

* Support additional DNS resolution functionality in the new CoreDNS plugin.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* Support allowing/denying of DNS lookups (core EgressFirewall functionality) in the new CoreDNS plugin.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

## Proposal

This enhancement proposes to introduce a new CoreDNS plugin (``egressfirewall``) and a new Custom Resource
(``DNSName``) to improve the integration of CoreDNS with EgressFirewall. The ``DNSName`` CR will be created
for each unique DNS name (both regular and wildcard DNS names) used in the EgressFirewall rules. This CR will
be used to store the DNS name along with the current IPs and the correspodning TTL and the next lookup time
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
based on the TTL. The OVN-K master will be responsible for the creation of the ``DNSName`` CRs. The ``DNSName``
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
CR is meant for communication between CoreDNS and OVN-K master. Thus it cannot be created or modified by an
OpenShift cluster adminisrator.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

The new plugin will inspect each DNS lookup and the corresponding response for the DNS lookup from other
plugins. If the DNS name in the query matches any ``DNSName`` CR(s) (regular or wildcard or both), then the
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
plugin will update the ``.status`` of the matching ``DNSName`` CR(s) with the DNS name along with the IPs and
the corresponding TTL and the next lookup time based on the TTL. The OVN-K master will watch the ``DNSName``
CRs. Whenever the IPs are updated for a ``DNSName`` CR, the OVN-K master will update the the underlying ``AdressSet``
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
referenced by the ACL rule(s) for the corresponding EgressFirewall rule(s).

OVN-K master will keep track of the TTL (or next lookup time) for each regular DNS name and send a DNS lookup
query to CoreDNS when the minimum TTL expires. However, for a wildcard DNS name a DNS lookup cannot be performed
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
directly on the DNS name as it will not return any IP. Thus, the lookups will be performed on the DNS names
which are updated in the ``.status`` of the corresponding wildcard ``DNSName`` CRs.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

### Workflow Description
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the the steps from the point of view of the DNS lookup - which steps are taken when there is an Egress FW, when there isn't, when there is a wildcard, when there isn't, when a DNS name is in multiple Egress FW, and when there is an address not even in Egress FW, what's the flow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DNS lookup details is added in the workflow of the plugin: https://github.com/openshift/enhancements/pull/1335/files#diff-b27f04c5e2673c47f922695835e746ecb2205a9f7800def32cc15c25af521758R333-R500

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to explain each scenario here as well.

when there is an Egress FW

For each unique DNS name used in all the Egress Firewalls, a corresponding DNSNameResolver CR will be created by the OVN-K cluster manager. The CoreDNS plugin will watch for the DNSNameResolver CRs and store regular and wildcard DNS names in 2 separate maps.

When a DNS lookup for a regular DNS name is intercepted and the DNS name matches either a regular or wildcard DNS name or both, only then the status of the corresponding CR is updated. For wildcard DNS name lookup it will be only checked with the wildcard DNS name map and if there's a match only then the corresponding wildcard DNS name CR will be updated.

when there isn't

If there are no EgressFirewall objects then there won't be any DNSNameResolver CRs created by the OVN-K cluster manager. Thus the 2 maps will be empty. Whenever a DNS lookup is intercepted as the maps are empty the plugin immediately sends the response to the next plugin in the chain.

when there is a wildcard, when there isn't

Already mentioned above. Copying it here again for completion.

When a DNS lookup for a regular DNS name is intercepted and the DNS name matches either a regular or wildcard DNS name or both, only then the status of the corresponding CR is updated. For wildcard DNS name lookup it will be only checked with the wildcard DNS name map and if there's a match only then the corresponding wildcard DNS name CR will be updated.

when a DNS name is in multiple Egress FW

As already mentioned, only a single DNSNameResolver CR will be created for a unique DNS name used in the EgressFirewall objects, even if the same DNS name is used in multiple EgressFirewall objects. The DNS lookup process will be the same as explained above.

when there is an address not even in Egress FW

I actually didn't get the question. Do you mean when there are no DNS names mentioned in any of the EgressFirewall objects? In that case, the OVN-K cluster manager won't create any DNSNameResolver CR. The DNS lookup process will be similar to the scenario where there are no EgressFirewall objects mentioned above.


The workflows for Create, Delete and Update events for EgressFirewall related to DNS names are explained in this section.

#### Create/Update

##### Regular DNS name

* An OpenShift cluster administrator creates/updates an EgressFirewall resource for a Namespace and adds rule(s) containing regular
DNS name(s).
* The OVN-K master will create corrresponding `DNSName` CRs for each of the DNS names in the EgressFirewall rules, if not
already created. Each CR will be created in the ``openshift-dns`` Namespace and the Name of the CR will be same as the DNS
name (barring any trailinng `.`). The ``.spec.isregular`` field of the CR will be set to true, even if it already exists.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* The OVN-K master will then perform DNS lookup for each of the regular DNS names added to the EgressFirewall rules.
* The ``egressfirewall`` CoreDNS plugin will intercept the request and the response for the DNS lookup for each of the
regular DNS names.
* As these DNS names have corresponding ``DNSName`` CRs, the ``egressfirewall`` plugin will update the ``.status`` of
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
the `DNSName` CRs with the DNS name and the corresponding current IPs along with the TTL and the next lookup time based
on the TTL. However, this update will only take place if there is a change in the exisiting IP
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
addresses or next time to lookup or both for the DNS name.
* The OVN-K master will watch the ``DNSName`` CRs. When the ``.status`` of a ``DNSName`` CR is updated, the OVN-K master
will update the ``AddressSet`` for the DNS name, which is linked with the ACL rule(s) for the corresponding EgressFirewall
rule(s).
* The OVN-K master will also receive the response of the DNS lookup query for the DNS name. The OVN-K master will check
the corresponding ``DNSName`` CR's ``.status`` and if the next lookup time in the status is greater than the next lookup
time based on the received TTL, then the corresponding CR's ``.status`` will be updated. The corresponding ``AddressSet``
will also be updated.

##### Wildcard DNS name

* An OpenShift cluster administrator creates/updates an EgressFirewall resource for a Namespace and adds rule(s) containing wildcard
DNS name(s).
* The OVN-K master will create corrresponding `DNSName` CRs for each of the wildcard DNS names in the EgressFirewall rules, if
not already created. Each CR will be created in the ``openshift-dns`` Namespace. The Name of the CR will be set to the wildcard
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
DNS name after replacing the ``*`` with ``wildcard`` (barring any trailinng `.`). For example, if the wildcard DNS name is
``*.example.com``, then the Name of the corresponding CR will be ``wildcard.example.com``. This is done to adhere to the
[Kubernetes object naming validations](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the fact that it replaces the name with something else that is also a valid DNS name... yes, you have isReguar/isWildcard to handle the case where you need rules for both *.example.com and wildcard.example.com, but... it's all just a mess. (The whole "deletes unless this is true and that is false in which case it doesn't delete etc etc etc" thing later.)

FTR, since the plugin and ovn-k both need to monitor all DNSName objects anyway, the names don't really matter and you could just store the DNS name in a different field. Though OTOH, if the object names weren't the DNS name, it's not obvious what they should be...

One possibility would be that, since the status already has to contain an array to be able to deal with wildcards, you could just let the spec contain an array of names too. Then instead of creating one DNSName per DNS name, you'd create one per EgressFirewall or whatever, containing all of the DNS names used by that firewall rule, and the status would be updated any time any of the DNS names in the spec changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Name field is set to the DNS name so that there's one to one mapping with the DNSName CR and a DNS name and the CR can be fetched by the CoreDNS plugin just by using the DNS name whenever the lookup happens. I am not sure how this can be achieved without directly using the DNS name. The problem comes with wildcard DNS name.

If we have one DNSName for each EgressFirewall and the .status of DNSName containing all the details of the IPs, would it not be better to just add the information about the IPs in the .status of the EgressFirewall itself and not create a new CRD?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the CR can be fetched by the CoreDNS plugin just by using the DNS name whenever the lookup happens

but the plugin needs to be keeping a cache of all existing DNSName objects anyway, so it could just keep a "name → DNSName object" map in addition to that

If we have one DNSName for each EgressFirewall and the .status of DNSName containing all the details of the IPs, would it not be better to just add the information about the IPs in the .status of the EgressFirewall itself and not create a new CRD?

yes, maybe...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Though that would imply that the plugin was EgressFirewall-specific and couldn't be used for other things.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already asked that wildcard dns not be a part of this e.p. It is not in the RFE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a single DNSName per cluster will probably have some impact on the performance when looking up DNS names in the status of the object.

Creating a DNSName for each EgressFirewall will introduce duplication of information as same DNS name can be used in multiple EgressFirewalls. So, the IPs associated with the DNS name have to be updated in all the corresponding DNSName objects.

I am more inclined towards having a DNSName object for each DNS name (regular and wildcard). The name of the DNSName can be a hash value and the spec will contain the corresponding DNS name. Thus the IsRegular and IsWildcard fields also will not be needed anymore. Going through the status of this DNSName object will have lesser performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, if there are 2 EgressFirewall DNS rules, one for www.redhat.com and another for *.openshift.com, then there will be 2 DNSName CRs for each one of them. The IsRegular and IsWildcard fields won't be required. Whenever, a lookup happens for www.redhat.com, CoreDNS will only fetch the CR corresponding to it and if required update the status. Similarly, if a lookup happens for any subdomain under *.openshift.com, then will only fetch the CR corresponding to it and if required update the status.

NOTE: The status of a regular DNS name will only contain one item in the resolvedNames field corresponding to the regular DNS name, whereas for a wildcard DNS name the resolvedNames will contain one item for the wildcard DNS name and additional items any other subdomains under it, if any.

...
metadata:
  name: dns-596764f49c
  namespace: openshift-dns
spec:
  name: "www.redhat.com"
status:
  resolvedNames:
  -  dnsName: "www.redhat.com"
     info:
     - ...
...
metadata:
  name: dns-747487bbfb
  namespace: openshift-dns
spec:
  name: "*.openshift.com"
status:
  resolvedNames:
  -  dnsName: "*.openshift.com"
     info:
     - ...
  -  dnsName: "docs.openshift.com"
     info:
     - ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having a single DNSName per cluster will probably have some impact on the performance when looking up DNS names in the status of the object.

I am more inclined towards having a DNSName object for each DNS name (regular and wildcard). The name of the DNSName can be a hash value and the spec will contain the corresponding DNS name. Thus the IsRegular and IsWildcard fields also will not be needed anymore. Going through the status of this DNSName object will have lesser performance impact.

When you're talking about performance impacts here, I'm going to assume that you'll be using an informer to watch the objects in some operator? Having many objects subscribed will result in far more events hitting the informer than a single shared object, I think in terms of informers a single object would be more efficient.

I think as well if you're watching a single object, any time it updates, you can calculate the diff and using an atomic map update keep a cache of the data, that could then be read from in another thread to action the changes. That said, I guess you can do that with multiple objects too 🤔

Is there some other performance impact you're thinking of? My suspicion is that your data access is the biggest performance hit since that will involve an API request, so, optimising that to a single request/watch sounds better to me no?

CoreDNS will only fetch the CR corresponding to it and if required update the status.

Is CoreDNS fetching the CR directly? Will it be using an informer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having many objects subscribed will result in far more events hitting the informer than a single shared object, I think in terms of informers a single object would be more efficient.

I think only the create and delete events will be the additional ones. However, the number of update events will be the same as update will happen whenever there's a change in the IPs for a DNS name, whether we use a single object or one object each for each DNS name. The additional impact will be proportional to the number of unique DNS names used in the EgressFirewall rules.

The performance impact that I am thinking about is the time spent to traverse through the status of the CR. The resolvedNames field in the status is a slice and if it contains all the DNS names used in EgressFirewall rules, including those which match wildcard DNS names, then every time OVN-K will have to go through the entire slice to get the latest update in the IP addresses. It'll also have spend time to check whether IPs for each DNS name changed or not and update only for the ones that changed. If we have a CR for each DNS name, then OVN-K will immediately know, during an update event, the IPs for which DNS name changed and take appropriate actions. Same for the CoreDNS plugin when it has to update the status of the single object.

Another issue is more from the perspective of parallel updates to the single object by CoreDNS and OVN-K. It was suggested here (#1335 (comment)) to have a feedback from OVN-K to CoreDNS. If we have a single object, parallel updates may cause some race condition problems with out of order updates. Additionally, CoreDNS pods run as DaemonSets and each pod runs independent of the other. Thus, more than one pod may receive DNS lookup requests for different DNS names simultaneously. When updating the status of the single object chances are there that one update may be done over the other one and some updates may get lost because of this.

Is CoreDNS fetching the CR directly? Will it be using an informer?

I was planning to have both an informer for the create and delete events in the CoreDNS plugin. But when updating the object, it would be first fetched then the status would be checked with the current IPs and lookup time information. If it requires an update then update the status. OVN-K will be creating and deleting the CRs, so it will be using an informer for the update event to get the changes in the IP information of a DNS name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The performance impact that I am thinking about is the time spent to traverse through the status of the CR. The resolvedNames field in the status is a slice and if it contains all the DNS names used in EgressFirewall rules, including those which match wildcard DNS names, then every time OVN-K will have to go through the entire slice to get the latest update in the IP addresses. It'll also have spend time to check whether IPs for each DNS name changed or not and update only for the ones that changed. If we have a CR for each DNS name, then OVN-K will immediately know, during an update event, the IPs for which DNS name changed and take appropriate actions. Same for the CoreDNS plugin when it has to update the status of the single object.

This makes sense to me. Keep in mind that there is potential fanout: An EgressFirewall can have rules with many DNS names or wildcards, and a wildcard can result in an EgressFirewallDNSName with many values under status.resolvedNames.

Another issue is more from the perspective of parallel updates to the single object by CoreDNS and OVN-K. It was suggested here (#1335 (comment)) to have a feedback from OVN-K to CoreDNS. If we have a single object, parallel updates may cause some race condition problems with out of order updates. Additionally, CoreDNS pods run as DaemonSets and each pod runs independent of the other. Thus, more than one pod may receive DNS lookup requests for different DNS names simultaneously. When updating the status of the single object chances are there that one update may be done over the other one and some updates may get lost because of this.

Agreed. We will have many CoreDNS pods updating the EgressFirewallDNSName objects to update status.resolvedNames (by default, a cluster has 1 DNS pod per node), and we don't want these pods to be constantly in conflict with each other, which would cause frequent retries and unnecessarily overhead for both the CoreDNS plugin and kube-apiserver.

The ``.spec.iswildcard`` field of the CR will be set to true, even if it already exists.
* The OVN-K master will not perform any DNS lookup for the wildcard DNS names directly.
* The ``egressfirewall`` CoreDNS plugin will intercept the request and the response for the DNS lookups from all the pods. If
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
the regular DNS name in the lookup matches with a wildcard DNS name, then the ``egressfirewall`` plugin will update the
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
``.status`` of the corresponding `DNSName` CR with the regular DNS name and the corresponding current IPs along with the TTL
and the next lookup time based on the TTL. However, this update will only take place if there is a change in the exisiting IP
addresses or the next lookup time or both for the DNS name.
* The OVN-K master will watch the ``DNSName`` CRs. When the ``.status`` of a ``DNSName`` CR is updated, the OVN-K master
will update the ``AddressSet`` for the wildcard DNS name, which is linked with the ACL rule for the corresponding EgressFirewall
rule(s).
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* The OVN-K master will also store the regular DNS name and the corresponding current IPs along with the TTL and the next time
to lookup. Based on the next time to lookup, the OVN-K master will follow the same method as that of the regular DNS names to get the
latest IPs and TTL information.

#### Delete/Update

##### Regular DNS name

* An OpenShift cluster administrator deletes an EgressFirewall resource for a Namespace containing rule(s) for regular DNS
name(s) OR updates an EgressFirewall resource for a Namespace and deletes rule(s) containing regular DNS name(s).
* The OVN-K master will delete the ACL rule(s) corresponding to the EgressFirewall rule(s) containing the regular DNS name(s).
* The OVN-K master will then check if the same regular DNS names are also used in the EgressFirewall rules in other Namespaces. If
they are not used, then the OVN-K master will delete the corrresponding ``AdressSet`` for each of the DNS names in the EgressFirewall
rules. The OVN-K master will also delete the corresponding ``DNSName`` CRs, only if the ``.spec.isregular`` field is set to true and
the ``.spec.iswildcard`` field is set to false. If both the fields are set to true, then the CR will not be deleted and the
``.spec.isregular`` field will be set to false.


##### Wildcard DNS name

* An OpenShift cluster administrator deletes an EgressFirewall resource for a Namespace containing rule(s) for wildcard DNS name(s)
OR updates an EgressFirewall resource for a Namespace and deletes rule(s) containing wildcard DNS name(s).
* The OVN-K master will delete the ACL rule(s) corresponding to the EgressFirewall rule(s) containing the wildcard DNS name(s).
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* The OVN-K master will then check if the same wildcard DNS names are also used in the EgressFirewall rules in other Namespaces. If
they are not used, then the OVN-K master will delete the corrresponding ``AdressSet`` for each of the DNS names in the EgressFirewall
rules. The OVN-K master will also delete the corresponding ``DNSName`` CRs, only if the ``.spec.isregular`` field is set to false and
the ``.spec.iswildcard`` field is set to true. If both the fields are set to true, then the CR will not be deleted and the
``.spec.iswildcard`` field will be set to false. From the ``.status`` field all the other DNS names' details will be removed and
only the details of the DNS name will be kept which matches with the Name field of the CR.


#### Variation [optional]
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved


### API Extensions

The validation of ``DNSName`` field in ``EgressFirewallDestination`` will be updated to accept wildcard DNS names as well.
It will be updated from ``^([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$`` which accepts only regular DNS names to
``^(\*\.)?([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$``.

````go
// EgressFirewallDestination is the endpoint that traffic is either allowed or denied to
type EgressFirewallDestination struct {
// ..

// dnsName is the domain name to allow/deny traffic to. If this is set, cidrSelector must be unset.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:validation:Pattern=^(\*\.)?([A-Za-z0-9-]+\.)*[A-Za-z0-9-]+\.?$
DNSName string `json:"dnsName,omitempty"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
}
````

The following ``DNSName`` CRD will be added to the ``k8s.ovn.org`` api-group.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are using the CoreDNS plugin with ovn-kubernetes, it's not logically bound to ovn-kubernetes. I think this should be something like dns.openshift.io

Copy link
Contributor

@candita candita Mar 23, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since this will only apply to Egress Firewall DNSNames, the objects should have names that reflect that, like EgressFirewallDNSName or similar. No one should be able to confuse DNSNames with traditional DNS names.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the group name makes it clear that this is for OVN or for EgressFirewall, then the CRD name doesn't necessarily need to indicate it. So, for example, if the group name is "k8s.ovn.org" or "egressfirewall.operators.openshift.io" or whatever, then the qualified resource name ends up as "dnsnames.k8s.ovn.org" or "dnsnames.egressfirewall.operators.openshift.io", which makes it clear that the resource is related to the OVN-CoreDNS integration for EgressFirewall.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanted to discuss the naming. I don't think egress firewall or ovn is necessary in the name, because the functionality of resolving DNS names to ips is quite generic. As an example, even in ovn we can use this functionality for features different from egressfirewall, e.g. network policy.
So maybe we can figure out the name that would explain what this object is used, e.g. DNSNameResolver?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the EP with the following changes that were discussed:

  • The CoreDNS plugin will not wait for OVN-K masters to update the underlying ACL rules
  • OVN-K cluster manager will create and delete the new CR.
  • A new controller will watch the CR and perform the re-resolution of the DNS names added to the .status of the CRs.
  • The api definition of the new CRD is updated.

@Miciah @danwinship @npinaeva @JoelSpeed PTAL and let me know if any other changes are required.

Some of the points which are still kind of unresolved:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per today's discussions, the name of the CRD is decided to be DNSNameResolver and it'll belong to network.openshift.io apigroup. The name of the new plugin will be ocp_dnsnameresolver.


````go
// DNSName describes a DNS name used in a EgressFirewall rule.
type DNSName struct {
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

// Specification of the desired behavior of DNSName.
Spec DNSNameSpec `json:"spec"`
// Observed status of DNSName
// +optional
Status DNSNameStatus `json:"status,omitempty"`
}

// DNSNameSpec is a desired state description of DNSName.
type DNSNameSpec struct {
// IsRegular denotes whether the resource is representing a regular DNS name.
// Cannot be false if IsWildcard is also false.
// +optional
IsRegular bool `json:"isregular,omitempty"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
// IsWildcard denotes whether the resource is representing a wildcar DNS name.
// Cannot be false if IsRegular is also false.
// +optional
IsWildcard bool `json:"iswildcard,omitempty"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
}

type DNSNameStatus struct {
// The list of matching DNS names and their corresponding IPs along with TTL and next
// time to DNS lookup.
Items []DNSNameStatusItem `json:"items"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
}

type DNSNameStatusItem struct {
DNSName string `json:"dnsname"`
Info []DNSNameInfo `json:"info"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
}

type DNSNameInfo struct {
IP string `json:"ip"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
TTL string `json:"ttl"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the TTL field use type int32 (or int64, as we use in the DNSRecord CR) instead of string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Wil change the type to int64.

NextLookupTime string `json:"nextlookuptime"`
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
}
````

### Implementation Details/Notes/Constraints [optional]

The implementation changes needed for the proposed enhancement are documented in this section for each of the components.

#### Cluster DNS Operator

Cluster DNS Operator will deploy CoreDNS with the ``egressfirewall`` plugin enabled by adding it to the corefile. As the
plugin will watch and update the ``DNSName`` CRs in the ``k8s.ovn.org`` api-group, proper RBAC permissions will be needed
to be added to the ``ClusterRole`` for CoreDNS.

#### CoreDNS

The new plugin ``egressfirewall`` will be added to CoreDNS. As the plugin will inspect the DNS lookup queries and response from
other plugins, it needs to be added before the other plugins (namely ``forward`` plugin) in the ``plugin.cfg`` file.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

The ``egressfirewall`` plugin will watch the ``DNSName`` CRs and whenever there is a DNS lookup which matches one of the ``DNSName``
CRs (either regular or wildcard DNS names or both), then it will update the ``.status`` of the ``DNSName`` CR(s) if there's any change
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please explain in a little more detail how the DNSName CR lookup is done—what kind of informer, index, or data structure will the plugin use?

How is the matching done, especially with wildcards?

How do you mitigate performance impact when there are no matching CRs?

Is the update done asynchronously? What happens if there is a conflict during an update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what kind of informer, index, or data structure will the plugin use?

I am planning to use SharedIndexInformer. As for data structure, I am planning to use maps to store DNSName CR information. Do you have any suggestions?

How is the matching done, especially with wildcards?

I have mentioned about this in this #1335 (comment). I'll add the details in the EP.

How do you mitigate performance impact when there are no matching CRs?

I have mentioned about this as well in this #1335 (comment). I'll also add these details in the EP.

Is the update done asynchronously? What happens if there is a conflict during an update?

Once the update to the DNSName CR is completed, then the response to the DNS lookup will be sent. If the update to the DNSName CR fails then the DNS lookup should also fail, otherwise this might cause discrepancy in the information between the pods and OVN-K. We can add retries to the DNSName CR update process.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for data structure, I am planning to use maps to store DNSName CR information. Do you have any suggestions?

I was trying to think of a suitable data structure that could accommodate both regular and wildcard names/lookups, but using two maps as you suggested in #1335 (comment) is probably better.

in the corresponding IPs and/or the TTL information.

#### OVN-K master

For every unique DNS name used in EgressFirewall rules, OVN-K master will create a corresponding ``DNSName`` CR. The Name of the CRs
will be same as that of the DNS name (for wildcard DNS name the ``*`` will be replaced by ``wildcard``) without any trailing ``.``.
This will follow the [Kubernetes object naming validations](https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names).

The OVN-K master will also watch the ``DNSName`` CRs. Whenever the ``.status`` of the CRs will be updated with new IPs and corresponding
TTL information for a DNS name, OVN-K master will update the ``AdressSet`` mapped to the DNS name. This ``AdressSet`` will be linked
to the ACL rule(s) for the EgressFirewall rule(s) in which the DNS name is used. This will ensure that the latest IPs are always updated in
the ``AddressSets``.

The OVN-K master will not directly query for the wildcard DNS names, rather it will query for the DNS names that get added to the
``.status`` of the corresponding ``DNSName`` CR. However, the list of the DNS names to lookup for a wildcard DNS name should also
not become stale if a DNS name belonging its subdomain is removed. To achieve this a retry counter will be used for the DNS name
lookups. If the lookup fails for a DNS name listed in the ``.status`` of a ``DNSName`` CR for threshold number of times (say 5),
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
then the DNS name will be removed from the ``.status``.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved


### Risks and Mitigations


### Drawbacks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can add a note here about extra delay for requests matching wildcarded DNS names, that don't have a wildcarded DNS record?


* Whenever there's a change in the IPs or the next lookup time for a DNS name, the additional step of updating the
related ``DNSName`` CRs will be executed. This might add some delay to the DNS lookup process. However, this will only
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
happen whenever there's a change in the DNS information.


## Design Details

### Open Questions [optional]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to find out how this impacts DNS lookups, and generally how this works, when there are multiple Egress Firewalls, possibly with overlapping wildcard DNS names.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I add a note in this section regarding the impact to DNS name lookups?


### Test Plan

* This enhancement will be tested through e2e tests by adding EgressFirewall rules containing regular DNS names
and wildcard DNS names.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
* Testing the feature where IPs are dynamically changed may be a little bit tricky as this will probably include
creation of DNS records and then changing the IP adresses of the DNS records through the e2e tests.

### Graduation Criteria

This is a user facing change and will directly go to GA. This feature requires an update to Openshift Docs.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

#### Dev Preview -> Tech Preview

N.A. This feature will go directly to GA.

#### Tech Preview -> GA

N.A. This feature will go directly to GA.

#### Removing a deprecated feature


### Upgrade / Downgrade Strategy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding/removing the plugin from CoreDNS will be interesting - need tests to make sure it doesn't greatly impact the upgrade/downgrade times.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will cover the scenario in e2e tests.


Upgrade expectations:
* On upgrade, the OVN-K master will create the corresponding ``DNSName`` CRs for each DNS name in the
existing EgressFirewall resources. The ``egressfirewall`` plugin will also start updating the ``.status``
fields of the ``DNSName`` CRs. The scenarios arising out of the order of the update of the various components
are dicussed in [Version Skew Strategy](#version-skew-strategy)

Downgrade expectations:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you update the regex for EgressFirewall DNS names, you may have some issues with downgrade. Need to cover both regexes.

* On downgrade, the ``DNSName`` CRs may still remain. However, these CRs would not have any impact on how
EgressFirewall ACL rules are implemented in the downgraded cluster. Deleting the CR Definition of ``DNSName``
from the cluster would remove all the ``DNSName`` CRs.

### Version Skew Strategy
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

The following 2 scenarios may occur during the upgrade process:
* Scenario 1: The Cluster DNS operator and the CoreDNS pods are upgraded first and then the OVN-K master pods.

In this scenario, the ``egressfirewall`` CoreDNS plugin will start inspecting each DNS lookup before the ``DNSName``
CRs are created by the OVN-K master. The plugin will just respond with the response received from other plugins for
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved
the DNS lookups. As OVN-K master will be continuing the DNS lookups for DNS names with expired TTLs, CoreDNS will
also be responding with the corresponding IPs and the TTLs.

* Scenario 2: The OVN-K master pods are upgraded first and then the Cluster DNS operator and the CoreDNS pods.

In this scenario, the OVN-K master will create ``DNSName`` CRs for each unique DNS name used in EgressFirewall rules.
However, as the Cluster DNS operator and the CoreDNS pods are still not upgraded, CoreDNS pods will not run the
``egressfirewall`` plugin. The OVN-K master will still receive the response for the DNS lookup queries it will send
to the CoreDNS.
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved

### Operational Aspects of API Extensions


#### Failure Modes


#### Support Procedures


## Implementation History


## Alternatives
arkadeepsen marked this conversation as resolved.
Show resolved Hide resolved


## Infrastructure Needed [optional]