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.

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

"error obtaining PEM" from Secrets not referenced by Ingress rule #1709

Closed
jfoy opened this issue Nov 14, 2017 · 10 comments · Fixed by #1710
Closed

"error obtaining PEM" from Secrets not referenced by Ingress rule #1709

jfoy opened this issue Nov 14, 2017 · 10 comments · Fixed by #1710

Comments

@jfoy
Copy link

jfoy commented Nov 14, 2017

Is this a request for help?: No

What keywords did you search in NGINX Ingress controller issues before filing this one?: "error obtaining PEM"


Is this a BUG REPORT or FEATURE REQUEST? (choose one): Bug report

NGINX Ingress controller version: 0.9.0-beta.17

Kubernetes version (use kubectl version): v1.7.6+coreos.0

Environment:

  • Cloud provider or hardware configuration: AWS
  • OS (e.g. from /etc/os-release): CoreOS 1465.7.0
  • Kernel (e.g. uname -a): 4.12.10-coreos
  • Install tools: custom CloudFormation
  • Others: command-line syntax as below
/nginx-ingress-controller
  --default-backend-service=$(POD_NAMESPACE)/default-http-backend
  --publish-service=$(POD_NAMESPACE)/ingress-nginx
  --configmap=$(POD_NAMESPACE)/nginx-configuration
  --tcp-services-configmap=$(POD_NAMESPACE)/tcp-services
  --udp-services-configmap=$(POD_NAMESPACE)/udp-services
  --logtostderr

What happened: When we installed nginx-ingress-controller on a cluster, we saw a steady stream of messages with the warning below. The Secret in the error is not referenced by any Ingress rules.

I1110 01:16:20.870067       7 backend_ssl.go:40] starting syncing of secret my-namespace/my-secret
W1110 01:16:20.870156       7 backend_ssl.go:44] error obtaining PEM from secret my-namespace/my-secret: no keypair or CA cert could be found in my-namespace/my-secret

We traced the frequency of warnings to a faulty controller that was spuriously updating the Secret, but it's still a mystery why we would get this warning for any Secret that isn't associated with Ingress.

What you expected to happen: We shouldn't see warnings about Secrets that aren't (and never have been) associated with Ingress rules.

How to reproduce it (as minimally and precisely as possible):
Start nginx-ingress-controller as described above. In a namespace watched for Ingress changes, update any Secret that doesn't contain a .pem file.

Anything else we need to know:
It seems strange that we're scanning every Secret unconditionally for Ingress credentials on every update. Would something like this be more correct?

index 8149891b..9a394b2b 100644
--- a/internal/ingress/controller/listers.go
+++ b/internal/ingress/controller/listers.go
@@ -122,8 +122,7 @@ func (n *NGINXController) createListers(stopCh chan struct{}) (*ingress.StoreLis
                UpdateFunc: func(old, cur interface{}) {
                        if !reflect.DeepEqual(old, cur) {
                                sec := cur.(*apiv1.Secret)
-                               key := fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)
-                               n.syncSecret(key)
+                               n.syncQueue.Enqueue(sec)
                        }
                },
                DeleteFunc: func(obj interface{}) {
@@ -143,7 +142,7 @@ func (n *NGINXController) createListers(stopCh chan struct{}) (*ingress.StoreLis
                        }
                        key := fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)
                        n.sslCertTracker.Delete(key)
-                       n.syncQueue.Enqueue(key)
+                       n.syncQueue.Enqueue(sec)
                },
        }

That change passes both make test and make e2e-test. I'll be happy to submit it as a PR as soon as our CLA is up-to-date.

@aledbf
Copy link
Member

aledbf commented Nov 14, 2017

@jfoy thank you for the report.
The change we need in the update method is:

        key := fmt.Sprintf("%v/%v", sec.Namespace, sec.Name)
	_, exists := ic.sslCertTracker.Get(key)
	if exists {
              n.syncSecret(key)
        }

@aledbf aledbf added the bug label Nov 14, 2017
@aledbf
Copy link
Member

aledbf commented Nov 14, 2017

It seems strange that we're scanning every Secret unconditionally for Ingress credentials on every update. Would something like this be more correct?

One of the issues (besides the bug) is that we cannot filter the informers or just watch a list of objects
(feature missing in client-go)

@whereisaaron
Copy link
Contributor

I just upgraded to 0.13.0 and started getting these type of log entries not referenced for TLS, but for basic auth. Is there a regression?

    nginx.ingress.kubernetes.io/auth-type: basic
    nginx.ingress.kubernetes.io/auth-secret: developers-basic-auth
    nginx.ingress.kubernetes.io/auth-realm: "Authentication Required"
W0421 00:08:04.853701       6 backend_ssl.go:48] error obtaining PEM from secret prometheus/developers-basic-auth: no keypair or CA cert could be found in prometheus/developers-basic-auth
W0421 00:08:04.859483       6 backend_ssl.go:48] error obtaining PEM from secret prometheus/developers-basic-auth: no keypair or CA cert could be found in prometheus/developers-basic-auth

@jpalomaki
Copy link

@aledbf I also see the error described by @whereisaaron

@aledbf
Copy link
Member

aledbf commented May 7, 2018

@jpalomaki please update to 0.14.0

@jpalomaki
Copy link

@aledbf will do

@jpalomaki
Copy link

@aledbf running 0.14.0, I still get error obtaining PEM from secret app/swagger-htpasswd: no keypair or CA cert could be found in app/swagger-htpasswd for a secret that contains basic auth credentials (i.e. no TLS certificate secrets). Relevant ingress annotation:

nginx.ingress.kubernetes.io/auth-secret: swagger-htpasswd

@jpalomaki
Copy link

@aledbf should this issue be re-opened?

@aledbf
Copy link
Member

aledbf commented May 11, 2018

@jpalomaki no. Please open a new issue

@jpalomaki
Copy link

@aledbf See #2506

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants