Skip to content

Commit 26e208b

Browse files
haywoodshCopilot
haywoodsh
and
Copilot
authored
Add nginx.org/ssl-redirect annotation support (#8656)
Add support for the new nginx.org/ssl-redirect annotation to deprecate existing ingress.kubernetes.io/ssl-redirect annotation. closes #5526 The new annotation provides the same SSL redirect functionality with precedence over the legacy annotation when both are present. - Add nginx.org/ssl-redirect annotation constant and validation - Implement precedence logic favouring new annotation - Implement AddedOrUpdatedWithWarning event emission when the deprecated annotation is used --------- Signed-off-by: Haywood Shannon <[email protected]> Co-authored-by: Copilot <[email protected]>
1 parent 193bb37 commit 26e208b

File tree

9 files changed

+208
-3
lines changed

9 files changed

+208
-3
lines changed

‎examples/ingress-resources/mergeable-ingress-types/README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ Minions cannot contain the following annotations:
3333
- nginx.org/proxy-hide-headers
3434
- nginx.org/proxy-pass-headers
3535
- nginx.org/redirect-to-https
36-
- ingress.kubernetes.io/ssl-redirect
36+
- ingress.kubernetes.io/ssl-redirect (deprecated, use nginx.org/ssl-redirect instead)
37+
- nginx.org/ssl-redirect
3738
- nginx.org/hsts
3839
- nginx.org/hsts-max-age
3940
- nginx.org/hsts-include-subdomains

‎internal/configs/annotations.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ const SSLPreferServerCiphersAnnotation = "nginx.org/ssl-prefer-server-ciphers"
3030
// UseClusterIPAnnotation is the annotation where the use-cluster-ip boolean is specified.
3131
const UseClusterIPAnnotation = "nginx.org/use-cluster-ip"
3232

33+
// SSLRedirectAnnotation is the annotation where the SSL redirect boolean is specified.
34+
const SSLRedirectAnnotation = "nginx.org/ssl-redirect"
35+
3336
// AppProtectPolicyAnnotation is where the NGINX App Protect policy is specified
3437
const AppProtectPolicyAnnotation = "appprotect.f5.com/app-protect-policy"
3538

@@ -62,6 +65,7 @@ var minionDenylist = map[string]bool{
6265
"nginx.org/proxy-pass-headers": true,
6366
"nginx.org/redirect-to-https": true,
6467
"ingress.kubernetes.io/ssl-redirect": true,
68+
SSLRedirectAnnotation: true,
6569
"nginx.org/hsts": true,
6670
"nginx.org/hsts-max-age": true,
6771
"nginx.org/hsts-include-subdomains": true,
@@ -263,7 +267,13 @@ func parseAnnotations(ingEx *IngressEx, baseCfgParams *ConfigParams, isPlus bool
263267
}
264268
}
265269

266-
if sslRedirect, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "ingress.kubernetes.io/ssl-redirect", ingEx.Ingress); exists {
270+
if sslRedirect, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, SSLRedirectAnnotation, ingEx.Ingress); exists {
271+
if err != nil {
272+
nl.Error(l, err)
273+
} else {
274+
cfgParams.SSLRedirect = sslRedirect
275+
}
276+
} else if sslRedirect, exists, err := GetMapKeyAsBool(ingEx.Ingress.Annotations, "ingress.kubernetes.io/ssl-redirect", ingEx.Ingress); exists {
267277
if err != nil {
268278
nl.Error(l, err)
269279
} else {

‎internal/configs/annotations_test.go‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -927,3 +927,87 @@ func TestClientBodyBufferSizeAnnotationInvalid(t *testing.T) {
927927
})
928928
}
929929
}
930+
931+
func TestSSLRedirectAnnotations(t *testing.T) {
932+
t.Parallel()
933+
934+
tests := []struct {
935+
name string
936+
annotations map[string]string
937+
expected bool
938+
}{
939+
{
940+
name: "nginx.org/ssl-redirect enabled",
941+
annotations: map[string]string{
942+
"nginx.org/ssl-redirect": "true",
943+
},
944+
expected: true,
945+
},
946+
{
947+
name: "nginx.org/ssl-redirect disabled",
948+
annotations: map[string]string{
949+
"nginx.org/ssl-redirect": "false",
950+
},
951+
expected: false,
952+
},
953+
{
954+
name: "deprecated ingress.kubernetes.io/ssl-redirect enabled",
955+
annotations: map[string]string{
956+
"ingress.kubernetes.io/ssl-redirect": "true",
957+
},
958+
expected: true,
959+
},
960+
{
961+
name: "deprecated ingress.kubernetes.io/ssl-redirect disabled",
962+
annotations: map[string]string{
963+
"ingress.kubernetes.io/ssl-redirect": "false",
964+
},
965+
expected: false,
966+
},
967+
{
968+
name: "nginx.org/ssl-redirect takes precedence when both present",
969+
annotations: map[string]string{
970+
"nginx.org/ssl-redirect": "false",
971+
"ingress.kubernetes.io/ssl-redirect": "true",
972+
},
973+
expected: false,
974+
},
975+
{
976+
name: "nginx.org/ssl-redirect enabled takes precedence",
977+
annotations: map[string]string{
978+
"nginx.org/ssl-redirect": "true",
979+
"ingress.kubernetes.io/ssl-redirect": "false",
980+
},
981+
expected: true,
982+
},
983+
{
984+
name: "no ssl-redirect annotations",
985+
annotations: map[string]string{},
986+
expected: true, // Default is true
987+
},
988+
}
989+
990+
for _, tt := range tests {
991+
tt := tt
992+
t.Run(tt.name, func(t *testing.T) {
993+
t.Parallel()
994+
995+
ingEx := &IngressEx{
996+
Ingress: &networking.Ingress{
997+
ObjectMeta: metav1.ObjectMeta{
998+
Name: "test-ingress",
999+
Namespace: "default",
1000+
Annotations: tt.annotations,
1001+
},
1002+
},
1003+
}
1004+
1005+
baseCfgParams := NewDefaultConfigParams(context.Background(), false)
1006+
result := parseAnnotations(ingEx, baseCfgParams, false, false, false, false, false)
1007+
1008+
if result.SSLRedirect != tt.expected {
1009+
t.Errorf("Test %q: expected SSLRedirect %t, got %t", tt.name, tt.expected, result.SSLRedirect)
1010+
}
1011+
})
1012+
}
1013+
}

