Deny location mapping in case of specific errors #95

aledbf · 2016-12-29T20:02:25Z

fixes #16
fixes #108

k8s-reviewable · 2016-12-29T20:02:31Z

This change is

euank

There are two problems I see with this change:

Misconfigured annotations should be a server error (5xx), not client.

There are a few reasons for this. These errors are actually server configuration problems, so that's more honest.

More importantly, most monitoring/notification systems ignore 4xx (as you must in the day of mass internet scanning, brute forcing, and DoSing), so the error is far less likely to be noticed by the operator.

There's still no error printed at a reasonable V. The message is nice (thanks!), but if it's printed at V5 no one will see it.

I'm okay with 2 in a followup. 1 I think needs to be addressed.

aledbf · 2017-01-05T18:18:13Z

Misconfigured annotations should be a server error (5xx), not client.

I choose 403 because most of the annotations are related to security.
I saw this from a user perspective. Exactly which code in the 5xx family you suggest?

euank · 2017-01-05T18:20:28Z

500 or 503, probably 503

I think we should also have a similar error for all annotations, even those not closely related to security, but these are definitely the most important ones to cover.

aledbf · 2017-01-05T18:45:19Z

There's still no error printed at a reasonable V. The message is nice (thanks!), but if it's printed at V5 no one will see it.

I think we can add an event and/or log the content of Denied error (just the first error to avoid flooding the log)

coveralls · 2017-01-05T21:27:00Z

Coverage decreased (-0.3%) to 40.398% when pulling 0a598daeaac2e5289b4da61176d697550a3cd53a on aledbf:deny into 5186e93 on kubernetes:master.

coveralls · 2017-01-06T14:23:44Z

Coverage decreased (-0.004%) to 40.729% when pulling 4d3ae91b4d90b8016f848d0e1b33e7d415f2b4c3 on aledbf:deny into 5186e93 on kubernetes:master.

coveralls · 2017-01-06T14:58:51Z

Coverage increased (+0.6%) to 41.285% when pulling 1102cbd4378ac282bede827f97cb502b9271442d on aledbf:deny into 5186e93 on kubernetes:master.

coveralls · 2017-01-07T03:43:56Z

Coverage increased (+0.6%) to 41.35% when pulling 854af9eaa98484978c6423c68c872c68168d2f87 on aledbf:deny into 5186e93 on kubernetes:master.

euank

I think this is a better model for annotation parsing.

My previous comment about error code still applies and I left a few more comments. I'll do another pass once both my comments and the 4xx vs 5xx bit are addressed.

euank · 2017-01-06T23:16:02Z

