-
Notifications
You must be signed in to change notification settings - Fork 338
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.
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
Add support for DNSNameResolver #4045
Conversation
d7d701d
to
9bd6ecb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this work @arkadeepsen !
Did a first round, feel free to ping me for more info/help on these comments :)
9bd6ecb
to
ce53e84
Compare
3b3d8f8
to
8d4dcf2
Compare
@npinaeva I have made the suggested changes. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice update! some more comments :)
c3b6f9b
to
ffbc8c6
Compare
✅ Deploy Preview for subtle-torrone-bb0c84 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ffbc8c6
to
6ccaadb
Compare
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getting better every time!
6ccaadb
to
abb6f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice progress!
go-controller/pkg/clustermanager/dnsnameresolver/resolver_info.go
Outdated
Show resolved
Hide resolved
Err: fmt.Errorf("error adding DNS name %s: addressSetFactory is nil", request.DNSName), | ||
} | ||
} | ||
asIndex := GetEgressFirewallDNSAddrSetDbIDs(request.DNSName, extEgDNS.controllerName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note to self: we may need to rename addrset owner to DNSName instead of egress firewall at some point
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
abb6f51
to
99eea14
Compare
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interface looks great!
99eea14
to
7607777
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we use underscores for folder names, so I think we should rename ovn/dns-name-resolver
folder.
Also, it may be a good time to start splitting this into commits, I am thinking we should have separate commits for
- vendor bump
- crd change
- cluster manager side
- everything else
go-controller/pkg/ovn/dns-name-resolver/dnsnameresolver-controller.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/egressfirewall_external_dns.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/ovn/dns-name-resolver/dnsnameresolver-controller.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/controller_test.go
Outdated
Show resolved
Hide resolved
go-controller/pkg/clustermanager/dnsnameresolver/controller_test.go
Outdated
Show resolved
Hide resolved
e38a75c
to
c9a2169
Compare
c9a2169
to
a3fe52a
Compare
a3fe52a
to
90bbe2f
Compare
90bbe2f
to
6788da1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for patience and a great contribution @arkadeepsen!
Thanks @npinaeva for providing your feedback and suggestions to the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add e2es to make sure this new feature works?
// objects in the cluster. | ||
type Controller struct { | ||
lock sync.Mutex | ||
ocpNetworkClient ocpnetworkclientset.Interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so does this feature work without openshift? I think there should be some update to our docs to talk about this new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the doc explaining how to use the feature.
// names and mark them as unreferenced. | ||
addrSetReferenced := map[string]bool{} | ||
predicate := libovsdbops.GetPredicate[*nbdb.AddressSet](predicateIDs, func(item *nbdb.AddressSet) bool { | ||
fmt.Printf("Address Set: %s\n", item.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
// Set addrSetReferenced[addrSetName] = true if referencing acl exists. | ||
_, err = libovsdbops.FindACLsWithPredicate(nbClient, func(item *nbdb.ACL) bool { | ||
for addrSetName := range addrSetReferenced { | ||
fmt.Printf("ACL: %s\n", item.Match) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
// acl then delete it. | ||
ops := []ovsdb.Operation{} | ||
for addrSetName, isReferenced := range addrSetReferenced { | ||
fmt.Printf("Address Set: %s, is referenced: %t\n", addrSetName, isReferenced) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
// Delete the stale address sets. | ||
_, err = libovsdbops.TransactAndCheck(nbClient, ops) | ||
if err != nil { | ||
return fmt.Errorf("failed to trasact db ops: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"failed to transact db ops to delete address sets..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the error message.
that is planned as a separate PR, as it requires some more work to add dnsresolver to the kind cluster here https://github.com/openshift/coredns-ocp-dnsnameresolver. The feature will need only the operator provided by ^ to work. In openshift it is deployed as a part of cluster-dns-operator, but we need some more time to make it work with kind. |
Signed-off-by: arkadeepsen <arsen@redhat.com>
…upport wildcard DNS name Signed-off-by: arkadeepsen <arsen@redhat.com>
Signed-off-by: arkadeepsen <arsen@redhat.com>
Signed-off-by: arkadeepsen <arsen@redhat.com>
Signed-off-by: arkadeepsen <arsen@redhat.com>
6788da1
to
0f7ff0f
Compare
The DNS name resolver should be configured in the following components for it to work | ||
in a cluster: | ||
|
||
### [DNSNameResolver operator](https://github.com/openshift/coredns-ocp-dnsnameresolver/tree/main/operator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will automate this in our kind.sh script, so maybe we can omit this section for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need these details if someone wants to deploy in any other cluster apart from kind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its a good idea to keep it for now until we have automated install. We also have the helm install now so we will need to make sure it installs with kind and helm. @flavio-fernandes fyi
cd go-controller && ./hack/update-codegen.sh The generated YAMLs for the CRDs were not reflecting the changes to IPAMClaims merged via: ovn-org#4165 There were also changes in `DNSNameResolver` that were missing: ovn-org#4045 Lastly, `x-kubernetes-list-type` was added as part of the generated CRDs. Signed-off-by: Flavio Fernandes <ffernandes@nvidia.com>
cd go-controller && ./hack/update-codegen.sh The generated YAMLs for the CRDs were not reflecting the changes to IPAMClaims merged via: ovn-org#4165 There were also changes in `DNSNameResolver` that were missing: ovn-org#4045 Lastly, `x-kubernetes-list-type` was added as part of the generated CRDs. Signed-off-by: Flavio Fernandes <ffernandes@nvidia.com>
cd go-controller && ./hack/update-codegen.sh The generated YAMLs for the CRDs were not reflecting the changes to IPAMClaims merged via: ovn-org#4165 There were also changes in `DNSNameResolver` that were missing: ovn-org#4045 Lastly, `x-kubernetes-list-type` was added as part of the generated CRDs. Signed-off-by: Flavio Fernandes <ffernandes@nvidia.com>
cd go-controller && ./hack/update-codegen.sh The generated YAMLs for the CRDs were not reflecting the changes to IPAMClaims merged via: ovn-org#4165 There were also changes in `DNSNameResolver` that were missing: ovn-org#4045 Lastly, `x-kubernetes-list-type` was added as part of the generated CRDs. Signed-off-by: Flavio Fernandes <ffernandes@nvidia.com>
This PR adds the support for DNSNameResolver in ovnk as per the enhancement proposal openshift/enhancements#1335.
This feature is enabled via a new flag
enable-dns-name-resolver
. However,enable-egress-firewall
flag should also be set for this feature to be enabled.cluster-manager watches for events related to EgressFirewall objects and creates/deletes the DNSNameResolver objects based on the events.
ovnkube-controller watches DNSNameResolver to get the latest IP addresses associated with a DNS name.
resolver
is initialized instead ofegressFirewallDNS
and it is used to create/destroy/update address sets related to DNS names.If the
enable-dns-name-resolver
is not enabled, then the behavior of ovnk doesn't change.