‎internal/configs/configurator_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1651,6 +1651,7 @@ func TestGetIngressAnnotations(t *testing.T) {
16511651
Annotations: map[string]string{
16521652
"appprotect.f5.com/app-protect-enable": "False",
16531653
"nginx.org/proxy-set-header": "X-Forwarded-ABC",
1654+
"nginx.org/ssl-redirect": "True",
16541655
"ingress.kubernetes.io/ssl-redirect": "True",
16551656
},
16561657
},
@@ -1667,6 +1668,7 @@ func TestGetIngressAnnotations(t *testing.T) {
16671668
expectedAnnotations := []string{
16681669
"appprotect.f5.com/app-protect-enable",
16691670
"nginx.org/proxy-set-header",
1671+
"nginx.org/ssl-redirect",
16701672
"ingress.kubernetes.io/ssl-redirect",
16711673
}
16721674

@@ -1748,6 +1750,7 @@ func TestGetMixedIngressAnnotations(t *testing.T) {
17481750
"alb.ingress.kubernetes.io/scheme": "internal",
17491751
"appprotect.f5.com/app-protect-enable": "False",
17501752
"nginx.org/proxy-set-header": "X-Forwarded-ABC",
1753+
"nginx.org/ssl-redirect": "True",
17511754
"ingress.kubernetes.io/ssl-redirect": "True",
17521755
},
17531756
},
@@ -1760,6 +1763,7 @@ func TestGetMixedIngressAnnotations(t *testing.T) {
17601763
}
17611764

