Added hsts settings, Refactored bool/integer parsing

[thetechnick]

I have added support for the HSTS header (#67)
While I was adding the new annotations I was bothered by the repeated code so I refactored the sections for ParseBool and added unittests for the new utility functions.

I was not yet able to test these changes on k8s.
The default value of the HSTS max-age directive is open for discussion.
I am not sure if the default should protect the user from setting it too long while testing,
or if it should reflect a useful production value.
ATM it is set to 1 Month.

[pleshakov]

While I was adding the new annotations I was bothered by the repeated code so I refactored the sections for ParseBool and added unittests for the new utility functions.

that's awesome!

The default value of the HSTS max-age directive is open for discussion.
I am not sure if the default should protect the user from setting it too long while testing,
or if it should reflect a useful production value.

I think it should reflect a useful production value.

When the user sets the incorrect values for hsts-max-age or hsts-include-subdomains, the default values get used. Do you think the controller should ignore all hsts keys in case of an error when parsing any of the hsts keys?

Another problem is what to do In case of errors when parsing annotations. Do you think the controller should proceed with creating the configuration for such an Ingress resource? I don't see a better solution in case of errors in the configmap other then reporting an error for the incorrect keys. And to stick with the same behavior when parsing annotations.

[thetechnick]

Thanks,
I think protecting the user from screwing up is a good thing to do,
but it is also a difficult process...

If the annotations based configuration of a ingress object do not make sense, or are wrong it is quite simple to emit a error and wait for the next time it is updated and the error is fixed.

But if the error is in the configmap I am not sure what I would prefer...
Maybe stop processing updates to ingress objects completely until the configmap is again valid?

What do you think about writing error messages into the ingress/configmap object, so the user does not need to grep through the ingress log?
Kubectl and the dashboard will render error messages as multiline strings nicely.

annotations:
  nginx.org/errors: |
    Invalid value for 'hsts-include-subdomains': 'tue' cannot be parsed to bool
    Invalid value for 'hsts-max-age': -199 a positive number or 0 is required

[pleshakov]

But if the error is in the configmap I am not sure what I would prefer...
Maybe stop processing updates to ingress objects completely until the configmap is again valid?

This can be a solution. However, for some of the keys we don't do validation at all, relying on NGINX to report an error when doing a config test. I suggest to stick to reporting errors and improving the reporting part for now.

What do you think about writing error messages into the ingress/configmap object, so the user does not need to grep through the ingress log?

I think the approach used in gce and nginx community controllers when the controller is emitting events is suitable. kubectl shows those events when using describe ing command. The dashboard doesn't show them for an Ingress object. However, it shows them for replication controllers, for example. The dashboard can be extended I guess to shows events for Ingress objects also.

[thetechnick]

You are completely right, I forgot about Events,
I haven`t noticed the other ingress controllers are emitting events on error cases... cool

So if I understand correctly we should add the current annotation error messages also to the ingress object as events.
Should we do the same thing for the configmaps?
Emitting a error message to the configmaps event log?
But I think this should be implemented in its own pull request.

So back to the original topic:
I will make the hsts config options depending on each other, so if the hsts-max-age (or any other option) is invalid it will use the default setting of hsts: 'False'.
This will save the user from setting hsts to 1 Month or longer on accident.

[pleshakov]

@thetechnick

So if I understand correctly we should add the current annotation error messages also to the ingress object as events.
Should we do the same thing for the configmaps?
Emitting a error message to the configmaps event log?

I think it's better to emit those Events to Ingress object only, so the user don't need to check for errors in multiple places

But I think this should be implemented in its own pull request.

A separate issue/pull request makes sense