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

fix: change placeholder protocol to 443 #1609

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: change placeholder protocol to 443 #1609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions functional_tests/empty_cluster_with_private_ip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
"properties": {
"cookieBasedAffinity": "Disabled",
"pickHostNameFromBackendAddress": false,
"port": 80,
"port": 443,
"probe": {
"id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Http"
"id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Https"
},
"protocol": "Http",
"protocol": "Https",
"requestTimeout": 30
}
}
Expand Down Expand Up @@ -71,16 +71,16 @@
],
"probes": [
{
"id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Http",
"name": "defaultprobe-Http",
"id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Https",
"name": "defaultprobe-Https",
"properties": {
"host": "localhost",
"interval": 30,
"match": {},
"minServers": 0,
"path": "/",
"pickHostNameFromBackendHttpSettings": false,
"protocol": "Http",
"protocol": "Https",
"timeout": 30,
"unhealthyThreshold": 3
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/appgw/backendhttpsettings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ func (c *appGwConfigBuilder) getBackendsAndSettingsMap(cbCtx *ConfigBuilderConte
}

defaultHTTPSetting := defaultBackendHTTPSettings(c.appGwIdentifier, n.ApplicationGatewayProtocolHTTP)
if cbCtx.EnvVariables.SetDefaultHTTPSettingProbePortTo443 {
defaultHTTPSetting = defaultBackendHTTPSettings(c.appGwIdentifier, n.ApplicationGatewayProtocolHTTPS)
}

serviceBackendPairMap := make(map[backendIdentifier]serviceBackendPortPair)
backendHTTPSettingsMap := make(map[backendIdentifier]*n.ApplicationGatewayBackendHTTPSettings)
httpSettingsCollection := make(map[string]n.ApplicationGatewayBackendHTTPSettings)
Expand Down
57 changes: 50 additions & 7 deletions pkg/appgw/backendhttpsettings_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
networking "k8s.io/api/networking/v1"

"github.com/Azure/application-gateway-kubernetes-ingress/pkg/annotations"
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/environment"
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/tests"
"github.com/Azure/application-gateway-kubernetes-ingress/pkg/utils"
)
Expand Down Expand Up @@ -63,6 +64,9 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: true,
},
}

// Action
Expand All @@ -72,8 +76,8 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(setting.Protocol).To(Equal(n.ApplicationGatewayProtocolHTTP), "default backend %s should have %s", *setting.Name, n.ApplicationGatewayProtocolHTTP)
Expect(probes[utils.GetLastChunkOfSlashed(*setting.Probe.ID)].Protocol).To(Equal(n.ApplicationGatewayProtocolHTTP), "default probe should have http")
Expect(setting.Protocol).To(Equal(n.ApplicationGatewayProtocolHTTPS), "default backend %s should have %s", *setting.Name, n.ApplicationGatewayProtocolHTTPS)
Expect(probes[utils.GetLastChunkOfSlashed(*setting.Probe.ID)].Protocol).To(Equal(n.ApplicationGatewayProtocolHTTPS), "default probe should have http")
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect(probes[utils.GetLastChunkOfSlashed(*setting.Probe.ID)].Protocol).To(Equal(n.ApplicationGatewayProtocolHTTPS), "default probe should have https") update the string

continue
}

Expand Down Expand Up @@ -105,6 +109,9 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: true,
},
}

// Action
Expand All @@ -114,8 +121,8 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(setting.Protocol).To(Equal(n.ApplicationGatewayProtocolHTTP), "default backend %s should have %s", *setting.Name, n.ApplicationGatewayProtocolHTTP)
Expect(probes[utils.GetLastChunkOfSlashed(*setting.Probe.ID)].Protocol).To(Equal(n.ApplicationGatewayProtocolHTTP), "default probe should have http")
Expect(setting.Protocol).To(Equal(n.ApplicationGatewayProtocolHTTPS), "default backend %s should have %s", *setting.Name, n.ApplicationGatewayProtocolHTTP)
Expect(probes[utils.GetLastChunkOfSlashed(*setting.Probe.ID)].Protocol).To(Equal(n.ApplicationGatewayProtocolHTTPS), "default probe should have http")
Copy link
Contributor

Choose a reason for hiding this comment

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

update string, same comment as above

continue
}

Expand All @@ -142,6 +149,9 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: true,
},
}

configBuilder.mem = memoization{}
Expand All @@ -154,7 +164,7 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(int32(80)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
Expect(int32(443)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
Copy link
Contributor

Choose a reason for hiding this comment

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

Expect(int32(443)).To(Equal(*setting.Port), "default backend port %d should be 443", *setting.Port) update the string with new value

} else if strings.Contains(*setting.Name, strconv.Itoa(int(tests.ContainerPort))) {
// http setting for ingress with service port as 80
Expect(tests.ContainerPort).To(Equal(*setting.Port), "setting %s backend port %d should be 9876", *setting.Name, *setting.Port)
Expand All @@ -177,6 +187,9 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: true,
},
}