17621765
expectedAnnotations := []string{
1766+
"nginx.org/ssl-redirect",
17631767
"ingress.kubernetes.io/ssl-redirect",
17641768
"nginx.org/proxy-set-header",
17651769
"appprotect.f5.com/app-protect-enable",

‎internal/configs/ingress.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,11 @@ func generateNginxCfg(p NginxCfgParams) (version1.IngressNginxConfig, Warnings)
132132
allWarnings := newWarnings()
133133
allWarnings.Add(rewriteTargetWarnings)
134134

135+
// Check for deprecated SSL redirect annotation and add warning
136+
if _, exists := p.ingEx.Ingress.Annotations["ingress.kubernetes.io/ssl-redirect"]; exists {
137+
allWarnings.AddWarningf(p.ingEx.Ingress, "The annotation 'ingress.kubernetes.io/ssl-redirect' is deprecated and will be removed. Please use 'nginx.org/ssl-redirect' instead.")
138+
}
139+
135140
var servers []version1.Server
136141
var limitReqZones []version1.LimitReqZone
137142

‎internal/configs/ingress_test.go‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2703,3 +2703,68 @@ func TestScaleRatelimit(t *testing.T) {
27032703
}
27042704
}
27052705
}
2706+
2707+
func TestGenerateNginxCfgForSSLRedirectDeprecationWarnings(t *testing.T) {
2708+
t.Parallel()
2709+
2710+
cafeIngressEx := createCafeIngressEx()
2711+
2712+
tests := []struct {
2713+
annotations map[string]string
2714+
expectedWarnings Warnings
2715+
msg string
2716+
}{
2717+
{
2718+
annotations: map[string]string{
2719+
"ingress.kubernetes.io/ssl-redirect": "true",
2720+
},
2721+
expectedWarnings: Warnings{
2722+
cafeIngressEx.Ingress: {"The annotation 'ingress.kubernetes.io/ssl-redirect' is deprecated and will be removed. Please use 'nginx.org/ssl-redirect' instead."},
2723+
},
2724+
msg: "deprecated annotation generates warning",
2725+
},
2726+
{
2727+
annotations: map[string]string{
2728+
"nginx.org/ssl-redirect": "true",
2729+
},
2730+
expectedWarnings: Warnings{},
2731+
msg: "new annotation does not generate warning",
2732+
},
2733+
{
2734+
annotations: map[string]string{
2735+
"ingress.kubernetes.io/ssl-redirect": "true",
2736+
"nginx.org/ssl-redirect": "false",
2737+
},
2738+
expectedWarnings: Warnings{
2739+
cafeIngressEx.Ingress: {"The annotation 'ingress.kubernetes.io/ssl-redirect' is deprecated and will be removed. Please use 'nginx.org/ssl-redirect' instead."},
2740+
},
2741+
msg: "both annotations present generates warning",
2742+
},
2743+
{
2744+
annotations: map[string]string{},
2745+
expectedWarnings: Warnings{},
2746+
msg: "no ssl-redirect annotations",
2747+
},
2748+
}
2749+
2750+
for _, test := range tests {
2751+
cafeIngressEx.Ingress.Annotations = test.annotations
2752+
configParams := NewDefaultConfigParams(context.Background(), false)
2753+
2754+
_, warnings := generateNginxCfg(NginxCfgParams{
2755+
staticParams: &StaticConfigParams{},
2756+
ingEx: &cafeIngressEx,
2757+
apResources: nil,
2758+
dosResource: nil,
2759+
isMinion: false,
2760+
isPlus: false,
2761+
BaseCfgParams: configParams,
2762+
isResolverConfigured: false,
2763+
isWildcardEnabled: false,
2764+
})
2765+
2766+
if !reflect.DeepEqual(test.expectedWarnings, warnings) {
2767+
t.Errorf("generateNginxCfg() returned %v but expected %v for the case of %s", warnings, test.expectedWarnings, test.msg)
2768+
}
2769+
}
2770+
}

