Skip to content

Commit da1d383

Browse files
Delyan Raychevakshaysngupta
Delyan Raychev
authored and
akshaysngupta
committed
Simplify Updating ingress IP address; Do only if new IP (#625)
1 parent 8b64060 commit da1d383

File tree

10 files changed

+158
-114
lines changed

10 files changed

+158
-114
lines changed

‎pkg/azure/client.go‎

Lines changed: 38 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ type azClient struct {
4141
subscriptionID SubscriptionID
4242
resourceGroupName ResourceGroup
4343
appGwName ResourceName
44+
memoizedIPs map[string]n.PublicIPAddress
4445

4546
ctx context.Context
4647
}
@@ -55,30 +56,44 @@ func NewAzClient(subscriptionID SubscriptionID, resourceGroupName ResourceGroup,
5556
subnetsClient: n.NewSubnetsClient(string(subscriptionID)),
5657
groupsClient: r.NewGroupsClient(string(subscriptionID)),
5758
deploymentsClient: r.NewDeploymentsClient(string(subscriptionID)),
58-
subscriptionID: subscriptionID,
59-
resourceGroupName: resourceGroupName,
60-
appGwName: appGwName,
59+
60+
subscriptionID: subscriptionID,
61+
resourceGroupName: resourceGroupName,
62+
appGwName: appGwName,
63+
memoizedIPs: make(map[string]n.PublicIPAddress),
6164

6265
ctx: context.Background(),
6366
authorizer: authorizer,
6467
}
6568

66-
az.appGatewaysClient.AddToUserAgent(userAgent)
69+
if err := az.appGatewaysClient.AddToUserAgent(userAgent); err != nil {
70+
glog.Error("Error adding User Agent to App Gateway client: ", userAgent)
71+
}
6772
az.appGatewaysClient.Authorizer = az.authorizer
6873

69-
az.publicIPsClient.AddToUserAgent(userAgent)
74+
if err := az.publicIPsClient.AddToUserAgent(userAgent); err != nil {
75+
glog.Error("Error adding User Agent to Public IP client: ", userAgent)
76+
}
7077
az.publicIPsClient.Authorizer = az.authorizer
71-
az.virtualNetworksClient.AddToUserAgent(userAgent)
78+
79+
if err := az.virtualNetworksClient.AddToUserAgent(userAgent); err != nil {
80+
glog.Error("Error adding User Agent to Virtual Networks client: ", userAgent)
81+
}
7282
az.virtualNetworksClient.Authorizer = az.authorizer
73-
az.subnetsClient.AddToUserAgent(userAgent)
83+
84+
if err := az.subnetsClient.AddToUserAgent(userAgent); err != nil {
85+
glog.Error("Error adding User Agent to Subnets client: ", userAgent)
86+
}
7487
az.subnetsClient.Authorizer = az.authorizer
75-
az.groupsClient.AddToUserAgent(userAgent)
76-
az.groupsClient.Authorizer = az.authorizer
7788

78-
az.publicIPsClient.AddToUserAgent(userAgent)
79-
az.publicIPsClient.Authorizer = az.authorizer
89+
if err := az.groupsClient.AddToUserAgent(userAgent); err != nil {
90+
glog.Error("Error adding User Agent to Groups client: ", userAgent)
91+
}
92+
az.groupsClient.Authorizer = az.authorizer
8093

81-
az.deploymentsClient.AddToUserAgent(userAgent)
94+
if err := az.deploymentsClient.AddToUserAgent(userAgent); err != nil {
95+
glog.Error("Error adding User Agent to Deployments client: ", userAgent)
96+
}
8297
az.deploymentsClient.Authorizer = az.authorizer
8398

8499
return az
@@ -100,8 +115,18 @@ func (az *azClient) UpdateGateway(appGwObj *n.ApplicationGateway) (err error) {
100115
}
101116

102117
func (az *azClient) GetPublicIP(resourceID string) (n.PublicIPAddress, error) {
118+
if ip, ok := az.memoizedIPs[resourceID]; ok {
119+
return ip, nil
120+
}
121+
103122
_, resourceGroupName, publicIPName := ParseResourceID(resourceID)
104-
return az.publicIPsClient.Get(az.ctx, string(resourceGroupName), string(publicIPName), "")
123+
124+
ip, err := az.publicIPsClient.Get(az.ctx, string(resourceGroupName), string(publicIPName), "")
125+
if err != nil {
126+
return n.PublicIPAddress{}, err
127+
}
128+
az.memoizedIPs[resourceID] = ip
129+
return ip, nil
105130
}
106131

107132
// DeployGateway is a method that deploy the appgw and related resources

‎pkg/controller/mutate_aks.go‎

Lines changed: 48 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -13,64 +13,81 @@ import (
1313

1414
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/annotations"
1515
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/appgw"
16+
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/azure"
1617
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/events"
1718
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/k8scontext"
1819
)
1920

21+
type ipResource string
22+
type ipAddress string
23+
2024
// MutateAKS applies changes to Kubernetes resources.
21-
func (c AppGwIngressController) MutateAKS(event events.Event) error {
25+
func (c AppGwIngressController) MutateAKS() error {
26+
appGw, cbCtx, err := c.getAppGw()
27+
if err != nil {
28+
return err
29+
}
30+
31+
ips := getIPsFromAppGateway(appGw, c.azClient)
32+
33+
// update all relevant ingresses with IP address obtained from existing App Gateway configuration
34+
cbCtx.IngressList = c.PruneIngress(appGw, cbCtx)
35+
for _, ingress := range cbCtx.IngressList {
36+
c.updateIngressStatus(appGw, cbCtx, ingress, ips)
37+
}
2238
return nil
2339
}
2440

25-
func (c AppGwIngressController) updateIngressStatus(appGw *n.ApplicationGateway, cbCtx *appgw.ConfigBuilderContext, event events.Event) {
26-
ingress, ok := event.Value.(*v1beta1.Ingress)
27-
if !ok {
28-
return
29-
}
41+
func (c AppGwIngressController) updateIngressStatus(appGw *n.ApplicationGateway, cbCtx *appgw.ConfigBuilderContext, ingress *v1beta1.Ingress, ips map[ipResource]ipAddress) {
3042

31-
// check if this ingress is for AGIC or not, it might have been updated
32-
if !k8scontext.IsIngressApplicationGateway(ingress) || !cbCtx.InIngressList(ingress) {
33-
if err := c.k8sContext.UpdateIngressStatus(*ingress, ""); err != nil {
34-
c.recorder.Event(ingress, v1.EventTypeWarning, events.ReasonUnableToUpdateIngressStatus, err.Error())
35-
}
43+
// determine what ipAddress to attach
44+
usePrivateIP, _ := annotations.UsePrivateIP(ingress)
45+
usePrivateIP = usePrivateIP || cbCtx.EnvVariables.UsePrivateIP == "true"
46+
47+
ipConf := appgw.LookupIPConfigurationByType(appGw.FrontendIPConfigurations, usePrivateIP)
48+
if ipConf == nil {
49+
glog.V(9).Info("[mutate_aks] No IP config for App Gwy: ", appGw.Name)
3650
return
3751
}
3852

39-
// determine what ip to attach
40-
usePrivateIP, _ := annotations.UsePrivateIP(ingress)
41-
usePrivateIP = usePrivateIP || cbCtx.EnvVariables.UsePrivateIP == "true"
42-
if ipConf := appgw.LookupIPConfigurationByType(appGw.FrontendIPConfigurations, usePrivateIP); ipConf != nil {
43-
if ipAddress, ok := c.ipAddressMap[*ipConf.ID]; ok {
44-
if err := c.k8sContext.UpdateIngressStatus(*ingress, ipAddress); err != nil {
45-
c.recorder.Event(ingress, v1.EventTypeWarning, events.ReasonUnableToUpdateIngressStatus, err.Error())
46-
}
53+
glog.V(5).Infof("[mutate_aks] Resolving IP for ID (%s)", *ipConf.ID)
54+
if newIP, found := ips[ipResource(*ipConf.ID)]; found {
55+
if err := c.k8sContext.UpdateIngressStatus(*ingress, k8scontext.IPAddress(newIP)); err != nil {
56+
c.recorder.Event(ingress, v1.EventTypeWarning, events.ReasonUnableToUpdateIngressStatus, err.Error())
57+
glog.Errorf("[mutate_aks] Error updating ingress %s/%s IP to %+v", ingress.Namespace, ingress.Name, newIP)
58+
return
4759
}
60+
glog.V(5).Infof("[mutate_aks] Updated Ingress %s/%s IP to %+v", ingress.Namespace, ingress.Name, newIP)
4861
}
4962
}
5063

51-
func (c AppGwIngressController) updateIPAddressMap(appGw *n.ApplicationGateway) {
64+
func getIPsFromAppGateway(appGw *n.ApplicationGateway, azClient azure.AzClient) map[ipResource]ipAddress {
65+
ips := make(map[ipResource]ipAddress)
5266
for _, ipConf := range *appGw.FrontendIPConfigurations {
53-
if _, ok := c.ipAddressMap[*ipConf.ID]; ok {
54-
return
67+
ipID := ipResource(*ipConf.ID)
68+
if _, ok := ips[ipID]; ok {
69+
continue
5570
}
5671

5772
if ipConf.PrivateIPAddress != nil {
58-
c.ipAddressMap[*ipConf.ID] = k8scontext.IPAddress(*ipConf.PrivateIPAddress)
59-
} else if ipAddress := c.getPublicIPAddress(*ipConf.PublicIPAddress.ID); ipAddress != nil {
60-
c.ipAddressMap[*ipConf.ID] = *ipAddress
73+
ips[ipID] = ipAddress(*ipConf.PrivateIPAddress)
74+
} else if ipAddress := getPublicIPAddress(*ipConf.PublicIPAddress.ID, azClient); ipAddress != nil {
75+
ips[ipID] = *ipAddress
6176
}
6277
}
78+
glog.V(5).Infof("[mutate_aks] Found IPs: %+v", ips)
79+
return ips
6380
}
6481

65-
// getPublicIPAddress gets the ip address associated to public ip on Azure
66-
func (c AppGwIngressController) getPublicIPAddress(publicIPID string) *k8scontext.IPAddress {
67-
// get public ip
68-
publicIP, err := c.azClient.GetPublicIP(publicIPID)
82+
// getPublicIPAddress gets the ipAddress address associated to public ipAddress on Azure
83+
func getPublicIPAddress(publicIPID string, azClient azure.AzClient) *ipAddress {
84+
// get public ipAddress
85+
publicIP, err := azClient.GetPublicIP(publicIPID)
6986
if err != nil {
70-
glog.Errorf("Unable to get Public IP Address %s. Error %s", publicIPID, err)
87+
glog.Errorf("[mutate_aks] Unable to get Public IP Address %s. Error %s", publicIPID, err)
7188
return nil
7289
}
7390

74-
ipAddress := k8scontext.IPAddress(*publicIP.IPAddress)
91+
ipAddress := ipAddress(*publicIP.IPAddress)
7592
return &ipAddress
7693
}

‎pkg/controller/mutate_app_gateway_test.go‎ renamed to ‎pkg/controller/mutate_aks_test.go‎

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/appgw"
2323
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/crd_client/agic_crd_client/clientset/versioned/fake"
2424
istio_fake "github.com/Azure/application-gateway-kubernetes-ingress/pkg/crd_client/istio_crd_client/clientset/versioned/fake"
25-
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/events"
2625
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/k8scontext"
2726
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/metricstore"
2827
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests"
@@ -43,7 +42,9 @@ var _ = Describe("process function tests", func() {
4342
},
4443
}
4544
publicIP := k8scontext.IPAddress("xxxx")
46-
privateIP := k8scontext.IPAddress("xxxx")
45+
privateIP := k8scontext.IPAddress("yyyy")
46+
var ips map[ipResource]ipAddress
47+
4748
BeforeEach(func() {
4849
stopChannel = make(chan struct{})
4950

@@ -82,16 +83,17 @@ var _ = Describe("process function tests", func() {
8283
DefaultAddressPoolID: to.StringPtr("xx"),
8384
DefaultHTTPSettingsID: to.StringPtr("yy"),
8485
}
86+
87+
ips = map[ipResource]ipAddress{"PublicIP": "xxxx", "PrivateIP": "yyyy"}
8588
})
89+
8690
AfterEach(func() {
8791
close(stopChannel)
8892
})
93+
8994
Context("test updateIngressStatus", func() {
90-
It("ensure that updateIngressStatus adds ip to ingress", func() {
91-
controller.updateIngressStatus(&appGw, cbCtx, events.Event{
92-
Type: events.Update,
93-
Value: ingress,
94-
})
95+
It("ensure that updateIngressStatus adds ipAddress to ingress", func() {
96+
controller.updateIngressStatus(&appGw, cbCtx, ingress, ips)
9597
updatedIngress, _ := k8sClient.ExtensionsV1beta1().Ingresses(ingress.Namespace).Get(ingress.Name, metav1.GetOptions{})
9698
Expect(updatedIngress.Status.LoadBalancer.Ingress).Should(ContainElement(v1.LoadBalancerIngress{
9799
Hostname: "",
@@ -100,28 +102,12 @@ var _ = Describe("process function tests", func() {
100102
Expect(len(updatedIngress.Status.LoadBalancer.Ingress)).To(Equal(1))
101103
})
102104

103-
It("ensure that updateIngressStatus removes ip to ingress not for AGIC", func() {
104-
ingress.Annotations[annotations.IngressClassKey] = "otheric"
105-
updatedIngress, _ := k8sClient.ExtensionsV1beta1().Ingresses(ingress.Namespace).Update(ingress)
106-
controller.k8sContext.UpdateIngressStatus(*ingress, k8scontext.IPAddress(publicIP))
107-
controller.updateIngressStatus(&appGw, cbCtx, events.Event{
108-
Type: events.Update,
109-
Value: ingress,
110-
})
111-
updatedIngress, _ = k8sClient.ExtensionsV1beta1().Ingresses(ingress.Namespace).Get(ingress.Name, metav1.GetOptions{})
112-
Expect(annotations.IsApplicationGatewayIngress(updatedIngress)).To(BeFalse())
113-
Expect(len(updatedIngress.Status.LoadBalancer.Ingress)).To(Equal(0))
114-
})
115-
116-
It("ensure that updateIngressStatus adds private ip when annotation is present", func() {
105+
It("ensure that updateIngressStatus adds private ipAddress when annotation is present", func() {
117106
ingress.Annotations[annotations.UsePrivateIPKey] = "true"
118107
updatedIngress, _ := k8sClient.ExtensionsV1beta1().Ingresses(ingress.Namespace).Update(ingress)
119108
Expect(annotations.UsePrivateIP(updatedIngress)).To(BeTrue())
120109

121-
controller.updateIngressStatus(&appGw, cbCtx, events.Event{
122-
Type: events.Update,
123-
Value: ingress,
124-
})
110+
controller.updateIngressStatus(&appGw, cbCtx, ingress, ips)
125111

126112
updatedIngress, _ = k8sClient.ExtensionsV1beta1().Ingresses(ingress.Namespace).Get(ingress.Name, metav1.GetOptions{})
127113
Expect(updatedIngress.Status.LoadBalancer.Ingress).Should(ContainElement(v1.LoadBalancerIngress{

‎pkg/controller/mutate_app_gateway.go‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,7 @@ type realClock struct{}
2525

2626
func (realClock) Now() time.Time { return time.Now() }
2727

28-
// MutateAppGateway applies App Gateway config.
29-
func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
28+
func (c AppGwIngressController) getAppGw() (*n.ApplicationGateway, *appgw.ConfigBuilderContext, error) {
3029
// Get current application gateway config
3130
appGw, err := c.azClient.GetGateway()
3231
c.metricStore.IncArmAPICallCounter()
@@ -36,14 +35,9 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
3635
if c.agicPod != nil {
3736
c.recorder.Event(c.agicPod, v1.EventTypeWarning, events.ReasonUnableToFetchAppGw, errorLine)
3837
}
39-
return ErrFetchingAppGatewayConfig
38+
return nil, nil, ErrFetchingAppGatewayConfig
4039
}
4140

42-
c.updateIPAddressMap(&appGw)
43-
44-
existingConfigJSON, _ := dumpSanitizedJSON(&appGw, false, to.StringPtr("-- Existing App Gwy Config --"))
45-
glog.V(5).Info("Existing App Gateway config: ", string(existingConfigJSON))
46-
4741
cbCtx := &appgw.ConfigBuilderContext{
4842
ServiceList: c.k8sContext.ListServices(),
4943
IngressList: c.k8sContext.ListHTTPIngresses(),
@@ -53,6 +47,19 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
5347
DefaultHTTPSettingsID: to.StringPtr(c.appGwIdentifier.HTTPSettingsID(appgw.DefaultBackendHTTPSettingsName)),
5448
}
5549

50+
return &appGw, cbCtx, nil
51+
}
52+
53+
// MutateAppGateway applies App Gateway config.
54+
func (c AppGwIngressController) MutateAppGateway() error {
55+
appGw, cbCtx, err := c.getAppGw()
56+
if err != nil {
57+
return err
58+
}
59+
60+
existingConfigJSON, _ := dumpSanitizedJSON(appGw, false, to.StringPtr("-- Existing App Gwy Config --"))
61+
glog.V(5).Info("Existing App Gateway config: ", string(existingConfigJSON))
62+
5663
if cbCtx.EnvVariables.EnableBrownfieldDeployment {
5764
prohibitedTargets := c.k8sContext.ListAzureProhibitedTargets()
5865
if len(prohibitedTargets) > 0 {
@@ -81,7 +88,7 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
8188
}
8289
}
8390

84-
cbCtx.IngressList = c.PruneIngress(&appGw, cbCtx)
91+
cbCtx.IngressList = c.PruneIngress(appGw, cbCtx)
8592

8693
if cbCtx.EnvVariables.EnableIstioIntegration {
8794
var gatewaysInfo []string
@@ -102,7 +109,7 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
102109
}
103110

104111
// Create a configbuilder based on current appgw config
105-
configBuilder := appgw.NewConfigBuilder(c.k8sContext, &c.appGwIdentifier, &appGw, c.recorder, realClock{})
112+
configBuilder := appgw.NewConfigBuilder(c.k8sContext, &c.appGwIdentifier, appGw, c.recorder, realClock{})
106113

107114
// Run validations on the Kubernetes resources which can suggest misconfiguration.
108115
if err = configBuilder.PreBuildValidate(cbCtx); err != nil {
@@ -133,10 +140,7 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
133140
}
134141
}
135142

136-
if c.configIsSame(&appGw) {
137-
// update ingresses with appgw gateway ip address
138-
c.updateIngressStatus(generatedAppGw, cbCtx, event)
139-
143+
if c.configIsSame(appGw) {
140144
glog.V(3).Info("cache: Config has NOT changed! No need to connect to ARM.")
141145
return nil
142146
}
@@ -150,12 +154,12 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
150154
if err != nil {
151155
// Reset cache
152156
c.configCache = nil
153-
configJSON, _ := dumpSanitizedJSON(&appGw, cbCtx.EnvVariables.EnableSaveConfigToFile, nil)
157+
configJSON, _ := dumpSanitizedJSON(appGw, cbCtx.EnvVariables.EnableSaveConfigToFile, nil)
154158
glogIt := glog.Errorf
155159
if cbCtx.EnvVariables.EnablePanicOnPutError {
156160
glogIt = glog.Fatalf
157161
}
158-
errorLine := fmt.Sprintf("Failed applying App Gwy configuration: %s -- %s", err, string(configJSON))
162+
errorLine := fmt.Sprintf("Failed applying App Gwy configuration:\n%s\n\nerror: %s", string(configJSON), err)
159163
glogIt(errorLine)
160164
if c.agicPod != nil {
161165
c.recorder.Event(c.agicPod, v1.EventTypeWarning, events.ReasonFailedApplyingAppGwConfig, errorLine)
@@ -164,7 +168,7 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
164168
return err
165169
}
166170
// Wait until deployment finshes and save the error message
167-
configJSON, _ := dumpSanitizedJSON(&appGw, cbCtx.EnvVariables.EnableSaveConfigToFile, nil)
171+
configJSON, _ := dumpSanitizedJSON(appGw, cbCtx.EnvVariables.EnableSaveConfigToFile, nil)
168172
glog.V(5).Info(string(configJSON))
169173

170174
// We keep this at log level 1 to show some heartbeat in the logs. Without this it is way too quiet.
@@ -186,10 +190,7 @@ func (c AppGwIngressController) MutateAppGateway(event events.Event) error {
186190
}
187191

188192
glog.V(3).Info("cache: Updated with latest applied config.")
189-
c.updateCache(&appGw)
190-
191-
// update ingresses with appgw gateway ip address
192-
c.updateIngressStatus(generatedAppGw, cbCtx, event)
193+
c.updateCache(appGw)
193194

194195
c.metricStore.IncArmAPIUpdateCallSuccessCounter()
195196

‎pkg/controller/prune_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,14 @@ var _ = Describe("prune function tests", func() {
5353
}
5454
appGw := fixtures.GetAppGateway()
5555

56-
It("removes the ingress using private ip and keeps others", func() {
56+
It("removes the ingress using private ipAddress and keeps others", func() {
5757
Expect(len(cbCtx.IngressList)).To(Equal(2))
5858
prunedIngresses := pruneNoPrivateIP(controller, &appGw, cbCtx, cbCtx.IngressList)
5959
Expect(len(prunedIngresses)).To(Equal(1))
6060
Expect(prunedIngresses[0].Annotations[annotations.UsePrivateIPKey]).To(Equal(""))
6161
})
6262

63-
It("keeps the ingress using private ip when public ip is present", func() {
63+
It("keeps the ingress using private ipAddress when public ipAddress is present", func() {
6464
appGw.FrontendIPConfigurations = &[]n.ApplicationGatewayFrontendIPConfiguration{
6565
fixtures.GetPublicIPConfiguration(),
6666
fixtures.GetPrivateIPConfiguration(),

0 commit comments

Comments
 (0)