configBuilder.mem = memoization{}
Expand All @@ -189,7 +202,7 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(int32(80)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
Expect(int32(443)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
} else if strings.Contains(*setting.Name, strconv.Itoa(int(tests.ContainerPort))) {
// http setting for ingress with service port as 80
Expect(tests.ContainerPort).To(Equal(*setting.Port), "setting %s backend port %d should be 9876", *setting.Name, *setting.Port)
Expand Down Expand Up @@ -226,6 +239,9 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: true,
},
}

configBuilder.mem = memoization{}
Expand All @@ -238,7 +254,7 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(int32(80)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
Expect(int32(443)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
} else if strings.Contains(*setting.Name, strconv.Itoa(int(tests.ContainerPort))) {
// http setting for ingress with service port as 80
Expect(tests.ContainerPort).To(Equal(*setting.Port), "setting %s backend port %d should be 9876", *setting.Name, *setting.Port)
Expand All @@ -254,4 +270,31 @@ var _ = Describe("Test the creation of Backend http settings from Ingress defini
})
})

Context("when env variable for set port to 443 is false", func() {
cbCtx := &ConfigBuilderContext{
IngressList: []*networking.Ingress{ingress},
ServiceList: []*v1.Service{service},
DefaultAddressPoolID: to.StringPtr("xx"),
DefaultHTTPSettingsID: to.StringPtr("yy"),
EnvVariables: environment.EnvVariables{
SetDefaultHTTPSettingProbePortTo443: false,
},
}

configBuilder.mem = memoization{}
configBuilder.newProbesMap(cbCtx)
httpSettings, _, _, _ := configBuilder.getBackendsAndSettingsMap(cbCtx)

It("should create the default with port 80", func() {
expectedhttpSettingsLen := 3
Expect(expectedhttpSettingsLen).To(Equal(len(httpSettings)), "httpSetting count %d should be %d", len(httpSettings), expectedhttpSettingsLen)

for _, setting := range httpSettings {
if *setting.Name == DefaultBackendHTTPSettingsName {
Expect(int32(80)).To(Equal(*setting.Port), "default backend port %d should be 80", *setting.Port)
break
}
}
})
})
})
21 changes: 18 additions & 3 deletions pkg/appgw/configbuilder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,11 +489,11 @@ var _ = Describe("Tests `appgw.ConfigBuilder`", func() {
-- "properties": {
-- "cookieBasedAffinity": "Disabled",
-- "pickHostNameFromBackendAddress": false,
-- "port": 80,
-- "port": 443,
-- "probe": {
-- "id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Http"
-- "id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Https"
-- },
-- "protocol": "Http",
-- "protocol": "Https",
-- "requestTimeout": 30
-- }
-- }
Expand Down Expand Up @@ -557,6 +557,21 @@ var _ = Describe("Tests `appgw.ConfigBuilder`", func() {
-- "timeout": 30,
-- "unhealthyThreshold": 3
-- }
-- },
-- {
-- "id": "/subscriptions/--subscription--/resourceGroups/--resource-group--/providers/Microsoft.Network/applicationGateways/--app-gw-name--/probes/defaultprobe-Https",
-- "name": "defaultprobe-Https",
-- "properties": {
-- "host": "localhost",
-- "interval": 30,
-- "match": {},
-- "minServers": 0,
-- "path": "/",
-- "pickHostNameFromBackendHttpSettings": false,
-- "protocol": "Https",
-- "timeout": 30,
-- "unhealthyThreshold": 3
-- }
-- }
-- ],
-- "redirectConfigurations": [],
Expand Down
4 changes: 4 additions & 0 deletions pkg/appgw/internaltypes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,10 @@ func defaultProbeName(protocol n.ApplicationGatewayProtocol) string {
func defaultBackendHTTPSettings(appGWIdentifier Identifier, protocol n.ApplicationGatewayProtocol) n.ApplicationGatewayBackendHTTPSettings {
defHTTPSettingsName := DefaultBackendHTTPSettingsName
defHTTPSettingsPort := int32(80)
if protocol == n.ApplicationGatewayProtocolHTTPS {
defHTTPSettingsPort = int32(443)
}

return n.ApplicationGatewayBackendHTTPSettings{
Name: &defHTTPSettingsName,
ID: to.StringPtr(appGWIdentifier.HTTPSettingsID(defHTTPSettingsName)),
Expand Down
Loading