‎internal/k8s/validation.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ const (
3636
clientMaxBodySizeAnnotation = "nginx.org/client-max-body-size"
3737
clientBodyBufferSizeAnnotation = "nginx.org/client-body-buffer-size"
3838
redirectToHTTPSAnnotation = "nginx.org/redirect-to-https"
39-
sslRedirectAnnotation = "ingress.kubernetes.io/ssl-redirect"
39+
sslRedirectAnnotationDeprecated = "ingress.kubernetes.io/ssl-redirect"
40+
sslRedirectAnnotation = "nginx.org/ssl-redirect"
4041
proxyBufferingAnnotation = "nginx.org/proxy-buffering"
4142
hstsAnnotation = "nginx.org/hsts"
4243
hstsMaxAgeAnnotation = "nginx.org/hsts-max-age"
@@ -188,6 +189,10 @@ var (
188189
validateRequiredAnnotation,
189190
validateBoolAnnotation,
190191
},
192+
sslRedirectAnnotationDeprecated: {
193+
validateRequiredAnnotation,
194+
validateBoolAnnotation,
195+
},
191196
sslRedirectAnnotation: {
192197
validateRequiredAnnotation,
193198
validateBoolAnnotation,

‎internal/k8s/validation_test.go‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1422,6 +1422,35 @@ func TestValidateNginxIngressAnnotations(t *testing.T) {
14221422
msg: "invalid nginx.org/redirect-to-https annotation",
14231423
},
14241424

1425+
{
1426+
annotations: map[string]string{
1427+
"nginx.org/ssl-redirect": "true",
1428+
},
1429+
specServices: map[string]bool{},
1430+
isPlus: false,
1431+
appProtectEnabled: false,
1432+
appProtectDosEnabled: false,
1433+
internalRoutesEnabled: false,
1434+
directiveAutoAdjust: false,
1435+
expectedErrors: nil,
1436+
msg: "valid nginx.org/ssl-redirect annotation",
1437+
},
1438+
{
1439+
annotations: map[string]string{
1440+
"nginx.org/ssl-redirect": "not_a_boolean",
1441+
},
1442+
specServices: map[string]bool{},
1443+
isPlus: false,
1444+
appProtectEnabled: false,
1445+
appProtectDosEnabled: false,
1446+
internalRoutesEnabled: false,
1447+
directiveAutoAdjust: false,
1448+
expectedErrors: []string{
1449+
`annotations.nginx.org/ssl-redirect: Invalid value: "not_a_boolean": must be a boolean`,
1450+
},
1451+
msg: "invalid nginx.org/ssl-redirect annotation",
1452+
},
1453+
14251454
{
14261455
annotations: map[string]string{
14271456
"ingress.kubernetes.io/ssl-redirect": "true",

‎internal/telemetry/collector_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -830,6 +830,7 @@ func TestStandardIngressAnnotations(t *testing.T) {
830830
annotations := map[string]string{
831831
"appprotect.f5.com/app-protect-enable": "False",
832832
"nginx.org/proxy-set-header": "X-Forwarded-ABC",
833+
"nginx.org/ssl-redirect": "True",
833834
"ingress.kubernetes.io/ssl-redirect": "True",
834835
"nginx.com/slow-start": "0s",
835836
}
@@ -851,6 +852,7 @@ func TestStandardIngressAnnotations(t *testing.T) {
851852
expectedAnnotations := []string{
852853
"appprotect.f5.com/app-protect-enable",
853854
"nginx.org/proxy-set-header",
855+
"nginx.org/ssl-redirect",
854856
"nginx.com/slow-start",
855857
"ingress.kubernetes.io/ssl-redirect",
856858
}

0 commit comments

Comments
 (0)