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

Removing secure-verify-ca-secret support #4695

Merged
merged 2 commits into from
Nov 8, 2019
Merged

Removing secure-verify-ca-secret support #4695

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31227d6
Removing secure-verify-ca-secret support and writing an error log if …
Oct 18, 2019
c76995b
Fixing comments
Oct 18, 2019
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
5 changes: 0 additions & 5 deletions docs/kubectl-plugin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,11 +121,6 @@ $ kubectl ingress-nginx backends -n ingress-nginx
}
},
"port": 0,
"secureCACert": {
"secret": "",
"caFilename": "",
"caSha": ""
},
"sslPassthrough": false,
"endpoints": [
{
Expand Down
1 change: 0 additions & 1 deletion docs/user-guide/nginx-configuration/annotations.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ You can add these Kubernetes annotations to specific Ingress objects to customiz
|[nginx.ingress.kubernetes.io/enable-rewrite-log](#enable-rewrite-log)|"true" or "false"|
|[nginx.ingress.kubernetes.io/rewrite-target](#rewrite)|URI|
|[nginx.ingress.kubernetes.io/satisfy](#satisfy)|string|
|[nginx.ingress.kubernetes.io/secure-verify-ca-secret](#secure-backends)|string|
|[nginx.ingress.kubernetes.io/server-alias](#server-alias)|string|
|[nginx.ingress.kubernetes.io/server-snippet](#server-snippet)|string|
|[nginx.ingress.kubernetes.io/service-upstream](#service-upstream)|"true" or "false"|
Expand Down
35 changes: 0 additions & 35 deletions internal/ingress/annotations/annotations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,41 +110,6 @@ func buildIngress() *networking.Ingress {
}
}

func TestSecureVerifyCACert(t *testing.T) {
ec := NewAnnotationExtractor(mockCfg{
MockSecrets: map[string]*apiv1.Secret{
"default/secure-verify-ca": {
ObjectMeta: metav1.ObjectMeta{
Name: "secure-verify-ca",
},
},
},
})

anns := []struct {
it int
annotations map[string]string
exists bool
}{
{1, map[string]string{backendProtocol: "HTTPS", annotationSecureVerifyCACert: "not"}, false},
{2, map[string]string{backendProtocol: "HTTP", annotationSecureVerifyCACert: "secure-verify-ca"}, false},
{3, map[string]string{backendProtocol: "HTTPS", annotationSecureVerifyCACert: "secure-verify-ca"}, true},
{4, map[string]string{backendProtocol: "HTTPS", annotationSecureVerifyCACert + "_not": "secure-verify-ca"}, false},
{5, map[string]string{backendProtocol: "HTTPS"}, false},
{6, map[string]string{}, false},
{7, nil, false},
}

for _, ann := range anns {
ing := buildIngress()
ing.SetAnnotations(ann.annotations)
su := ec.Extract(ing).SecureUpstream
if (su.CACert.CAFileName != "") != ann.exists {
t.Errorf("Expected exists was %v on iteration %v", ann.exists, ann.it)
}
}
}

func TestSSLPassthrough(t *testing.T) {
ec := NewAnnotationExtractor(mockCfg{})
ing := buildIngress()
Expand Down
27 changes: 4 additions & 23 deletions internal/ingress/annotations/secureupstream/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,8 @@ limitations under the License.
package secureupstream

import (
"fmt"

"github.com/pkg/errors"
networking "k8s.io/api/networking/v1beta1"
"k8s.io/klog"

"k8s.io/ingress-nginx/internal/ingress/annotations/parser"
"k8s.io/ingress-nginx/internal/ingress/resolver"
Expand All @@ -43,27 +41,10 @@ func NewParser(r resolver.Resolver) parser.IngressAnnotation {
// Parse parses the annotations contained in the ingress
// rule used to indicate if the upstream servers should use SSL
func (a su) Parse(ing *networking.Ingress) (interface{}, error) {
bp, _ := parser.GetStringAnnotation("backend-protocol", ing)
ca, _ := parser.GetStringAnnotation("secure-verify-ca-secret", ing)
secure := &Config{
CACert: resolver.AuthSSLCert{},
}

if (bp != "HTTPS" && bp != "GRPCS") && ca != "" {
return secure,
errors.Errorf("trying to use CA from secret %v/%v on a non secure backend", ing.Namespace, ca)
}
if ca == "" {
return secure, nil
}
caCert, err := a.r.GetAuthCertificate(fmt.Sprintf("%v/%v", ing.Namespace, ca))
if err != nil {
return secure, errors.Wrap(err, "error obtaining certificate")
}
if caCert == nil {
return secure, nil
if ca != "" {
klog.Errorf("NOTE! secure-verify-ca-secret is not suppored anymore. Please use proxy-ssl-secret instead")
}
return &Config{
CACert: *caCert,
}, nil
return nil, nil
}
23 changes: 20 additions & 3 deletions internal/ingress/annotations/secureupstream/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func TestAnnotations(t *testing.T) {
"default/secure-verify-ca": {},
},
}).Parse(ing)
if err != nil {
if err == nil {
Copy link

Choose a reason for hiding this comment

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

This should be an error here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is right. Fixing.

t.Errorf("Unexpected error on ingress: %v", err)
}
}
Expand All @@ -116,7 +116,7 @@ func TestSecretNotFound(t *testing.T) {
data[parser.GetAnnotationWithPrefix("secure-verify-ca-secret")] = "secure-verify-ca"
ing.SetAnnotations(data)
_, err := NewParser(mockCfg{}).Parse(ing)
if err == nil {
if err != nil {
t.Error("Expected secret not found error on ingress")
}
}
Expand All @@ -132,7 +132,24 @@ func TestSecretOnNonSecure(t *testing.T) {
"default/secure-verify-ca": {},
},
}).Parse(ing)
if err == nil {
if err != nil {
t.Error("Expected CA secret on non secure backend error on ingress")
}
}

func TestUnsupportedAnnotation(t *testing.T) {
ing := buildIngress()
data := map[string]string{}
data[parser.GetAnnotationWithPrefix("backend-protocol")] = "HTTPS"
data[parser.GetAnnotationWithPrefix("secure-verify-ca-secret")] = "secure-verify-ca"
ing.SetAnnotations(data)

_, err := NewParser(mockCfg{
certs: map[string]resolver.AuthSSLCert{
"default/secure-verify-ca": {},
},
}).Parse(ing)
if err != nil {
t.Errorf("Unexpected error on ingress: %v", err)
}
}
4 changes: 0 additions & 4 deletions internal/ingress/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -706,8 +706,6 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
klog.V(3).Infof("Creating upstream %q", defBackend)
upstreams[defBackend] = newUpstream(defBackend)

upstreams[defBackend].SecureCACert = anns.SecureUpstream.CACert

upstreams[defBackend].UpstreamHashBy.UpstreamHashBy = anns.UpstreamHashBy.UpstreamHashBy
upstreams[defBackend].UpstreamHashBy.UpstreamHashBySubset = anns.UpstreamHashBy.UpstreamHashBySubset
upstreams[defBackend].UpstreamHashBy.UpstreamHashBySubsetSize = anns.UpstreamHashBy.UpstreamHashBySubsetSize
Expand Down Expand Up @@ -771,8 +769,6 @@ func (n *NGINXController) createUpstreams(data []*ingress.Ingress, du *ingress.B
upstreams[name] = newUpstream(name)
upstreams[name].Port = path.Backend.ServicePort

upstreams[name].SecureCACert = anns.SecureUpstream.CACert

upstreams[name].UpstreamHashBy.UpstreamHashBy = anns.UpstreamHashBy.UpstreamHashBy
upstreams[name].UpstreamHashBy.UpstreamHashBySubset = anns.UpstreamHashBy.UpstreamHashBySubset
upstreams[name].UpstreamHashBy.UpstreamHashBySubsetSize = anns.UpstreamHashBy.UpstreamHashBySubsetSize
Expand Down
4 changes: 0 additions & 4 deletions internal/ingress/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ import (
"k8s.io/ingress-nginx/internal/ingress/annotations/ratelimit"
"k8s.io/ingress-nginx/internal/ingress/annotations/redirect"
"k8s.io/ingress-nginx/internal/ingress/annotations/rewrite"
"k8s.io/ingress-nginx/internal/ingress/resolver"
)

var (
Expand Down Expand Up @@ -86,9 +85,6 @@ type Backend struct {
Name string `json:"name"`
Service *apiv1.Service `json:"service,omitempty"`
Port intstr.IntOrString `json:"port"`
// SecureCACert has the filename and SHA1 of the certificate authorities used to validate
// a secured connection to the backend
SecureCACert resolver.AuthSSLCert `json:"secureCACert"`
// SSLPassthrough indicates that Ingress controller will delegate TLS termination to the endpoints.
SSLPassthrough bool `json:"sslPassthrough"`
// Endpoints contains the list of endpoints currently running
Expand Down
3 changes: 0 additions & 3 deletions internal/ingress/types_equals.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,6 @@ func (b1 *Backend) Equal(b2 *Backend) bool {
if b1.Port != b2.Port {
return false
}
if !(&b1.SecureCACert).Equal(&b2.SecureCACert) {
return false
}
if b1.SSLPassthrough != b2.SSLPassthrough {
return false
}
Expand Down
1 change: 0 additions & 1 deletion internal/ingress/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion rootfs/etc/nginx/lua/test/balancer_test.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ local function reset_backends()
backends = {
{
name = "access-router-production-web-80", port = "80", secure = false,
secureCACert = { secret = "", caFilename = "", caSha = "" },
sslPassthrough = false,
endpoints = {
{ address = "10.184.7.40", port = "8080", maxFails = 0, failTimeout = 0 },
Expand Down