core/pkg/ingress/annotations/auth/main.go

 )

 func init() {
 	// TODO: check permissions required
-	os.MkdirAll(DefAuthDirectory, 0655)
+	os.MkdirAll(authDirectory, 0655)


Maybe we should move this to part of either NewParser or Parse and the authDirectory as an argument to NewParser?

Right now I think the unit test will run this init block and create /etc/ingress... on my machine before the test gets a chance to override it.

And init blocks that can be avoided probably should be.

And init blocks that can be avoided probably should be.

Right. I will try to remove the init func.

euank · 2017-01-06T23:21:08Z

core/pkg/ingress/annotations/auth/main.go

@@ -65,40 +56,49 @@ type BasicDigest struct {
 	Secured bool   `json:"secured"`
 }

-// ParseAnnotations parses the annotations contained in the ingress
+type auth struct {
+	fn func(string) (*api.Secret, error)


nit: this can definitely be named better.

While we're refactoring though, this could also be an interface and I think that would be a bit more idiomatic.

e.g. SecretResolver { GetSecret(string) ...

And then cfg could just fulfil that interface and cfg could directly be passed in.

euank · 2017-01-07T00:00:54Z

core/pkg/ingress/annotations/authtls/main.go

@@ -39,27 +39,36 @@ type SSLCert struct {
 	PemSHA       string `json:"pemSha"`
 }

+type authTLS struct {
+	fn func(secret string) (*SSLCert, error)


same comment above about interfaces

euank · 2017-01-07T00:01:40Z

core/pkg/ingress/annotations/authtls/main.go

 	}

 	if str == "" {
-		return &SSLCert{}, fmt.Errorf("an empty string is not a valid secret name")
+		return nil, ing_errors.LocationDenied{


for consistency, NewLocationDenied here, or use this sorta constructor everywhere and delete the New

euank · 2017-01-07T00:01:57Z

core/pkg/ingress/annotations/authtls/main.go

 	}

 	_, _, err = k8s.ParseNameNS(str)
 	if err != nil {
-		return &SSLCert{}, err
+		return nil, ing_errors.LocationDenied{


same for this err

euank · 2017-01-07T20:42:33Z

core/pkg/ingress/controller/annotations.go

+
+func (e *annotationExtractor) Extract(ing *extensions.Ingress) map[string]interface{} {
+	anns := make(map[string]interface{}, 0)
+	for name, fn := range e.annotations {


fn isn't a func, so this naming is confusing.

Maybe name, annotationParser?

euank · 2017-01-07T20:44:38Z

core/pkg/ingress/controller/controller.go

+					}
+					err := mergo.Map(loc, anns)
+					if err != nil {
+						glog.V(2).Infof("unexpected error merging extracted annotations in location type: %v", err)


Should this be a Warn or Error? When would this happen?

in theory never :)
I will switch this to default level and Error

coveralls · 2017-01-07T23:20:46Z

Coverage increased (+0.6%) to 41.858% when pulling 5f6a472b9ce5336a8ccb9a4321e46b7cc5909ba0 on aledbf:deny into b2d084a on kubernetes:master.

aledbf · 2017-01-08T02:03:05Z

My previous comment about error code still applies and I left a few more comments.

With the new errors I changed the code to always print the errors. If the error is related to missing annotations (not an error really) that appears only in V(5).

I'll do another pass once both my comments and the 4xx vs 5xx bit are addressed.

Error code changed to 503

coveralls · 2017-01-08T02:20:36Z

Coverage increased (+0.8%) to 42.069% when pulling 24d61bda0d010e4b3e789b3d81cc347ff8188b9a on aledbf:deny into e58f510 on kubernetes:master.

euank

Mostly LGTM.

Would have been nice if the review comments were addressed in a new commit on top, not squashed in immediately.

euank · 2017-01-09T22:29:21Z

controllers/nginx/pkg/template/configmap.go

@@ -69,6 +70,12 @@ func ReadConfig(conf *api.ConfigMap) config.Configuration {
 	to.SkipAccessLogURLs = skipUrls
 	to.WhitelistSourceRange = whitelist

+	h, err := dns.GetSystemNameServers()


This change seems like it should be a separate PR; it doesn't seem related to the rest of this.

Right. Removed.

Moved to PR #123

euank · 2017-01-09T22:37:52Z

controllers/nginx/pkg/template/template.go

+func isLocationAllowed(input interface{}) bool {
+	loc, ok := input.(*ingress.Location)
+	if !ok {
+		return false


log an error too, this shouldn't ever happen

euank · 2017-01-10T02:19:53Z

core/pkg/ingress/annotations/authtls/main.go

+// AuthCertificate has a method that searchs for a secret
+// that contains a SSL certificate.
+// The secret must contain 3 keys named:
+type AuthCertificate interface {


Should this also be in resolver for consistency (resolver.AuthCertificate to match resolver.Secret)?

It's already there :( I'm not sure how to remove the cycle error that this introduces.

The easiest thing is probably to move the type up into resolver as well.

Something like this: euank@608cf8e

I think that's cleaner than the current interface duplication.

euank · 2017-01-10T02:32:49Z

core/pkg/ingress/controller/util.go

@@ -93,3 +96,16 @@ func IsValidClass(ing *extensions.Ingress, class string) bool {

 	return cc == class
 }
+
+const denied = "Denied"


Export and re-use it as a key in the other places too

aledbf · 2017-01-10T02:53:12Z

Would have been nice if the review comments were addressed in a new commit on top, not squashed in immediately.

Sorry about that. I always be told to squash the commits.

euank · 2017-01-10T06:23:46Z

Squash before merge, definitely, but it's often nicer to have review comments separate up until before merge.

coveralls · 2017-01-10T12:37:40Z

Coverage increased (+1.2%) to 42.38% when pulling f45d5c65ff0e21f2a8d6797886dd823a1a8deff5 on aledbf:deny into f90e9ee on kubernetes:master.

coveralls · 2017-01-10T12:49:04Z

Coverage increased (+1.2%) to 42.38% when pulling f45d5c65ff0e21f2a8d6797886dd823a1a8deff5 on aledbf:deny into f90e9ee on kubernetes:master.

coveralls · 2017-01-12T17:29:39Z

Coverage increased (+1.2%) to 42.278% when pulling 8191245 on aledbf:deny into c49b03f on kubernetes:master.

euank · 2017-01-12T18:07:45Z

core/pkg/ingress/controller/annotations.go

+		val, err := annotationParser.Parse(ing)
+		glog.V(5).Infof("annotation %v in Ingress %v/%v: %v", name, ing.GetNamespace(), ing.GetName(), val)
+		if err != nil {
+			_, de := anns["Denied"]


Would be nice to use the const here too. It makes it easier to find where that key's set programmatically.

nit, 'de' -> 'alreadyDenied' would be more readable imo

euank · 2017-01-12T18:15:01Z

core/pkg/ingress/annotations/authtls/main.go

 	}

-	return fn(str)
+	return a.certResolver.GetAuthCertificate(str)


I think it makes sense to wrap the error returned from this in a location denied.

This errors if the referenced cert can't be resolved, and that sounds like excellent grounds to deny.

euank · 2017-01-12T18:20:05Z

core/pkg/ingress/controller/annotations.go

+				glog.Errorf("error reading %v annotation in Ingress %v/%v: %v", name, ing.GetNamespace(), ing.GetName(), err)
+				continue
+			}
+			glog.V(5).Infof("error reading %v annotation in Ingress %v/%v: %v", name, ing.GetNamespace(), ing.GetName(), err)


The GetAuthCertificate thing above I think is an argument for changing the logic about setting the "Denied" key to default to true for any error, and only be skipped for a subset of recognized errors (e.g. AnnotationMissing).

For now we could just set denied here and refactor further if there are more errors that need to be treated differently.

You are right. Without this change we can end with the same error we have in 0.8.3.
Key "Denied" changes for any error now.

euank · 2017-01-12T18:23:19Z

Review pass done, apologies for the delay on it. If you want to just handle the GetAuthCertificate bit now and consider more aggressive denying as a followup, I'm okay with that.

coveralls · 2017-01-12T22:18:18Z

Coverage increased (+1.1%) to 42.225% when pulling 4a2146b on aledbf:deny into c49b03f on kubernetes:master.

coveralls · 2017-01-12T22:23:08Z

Coverage increased (+1.08%) to 42.192% when pulling 4a2146b on aledbf:deny into c49b03f on kubernetes:master.

euank

LGTM (with appropriate squashing ofc)

aledbf · 2017-01-13T00:17:04Z

@euank thanks for the review!

Sync upstream

aledbf self-assigned this Dec 29, 2016

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 29, 2016

aledbf added backend/generic nginx and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 29, 2016

aledbf mentioned this pull request Jan 5, 2017

annotations: print error and skip if malformed #108

Closed

euank suggested changes Jan 5, 2017

View reviewed changes

aledbf force-pushed the deny branch from 99c85eb to 1bc0c1d Compare January 5, 2017 20:44

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 5, 2017

aledbf force-pushed the deny branch 2 times, most recently from 5aca8b6 to 4d3ae91 Compare January 6, 2017 14:07

aledbf force-pushed the deny branch from 4d3ae91 to 1102cbd Compare January 6, 2017 14:41

aledbf force-pushed the deny branch from 1102cbd to 854af9e Compare January 7, 2017 03:26

euank reviewed Jan 7, 2017

View reviewed changes

aledbf force-pushed the deny branch 2 times, most recently from a882646 to 5f6a472 Compare January 7, 2017 22:59

aledbf force-pushed the deny branch from 5f6a472 to 24d61bd Compare January 8, 2017 02:00

aledbf force-pushed the deny branch from 24d61bd to 2c5d42e Compare January 8, 2017 15:32

aledbf mentioned this pull request Jan 9, 2017

[Ingress] 0.8.4 Release? kubernetes-retired/contrib#2010

Closed

whereisaaron mentioned this pull request Jan 9, 2017

nginx-ingress: configurable ingress class kubernetes-retired/contrib#1880

Closed

euank reviewed Jan 10, 2017

View reviewed changes

aledbf force-pushed the deny branch from 237bc6e to 2d4ad00 Compare January 10, 2017 02:49

aledbf mentioned this pull request Jan 10, 2017

Successfully healthcheck only after generating the right template #122

Closed

aledbf force-pushed the deny branch from a6e0101 to f45d5c6 Compare January 10, 2017 12:32

aledbf mentioned this pull request Jan 10, 2017

Split implementations from generic code #15

Merged

aledbf added 3 commits January 12, 2017 13:40

Deny location mapping in case of specific errors

597a0e6

Update godeps

9085e24

Fix interface duplication

8191245

aledbf force-pushed the deny branch from f45d5c6 to 8191245 Compare January 12, 2017 17:12

euank reviewed Jan 12, 2017

View reviewed changes

Address comments about consistency in the code

4a2146b

aledbf force-pushed the deny branch from 235eecb to 4a2146b Compare January 12, 2017 22:06

euank approved these changes Jan 12, 2017

View reviewed changes

aledbf merged commit 91c710f into kubernetes:master Jan 13, 2017

aledbf deleted the deny branch January 20, 2017 21:17

hnrytrn pushed a commit to hnrytrn/ingress-nginx that referenced this pull request Aug 9, 2018

Merge pull request kubernetes#95 from Shopify/sync-upstream

823ed43

Sync upstream

haoqing0110 pushed a commit to stolostron/management-ingress that referenced this pull request Mar 5, 2021

tag and push rhel image (kubernetes#95)

3e145d4

Deny location mapping in case of specific errors #95

Deny location mapping in case of specific errors #95

Conversation

aledbf commented Dec 29, 2016 • edited Loading

k8s-reviewable commented Dec 29, 2016

euank left a comment • edited Loading

Choose a reason for hiding this comment

aledbf commented Jan 5, 2017

euank commented Jan 5, 2017 • edited Loading

aledbf commented Jan 5, 2017

coveralls commented Jan 5, 2017

coveralls commented Jan 6, 2017

coveralls commented Jan 6, 2017

coveralls commented Jan 7, 2017

euank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jan 7, 2017

aledbf commented Jan 8, 2017

coveralls commented Jan 8, 2017

euank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented Jan 10, 2017

euank commented Jan 10, 2017

coveralls commented Jan 10, 2017

coveralls commented Jan 10, 2017

coveralls commented Jan 12, 2017

Choose a reason for hiding this comment

euank Jan 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

euank commented Jan 12, 2017

coveralls commented Jan 12, 2017

coveralls commented Jan 12, 2017

euank left a comment • edited Loading

Choose a reason for hiding this comment

aledbf commented Jan 13, 2017

aledbf commented Dec 29, 2016 •

edited

Loading

euank left a comment •

edited

Loading

euank commented Jan 5, 2017 •

edited

Loading

euank Jan 12, 2017 •

edited

Loading

euank left a comment •

edited

Loading