-
Notifications
You must be signed in to change notification settings - Fork 57
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
AV-203833: Initial commit for restricting fqdn #1520
Conversation
No JIRA Ids found for the PR. Jira id is mandatory to update fix version in jira. Please update respective Jira id in PR title or commit message if the PR is intented for default branches of repo. For mandatory exemptions, comment trigger phrase 'skip jira-id-check' in PR. For manual trigger, comment trigger phrase 'run jira-id-check' in PR. |
e9ef70f
to
d00bcbf
Compare
No JIRA Ids found for the PR. Jira id is mandatory to update fix version in jira. Please update respective Jira id in PR title or commit message if the PR is intented for default branches of repo. For mandatory exemptions, comment trigger phrase 'skip jira-id-check' in PR. For manual trigger, comment trigger phrase 'run jira-id-check' in PR. |
skip jira-id-check |
No JIRA Ids found for the PR. Jira id is mandatory to update fix version in jira. Please update respective Jira id in PR title or commit message if the PR is intented for default branches of repo. For mandatory exemptions, comment trigger phrase 'skip jira-id-check' in PR. For manual trigger, comment trigger phrase 'run jira-id-check' in PR. |
d00bcbf
to
abd581d
Compare
Could you please resolve lint issues @akshayhavile ? |
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.
Overall changes look good.
abd581d
to
649c0c5
Compare
649c0c5
to
99b8f60
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.
LGTM
99b8f60
to
2d1824f
Compare
2d1824f
to
fe03487
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.
LGTM
utils.AviLog.Debugf("Populating Ingress mapping for host : %s", host) | ||
for _, ing := range ings { | ||
namespace, _, name := lib.ExtractTypeNameNamespace(ing) | ||
// Fetch ingress using clientset. From informer couldn't fetch it. |
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.
Instead of making Get calls using k8s client, should we start the informers before calling PopulateCache and use them here?
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.
Populate cache may fail and then AKO will error out from there. So my intention was to avoid initialising delay by keeping booting sequence same.
What is your thoughts on this?
if lib.AKOFQDNReusePolicy() == lib.FQDNReusePolicyStrict { | ||
isValid = isRouteAcceptedWithFQDNRestriction(key, routeObj) | ||
if isValid { | ||
utils.AviLog.Debugf("Route %s is added to active list. Enqueuing it", key) |
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.
IMO, we should log the routes/ingress which we are not adding to the active list.
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.
done
internal/k8s/controller.go
Outdated
c.workqueue[bkt].AddRateLimited(key) | ||
lib.IncrementQueueCounter(utils.ObjectIngestionLayer) | ||
utils.AviLog.Debugf("key: %s, msg: %s for namespace: %s", key, msg, namespace) | ||
isAdded := true |
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.
nit: isAdded -> toBeAdded
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.
done
utils.AviLog.Debugf("key: %s, msg: ADD", key) | ||
} else { | ||
// update the status - already host claimed | ||
status.UpdateRouteStatusWithErrMsg(key, route.Name, namespace, lib.HostAlreadyClaimed) |
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.
above code block can be restructured for readability and it also removes use of isAdded
if lib.AKOFQDNReusePolicy() == lib.FQDNReusePolicyStrict && !isRouteAcceptedWithFQDNRestriction(key, route) {
status.UpdateRouteStatusWithErrMsg(key, route.Name, namespace, lib.HostAlreadyClaimed)
return
}
bkt := utils.Bkt(namespace, numWorkers)
if !lib.HasValidBackends(route.Spec, route.Name, namespace, key) {
status.UpdateRouteStatusWithErrMsg(key, route.Name, namespace, lib.DuplicateBackends)
}
c.workqueue[bkt].AddRateLimited(key)
lib.IncrementQueueCounter(utils.ObjectIngestionLayer)
utils.AviLog.Debugf("key: %s, msg: ADD", key)
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.
similar changes can be done in other places as well
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.
done
internal/k8s/controller.go
Outdated
isOldAccepted := isRouteAcceptedWithFQDNRestriction(key, oldRoute) | ||
isNewAccepted := isRouteAcceptedWithFQDNRestriction(key, newRoute) | ||
if isOldAccepted || isNewAccepted { | ||
c.workqueue[bkt].AddRateLimited(key) |
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.
If old hostname was accepted and the hostname has changed, we should be calling
objects.SharedUniqueNamespaceLister().DeleteHostnameToRoute(route.Spec.Host, routeNamespaceName)
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.
Also, I am not clear on the handling of newRoute/Ingress is done if the new hostname is already claimed post update - I think it should be handled like a Deletion event, but right now, I think it will become a no OP as EnqueuIng will skip it before processing and the corresponding VS in Avi will remain up and running.
We need to test this case
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.
When key gets enqueued, model will not be created in graph layer. But object present in rest layer cache, so it will be delete call.
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.
done.
@@ -1083,24 +1098,27 @@ func ProcessInsecureHostsForEVH(routeIgrObj RouteIngressModel, key string, parse | |||
|
|||
// Create one evh child per host and associate http policies for each path. | |||
modelGraph := aviModel.(*AviObjectGraph) | |||
modelGraph.BuildModelGraphForInsecureEVH(routeIgrObj, host, infraSetting, key, pathsvcmap) | |||
flagIngToProcess = modelGraph.BuildModelGraphForInsecureEVH(routeIgrObj, host, infraSetting, key, pathsvcmap) |
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.
given the called function is only returning true, do we really need this flag?
if len(ingresses) > 0 { | ||
nsIngressname := strings.Split(ingresses[0], "/") | ||
if len(nsIngressname) > 0 { | ||
if nsIngressname[0] != namespace { |
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 we have this check here? I think we can filter the routes/ingresses in ingestion layer itself
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 is done for multipath ingress definition where some hosts are valid and other are valid to add extra layer of blocking.
internal/lib/constants.go
Outdated
@@ -207,6 +208,8 @@ const ( | |||
VrfContextObjectNotFoundError = "VrfContext object not found" | |||
NetworkNotFoundError = "Network object not found" | |||
CtrlVersion_22_1_6 = "22.1.6" | |||
FQDNReusePolicyStrict = "strict" | |||
FQDNReusePolicyOpen = "interNamespaceallowed" |
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.
interNamespaceallowed -> internamespaceallowed
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.
done
// each entry will be objecttype/route-namespace/route-name | ||
RouteNSRouteName string | ||
// store creation time | ||
CreationTime metav1.Time |
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.
why do we need the creation time, not sure if it's used anywhere
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.
Previously we had active and inactive list. When all routes from active list gets deleted, we had to take out route with earliest creation time so that VS for that route will be created. But we have taken out functionality to dynamically updates routes. In future, if we decide to re-introduce functionality for that purpose we have kept this functionality.
g.Expect(len(nodes)).To(gomega.Equal(1)) | ||
g.Expect(len(nodes[0].EvhNodes)).To(gomega.Equal(0)) | ||
|
||
err = KubeClient.NetworkingV1().Ingresses("red").Delete(context.TODO(), "ingress-multi2", metav1.DeleteOptions{}) |
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.
We should check that model corresponding to this Ingress is not found and the status is updated correctly.
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.
model will be same as that for default namespace ingress. we are checking number of evhVs node and number of PG refs, and httpps ref... which should be 1.
6860f26
@@ -73,6 +75,7 @@ func NewAviObjCache() *AviObjCache { | |||
c.VrfCache = NewAviCache() | |||
c.PKIProfileCache = NewAviCache() | |||
c.ClusterStatusCache = NewAviCache() | |||
c.FQDNPolicyCache = NewAviCache() |
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.
same here.
utils.AviLog.Debugf("Populating Ingress mapping for host : %s", host) | ||
for _, ing := range ings { | ||
namespace, _, name := lib.ExtractTypeNameNamespace(ing) | ||
// Fetch ingress using clientset. From informer couldn't fetch it. |
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.
Populate cache may fail and then AKO will error out from there. So my intention was to avoid initialising delay by keeping booting sequence same.
What is your thoughts on this?
internal/k8s/controller.go
Outdated
isOldAccepted := isRouteAcceptedWithFQDNRestriction(key, oldRoute) | ||
isNewAccepted := isRouteAcceptedWithFQDNRestriction(key, newRoute) | ||
if isOldAccepted || isNewAccepted { | ||
c.workqueue[bkt].AddRateLimited(key) |
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.
When key gets enqueued, model will not be created in graph layer. But object present in rest layer cache, so it will be delete call.
internal/k8s/controller.go
Outdated
c.workqueue[bkt].AddRateLimited(key) | ||
lib.IncrementQueueCounter(utils.ObjectIngestionLayer) | ||
utils.AviLog.Debugf("key: %s, msg: %s for namespace: %s", key, msg, namespace) | ||
isAdded := true |
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.
done
internal/k8s/controller.go
Outdated
isOldAccepted := isRouteAcceptedWithFQDNRestriction(key, oldRoute) | ||
isNewAccepted := isRouteAcceptedWithFQDNRestriction(key, newRoute) | ||
if isOldAccepted || isNewAccepted { | ||
c.workqueue[bkt].AddRateLimited(key) |
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.
done.
@@ -55,6 +56,7 @@ type AviObjCache struct { | |||
VsCacheMeta *AviCache | |||
VsCacheLocal *AviCache | |||
ClusterStatusCache *AviCache | |||
FQDNPolicyCache *AviCache |
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.
done
internal/lib/lib.go
Outdated
@@ -1795,6 +1797,10 @@ func ValidateSvcforClass(key string, svc *corev1.Service) bool { | |||
return false | |||
} | |||
|
|||
func AKOFQDNReusePolicy() string { | |||
return AKOControlConfig().GetAKOFQDNReusePolicy() | |||
} |
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.
done
// each entry will be objecttype/route-namespace/route-name | ||
RouteNSRouteName string | ||
// store creation time | ||
CreationTime metav1.Time |
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.
Previously we had active and inactive list. When all routes from active list gets deleted, we had to take out route with earliest creation time so that VS for that route will be created. But we have taken out functionality to dynamically updates routes. In future, if we decide to re-introduce functionality for that purpose we have kept this functionality.
@@ -1216,6 +1240,7 @@ func (o *AviObjectGraph) BuildModelGraphForInsecureEVH(routeIgrObj RouteIngressM | |||
evhNode.AddFQDNAliasesToHTTPPolicy(evhNode.VHDomainNames, key) | |||
evhNode.AviMarkers.Host = evhNode.VHDomainNames | |||
objects.SharedCRDLister().UpdateFQDNToAliasesMappings(host, evhNode.VHDomainNames) | |||
return true |
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.
g.Expect(len(nodes)).To(gomega.Equal(1)) | ||
g.Expect(len(nodes[0].EvhNodes)).To(gomega.Equal(0)) | ||
|
||
err = KubeClient.NetworkingV1().Ingresses("red").Delete(context.TODO(), "ingress-multi2", metav1.DeleteOptions{}) |
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.
model will be same as that for default namespace ingress. we are checking number of evhVs node and number of PG refs, and httpps ref... which should be 1.
6860f26
to
6193d08
Compare
6193d08
to
7338f0e
Compare
…ostTonamespace mapping
utils.AviLog.Debugf("Populating Ingress mapping for host : %s", host) | ||
for _, ing := range ings { | ||
namespace, _, name := lib.ExtractTypeNameNamespace(ing) | ||
// Fetch ingress using clientset. From informer couldn't fetch it. |
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.
remove stale comment
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.
Approvig this PR with some minor comments
} else { | ||
routeObj, err := utils.GetInformers().RouteInformer.Lister().Routes(namespace).Get(name) | ||
if err != nil { | ||
utils.AviLog.Errorf("Unable to retrieve the ingress %s/%s during populating host to ingress map in populate cache: %s", namespace, name, 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.
Unable to retrieve the ingress -> Unable to retrieve the route
@@ -1286,10 +1351,18 @@ func (c *AviController) SetupEventHandlers(k8sinfo K8sinformers) { | |||
return | |||
} | |||
objects.SharedResourceVerInstanceLister().Delete(key) | |||
// Add validation here |
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.
Might want to remove this comment
@@ -1046,8 +1056,13 @@ func (o *AviObjectGraph) BuildPolicyPGPoolsForEVH(vsNode []*AviEvhVsNode, childN | |||
|
|||
func ProcessInsecureHostsForEVH(routeIgrObj RouteIngressModel, key string, parsedIng IngressConfig, modelList *[]string, Storedhosts map[string]*objects.RouteIngrhost, hostsMap map[string]*objects.RouteIngrhost) { | |||
utils.AviLog.Debugf("key: %s, msg: Storedhosts before processing insecurehosts: %s", key, utils.Stringify(Storedhosts)) | |||
//flagIngToProcess := true |
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.
Is the commented code in this function required ?
if err != nil { | ||
t.Fatalf("Couldn't DELETE the Ingress %v", err) | ||
} | ||
lib.AKOControlConfig().SetAKOFQDNReusePolicy("internamespaceallowed") |
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 we add checks after reverting the AKOFQDNReusePolicy back to internamespaceallowed for ingress here ?
TODO: